Add persistent cookies option to Client

[MarkWine]

Summary

Closes #2992

Checklist

• I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
• I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
• I've updated the documentation accordingly.

[MarkWine]

Happy to change argument name in api implementation.

For tests, not explicitly testing the persistence as True option, but the existing persistence test fails if default set to False (as expected). Also not directly testing AsyncClient, but that is true for other cookie tests as well.

[rafalkrupinski]

Cool, the issue just came to me too.

I think the naming convention here would be persist_cookies, similar to follow_redirects. I'm asking the client to persist them, not informing it that I'm doing the persistence.
Parameters and fields are more or less logically grouped (data, config). Do you think moving it somewhere near follow_redirects would make sense?

Can you add an override parameter to all request methods (request, send, get, post, etc...), with the default value USE_CLIENT_DEFAULT?

You've added test for making a request without cookie persistence; is there a test that specifically tests cookie persistence? Ah, just found your comment on this. I think it's a good idea to add a test that specifically tests cookie persistence.

There's a deprecation warning about cookie persistence in Client.request. IMO with your changes it would no longer apply.

[rafalkrupinski]

Just a thought, doesn't persistence suggest that cookies survive the client itself? Like storing to a file?
How about merge_response_cookies?

[MarkWine]

Yeah, can make all those changes.

For the persistence on test, there is an existing one explicitly about persistence (named test_cookie_persistence). The client in that one is just set up like:

client = httpx.Client(transport=httpx.MockTransport(get_and_set_cookies))

Would it be better to explicitly add the parameter value to True, leave as default, or do both in different tests?

For the deprecation warning:

        if cookies is not None:
            message = (
                "Setting per-request cookies=<...> is being deprecated, because "
                "the expected behaviour on cookie persistence is ambiguous. Set "
                "cookies directly on the client instance instead."
            )
            warnings.warn(message, DeprecationWarning)

This should be removed? I'm not sure we fully remove the ambiguity referenced here, as this doesn't affect what happens when cookies are set at the request level. I might be misreading the intention behind this warning (although I would definitely note that reading this solidifies my agreement that something like merge_response_cookies is a better name than persist_cookies).

[rafalkrupinski]

Would it be better to explicitly add the parameter value to True, leave as default, or do both in different tests?

Just have a look how follow_redirects is tested.

For the deprecation warning:

You're right, that deprecation warning is related to keeping cookies on redirect, nothing to do with your patch. One more reason not to call this one persist_cookies :)

[MarkWine]

#3071 Given that cookie params are being removed from the request methods (following the deprecation warning noted above), should this option also only exist at the client level?

[rafalkrupinski]

I sure hope they don't :)

[rafalkrupinski]

I'm alright with the code, but it probably should be mentioned somewhere in the documentation.
I think right now the only mention is Cookie persistence across requests. in extra feature section.

I'm looking at follow_redirects documentation... it's covered in the quick start, but merge_response_cookies applies only to client usage, not covered there. So I guess it should go to advanced, a new section on cookies.

WDYT @tomchristie ?

[rafalkrupinski]

#3071 Given that cookie params are being removed from the request methods (following the deprecation warning noted above), should this option also only exist at the client level?

Come to think about it, even if per-request cookies were removed, it still made sense to include this feature.
The flag merge_response_cookies tells whether the response object is allowed to modify the client state or not, at least regarding the cookies.

[rafalkrupinski]

BTW
#1533
#2271

One mentioned workaround is to pass custom no-op CookieJar. I guess it forbids any Cookies, unless you set them manually in headers.

[MarkWine]

I added an internal pull request with all the method overrides. It's a bit on the big/boring side because of all the public methods available. I can merge if it looks good.

MarkWine#1

Not sure on documentation. It might be an esoteric enough option that for now the client docstrings might be enough? I can take a shot if new section is warranted, or probably defer to someone more familiar with the documentation style for httpx.