Create Set-Cookie header from the cookie jar (fix #17) #143
Conversation
There was a problem hiding this comment.
- Revert Add known issue for cookies in Sessions #99
- Please add a release note. (
pip install -r rtfd-requirements; reno new set-session-cookiesor whatever for the name).
Hey,
Thanks for the PR. I'm particularly interested here if this was something you have hit as a limitation, or came across it looking for a bug to fix? The thing that has always concerned me about adding cookies to the session is that the order of your unit tests can become a little unstable.
I've always said that requests-mock should be order independent and that if you were looking to test a big sequence you'd be doing integration testing, and this means that cookies set on the session will now affect subsequent calls.
However, it's been a long time since i've looked at this particular code, and I honestly can't remember why. It's not about requests-mock, it's about how requests handles cookies, and you should expect it to work based on the calls you make, so I can be convinced on the change.
I'm trying to think if i'd consider this a breaking change. I don't think so, but it would definitely need a release note. I can make these changes and merge it if you just want it in, let me know.
Thanks again. Sorry it's taken a while.
| """ | ||
| :param requests.packages.urllib3.response response: the Response that will | ||
| be updated with the Set-Cookie header | ||
| :param requests.cookies.RequestsCookieJar jar: a CookieJar containing all |
There was a problem hiding this comment.
jar would not appear to be used in function.
I'd say you may as well merge this with the _extract_cookie function? it's basically extending that logic. Otherwise i'd have said please add a _, everything else is explicitly marked private.
|
|
||
|
|
||
| class SessionCookiesTest(base.TestCase): | ||
| def test_cookie(self): |
There was a problem hiding this comment.
Another test is required that contains two cookies, as the behavior only seems to work for one cookie. See #17 (comment)
There was a problem hiding this comment.
This answer adds some insight, indicating to me that semicolon separated values aren't appropriate for Set-Cookie. I tried comma-separated values, but that didn't work either... and I've not found a way to indicate more than one Set-Cookie header (because response.headers is a dict).
Hi!
This PR hopefully fixes issue #17 by manually creating the
Set-CookieHTTP header in thecreate_responsefunction.I also had to create
compat._FakeHTTPResponsein order to make Requests happy: as you mentioned in the issue,extract_cookies_to_jarworks by accessingHTTPResponse._original_response, which wasn't populated.I also added a test which makes sure that cookies are propagated down to the Session object.
Let me know what you think :)