FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Feature/enable cookie store at request level by tranchitam · Pull Request #1567 · AsyncHttpClient/async-http-client · GitHub

/ async-http-client Public

Feature/enable cookie store at request level - #1567

Open
tranchitam wants to merge 4 commits into
AsyncHttpClient:mainfrom
tranchitam:feature/enable-cookie-store-at-request-level
ConversationCommits (4)ChecksFiles changed
Open

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

Conversation

tranchitam commented Jul 31, 2018 •
edited
Loading

Copy link
Copy Markdown

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 mentioned this pull request Jul 31, 2018
Open
slandelle suggested changes Aug 20, 2018
View reviewed changes
Comment thread 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();

slandelle Aug 20, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

tranchitam Apr 30, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I already updated.

Comment thread 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) {

slandelle Aug 20, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Please restore

tranchitam Apr 30, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Restored

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

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

slandelle Aug 20, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Please restore

tranchitam Apr 30, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Restored

slandelle commented Aug 27, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

@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 force-pushed the master branch 5 times, most recently from 6ea11f4 to f8fab66 Compare February 7, 2020 12:25

tranchitam commented Apr 30, 2020

Copy link
Copy Markdown
Author

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

mkurz commented May 21, 2020

Copy link
Copy Markdown

@slandelle Can you have a look?

mkurz commented May 21, 2020

Copy link
Copy Markdown

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

leomaiwego commented Feb 14, 2022

Copy link
Copy Markdown

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

tranchitam commented Mar 3, 2022

Copy link
Copy Markdown
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.

hyperxpro force-pushed the main branch 2 times, most recently from c6640bd to 83e552e Compare August 9, 2026 22:30
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
slandelle slandelle requested changes
Reviewers whose approvals may not affect merge requirements

At least 0 approving reviews are 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.

5 participants

Back | FazBrowse Home | New Git URL