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.

Sign up for GitHub

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
765f175
feat(headers): use a Vec + a map for Cookie
dorfsmay Apr 27, 2017
e7091f1
refactor(headers): change HashMap to MapVec
dorfsmay Apr 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 97 additions & 15 deletions src/header/common/cookie.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use header::{Header, Raw};
use std::fmt::{self, Display};
use header::{Header, Raw, VecMap};
use std::fmt;
use std::str::from_utf8;
use std::borrow::Borrow;

/// `Cookie` header, defined in [RFC6265](http://tools.ietf.org/html/rfc6265#section-5.4)
///
Expand All @@ -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.

#[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());

}


__hyper__deref!(Cookie => Vec<String>);
//__hyper__deref!(Cookie => Vec<String>);

impl Header for Cookie {
fn header_name() -> &'static str {
Expand All @@ -39,16 +45,28 @@ impl Header for Cookie {
}

fn parse_header(raw: &Raw) -> ::Result<Cookie> {
let mut cookies = Vec::with_capacity(raw.len());
let mut cookies = Cookie::with_capacity(raw.len());
for cookies_raw in raw.iter() {
let cookies_str = try!(from_utf8(&cookies_raw[..]));
for cookie_str in cookies_str.split(';') {
cookies.push(cookie_str.trim().to_owned())
let cookie_trimmed = cookie_str.trim();
if cookie_trimmed.len() == 0 {
continue;
}
let mut kv_iterator = cookie_trimmed.splitn(2, '=');
// split returns at least one element - unwrap is safe
let k = kv_iterator.next().unwrap().trim();
let v = match kv_iterator.next() {
Some(value) => value.trim(),
None => "",
};

cookies.push(k, v);
}
}

cookies.shrink_to_fit();
if !cookies.is_empty() {
Ok(Cookie(cookies))
Ok(cookies)
} else {
Err(::Error::Header)
}
Expand All @@ -59,18 +77,82 @@ impl Header for Cookie {
}
}

impl Cookie {
/// Create an empty Cookie header.
pub fn new() -> Cookie {
Cookie::with_capacity(0)
}

/// Create a Cookie header of a certain size.
pub fn with_capacity(capacity: usize) -> Cookie {
Cookie {
cookies: Vec::with_capacity(capacity),
index: VecMap::with_capacity(capacity),
}
}

/// Shrink the Cookie header internal elements to the currently used size.
pub fn shrink_to_fit(&mut self) {
self.cookies.shrink_to_fit();
}

/// Append a new cookie to the Cookie header.
pub fn push<T: Into<String>>(&mut self, name_tref: T, value_tref: T) {
let name = name_tref.into();
let value = value_tref.into();
self.cookies.push((name.clone(), value.clone()));
if self.index.get(&name) == None {
self.index.insert(name, value);
}
}


/// Get value of cookie from name. If duplicate names were pushed to the
/// Cookie header, this function will only return the first one.
pub fn get<T: Borrow<String>>(&self, name: T) -> Option<String> {
match self.index.get(name.borrow()) {
Some(value_ref) => Some((*value_ref).clone()),
None => None,
}
}


/// 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.

self.cookies.clear();
self.index.clear();
self.push(name_tref, value_tref);
}

/// Check if there are any exiting cookie.
pub fn is_empty(&self) -> bool {
self.cookies.is_empty()
}
}

impl fmt::Display for Cookie {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
let cookies = &self.0;
for (i, cookie) in cookies.iter().enumerate() {
if i != 0 {
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.

if !first {
try!(f.write_str(" "));
} else {
first = false
}
try!(Display::fmt(&cookie, f));
try!(f.write_str(&cookie));
try!(f.write_str(";"));


}
Ok(())

}
}

bench_header!(bench, Cookie, { vec![b"foo=bar; baz=quux".to_vec()] });

/*
bench_header!(bench, Cookie, {
vec![b"foo=bar; baz=quux".to_vec()]
});
*/