HttpRequest.__deepcopy__ doesn't deepcopy attributes

[Adam Johnson]

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.

[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 ​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.

[Mariusz Felisiak]

Adam, Would you like to revert both? (d7f5bfd241666c0a76e90208da1e9ef81aec44db and 6220c445c40a6a7f4d442de8bde2628346153963.)

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

[Mariusz Felisiak]

​PR

[Mariusz Felisiak <felisiak.mariusz@…>]

In 280ca147:

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.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 2feb933:

[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