Skip to content

Feature/enable cookie store at request level#1567

Open
tranchitam wants to merge 4 commits intoAsyncHttpClient:mainfrom
tranchitam:feature/enable-cookie-store-at-request-level
Open

Feature/enable cookie store at request level#1567
tranchitam wants to merge 4 commits intoAsyncHttpClient:mainfrom
tranchitam:feature/enable-cookie-store-at-request-level

Conversation

@tranchitam
Copy link

@tranchitam tranchitam commented Jul 31, 2018 •
edited
Loading

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 suggested 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

@slandelle slandelle Aug 20, 2018

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

@tranchitam tranchitam Apr 30, 2020

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
CookieStore cookieStore = config.getCookieStore();
if (cookieStore != null) {
CookieStore requestCookieStore = request.getCookieStore();
if (cookieStore != null || requestCookieStore != null) {
Copy link
Contributor

@slandelle slandelle Aug 20, 2018

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

@tranchitam tranchitam Apr 30, 2020

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

@slandelle slandelle Aug 20, 2018

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

@tranchitam tranchitam Apr 30, 2020

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
Loading

@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

tranchitam commented Apr 30, 2020

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.

@leomaiwego
Copy link

leomaiwego commented Feb 14, 2022

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

@tranchitam
Copy link
Author

tranchitam commented Mar 3, 2022

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

1 more reviewer

@slandelle slandelle slandelle requested changes

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tranchitam @slandelle @mkurz @leomaiwego