Support str and datetime on expires parameter on the set_cookie method

[oskipa]

• Closes Wrong typing for expires in set_cookie #1878

What?

The PR implements the type changes suggested in the discussion. We are adding typing.Optional[typing.Union[datetime, str] to set cookie.

Why?

Expiration was previously set as an integer. Reviewing https://www.rfc-editor.org/rfc/rfc6265 and https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie doesn't show that an integer is the right type for a cookie date.

How?

Following the instructions given in the discussion.

Notes

This is my first PR to this project, as part of Hacktober fest :)

Edit by @Kludex :

Missing

• Update documentation.
• Add test with naive timezone.

[Kludex]

One test is enough. Make sure to pass a datetime, and assert the cookie has the format specified.

[oskipa]

The build is failing on some database test. The tests pass locally. I don't think it is related to the changes I made, but if it is, please let me know, and I can try to figure out what is going on to fix it.

I saw another PR that has the same database error.

[oskipa]

The tests are passing now :)

[Kludex]

Cool. Thank you.

I just have one doubt here... Why does being an int works on Chrome? Does it work on other browsers as well?

[oskipa]

Cool. Thank you.

I just have one doubt here... Why does being an int works on Chrome? Does it work on other browsers as well?

Let me investigate that this morning

[oskipa]

Yes, int works in Firefox too. This behavior doesn't match the behavior described in https://www.rfc-editor.org/rfc/rfc6265#section-5.1.1 It does match the max-age behavior, which does take an integer that represents the seconds which the cookie is allowed to live.

My guess is that at some point browser programmers thought that max-age and expires were semantically the same, and allowed both integers and dates to set one or the other. In Firefox, dev tools shows expires as Expires/Max-Age. Then, although not part of the RFC, it seems to be a de-facto standard, either because the other browsers implement it that way, or because it was implemented this way years ago and no one touches the cookie code since it works.

[oskipa]

Is there anything else to do?

[Kludex]

Actually, I've found out why we accept int. It's because of how SimpleCookie is implemented in CPython:

https://github.com/python/cpython/blob/921f2353675ded86899c6dfdfc1c5c57bb998638/Lib/http/cookies.py#L408

werkzeug implements this logic like this: https://github.com/pallets/werkzeug/blob/3115aa6a6276939f5fd6efa46282e0256ff21f1a/src/werkzeug/http.py#L1228-L1230C29

Just some references.

[Kludex]

If no one has anything against, I'm going to merge this when the release is ready.

[iudeen]

Looks fine to me.

[Kludex]

@florimondmanca I tried on this commit 6db2870

But it was flaky... Do you mean something different?

[florimondmanca]

@Kludex I played with it locally. I feel like pytest's monkeypatch is just enough for our use case here, not really a need for a full-fledged extra library?

Diff:

diff --git a/requirements.txt b/requirements.txt
index 500a9b5..15aa541 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -9,7 +9,6 @@ flake8==3.9.2
 importlib-metadata==4.13.0
 isort==5.10.1
 mypy==0.991
-time-machine==2.9.0
 typing_extensions==4.4.0
 types-contextvars==2.4.7
 types-PyYAML==6.0.12.3
diff --git a/tests/test_responses.py b/tests/test_responses.py
index bf54772..aa2f5c4 100644
--- a/tests/test_responses.py
+++ b/tests/test_responses.py
@@ -1,6 +1,6 @@
 import os
-from datetime import datetime, timedelta, timezone
-from email.utils import format_datetime
+import datetime as dt
+import time
 from http.cookies import SimpleCookie
 
 import anyio
@@ -291,8 +291,10 @@ def test_file_response_with_inline_disposition(tmpdir, test_client_factory):
     assert response.headers["content-disposition"] == expected_disposition
 
 
-def test_set_cookie(test_client_factory, time_machine):
-    time_machine.move_to(datetime(2100, 1, 22, 12, 0, 0))
+def test_set_cookie(test_client_factory, monkeypatch):
+    # Mock time used as a reference for `Expires` by stdlib `SimpleCookie`.
+    mocked_now = dt.datetime(2100, 1, 22, 12, 0, 0, tzinfo=dt.timezone.utc)
+    monkeypatch.setattr(time, "time", lambda: mocked_now.timestamp())
 
     async def app(scope, receive, send):
         response = Response("Hello, world!", media_type="text/plain")
