[Django] #34484: HttpRequest.__deepcopy__ doesn't deepcopy attributes
Django
-------------------------------------------+------------------------
Reporter: Adam Johnson | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: 4.2
Severity: Release blocker | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------------+------------------------
Regression in Django 4.2. Deepcopying a HttpRequest no longer deepcopies
its attributes, including attached ones like `session`.
Leads to test pollution where a request is created in setUpTestData, for
example:
{{{
from django.test import TestCase
from django.test import RequestFactory
class ExampleTests(TestCase):
@classmethod
def setUpTestData(cls):
cls.request = RequestFactory().get("/")
cls.request.session = {}
def test_adding(self):
self.request.session["foo"] = 1
self.assertEqual(self.request.session, {"foo": 1})
def test_looking(self):
self.assertEqual(self.request.session, {})
}}}
(Simplified from a real test suite.)
Bisected to #29186 / 6220c445c40a6a7f4d442de8bde2628346153963, using these
commands to run the above test case:
{{{
git bisect start facc153af7 ff8e5eacda
git bisect run sh -c 'cd tests && ./runtests.py test_regression -v 2'
}}}
I see #34482 was also just opened as a regression from that same ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/34484>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Adam Johnson | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: 4.2
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by Adam Johnson:
Old description:
> Regression in Django 4.2. Deepcopying a HttpRequest no longer deepcopies
> its attributes, including attached ones like `session`.
>
> Leads to test pollution where a request is created in setUpTestData, for
> example:
>
> {{{
> from django.test import TestCase
> from django.test import RequestFactory
>
> class ExampleTests(TestCase):
> @classmethod
> def setUpTestData(cls):
> cls.request = RequestFactory().get("/")
> cls.request.session = {}
>
> def test_adding(self):
> self.request.session["foo"] = 1
> self.assertEqual(self.request.session, {"foo": 1})
>
> def test_looking(self):
> self.assertEqual(self.request.session, {})
> }}}
>
> (Simplified from a real test suite.)
>
> Bisected to #29186 / 6220c445c40a6a7f4d442de8bde2628346153963, using
> these commands to run the above test case:
>
> {{{
> git bisect start facc153af7 ff8e5eacda
> git bisect run sh -c 'cd tests && ./runtests.py test_regression -v 2'
> }}}
>
> I see #34482 was also just opened as a regression from that same ticket.
New description:
Regression in Django 4.2. Deepcopying a HttpRequest no longer deepcopies
its attributes, including attached ones like `session`.
Leads to test pollution where a request is created in setUpTestData, for
example:
{{{
from django.test import TestCase
from django.test import RequestFactory
class ExampleTests(TestCase):
@classmethod
def setUpTestData(cls):
cls.request = RequestFactory().get("/")
cls.request.session = {}
def test_adding(self):
self.request.session["foo"] = 1
self.assertEqual(self.request.session, {"foo": 1})
def test_looking(self):
self.assertEqual(self.request.session, {})
}}}
Leading to:
{{{
test_adding (test_regression.ExampleTests.test_adding) ... ok
test_looking (test_regression.ExampleTests.test_looking) ... FAIL
======================================================================
FAIL: test_looking (test_regression.ExampleTests.test_looking)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/chainz/Documents/Projects/django/tests/test_regression.py",
line 16, in test_looking
self.assertEqual(self.request.session, {})
AssertionError: {'foo': 1} != {}
- {'foo': 1}
+ {}
}}}
(Simplified from a real test suite.)
Bisected to #29186 / 6220c445c40a6a7f4d442de8bde2628346153963, using these
commands to run the above test case:
{{{
git bisect start facc153af7 ff8e5eacda
git bisect run sh -c 'cd tests && ./runtests.py test_regression -v 2'
}}}
I see #34482 was also just opened as a regression from that same ticket.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/34484#comment:1>
Django
---------------------------------+--------------------------------------
Reporter: Adam Johnson | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: 4.2
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+--------------------------------------
Comment (by Adam Johnson):
I think there are a couple of issues in the original patch, which is why
we have two tickets:
(1) `__getstate__` completely drops attributes, so unpickled request
objects end up missing attributes, hence #34482. Dropping attributes
entirely in `__getstate__` should only be done where the interface of the
class works with a missing private attribute, otherwise unpickling is
broken.
(2) `__deepcopy__` performs a shallow copy of the object, hence this
ticket. It looks like this was added after
[https://github.com/django/django/pull/15937#issuecomment-1217486491 this
comment from Mariusz] during review, in order to fix test failures. I
think in reality the failures were exposing problems in the `__getstate__`
implementation, and adding a `__deepcopy__` only papered over them.
I reproduced the failure Mariusz talked about in that comment, by removing
`__deepcopy__`:
{{{
======================================================================
ERROR: test_password_reset_change_view
(auth_tests.test_templates.AuthTemplateTests.test_password_reset_change_view)
----------------------------------------------------------------------
Traceback (most recent call last):
...
File
"/Users/chainz/Documents/Projects/django/tests/auth_tests/test_templates.py",
line 115, in test_password_reset_change_view
response =
PasswordChangeView.as_view(success_url="dummy/")(self.request)
^^^^^^^^^^^^^^^^^
...
csrf_secret = request.COOKIES[settings.CSRF_COOKIE_NAME]
^^^^^^^^^^^^^^^^^
File
"/Users/chainz/Documents/Projects/django/django/utils/functional.py", line
57, in __get__
res = instance.__dict__[self.name] = self.func(instance)
^^^^^^^^^^^^^^^^^
File
"/Users/chainz/Documents/Projects/django/django/core/handlers/wsgi.py",
line 111, in COOKIES
raw_cookie = get_str_from_wsgi(self.environ, "HTTP_COOKIE", "")
^^^^^^^^^^^^^^^^^
AttributeError: 'WSGIRequest' object has no attribute 'environ'
}}}
A missing attribute is exactly the problem as in (1).
I lean towards reverting the patch, and then working on a new one. This
stuff is hard to get right, and it would be better to work on it calmly
rather than rushing through a fix. I'll also note the patch also missed
tests for `WSGIRequest` and `ASGIRequest`.
--
Ticket URL: <https://code.djangoproject.com/ticket/34484#comment:2>
Django
---------------------------------+--------------------------------------
Reporter: Adam Johnson | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: 4.2
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+--------------------------------------
Comment (by Mariusz Felisiak):
Adam, Would you like to revert both?
(d7f5bfd241666c0a76e90208da1e9ef81aec44db and
6220c445c40a6a7f4d442de8bde2628346153963.)
--
Ticket URL: <https://code.djangoproject.com/ticket/34484#comment:3>
Django
---------------------------------+--------------------------------------
Reporter: Adam Johnson | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: 4.2
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+--------------------------------------
Comment (by Adam Johnson):
Maybe...
On the HttpResponse change, it does seem quite hacky to have HttpResponse
know about the attributes the test client adds and remove them at pickle
time. It will cause issues in tests that end up copying responses and
expect the attributes to still exist.
There are definitely tests out there that make requests within setUp and
store the responses for assertions within actual test methods - this is a
pattern that Will Vincent promotes in his books... The issue will occur if
such a test is converted to use setUpTestData, which can easily happen
since it's promoted for speed.
--
Ticket URL: <https://code.djangoproject.com/ticket/34484#comment:4>
Django
---------------------------------+------------------------------------
Type: Bug | Status: new
Component: HTTP handling | Version: 4.2
Severity: Release blocker | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/34484#comment:5>
Django
Reporter: Adam Johnson | Owner: Mariusz
| Felisiak
Type: Bug | Status: assigned
Component: HTTP handling | Version: 4.2
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* owner: nobody => Mariusz Felisiak
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/34484#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: Mariusz
| Felisiak
Type: Bug | Status: assigned
Component: HTTP handling | Version: 4.2
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Mariusz Felisiak):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/16755 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/34484#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: Mariusz
| Felisiak
Component: HTTP handling | Version: 4.2
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"280ca147af9cdfce1ca9cb14cc3c5527ff6c7a02" 280ca147]:
{{{
#!CommitTicketReference repository=""
revision="280ca147af9cdfce1ca9cb14cc3c5527ff6c7a02"
Fixed #34484, Refs #34482 -- Reverted "Fixed #29186 -- Fixed pickling
HttpRequest and subclasses."
This reverts commit 6220c445c40a6a7f4d442de8bde2628346153963.
Thanks Adam Johnson and Márton Salomváry for reports.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34484#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: Mariusz
| Felisiak
Type: Bug | Status: closed
Component: HTTP handling | Version: 4.2
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"2feb9333e7044df348c45417d23ee20b08c1a7fd" 2feb933]:
{{{
#!CommitTicketReference repository=""
revision="2feb9333e7044df348c45417d23ee20b08c1a7fd"
[4.2.x] Fixed #34484, Refs #34482 -- Reverted "Fixed #29186 -- Fixed
pickling HttpRequest and subclasses."
This reverts commit 6220c445c40a6a7f4d442de8bde2628346153963.
Thanks Adam Johnson and Márton Salomváry for reports.
Backport of 280ca147af9cdfce1ca9cb14cc3c5527ff6c7a02 from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34484#comment:9>