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

Add cookie management to axum-extra #816

Merged
merged 12 commits into from
Mar 4, 2022
Merged

Add cookie management to axum-extra #816

merged 12 commits into from
Mar 4, 2022

Conversation

davidpdrsn
Copy link
Member

@davidpdrsn davidpdrsn commented Mar 2, 2022

This adds extractors and responses for dealing with cookies to axum-extra. It uses the new IntoResponseParts APIs such that no middleware and Arc<Mutex<_>>s are required. This should make things more composable as there is no global "cookie manager" that all changes must go through. It just reads the Cookie header from the request and appends Set-Cookie headers to the response.

Example usage:

async fn set_cookie(mut jar: CookieJar) -> impl IntoResponse {
    jar.add(Cookie::new("key", "value"))
}

async fn get_cookie(jar: CookieJar) {
    dbg!(jar.get("key"));
}

All the heavy lifting is done by the cookie crate.

Co-authored-by: Jonas Platte <jplatte+git@posteo.de>
Copy link
Member

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

Instead of having a separate delta type in the API, why not change add / remove to owned-self methods that return Self instead of that separate type, and implement IntoResponseParts for the jars directly?

If the jar types have a must_use attribute, that should make it equally hard to misuse while making chaining possible and slightly simplifying the API surface. You also wouldn't have to have the jar be a mutable variable then.

axum-extra/src/extract/cookie.rs Outdated Show resolved Hide resolved
axum-extra/src/extract/cookie.rs Outdated Show resolved Hide resolved
axum-extra/src/extract/cookie.rs Outdated Show resolved Hide resolved
axum-extra/src/extract/cookie.rs Outdated Show resolved Hide resolved
axum-extra/src/extract/cookie.rs Outdated Show resolved Hide resolved
@jplatte
Copy link
Member

jplatte commented Mar 3, 2022

Did you see my overall comment as part of the review?

Instead of having a separate delta type in the API, why not change add / remove to owned-self methods that return Self instead of that separate type, and implement IntoResponseParts for the jars directly?

If the jar types have a must_use attribute, that should make it equally hard to misuse while making chaining possible and slightly simplifying the API surface. You also wouldn't have to have the jar be a mutable variable then.

@davidpdrsn
Copy link
Member Author

Ah no I missed that somehow. Good idea!

@davidpdrsn davidpdrsn added this to the 0.5 milestone Mar 4, 2022
@davidpdrsn
Copy link
Member Author

@jplatte so much nicer now 😃

Copy link
Member

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

Two more comments:

  • The upstream CookieJars have an add_original method that only modifies the jar if a cookie of the given name doesn't exist yet. Maybe that should be mirrored? (although the one thing I can immediately think of it being used for would be tracking cookies.. yuck)
  • Adding an example for this would be nice

@davidpdrsn
Copy link
Member Author

Hm not sure. When extracting the jars I call add_original with headers from the request as per the docs

This method is intended to be used to seed the cookie jar with cookies received from a client’s HTTP message.

@jplatte
Copy link
Member

jplatte commented Mar 4, 2022

Right. Let's leave it out for now then!

@davidpdrsn davidpdrsn merged commit f045f23 into main Mar 4, 2022
@davidpdrsn davidpdrsn deleted the cookie branch March 4, 2022 09:53
@demurgos
Copy link

demurgos commented Mar 15, 2022

Hello!
Any idea when a new release will be released to crates.io? Cookie support is the last thing I need to migrate to Axum.

@davidpdrsn
Copy link
Member Author

Hopefully sometimes this month but no guarantees. Take a look at tower-cookies in the mean time. main is also fairly stable so if you wanna go bleeding edge and use that via a git dependency then thats probably fine.

@demurgos
Copy link

I was able to migrate my website to axum using the main branch: cookies work.
I am still hoping that this change will be published to crates.iosoon so it's easier to depend on it.

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

Successfully merging this pull request may close these issues.

3 participants