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

Jump to bottom

Cookie Improvement #170

Merged
merged 8 commits into from
Apr 24, 2019
Merged

Cookie Improvement #170

merged 8 commits into from
Apr 24, 2019

Conversation

mmrath
Copy link
Contributor

@mmrath mmrath commented Apr 13, 2019 •
edited
Loading

Description

This change adds support for adding, removing cookies on top of currently supported retrieval.

Breaking changes:

  1. Added a new middleware. This middleware is required to be added for the features to work
  2. ExtractCookies is renamed to CookiesExt because this is no longer just extraction feature

Motivation and Context

Every web application needs to deal with cookies at some stage. Hopefully this approach is simple and ergonomic

How Has This Been Tested?

Test cases written for simple cases. Examples included for basic features.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@mmrath
Copy link
Contributor Author

mmrath commented Apr 13, 2019

/ping @aturon @yoshuawuyts @bIgBV
Ready for initial feedback

bIgBV
bIgBV reviewed Apr 14, 2019
View reviewed changes
Copy link
Contributor

@bIgBV bIgBV left a comment

Choose a reason for hiding this comment

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

I love how simple adding extensions is. This is a great change.

I've left some comments which are mostly around error handling.

examples/cookies.rs Show resolved Hide resolved
src/cookies.rs Show resolved Hide resolved
src/middleware/cookies.rs Outdated Show resolved Hide resolved
@mmrath
Copy link
Contributor Author

mmrath commented Apr 14, 2019

I think I should change CookiesExt to return Result. And send an internal error response in case of middleware.

bIgBV
bIgBV approved these changes Apr 15, 2019
View reviewed changes
Copy link
Contributor

@bIgBV bIgBV left a comment

Choose a reason for hiding this comment

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

LGTM!

@mmrath mmrath changed the title [WIP] Cookie Improvement - For feedback Cookie Improvement Apr 16, 2019
@mmrath
Copy link
Contributor Author

mmrath commented Apr 17, 2019

Ready for merge. Build fail because of futures api change.

@secretfader
Copy link

LGTM. The only thing I'd say (and this is just commentary) is that we may want to come up with a better term for these extension traits. If _Ext is to be the standard (which I'm fine with), I should probably rename the trait in #175.

Then again, we can rename them both so long as it happens before a release, now that 0.1.0 is out in the wild.

aturon
aturon approved these changes Apr 24, 2019
View reviewed changes
Copy link
Collaborator

@aturon aturon left a comment

Choose a reason for hiding this comment

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

Fantastic PR! Just left a couple of minor nits.

src/cookies.rs
let arc_jar = cookie_data.content.clone();
let locked_jar = arc_jar
.read()
.map_err(|e| StringError(format!("Failed to get write lock: {}", e)))?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be "read lock"

src/cookies.rs

cookie
let arc_jar = cookie_data.content.clone();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this clone required?

src/cookies.rs
.extensions()
.get::<CookieData>()
.ok_or_else(|| StringError(MIDDLEWARE_MISSING_MSG.to_owned()))?;
let jar = cookie_data.content.clone();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this clone required?

src/cookies.rs
.get::<CookieData>()
.ok_or_else(|| StringError(MIDDLEWARE_MISSING_MSG.to_owned()))?;

let jar = cookie_data.content.clone();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this clone required?

@aturon
Copy link
Collaborator

aturon commented Apr 24, 2019

@secretfader Agreed re: needing a solid convention here. I've opened an issue.

@aturon
Copy link
Collaborator

aturon commented Apr 24, 2019

I'm gonna go ahead and merge and will fix the nits in follow-up.

Thanks so much @mmrath, this is great work!

@aturon aturon merged commit a4464e8 into http-rs:master Apr 24, 2019
@mmrath mmrath mentioned this pull request Apr 25, 2019
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yoshuawuyts yoshuawuyts yoshuawuyts left review comments

@aturon aturon aturon approved these changes

@bIgBV bIgBV bIgBV approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mmrath @secretfader @aturon @yoshuawuyts @bIgBV