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
Support cookies with value None
#3096
base: master
Are you sure you want to change the base?
Conversation
@@ -1008,7 +1008,7 @@ async def aclose(self) -> None: | |||
await self.stream.aclose() | |||
|
|||
|
|||
class Cookies(typing.MutableMapping[str, str]): | |||
class Cookies(typing.MutableMapping[str, typing.Optional[str]]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class Cookies(typing.MutableMapping[str, typing.Optional[str]]): | |
class Cookies(typing.MutableMapping[str, str | None]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, no problem then...
Is there anything else I can do before we merge it to the main branch? |
Do |
According to the RFC for HTTP protocol :
So it may depend on the server implementation, but I can confirm a case where the server considered |
0ce0b2b
to
a97599a
Compare
@@ -1080,23 +1082,26 @@ def get( # type: ignore | |||
default: str | None = None, | |||
domain: str | None = None, | |||
path: str | None = None, | |||
raise_when_not_found: bool = False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we avoid adding this API as part of the pull request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, I wanted to preserve this responsibility of raising the KeyError
exception to the __getitem__()
method.
However, if it's safe to raise it whenever it's not found through get()
, then we could remove raise_when_not_found
parameter.
What do you think?
tests/models/test_cookies.py
Outdated
|
||
cookies.update(more_cookies) | ||
assert dict(cookies) == {"no-value": None} | ||
assert cookies.get("no-value", domain="example.com") is None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not absolutely clear to me that allowing None
here is an API improvement. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the test to assert cookies["no-value"] is None
.
I also added test_get_missing_cookie_raises_exception
to highlight the difference between getting a missing cookie (raises exception) and getting a cookie whose value is None
.
Is it better?
It highlights the difference with a cookie whose value is `None`.
Summary
There are 3 kinds of request cookie headers.
"name=value"
This is the classic case, already supported through :
"name="
Please note the trailing
=
sign here.This is also supported, by passing an empty string as the cookie value :
"name"
Please note there is no trailing
=
sign here.This is not officially supported (yet), but can be achieved with :
It works because the cookie jar implementation in the standard library already deals with it.
Contrary to the
httpx.Cookies.set
method, the standardCookie
doesn't define strict typing for thevalue
.So this PR changes the
httpx._models
in order to support a cookievalue
which isNone
.Otherwise the typing is incorrect, forcing the
httpx
to add a# type: ignore
instruction.The getter functions had to be updated as well, as the
None
was considered as a not found value.2 new test cases are added in this PR to ensure the proper behavior.
Checklist