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

Ported Cookies implementation from archive-split #380

Merged
merged 12 commits into from
Jan 30, 2020

Conversation

nhellwig
Copy link
Contributor

This is a port of the cookies implementation from the time when the tide was split into multiple crates.
Changes are:

  • ContextExt into RequestExt
  • StringError was needed again
  • appendHeader also needed

Tests have been ported as well. I was unsure how the strategy will be regarding Error handling so feel free to comment and suggest alternatives. So far I have tried to stay as close to the original version.

Cheers,
Niko

src/middleware/cookies.rs Outdated Show resolved Hide resolved
src/middleware/cookies.rs Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@nhellwig
Copy link
Contributor Author

Thanks for the comments. Have some time probably next week and will work on it.

@yoshuawuyts
Copy link
Member

Something else I'm wondering about now is whether we could make it so parsing the incoming request cookies into the CookieJar could be done on the first call to cookie/set_cookie/remove_cookie. That way APIs that don't use cookies at all wouldn't ever pay the parsing cost, and it would be effectively be zero-cost.

src/middleware/cookies.rs Outdated Show resolved Hide resolved
src/middleware/cookies.rs Outdated Show resolved Hide resolved
src/middleware/cookies.rs Outdated Show resolved Hide resolved
src/middleware/cookies.rs Outdated Show resolved Hide resolved
src/middleware/cookies.rs Show resolved Hide resolved
src/request.rs Outdated Show resolved Hide resolved
tests/cookies.rs Show resolved Hide resolved
src/response/mod.rs Show resolved Hide resolved
src/middleware/cookies.rs Outdated Show resolved Hide resolved
src/request.rs Outdated Show resolved Hide resolved
@yoshuawuyts
Copy link
Member

@nhellwig phew, that was a large review. I hadn't thought too much about cookie handling until now, but I feel we're really on track for something good here! The comments so far are part architectural, and part implementation specific. Perhaps best would be to start with the architectural ones:

When put together I'm unsure how well these ideas go. But I think they're the right direction for these APIs. The other comments are mostly implementation-specific.

Thanks so much for putting the work in! I'm very excited about the direction this is headed!

nhellwig and others added 3 commits January 21, 2020 20:46
Co-Authored-By: Yoshua Wuyts <yoshuawuyts+github@gmail.com>
Co-Authored-By: Yoshua Wuyts <yoshuawuyts+github@gmail.com>
@yoshuawuyts
Copy link
Member

@nhellwig ohh, yeah this is getting pretty close now!

The biggest thing that still seems to be missing is enabling the middleware by default. That would allow us to get rid of the Result from all cookie methods as well (replacing it with unwrap instead), which would help simplify the API even further!

@nhellwig
Copy link
Contributor Author

Yeah that is worth looking at ^^.

Just moved the set_cookie and remove_cookie to Response.
Next I will add doc tests as well.

Copy link
Member

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

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

One final small set of changes, and I think we should be good to merge!

src/request.rs Outdated Show resolved Hide resolved
Copy link
Member

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

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

Yessss, I think we're good to merge now. This is great; thanks @nhellwig!

@yoshuawuyts yoshuawuyts merged commit 4e2857a into http-rs:master Jan 30, 2020
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.

None yet

2 participants