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(headers): use a Vec + a map for Cookie #1153

Closed
wants to merge 2 commits into from
Closed

feat(headers): use a Vec + a map for Cookie #1153

wants to merge 2 commits into from

Conversation

dorfsmay
Copy link
Contributor

Use a Vec and a map for the Cookie header in order to satisfy the need
for both duplicate names (keys) from a client perspective and single
value for a given key from a server perspective .

BREAKING CHANGE: This change the format of the cookie header, any
previous implementation will have to be changed.

Use a Vec and a map for the Cookie header in order to satisfy the need
for both duplicate names (keys) from a client perspective and single
value for a given key from a server perspective .

BREAKING CHANGE: This change the format of the cookie header, any
previous implementation will have to be changed.
@dorfsmay
Copy link
Contributor Author

dorfsmay commented Apr 27, 2017

@seanmonstar @LegNeato

As part of the larger discussion in #1145

This is a first stab to make sure that's going in the right direction.

  • It doesn't matter how you dice it, at the end of the day, a "Cookie header" is really a string representation of a Vec<(String, String)>, so let's use that, and use methods and/or additional elements in the structure to provide the functionalities we need, such as a get() returning the first value for a given key.
  • I know HashMap, I'll re-submit a different PR with VecMap, right now, this compiles and demonstrates the concepts.
  • I use a Vec<(String, String)> + a HashMap (VecMap later), in order to be able to both not lose duplicate keys, and only give the value for the first key on a et
  • Another way to do the above would be to just use a Vec<(String, String)>, and make the get() method walk it. It would be cheaper in memory, but more expensive if an implementer looks up more than one key.
  • Using owned value in the HasMap is a waste of memory, but I am very green to Rust (weeks), I wasted hours trying to make it use reference to the owned values inside the Vec and make it work in hyper... Again this is more to show what I think would be the right implementation for the Cookie header. Also, we could roll with this as it gets the signatures right, and implement internal references later if it is worth it (and maybe somebody more experience add that!).
  • I added a set, diff from a push (append) as per @LegNeato sugestion
  • Still missing: a proper iterator. I wasted another few hours on this, will work more on that later.

@GitCop
Copy link

GitCop commented Apr 28, 2017

Thanks for contributing! Unfortunately, I'm here to tell you there were the following style issues with your Pull Request:

  • Commit: 6c74b5f
  • Commits must be in the following format: %{type}(%{scope}): %{description}

Guidelines are available at https://github.com/hyperium/hyper/blob/master/CONTRIBUTING.md


This message was auto-generated by https://gitcop.com

Use MapVec instead of HashMap for the Cookie header as per sugestion
from @seanmonstar for performance purpose.
Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for exploring this with me. I believe the code in #1152 may be in a slightly better place, but we wouldn't have gotten there without your efforts to bring up the question and discuss how to solve it!



/// Clear the current Cookie, and add one with specified name and value.
pub fn set<T: Into<String>>(&mut self, name_tref: T, value_tref: T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding of the suggestion for this method was to clear the values of only this key. This would seem confusing:

let mut cookie = Cookie::new();
cookie.set("foo", "bar");
cookie.set("baz", "quux");
// where did my foo=bar go!
assert_eq!(cookie.to_string(), "baz=quux");

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understood the suggestion as push()/append() to add an extra cookie name/value to the header, and set() to for the entire header to this one value.

try!(f.write_str("; "));
let mut first = true;
for pair in self.cookies.clone() {
let cookie = format!("{}={}", pair.0, pair.1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would allocate a brand new String, before then writing it to the formatter. It can be done without the temporary allocated, such as write!(f, "{}={}", key, val).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point. Thanks.

#[derive(Clone)]
pub struct Cookie {
cookies: Vec<(String, String)>,
index: VecMap<String, String>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The VecMap is sufficient on it's own. There no need to double the strings here. The VecMap has insert and append, and iter gets all the key-value pairs, with duplicates.

Also, a Cow<'static, str> can be used, which allows someone who wants to set the same key and value on all cookies to do so without allocating and copying memory for it:

let mut cookie = Cookie::new();
// no allocation or copy required
cookie.set("foo", "bar");
// dynamic string, use a String
cookie.set(nonce_key.to_string(), nonce_val.to_string());

@@ -27,10 +28,15 @@ use std::str::from_utf8;
/// ])
/// );
/// ```
#[derive(Clone, PartialEq, Debug)]
pub struct Cookie(pub Vec<String>);
#[allow(missing_debug_implementations)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd still want a Debug implementation. It can use formatter.debug_map().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, but with my implementation, I would have had to add a Debug trait to VecMap. I wanted to get the code out fast for sake of discussion, rather than spend too many hours on an implementation that wasn't going to be used.

@dorfsmay
Copy link
Contributor Author

Thanks for taking the time to go through my code and the comments, this is really helpful.

@dorfsmay dorfsmay closed this Apr 29, 2017
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