Code

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#12049 closed (fixed)

New LazyObject-wrapped User breaks queries in template tags

Reported by: chipx86 Owned by: nobody
Component: contrib.auth Version: master
Severity: Keywords:
Cc: chipx86@… Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

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)

user-query-test.diff (1.5 KB) - added by chipx86 4 years ago.
Test case for building a query with a wrapped User.
lazyobject-deepcopy.diff (2.2 KB) - added by chipx86 4 years ago.
Implements deepcopy for LazyObject

Download all attachments as: .zip

Change History (8)

Changed 4 years ago by chipx86

Test case for building a query with a wrapped User.

comment:1 Changed 4 years ago by chipx86

  • Cc chipx86@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 4 years ago by lukeplant

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.

Changed 4 years ago by chipx86

Implements deepcopy for LazyObject

comment:3 Changed 4 years ago by chipx86

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 Changed 4 years ago by lukeplant

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 Changed 4 years ago by lukeplant

  • Resolution set to fixed
  • Status changed from new to closed

(In [11634]) Fixed #12049 - LazyObject-wrapped User breaks queries in template tags

Thanks to chipx86 for the report and patch.

comment:6 Changed 4 years ago by lukeplant

(In [11635]) [1.1.X] Fixed #12049 - LazyObject-wrapped User breaks queries in template tags

Thanks to chipx86 for the report and patch.

Backport of r11634 from trunk

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.