Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#26122 closed Bug (fixed)

copy.copy() broken with SimpleLazyObject in 1.8.5

Reported by: Tom Carrick Owned by: Ben Kraft
Component: Utilities Version: 1.8
Severity: Release blocker Keywords:
Cc: Ben Kraft 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 Tom Carrick)

Smallest example I could get to:

# forms.py
from django import forms
from django.contrib.auth import get_user_model


class InlineEditEmailForm(forms.ModelForm):
    class Meta:
        model = get_user_model()
        fields = ['email']


# views.py
import copy

from django.http import HttpResponse

from .forms import InlineEditEmailForm


def edit_user_email(request):
    user = request.user
    old_user_data = copy.copy(user)
    form = InlineEditEmailForm(data=request.POST, instance=user)
    if form.is_valid():
        new_user_data = form.cleaned_data
        user = form.save()

        if old_user_data.email != new_user_data['email']:
            response = HttpResponse()
            response.status_code = 200
            return response

    response = HttpResponse()
    response.status_code = 400
    return response


# tests.py
from django.contrib.auth import get_user_model
from django.test import TestCase


class EditUserEmailTestCase(TestCase):

    def test_post(self):
        get_user_model().objects.create_user('test', 'x@example.com', 'pw')
        data = {'email': 'test@example.com'}
        self.client.login(username='test', password='pw')
        response = self.client.post('/', data)
        self.assertEqual(response.status_code, 200)

  • Works in 1.8.4, doesn't work in 1.8.5+ or 1.9.
  • Works when using a browser, only fails during tests.
  • Changing first line of the view to user = get_user_model().objects.create_user(...) makes the test pass again.

Change History (10)

comment:1 Changed 4 years ago by Tom Carrick

Description: modified (diff)

comment:2 Changed 4 years ago by Tim Graham

All the changes in 1.8.5 are documented in the release notes. At first glance, #25431 looks like it might be the cause, though I didn't study it long enough to say why. Can you provide any more analysis with that information?

comment:3 Changed 4 years ago by Tom Carrick

I managed to bisect it to this commit: https://github.com/django/django/commit/35355a4ffedb2aeed52d5fe3034380ffc6a438db

But that's all I have to go on.

comment:4 Changed 4 years ago by Tim Graham

Cc: Ben Kraft added
Component: Testing frameworkUtilities
Summary: Weird test regression in 1.8.5copy.copy() broken with SimpleLazyObject in 1.8.5
Version: 1.91.8

It looks like your bisection is correct and copy.copy() no longer has the same behavior with SimpleLazyObject (request.user in your code). Let's see if the author of the pickling patch, Ben, can advise.

comment:5 Changed 4 years ago by Ben Kraft

Oh, yeah, I can see now how the dummy __getstate__ might break this, because it seems like if there's no __copy__, copy will use __getstate__ but not __reduce__ -- sorry I didn't realize before. (This stuff is not very well documented so I'll need to look more closely to be sure that that's the issue but it definitely seems plausible.)

I'm not entirely sure, but the simplest way to solve this is probably just writing a __copy__ method for LazyObject, either by just proxying the wrapped object's __copy__ or by doing something similar to the __deepcopy__ implementation. I'm not immediately sure which one of those is more correct.

comment:6 Changed 4 years ago by Tim Graham

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

comment:7 Changed 4 years ago by Ben Kraft

Has patch: set
Owner: changed from nobody to Ben Kraft
Status: newassigned

Fixed in https://github.com/django/django/pull/6030, along with regression tests (similar to those for deepcopy, which I also improved a bit). This reverts to the behavior that matches deepcopy of not initializing the object if it hasn't already been initialized (instead we just create another object with the same initializer). Incidentally, this seems to have been broken in various ways in other past versions; the tests I added fail even before 35355a4.

comment:8 Changed 4 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 13023ba8:

Fixed #26122 -- Fixed copying a LazyObject

Shallow copying of django.utils.functional.LazyObject or its subclasses has
been broken in a couple of different ways in the past, most recently due to
35355a4.

comment:9 Changed 4 years ago by Tim Graham <timograham@…>

In dee5896:

[1.9.x] Fixed #26122 -- Fixed copying a LazyObject

Shallow copying of django.utils.functional.LazyObject or its subclasses has
been broken in a couple of different ways in the past, most recently due to
35355a4.

Backport of 13023ba86746980aace2341ba32a9419e7567751 from master

comment:10 Changed 4 years ago by Tim Graham <timograham@…>

In 79c39505:

[1.8.x] Fixed #26122 -- Fixed copying a LazyObject

Shallow copying of django.utils.functional.LazyObject or its subclasses has
been broken in a couple of different ways in the past, most recently due to
35355a4.

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