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

Unintuive Cookie API on ClientBuilder #2105

Closed
DerTiedemann opened this issue Jan 22, 2024 · 1 comment
Closed

Unintuive Cookie API on ClientBuilder #2105

DerTiedemann opened this issue Jan 22, 2024 · 1 comment

Comments

@DerTiedemann
Copy link

Description

When building a Client that is supposed to interact with a server that makes use of cookies you have to enable the cookie feature and either:

  • call cookie_store(true) on the client builder -> this will internally create a new cookie store regardless if a store is already present. see the code here.
  • call cookie_provider(...) on the client builder with an Arc<dyn CookieStore> usually a cookie jar from the cookie_store crate. -> this will set the internal cookie store to the one given as an arguement. see the code here.

The issue now arises from the possible ordering of operations on the builder. You can accedentailly overwrite your own set cookie store by activating a cookie store. This behavior is not documented and was at least to me very confusing.

Possible Solutions

  • Add documentation to the cookie_store() method to make clear that this will create a new cookie store and overwrite any existing one.
  • Adapt the cookie_store() method to respect any existing cookie stores, not overwriting them. Though this would be an API change, no idea how many ppl are relying on this behavior.

Previous Work

#1104

@pfernie
Copy link
Contributor

pfernie commented Jan 24, 2024

I agree the documentation/API here is not very clear. Ideally, I think we would have one method taking some enum for disabling/enabling/or setting the store, but that would be an API change. Making the methods fallible (Err if the store isset multiple times) would also be an API change. #2111 attempts to clarify the documentation, instead.

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

No branches or pull requests

3 participants