Opened 12 months ago

Closed 12 months ago

Last modified 12 months ago

#34484 closed Bug (fixed)

HttpRequest.__deepcopy__ doesn't deepcopy attributes

Reported by: Adam Johnson Owned by: Mariusz Felisiak
Component: HTTP handling Version: 4.2
Severity: Release blocker Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by 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.

Change History (9)

comment:1 by Adam Johnson, 12 months ago

Description: modified (diff)

comment:2 by Adam Johnson, 12 months ago

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.

comment:3 by Mariusz Felisiak, 12 months ago

comment:4 by Adam Johnson, 12 months ago

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.

comment:5 by Mariusz Felisiak, 12 months ago

Triage Stage: UnreviewedAccepted

comment:6 by Mariusz Felisiak, 12 months ago

Owner: changed from nobody to Mariusz Felisiak
Status: newassigned

comment:7 by Mariusz Felisiak, 12 months ago

Has patch: set

comment:8 by Mariusz Felisiak <felisiak.mariusz@…>, 12 months ago

Resolution: fixed
Status: assignedclosed

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.

comment:9 by Mariusz Felisiak <felisiak.mariusz@…>, 12 months ago

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

Note: See TracTickets for help on using tickets.
Back to Top