Combine cookies from original request and session file #932
Conversation
gmelodie
force-pushed
the
combine-original-and-new-cookies
branch
from
bda90d1
to
d58e1cd
Compare
| @@ -79,6 +81,12 @@ def update_headers(self, request_headers: RequestHeadersDict): | |||
| if name == 'User-Agent' and value.startswith('HTTPie/'): | |||
| continue | |||
|
|
|||
| if name.lower() == 'cookie': | |||
| for cookie_name, morsel in SimpleCookie(value).items(): | |||
| self['cookies'][cookie_name] = {'value': morsel.value} | |||
There was a problem hiding this comment.
What is missing now is to define & document & test priority rules. With this implementation, request cookies seem to win over response cookies. But it feels like it should be the other way, i.e., last seen cookies should be the ones that get stored in the session. What do you think?
There was a problem hiding this comment.
We added a new test, test_cookie_storage_priority in TestSessions to define and document the priority rules! Is this sufficient, or should there also be documentation in the docs?
There was a problem hiding this comment.
After our call this morning, @gmelodie and I discussed the possible behaviours of cookies expiring in sessions and which cookies should take precedent.
Our understanding was regardless of the expiry behaviour, the storage priority should still be server set > command line set > session file. For that reason, we didn't write further related tests. Here's a couple of example cases illustrating why we didn't think further tests were necessary:
- Case 1: Server expires a cookie. If a server expires a cookie, it will be removed from the session file regardless of how it was set (
test_sessions.pyalready has test covering this) - Case 2: Cookies in session file expire. Cookies stored in a session file that expire will never be sent to the server and will be removed when the request is made
- Case 3: User sets an expired cookie in the command line. We haven't been able to find a way for a user to set an expired cookie in the command line. However, if one is set, we would assume the intention would be to remove the cookie from the session. As this cookie would replace the cookie in the session file, and expired cookies are cleared from the session file when the request is sent, this behaviour is satisfied.
There was a problem hiding this comment.
Our understanding was regardless of the expiry behaviour, the storage priority should still be server set > command line set > session file.
Agree 👍🏻
kbanc
force-pushed
the
combine-original-and-new-cookies
branch
from
dd36cf2
to
5b98e4a
Compare
|
Hey @jakubroztocil we worked on some refactoring today. We added what you mentioned on Obs: we were trying out some git stuff so we apologize for the commits not being the best. We'll be working to fix that later today |
gmelodie
force-pushed
the
combine-original-and-new-cookies
branch
from
5b98e4a
to
82ca7f1
Compare
kbanc
force-pushed
the
combine-original-and-new-cookies
branch
from
82ca7f1
to
06af281
Compare
| 3. Manually set cookie parameters in the json file of the session | ||
|
|
||
| .. code-block:: json | ||
|
|
There was a problem hiding this comment.
We tried to emphasize lines 12-18 in this code block using :emphasize-lines:, but we weren't able to get it to render properly.
This pull request should address #824. Cookies that are set through a request in the CLI are added to the session cookies. @gmelodie