@@ -320,21 +322,23 @@ def test_set_cookie(test_client_factory, time_machine):
 
 
 @pytest.mark.parametrize(
-    "expires_factory",
+    "expires",
     [
-        lambda: datetime.now(timezone.utc) + timedelta(seconds=10),
-        lambda: format_datetime(
-            datetime.now(timezone.utc) + timedelta(seconds=10), usegmt=True
+        pytest.param(
+            dt.datetime(2100, 1, 22, 12, 0, 10, tzinfo=dt.timezone.utc), id="datetime"
         ),
-        lambda: 10,
+        pytest.param("Fri, 22 Jan 2100 12:00:10 GMT", id="str"),
+        pytest.param(10, id="int"),
     ],
 )
-def test_expires_on_set_cookie(test_client_factory, expires_factory, time_machine):
-    time_machine.move_to(datetime(2100, 1, 22, 12, 0, 0))
+def test_expires_on_set_cookie(test_client_factory, monkeypatch, expires):
+    # Mock time used as a reference for `Expires` by stdlib `SimpleCookie`.
+    mocked_now = dt.datetime(2100, 1, 22, 12, 0, 0, tzinfo=dt.timezone.utc)
+    monkeypatch.setattr(time, "time", lambda: mocked_now.timestamp())
 
     async def app(scope, receive, send):
         response = Response("Hello, world!", media_type="text/plain")
-        response.set_cookie("mycookie", "myvalue", expires=expires_factory())
+        response.set_cookie("mycookie", "myvalue", expires=expires)
         await response(scope, receive, send)
 
     client = test_client_factory(app)

[Kludex]

@Kludex I played with it locally. I feel like pytest's monkeypatch is just enough for our use case here, not really a need for a full-fledged extra library?

Diff:

diff --git a/requirements.txt b/requirements.txt
index 500a9b5..15aa541 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -9,7 +9,6 @@ flake8==3.9.2
 importlib-metadata==4.13.0
 isort==5.10.1
 mypy==0.991
-time-machine==2.9.0
 typing_extensions==4.4.0
 types-contextvars==2.4.7
 types-PyYAML==6.0.12.3
diff --git a/tests/test_responses.py b/tests/test_responses.py
index bf54772..aa2f5c4 100644
--- a/tests/test_responses.py
+++ b/tests/test_responses.py
@@ -1,6 +1,6 @@
 import os
-from datetime import datetime, timedelta, timezone
-from email.utils import format_datetime
+import datetime as dt
+import time
 from http.cookies import SimpleCookie
 
 import anyio
@@ -291,8 +291,10 @@ def test_file_response_with_inline_disposition(tmpdir, test_client_factory):
     assert response.headers["content-disposition"] == expected_disposition
 
 
-def test_set_cookie(test_client_factory, time_machine):
-    time_machine.move_to(datetime(2100, 1, 22, 12, 0, 0))
+def test_set_cookie(test_client_factory, monkeypatch):
+    # Mock time used as a reference for `Expires` by stdlib `SimpleCookie`.
+    mocked_now = dt.datetime(2100, 1, 22, 12, 0, 0, tzinfo=dt.timezone.utc)
+    monkeypatch.setattr(time, "time", lambda: mocked_now.timestamp())
 
     async def app(scope, receive, send):
         response = Response("Hello, world!", media_type="text/plain")
@@ -320,21 +322,23 @@ def test_set_cookie(test_client_factory, time_machine):
 
 
 @pytest.mark.parametrize(
-    "expires_factory",
+    "expires",
     [
-        lambda: datetime.now(timezone.utc) + timedelta(seconds=10),
-        lambda: format_datetime(
-            datetime.now(timezone.utc) + timedelta(seconds=10), usegmt=True
+        pytest.param(
+            dt.datetime(2100, 1, 22, 12, 0, 10, tzinfo=dt.timezone.utc), id="datetime"
         ),
-        lambda: 10,
+        pytest.param("Fri, 22 Jan 2100 12:00:10 GMT", id="str"),
+        pytest.param(10, id="int"),
     ],
 )
-def test_expires_on_set_cookie(test_client_factory, expires_factory, time_machine):
-    time_machine.move_to(datetime(2100, 1, 22, 12, 0, 0))
+def test_expires_on_set_cookie(test_client_factory, monkeypatch, expires):
+    # Mock time used as a reference for `Expires` by stdlib `SimpleCookie`.
+    mocked_now = dt.datetime(2100, 1, 22, 12, 0, 0, tzinfo=dt.timezone.utc)
+    monkeypatch.setattr(time, "time", lambda: mocked_now.timestamp())
 
     async def app(scope, receive, send):
         response = Response("Hello, world!", media_type="text/plain")
-        response.set_cookie("mycookie", "myvalue", expires=expires_factory())
+        response.set_cookie("mycookie", "myvalue", expires=expires)
         await response(scope, receive, send)
 
     client = test_client_factory(app)

I've applied this. Thanks!