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

Parsing set-cookie #30

Closed
Heliozoa opened this issue Nov 22, 2020 · 4 comments
Closed

Parsing set-cookie #30

Heliozoa opened this issue Nov 22, 2020 · 4 comments

Comments

@Heliozoa
Copy link

Hi, thanks for your work on hyperx.

Currently, hyperx::headers::SetCookie is just a newtype for Vec<String>, each string looking something like "lang=en-US; Expires=Wed, 09 Jun 2021 10:18:14 GMT". I think it would be nice if the header was a bit easier to work with, I was initially hoping for an interface closer to hyperx::headers::Cookie. For my specific use case, I just need the cookie name and value which are easy enough to parse manually, but I imagine if you need to read some of the other properties that can be included it might be a bit trickier. Would this kind of functionality have a place in hyperx?

Thanks.

@dekellum
Copy link
Owner

Potentially more could be added here, but it should be backward compatible. I wonder if the cookie crate has this well covered already, with an apparently full parser, and if its easy enough to use in combination with this crate (or for that matter, with the set-cookie headers read directly from HeaderMap)?

@Heliozoa
Copy link
Author

I think I took a look at cookie before, but I didn't realise it can also be used for set-cookie values. I guess I just assumed the syntax is different for the two. Looks like it does the job! As far as I can tell, it only supports parsing them from strings (Into<Cow<'c, str>> to be precise), but since hyperx::header::SetCookie already contains the values as strings, working with it for my use case (just extracting stuff the set-cookie values) is pretty easy.

    let header_value = http::HeaderValue::from_static("Set-Cookie: id=a3fWa; Max-Age=2592000");
    let set_cookie: hyperx::header::SetCookie =
        hyperx::header::Header::parse_header(&&header_value).unwrap();

    let ck = cookie::Cookie::parse(&set_cookie[0]).unwrap();
    assert_eq!(
        ck.max_age().unwrap(),
        std::time::Duration::from_secs(2592000)
    );

I still think it would be nice to have the functionality in hyperx, but I'm not sure what a good, backwards compatible solution would look like. A feature flag that pulls in cookie and implements traits like hyperx::header::Header for cookie::Cookie, maybe? Keeping this in mind if a 2.0 version ever becomes a thing and maybe a note in the docs about being able to parse the values with cookie would be good enough for me, I think.

@dekellum
Copy link
Owner

hyperx::header::SetCookie will just silently exclude any non-UTF8 header values (ASCII-only per spec), so directly parsing from HeaderMap is roughly equivalent:

    use http::header::SET_COOKIE;

    let mut headers = http::HeaderMap::new();
    for h in &[
        "cookie_1=value 1; path=/path; Domain=example.com; \
         Expires=Wed, 30 Nov 2020 13:28:00 GMT",
        "cookie_2=valu%C3%A9 2; path=/; Domain=par.example.com; \
         Expires=Wed, 29 Nov 2020 18:00:00 GMT" ] {
        headers.append(SET_COOKIE, h.parse().unwrap());
    }

    // Parse all set-cookie headers, excluding non-ASCII values or other parse
    // errors.
    let cookies: Vec<_> = headers.get_all(SET_COOKIE)
        .iter()
        .filter_map(|hv| hv.to_str().ok())
        .filter_map(|vs| cookie::Cookie::parse_encoded(vs).ok())
        .collect();

    assert_eq!(cookies.len(), 2);
    assert_eq!(cookies[0].name(), "cookie_1");
    assert_eq!(cookies[0].path(), Some("/path"));
    assert_eq!(cookies[1].name(), "cookie_2");
    assert_eq!(cookies[1].value(), "valué 2");
    assert_eq!(cookies[1].expires().map(|t| t.month()), Some(11))

Or for lookup by name, a cookie::CookieJar could be used instead of the last Vec. Some doc referencing the cookie crate and this issue might be helpful. Given that hyperx::header::SetCookie is, and likely should remain a Vec<String>, I doubt its worth the dependency weight to directly integrate the cookie crate here.

@dekellum
Copy link
Owner

Added docs and reference to the above usage example in 0b05e9a (main).

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

No branches or pull requests

2 participants