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

Adding client-level cookie container #2042

Merged
merged 3 commits into from
Apr 3, 2023
Merged

Conversation

alexeyzimarev
Copy link
Member

  • Added a custom cookie container to RestClientOptions
  • It will not be added to the message handler
  • Cookies will be added as headers the same way as request cookies

@kendallb
Copy link
Contributor

kendallb commented Apr 1, 2023

My concern would be adding the cookies to the container in the request because if the user is using a request specific container that they are maintaining, it means the client ones will end up in that container after the request.

cookieContainer.Add(Options.CookieContainer.GetCookies(url));

Is it feasible to simply call headers.AddCookieHeaders(cookieContainer, url) for both cookie containers rather than combine them?

@alexeyzimarev
Copy link
Member Author

Yeah, I thought about that as one of the options, but I haven't considered the request container concern. You are right, will change it now.

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Apr 1, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: c121623
Status: ✅  Deploy successful!
Preview URL: https://92532ab6.restsharp.pages.dev
Branch Preview URL: https://client-level--cookies.restsharp.pages.dev

View logs

@alexeyzimarev
Copy link
Member Author

@kendallb I updated it following your comment.

@kendallb
Copy link
Contributor

kendallb commented Apr 1, 2023

Looks good to me

@alexeyzimarev alexeyzimarev merged commit 4287eaf into dev Apr 3, 2023
@repo-ranger repo-ranger bot deleted the client-level_-cookies branch April 3, 2023 09:51
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.

2 participants