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

Support cookies with value None #3096

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 14 additions & 12 deletions httpx/_models.py
Expand Up @@ -1010,7 +1010,7 @@ async def aclose(self) -> None:
await self.stream.aclose()


class Cookies(typing.MutableMapping[str, str]):
class Cookies(typing.MutableMapping[str, typing.Optional[str]]):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
class Cookies(typing.MutableMapping[str, typing.Optional[str]]):
class Cookies(typing.MutableMapping[str, str | None]):

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @T-256 ! Actually, That was my initial attempt, but the test workflow failed with this notation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, no problem then...

"""
HTTP Cookies, as a mutable mapping.
"""
Expand Down Expand Up @@ -1048,7 +1048,9 @@ def set_cookie_header(self, request: Request) -> None:
urllib_request = self._CookieCompatRequest(request)
self.jar.add_cookie_header(urllib_request)

def set(self, name: str, value: str, domain: str = "", path: str = "/") -> None:
def set(
self, name: str, value: str | None, domain: str = "", path: str = "/"
) -> None:
"""
Set a cookie value by name. May optionally include domain and path.
"""
Expand Down Expand Up @@ -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,
Copy link
Member

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?

Copy link
Author

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?

) -> str | None:
"""
Get a cookie by name. May optionally include domain and path
in order to specify exactly which cookie to retrieve.
"""
value = None
value = default
found = False
for cookie in self.jar:
if cookie.name == name:
if domain is None or cookie.domain == domain:
if path is None or cookie.path == path:
if value is not None:
if found:
message = f"Multiple cookies exist with name={name}"
raise CookieConflict(message)
value = cookie.value
found = True

if value is None:
return default
if not found and raise_when_not_found:
raise KeyError(name)
return value

def delete(
Expand Down Expand Up @@ -1141,14 +1146,11 @@ def update(self, cookies: CookieTypes | None = None) -> None: # type: ignore
for cookie in cookies.jar:
self.jar.set_cookie(cookie)

def __setitem__(self, name: str, value: str) -> None:
def __setitem__(self, name: str, value: str | None) -> None:
return self.set(name, value)

def __getitem__(self, name: str) -> str:
value = self.get(name)
if value is None:
raise KeyError(name)
return value
def __getitem__(self, name: str) -> str | None:
return self.get(name, raise_when_not_found=True)

def __delitem__(self, name: str) -> None:
return self.delete(name)
Expand Down
22 changes: 21 additions & 1 deletion tests/models/test_cookies.py
Expand Up @@ -30,6 +30,16 @@ def test_cookies_update():
assert cookies.get("name", domain="example.com") == "value"


def test_cookie_value_can_be_none():
cookies = httpx.Cookies()
more_cookies = httpx.Cookies()
more_cookies.set("no-value", None, domain="example.com")

cookies.update(more_cookies)
assert dict(cookies) == {"no-value": None}
assert cookies.get("no-value", domain="example.com") is None
Copy link
Member

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. 🤔

Copy link
Author

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?



def test_cookies_with_domain():
cookies = httpx.Cookies()
cookies.set("name", "value", domain="example.com")
Expand Down Expand Up @@ -91,8 +101,18 @@ def test_cookies_repr():
cookies = httpx.Cookies()
cookies.set(name="foo", value="bar", domain="http://blah.com")
cookies.set(name="fizz", value="buzz", domain="http://hello.com")
cookies.set(name="only-name", value=None, domain="http://hello.com")

assert repr(cookies) == (
"<Cookies[<Cookie foo=bar for http://blah.com />,"
" <Cookie fizz=buzz for http://hello.com />]>"
" <Cookie fizz=buzz for http://hello.com />,"
" <Cookie only-name=None for http://hello.com />]>"
)


def test_cookie_headers():
cookies = httpx.Cookies()
cookies.set("foo", "bar", domain="example.org")
cookies.set("no-value", None, domain="example.org")
request = httpx.Request("GET", "https://www.example.org", cookies=cookies)
assert request.headers["cookie"] == "foo=bar; no-value"