#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 )
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 , 9 years ago
Description: | modified (diff) |
---|
comment:2 by , 9 years ago
comment:3 by , 9 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 , 9 years ago
Cc: | 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 , 9 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 , 9 years ago
Severity: | Normal → Release blocker |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:7 by , 9 years ago
Has patch: | set |
---|---|
Owner: | changed from | to
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.
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?