Opened 10 years ago

Closed 10 years ago

Last modified 10 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 by Tom Carrick, 10 years ago

Description: modified (diff)

comment:2 by Tim Graham, 10 years ago

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 by Tom Carrick, 10 years ago

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 by Tim Graham, 10 years ago

Cc: Ben Kraft added
Component: Testing framework → Utilities
Summary: Weird test regression in 1.8.5 → copy.copy() broken with SimpleLazyObject in 1.8.5
Version: 1.9 → 1.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 by Ben Kraft, 10 years ago

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 by Tim Graham, 10 years ago

Severity: Normal → Release blocker
Triage Stage: Unreviewed → Accepted

comment:7 by Ben Kraft, 10 years ago

Has patch: set
Owner: changed from nobody to Ben Kraft
Status: new → assigned

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 by Tim Graham <timograham@…>, 10 years ago

Resolution: → fixed
Status: assigned → closed

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 by Tim Graham <timograham@…>, 10 years ago

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 by Tim Graham <timograham@…>, 10 years ago

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