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

Feature/enable cookie store at request level #1567

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Feature/enable cookie store at request level #1567

wants to merge 4 commits into from

Conversation

tranchitam
Copy link

@tranchitam tranchitam commented Jul 31, 2018 •
edited

Motivations:

  • Enable the ability to pass CookieStore at request level -> cookies between different requests having the same URI will be independent and will not override each other.
  • Still keep global CookieStore from config.

Changes:

  • Request has getCookieStore() method.
  • RequestBuilder has setCookieStore() method to set cookie store -> cookie store at request level is null by default.
  • When http responses come -> add cookies to request cookie store if it's present, otherwise add cookies to global cookie store.
  • When setting cookies for redirect request -> if request cookie store is present, get cookies from request cookie store and set them to redirect request. Otherwise, if global cookie store is present, get cookies from global cookie store and and them to redirect request.

Results:

  • Still keep global cookie store.
  • If request has its own cookie store -> cookies for different requests having the same URI will be independent and not override each other

Related issue:

#1565

@tranchitam tranchitam mentioned this pull request Jul 31, 2018
Open
slandelle
slandelle requested changes Aug 20, 2018
View reviewed changes
client/src/main/java/org/asynchttpclient/netty/handler/intercept/Interceptors.java Outdated
@@ -71,12 +71,19 @@ public boolean exitAfterIntercept(Channel channel,

// This MUST BE called before Redirect30xInterceptor because latter assumes cookie store is already updated
CookieStore cookieStore = config.getCookieStore();
Copy link
Contributor

Choose a reason for hiding this comment

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

CookieStore cookieStore = request.getCookieStore() != null ? request.getCookieStore() : config.getCookieStore() ;

Copy link
Author

Choose a reason for hiding this comment

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

I already updated.

client/src/main/java/org/asynchttpclient/netty/handler/intercept/Interceptors.java Outdated
@@ -71,12 +71,19 @@ public boolean exitAfterIntercept(Channel channel,

// This MUST BE called before Redirect30xInterceptor because latter assumes cookie store is already updated
CookieStore cookieStore = config.getCookieStore();
if (cookieStore != null) {
CookieStore requestCookieStore = request.getCookieStore();
if (cookieStore != null || requestCookieStore != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please restore

Copy link
Author

Choose a reason for hiding this comment

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

Restored

client/src/main/java/org/asynchttpclient/netty/handler/intercept/Interceptors.java Outdated

if (requestCookieStore != null) {
requestCookieStore.add(request.getUri(), c);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please restore

Copy link
Author

Choose a reason for hiding this comment

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

Restored

@slandelle
Copy link
Contributor

slandelle commented Aug 27, 2018 •
edited

@tranchitam Did you get a chance to have a look at my comments. The idea is to be able to override the global CookieStore with one scoped at request level. Those 2 are exclusive and only one should be updated.

@slandelle slandelle force-pushed the master branch 5 times, most recently from 6ea11f4 to f8fab66 Compare February 7, 2020 12:25
@tranchitam
Copy link
Author

Hi @slandelle, I already updated the pull request to fix your comments. Please take a look. Thanks

@mkurz
Copy link

mkurz commented May 21, 2020

@slandelle Can you have a look?

@mkurz
Copy link

mkurz commented May 21, 2020

Also there is #1610 now, however seems liks it's just re-using the commits from this pull request here.

@TomGranot TomGranot added this to To do in Triage Mar 17, 2021
@TomGranot TomGranot moved this from Triage to In progress in Triage Mar 17, 2021
@leomaiwego
Copy link

@slandelle Could you please take a look to this one?

@tranchitam
Copy link
Author

Hi @slandelle, @TomGranot have you got time to take a look into this pull request again? If there is no issue, we really want to merge this PR to master as soon as possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slandelle slandelle slandelle requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
Triage
  
In progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@tranchitam @slandelle @mkurz @leomaiwego