Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(method): provide HashMap from Cookie header #1142

Closed
wants to merge 2 commits into from
Closed

feat(method): provide HashMap from Cookie header #1142

wants to merge 2 commits into from

Conversation

dorfsmay
Copy link
Contributor

Cookie headers are always key/value only. Apps servers typically provide
facilities to test existence and access them by name without additional
libraries.

Closes #1139

Cookie headers are always key/value only. Apps servers typically provide
facilities to test existence and access them by name without additional
libraries.

Closes #1139
@seanmonstar
Copy link
Member

Thanks for submitting this work! As suggested in a previous issue, I'm not certain whether this fits within the scope of hyper. It's possible and quite likely that anyone dealing with cookies much would want much more that a simple map.

The scarier part, is that once hyper commits to an API, it can't easily break it. So, it should probably at least be explored and published as a separate thing, and be considered in the future. Does that make sense?

@dorfsmay
Copy link
Contributor Author

I understand your concern about long term support, it totally makes sense... I started writing a quick answer, but turned into a novel - sorry about that. I don't want to be argumentative, I am not trying to push for this PR just for its own sake, but hope the need for a better parsing of the Cookie header isn't missed.

This PR is not trying to deal with cookie life cycle, it is very specific to the http "Cookie" header. Parsing cookies from the "Cookie" header will never need to be more complicated, as it is exactly this, a succession of keys and values (separated by ; and =) - not to be confused with the Set-Cookie header and the issues of storing cookies, determining their scope, etc... In my view, the current implementation is not a good representation of the "Cookie" header, and this PR is basically trying to bring it at par with what has been done for hyper::header::SetCookie.

As you mentioned in #1139, dealing with cookie life cycle is a much bigger job and the cookie crate can be used for this. But in my experience, the need to be bale to just check specific cookies on incoming requests (so pure k/v) arise quite often.

Is there any way to get feedback form people using hyper? I wonder how many make use of the "Cookie" header without wanting to use a heavy gun such as cookie-rs, and end up having to write their own version of this...

@seanmonstar
Copy link
Member

Thinking about it, I've realized the error of my ways. The real problem here is not adding this method, it's that Cookie is just a Vec<String>! We only ever receive headers like this: Cookie: foo=bar; session=sean; hello=world, and having that be a vector of ["foo=bar", "session=sean", "hello=world"] doesn't really help anyone.

We should change the Cookie header to be a map-like object. I've opened an issue for discussion/design: #1145

@seanmonstar
Copy link
Member

I'm going to close this, as a different solution is discussed in #1145.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants