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(header): change Cookie to be map-like #1152

Merged
merged 1 commit into from
Apr 28, 2017
Merged

feat(header): change Cookie to be map-like #1152

merged 1 commit into from
Apr 28, 2017

Conversation

seanmonstar
Copy link
Member

The Cookie header now has get and set methods, to tree the list of
cookies as a map.

Closes #1145

cc @dorfsmay

@dorfsmay
Copy link
Contributor

Ah. That's unfortunate. I'm working on a different implementation, but had problems fighting the borrow checker...

Now I actually believe a map is not the right structure for the cookie header.
The data should be a Vec of tuples, because that really what it is, but it should have a push method to add more cookies to it, including duplicate keys, because that is legal from a client perspective, and a get method that will only show the first key when duplicate exists.

This is what I was working on... Take a look, maybe what I am saying here will make more sense. Keep in mind that some of the complication in there is due to trying to work around the borrow checker: https://is.gd/MxBTqt

@seanmonstar
Copy link
Member Author

Ah. That's unfortunate. I'm working on a different implementation

@dorfsmay thats ok! I wasn't sure if you were, and I was hoping to get this in before tagging 0.11, so that's why I worked on this. But it's good also, because you bring up an interesting point. Clients (such as Servo) would want to push all cookies that relate to a certain resource, even if they have the same name...

@dorfsmay
Copy link
Contributor

When do you plan to release 0.11?
Are you going to implement something, or should I push harder on this one?

Maybe I can just use memory for now to get it in, and work on making reference from the Hash (VecMap) to the Vec later as an improvement...

@seanmonstar
Copy link
Member Author

To get the desired semantics, what if set is removed, and an append method added?

let mut cookie = Cookie::new();
cookie.append("foo", "bar");
cookie.append("foo", "otherbar");
assert_eq!(cookie.to_string(), "foo=bar; foo=otherbar");

@LegNeato
Copy link

LegNeato commented Apr 27, 2017

I actually like the solution form-data in npm uses, as they have the same problem. They have set that replaces all values with the key and append which tacks it on (possibly duplicating the key).

let mut cookie = Cookie::new();
cookie.append("foo", "bar");
cookie.append("foo", "otherbar");
assert_eq!(cookie.to_string(), "foo=bar; foo=otherbar");
cookie.set("foo", "baz");
assert_eq!(cookie.to_string(), "foo=baz");

@seanmonstar seanmonstar force-pushed the cookie branch 2 times, most recently from 2a046ae to f2a3081 Compare April 27, 2017 01:16
@dorfsmay
Copy link
Contributor

I'm happy with happy append (or push in my solution above) vs set, but get has to return the first value, that was set/appended for that key, because that is the way everybody expect servers to work.

Client are supposed to be appending in order, even duplicate keys.
Sever should only care about the first key when there are duplicates.
The raw value, or an iterator for that matter, should make everything available in the order it was set.

The `Cookie` header now has 'set' and `get` methods, to treat the list of
cookies as a map.

Closes #1145

BREAKING CHANGE: The `Cookie` header is no longer a wrapper over a
  `Vec<String>`. It must be accessed via its `get` and `append` methods.
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.

3 participants