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

[CookieStore] Only set Cookies if they are not already set #2033

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

[CookieStore] Only set Cookies if they are not already set #2033

wants to merge 3 commits into from
+55 −9

Conversation

pickypg
Copy link

@pickypg pickypg commented Nov 14, 2024

This changes the behavior of the automatic usage of the CookieStore to avoid overwriting already-set Cookies and, instead only sets them if they do not exist yet.

Closes #1964

@pickypg
[CookieStore] Only set Cookies if they are not already set
1cc44f8 
This changes the behavior of the automatic usage of the
`CookieStore` to avoid overwriting already-set `Cookie`s and,
instead only sets them if they do not exist yet.
@pickypg pickypg mentioned this pull request Nov 14, 2024
Open
@hyperxpro
Copy link
Member

I'll take a look this weekend.

@hyperxpro
Copy link
Member

Sadly this is a breaking change. RequestBuilderBase is an abstract class and adding anything will break compatibility. Perhaps, you might want to take a different approach.

@pickypg
Copy link
Author

pickypg commented Nov 16, 2024

I'm happy to if you have a suggestion? The current functionality is buggy given that it overrides explicitly provided cookies.

@hyperxpro
Copy link
Member

Let me try something

@hyperxpro
Copy link
Member

I'd say change this directly in RequestBuilder. People extending RequestBuilderBase will be safe from this breaking change.

Copilot
Copilot AI reviewed Nov 23, 2024
View reviewed changes

Choose a reason for hiding this comment

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

Copilot reviewed 4 out of 4 changed files in this pull request and generated no suggestions.
@pickypg
Copy link
Author

pickypg commented Nov 23, 2024

That's doable, but this breaking change is clearly necessary.

@hyperxpro
Copy link
Member

Yup, I do feel this but as a library maintainer, I cannot. I am at fault, I should have made that as interface. But doing with directly changing should be best because people extending RequestBuilderBase should always take extra care of their custom implementations.

@hyperxpro
Copy link
Member

Fire this up so I can merge and cut a release.

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

Copilot Copilot left review comments

@hyperxpro hyperxpro hyperxpro 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.

Cookie explicitly set on the request is getting discarded in favor of the one from the cookie jar
2 participants
@pickypg @hyperxpro