Code

Opened 6 months ago

Closed 4 months ago

Last modified 4 months ago

#21840 closed Bug (fixed)

LazyObject wrapped instances can no longer be coerced to Bool due to #20924

Reported by: gabejackson Owned by: bmispelon
Component: Utilities Version: master
Severity: Release blocker Keywords: LazyObject utils
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

https://code.djangoproject.com/ticket/20924 adds proxy methods for __contains__ and __len__ to support lists and dicts wrapped by LazyObject. The problem is that evaluating truth for such LazyObjects will break if __len__ is not defined on the wrapper instance.

This can be demonstrated as follows:

>>> from django.core.files.storage import default_storage
>>> if default_storage: print('yes')
... 
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/home/tisba/Programming/testbed/boogz/django/utils/functional.py", line 224, in inner
    return func(self._wrapped, *args)
TypeError: object of type 'FileSystemStorage' has no len()

this is because converting an object to a Bool calls object.__nonzero__ which in turn calls __len__ as stated here:
http://docs.python.org/2/reference/datamodel.html#object.__nonzero

This will break a lot of apps using LazyObject. This problem exists in the current master of imagekit as well:

class JustInTime(object):
    """
    A strategy that ensures the file exists right before it's needed.

    """
    def on_existence_required(self, file):
        file.generate()

    def on_content_required(self, file):
        file.generate()

class StrategyWrapper(LazyObject):
    def __init__(self, strategy):
        if isinstance(strategy, six.string_types):
            strategy = get_singleton(strategy, 'cache file strategy')
        elif isinstance(strategy, dict):
            strategy = DictStrategy(strategy)
        elif callable(strategy):
            strategy = strategy()
        self._wrapped = strategy

    def __getstate__(self):
        return {'_wrapped': self._wrapped}

    def __setstate__(self, state):
        self._wrapped = state['_wrapped']

    def __unicode__(self):
        return force_text(self._wrapped)

    def __str__(self):
        return str(self._wrapped)

calling the LazyObject in a statement like if lazy_object will yield:

In [2]: p.avatar_thumbnail
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-2-860e30762713> in <module>()
----> 1 p.avatar_thumbnail

/Users/gabejackson/venv/hvad-test/lib/python2.7/site-packages/imagekit/models/fields/utils.pyc in __get__(self, instance, owner)
     15             spec = self.field.get_spec(source=source)
     16             import ipdb; ipdb.set_trace()
---> 17             file = ImageCacheFile(spec)
     18             instance.__dict__[self.attname] = file
     19             return file

/Users/gabejackson/venv/hvad-test/lib/python2.7/site-packages/imagekit/cachefiles/__init__.pyc in __init__(self, generator, name, storage, cachefile_backend, cachefile_strategy)
     55         self.cachefile_strategy = (
     56             cachefile_strategy
---> 57             or getattr(generator, 'cachefile_strategy', None)
     58             or get_singleton(settings.IMAGEKIT_DEFAULT_CACHEFILE_STRATEGY, 'cache file strategy')
     59         )

/Users/gabejackson/venv/hvad-test/lib/python2.7/site-packages/django/utils/functional.pyc in inner(self, *args)
    222         if self._wrapped is empty:
    223             self._setup()
--> 224         return func(self._wrapped, *args)
    225     return inner
    226

TypeError: object of type 'JustInTime' has no len()

You can see that the or statement on line 57 is forcing the coercion of the JustInTime object to a Boolean. Doing this calls __len__ which is now defined on LazyObject and will cause the TypeError stated.

Attachments (0)

Change History (12)

comment:1 Changed 6 months ago by bmispelon

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 6 months ago by gabejackson

Just to comment why this worked before:

"If a class defines neither __len__() nor __nonzero__(), all its instances are considered true."

that is from http://docs.python.org/2/reference/datamodel.html#object.__nonzero

Last edited 6 months ago by gabejackson (previous) (diff)

comment:3 Changed 6 months ago by gabejackson

further notes:
It appears many of the proxy methods should be on LazyObject instead of on SimpleLazyObject because the latter inherits from the LazyObject.

SimpleLazyObject was introduced here: https://github.com/django/django/commit/a2d8acbacda353248fd191b97155cd4cf27dcd66
Proxies were added here: https://github.com/django/django/commit/60cf3f2f842b6e56132b8880c70acc87bd753c2e

a simple solution seems to just add: __nonzero__ = new_method_proxy(bool) to LazyObject.
I'm not familiar with these undocumented classes, but wouldn't it be possible to move all the proxying to LazyObject?

comment:4 Changed 6 months ago by aaugustin

comment:5 Changed 6 months ago by bmispelon

  • Has patch set

Pull request here: https://github.com/django/django/pull/2221

As far as I can tell, there's no reason for all the dunder methods to exist on SimpleLazyObject instead of LazyObject and moving them all to the parent class fixes this particular issue.

I also added a regression test and the full test suite passes (on sqlite at least).

comment:6 Changed 5 months ago by timo

  • Triage Stage changed from Accepted to Ready for checkin

comment:7 Changed 5 months ago by mjtamlyn

  • Triage Stage changed from Ready for checkin to Accepted

On discussion with bmispelon - we would like to add more tests for this before committing. That said, it can be merged as is if not landed before beta.

comment:8 Changed 5 months ago by bmispelon

  • Needs tests set
  • Owner changed from nobody to bmispelon
  • Patch needs improvement set
  • Status changed from new to assigned

The current PR is broken (which makes me glad I decided to hold off and write some tests) since not all dunder methods can be moved as-is (repr being one of them for example).

comment:9 Changed 5 months ago by bmispelon

  • Needs tests unset
  • Patch needs improvement unset

Updated the PR. It should be ready for final review now: https://github.com/django/django/pull/2221/

comment:10 Changed 4 months ago by timo

  • Triage Stage changed from Accepted to Ready for checkin

Reviewed by Shai and myself.

comment:11 Changed 4 months ago by Baptiste Mispelon <bmispelon@…>

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

In 61917aa08b4ab2bc35f3ffe87b7693bd8b58e205:

Fixed #21840 -- Moved dunder methods from SimpleLazyObject to LazyObject.

This commit also added tests for LazyObject and refactored
the testsuite of SimpleLazyObject so that it can share
test cases with LazyObject.

comment:12 Changed 4 months ago by Andrew Godwin <andrew@…>

In 356f064c49f926dd771e6ee590b43c8ac5dc4efc:

Merge pull request #2221 from bmispelon/LazyObject-refactor

Fixed #21840 -- Moved dunder methods from SimpleLazyObject to LazyObject...

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.