#12049 closed (fixed)
New LazyObject-wrapped User breaks queries in template tags
| Reported by: | Christian Hammond | Owned by: | nobody |
|---|---|---|---|
| Component: | contrib.auth | Version: | dev |
| Severity: | Keywords: | ||
| Cc: | chipx86@… | Triage Stage: | Unreviewed |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
I spent a couple days tracking this down since the last fix (ticket #12037) for the new lazy-wrapped User. Basically, if you have a template tag that grabs the User from the context and performs a complex query with two or more Q()'s chained together, it will fail. The failure case on Python 2.4 is fatal, while on 2.5/2.6 it seems to not fail but does log internally caught exception spew.
Basically, when a deepcopy() is performed on the node list, it sees a function() for the wrapped User and just fails.
Imagine you have a template tag that takes in the context and starts to build a query, like so:
query = Q(user=context['user']) & Q(someflag=True)
The process of using a Q() on a wrapped User and chaining with another Q() will break. On Python 2.4, we see:
File "/usr/lib/python2.4/site-packages/django/db/models/query_utils.py", line 162, in __and__
return self._combine(other, self.AND)
File "/usr/lib/python2.4/site-packages/django/db/models/query_utils.py", line 154, in _combine
obj = deepcopy(self)
File "/usr/lib/python2.4/copy.py", line 185, in deepcopy
y = copier(x, memo)
File "/usr/lib/python2.4/site-packages/django/utils/tree.py", line 61, in __deepcopy__
obj.children = deepcopy(self.children, memodict)
File "/usr/lib/python2.4/copy.py", line 174, in deepcopy
y = copier(x, memo)
File "/usr/lib/python2.4/copy.py", line 241, in _deepcopy_list
y.append(deepcopy(a, memo))
File "/usr/lib/python2.4/copy.py", line 174, in deepcopy
y = copier(x, memo)
File "/usr/lib/python2.4/copy.py", line 248, in _deepcopy_tuple
y.append(deepcopy(a, memo))
File "/usr/lib/python2.4/copy.py", line 204, in deepcopy
y = _reconstruct(x, rv, 1, memo)
File "/usr/lib/python2.4/copy.py", line 351, in _reconstruct
state = deepcopy(state, memo)
File "/usr/lib/python2.4/copy.py", line 174, in deepcopy
y = copier(x, memo)
File "/usr/lib/python2.4/copy.py", line 268, in _deepcopy_dict
y[deepcopy(key, memo)] = deepcopy(value, memo)
File "/usr/lib/python2.4/copy.py", line 204, in deepcopy
y = _reconstruct(x, rv, 1, memo)
File "/usr/lib/python2.4/copy.py", line 336, in _reconstruct
y = callable(*args)
File "/usr/lib/python2.4/copy_reg.py", line 92, in __newobj__
return cls.__new__(cls, *args)
TypeError: function() takes at least 2 arguments (0 given)
On Python 2.5 and 2.6, the call succeeds (or at least doesn't throw a fatal exception, but I don't know if there are internal side effects here), but does log the following:
Exception RuntimeError: 'maximum recursion depth exceeded while calling a Python object' in <type 'exceptions.AttributeError'> ignored
I'm attaching a diff that simply amends the test_user_attrs testcase to simulate this. After the other checks, it pulls out the User and then builds a dummy query.
Attachments (2)
Change History (8)
by , 16 years ago
| Attachment: | user-query-test.diff added |
|---|
comment:1 by , 16 years ago
| Cc: | added |
|---|
comment:2 by , 16 years ago
Thanks for the test, that's really useful. Hopefully I look at fixing this on Monday. If you want to do it yourself, I imagine the fix will be to implement __deepcopy__ on SimpleLazyObject. I think the implementation is straightforward - if self._wrapped is None, then you just assign the same _setupfunc function to the copy (no need to make a copy of the function). If self._wrapped is not None, you copy the wrapped object itself.
In this context, I don't think we need a deep copy of the wrapped object, because it is just going to be thrown away and not modified, but to obey the __deepcopy__ contract, you might want to make it a deep copy. Alternatively, you could make a ContextLazyObject which subclasses SimpleLazyObject, and has a __deepcopy__ method that does shallow copies, which should be safe if ContextLazyObject is only used for this purpose. Perhaps. I haven't thought about it properly.
comment:3 by , 16 years ago
The attached patch implements __deepcopy__ for LazyObject. I like the idea of providing the implementation there rather than in a subclass as it seems that this functionality should exist higher up, but if you think of a case where we really should limit this to a subclass, we can change that.
I kept the testcase in the same place even though really the fix isn't part of the auth framework. I couldn't find anything that, as far as I could tell, tested LazyObject specifically, and since the User wrapper is a real-world case of breakage (which could break again if User was wrapped in something else), I thought it'd be nice to continue testing this case. However, I'll leave this up to you to decide.
Ran the unit tests with Python 2.4, 2.5 and 2.6. All tests succeeded.
comment:4 by , 16 years ago
Great work, cheers! Sorry for the lost hours debugging, it's one of those use cases which is just really difficult to anticipate. I'll commit the patch soon.
comment:5 by , 16 years ago
| Resolution: | → fixed |
|---|---|
| Status: | new → closed |
Test case for building a query with a wrapped User.