Opened 7 years ago

Closed 2 years ago

Last modified 3 months ago

#28358 closed Bug (fixed)

LazyObject defines attribute that don't exist on wrapped object

Reported by: Andrey Fedoseev Owned by: Theofilos Alexiou
Component: Utilities Version: 1.11
Severity: Normal Keywords:
Cc: Collin Anderson 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

LazyObject defines magic methods (__getitem__, __iter__) which may be missing from the wrapped object. This leads to the following errors:

some_variable = request.user

if hasattr(some_variable, "__getitem__"):
    foo = some_variable["foo"]  # raises TypeError: 'User' object has no attribute '__getitem__'

if hasattr(some_variable, "__iter__"):
    for item in some_variable:  # raises TypeError: 'User' object is not iterable

Change History (23)

comment:1 by Tim Graham, 7 years ago

Triage Stage: UnreviewedAccepted

comment:2 by Sergey Fedoseev, 7 years ago

Cc: Sergey Fedoseev added

comment:3 by Sergey Fedoseev, 7 years ago

Owner: changed from nobody to Sergey Fedoseev
Status: newassigned

comment:4 by Sergey Fedoseev, 7 years ago

Has patch: set

comment:5 by Tim Graham, 7 years ago

Patch needs improvement: set

comment:6 by Mariusz Felisiak, 3 years ago

Owner: Sergey Fedoseev removed
Status: assignednew

comment:7 by Sergey Fedoseev, 3 years ago

Cc: Sergey Fedoseev removed

comment:8 by Theofilos Alexiou, 2 years ago

Owner: set to Theofilos Alexiou
Status: newassigned

comment:9 by Theofilos Alexiou, 2 years ago

Patch needs improvement: unset

comment:10 by Mariusz Felisiak, 2 years ago

Triage Stage: AcceptedReady for checkin

comment:11 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

Resolution: fixed
Status: assignedclosed

In 97d7990a:

Fixed #28358 -- Prevented LazyObject from mimicking nonexistent attributes.

Thanks Sergey Fedoseev for the initial patch.

comment:12 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

In b2ed0d78:

Refs #28358 -- Fixed infinite recursion in LazyObject.getattribute().

Regression in 97d7990abde3fe4b525ae83958fd0b52d6a1d13f.

Co-authored-by: Mariusz Felisiak <felisiak.mariusz@…>
Co-authored-by: Theo Alexiou <theofilosalexiou@…>

comment:13 by Dylan Young, 2 years ago

I'm not 100%, but it looks like these fixes might have broken Django on PyPy.

django-csp/.tox/pypy3.9-main/lib/pypy3.9/site-packages/django/contrib/admin/decorators.py", line 107, in _model_admin_wrapper
    raise ValueError("site must subclass AdminSite")
ValueError: site must subclass AdminSite

mro looks like this (not sure what's expected)

[<class 'django.contrib.admin.sites.DefaultAdminSite'>, <class 'django.utils.functional.LazyObject'>, <class 'object'>]

Works fine in 4.0.

Created a PyPy issue here https://foss.heptapod.net/pypy/pypy/-/issues/3751

Last edited 2 years ago by Dylan Young (previous) (diff)

comment:14 by Mariusz Felisiak, 2 years ago

Dylan, Can you confirm that it works for you with Django 4.0? Which commit introduces a regression? Is it not an issue in pypy itself?

in reply to:  13 comment:15 by Mariusz Felisiak, 2 years ago

Replying to Dylan Young:

Works fine in 4.0.

Created a PyPy issue here https://foss.heptapod.net/pypy/pypy/-/issues/3751

Basic test project works for me on PyPy and Django 4.0+.

in reply to:  13 ; comment:16 by Theofilos Alexiou, 2 years ago

Replying to Dylan Young:

I'm not 100%, but it looks like these fixes might have broken Django on PyPy.

django-csp/.tox/pypy3.9-main/lib/pypy3.9/site-packages/django/contrib/admin/decorators.py", line 107, in _model_admin_wrapper
    raise ValueError("site must subclass AdminSite")
ValueError: site must subclass AdminSite

Not sure if this is of any help or not. The error is raised on line 107 but looking at the 4.1 brach I see that this should be in line 102 Is this perhaps a modified brach of Django and some other change causes it?

in reply to:  16 comment:17 by Dylan Young, 2 years ago

It's main, not 4.1. I'll try 4.1 and see if it shows up there too.

Replying to Theofilos Alexiou:

Replying to Dylan Young:

I'm not 100%, but it looks like these fixes might have broken Django on PyPy.

django-csp/.tox/pypy3.9-main/lib/pypy3.9/site-packages/django/contrib/admin/decorators.py", line 107, in _model_admin_wrapper
    raise ValueError("site must subclass AdminSite")
ValueError: site must subclass AdminSite

Not sure if this is of any help or not. The error is raised on line 107 but looking at the 4.1 brach I see that this should be in line 102 Is this perhaps a modified brach of Django and some other change causes it?

comment:18 by Dylan Young, 2 years ago

Replicated on 4.1a1

  File "django-csp/.tox/pypy3.9-4.1.x/lib/pypy3.9/site-packages/django/contrib/auth/admin.py", line 29, in <module>
    class GroupAdmin(admin.ModelAdmin):
  File "django-csp/.tox/pypy3.9-4.1.x/lib/pypy3.9/site-packages/django/contrib/admin/decorators.py", line 102, in _model_admin_wrapper
    raise ValueError("site must subclass AdminSite")
ValueError: site must subclass AdminSite

I was able to narrow this down to some kind of interaction with coverage or pytest-cov (see https://foss.heptapod.net/pypy/pypy/-/issues/3751). There's a larger traceback on that issue as well.

4.0 is fine. Not sure which commit introduces it at this point. These ones just seemed like the likely culprit.

LOL, it was probably my debugging code throwing off the line numbers:

        # admin_site = admin_site._wrapped
        # print(admin_site, type(admin_site), type(admin_site).mro())
        # for klass in type(admin_site).mro():
        #     print(klass)
        #     print(dir(klass))
Last edited 2 years ago by Dylan Young (previous) (diff)

comment:19 by Dylan Young, 2 years ago

Introduced by 97d7990abde3fe4b525ae83958fd0b52d6a1d13f

Confirmed 1d071ec1aa8fa414bb96b41f7be8a1bd01079815 does not exhibit the error.

in reply to:  14 comment:20 by Dylan Young, 2 years ago

Yes, I would guess that it's an issue with PyPy or perhaps coverage or pytest-cov using non-spec CPython implementation details, but that's just a guess.

Replying to Mariusz Felisiak:

Dylan, Can you confirm that it works for you with Django 4.0? Which commit introduces a regression? Is it not an issue in pypy itself?

comment:21 by Carl Friedrich Bolz-Tereick, 19 months ago

hi everyone! PyPy dev here. So after some investigation (you can find the gory details on this CPython issue: https://github.com/python/cpython/issues/98148 ) it turns out that this bug also exists in CPython, if you run coverage in pure python mode. The bug occurs only if you: 1) use super 2) define __class__ in the class body yourself 3) use a pure python trace hook, or call locals() in the class body.

At this point there has been no reaction from CPython yet, but the way I see things from the PyPy side it'll be on the tricky to fix this so it might take a while (I'm waiting for CPy feedback because the two implementations use exactly the same approaches here, to the point of ending up with exactly the same bug).

I was wondering whether Django might be open to a PR that works around the bug for the time being? A pragmatic approach would be to stop using super in LazyObject.__getattribute__, which was introduced in 97d7990abde3fe4b525ae83958fd0b52d6a1d13f, and just call object.__getattribute__ instead. If that sounds like a reasonable solution, I am happy to work on the PR. (there would be some trickiness to how to unit test this, but I can probably think of something). Please let me know!

comment:22 by Collin Anderson, 18 months ago

Cc: Collin Anderson added

the object.__getattribute__(self, name) workaround mentioned above fixed the issue for me running pure-python coverage.

I made a basic PR but don't plan on pushing it through to the end. I'll opt for actually installing coverage rather than running it straight from a git checkout but at least wanted to report that the workaround does work.

https://github.com/django/django/pull/16541

For testing, somehow running a pure-python version of coverage as part of continuous integration would show the issue, though ideally you want to test both with and without c-speedups.

Edit: Also here's a direct link to the Python issue: https://github.com/python/cpython/issues/98148

Last edited 18 months ago by Collin Anderson (previous) (diff)

comment:23 by Lily Foote, 3 months ago

The PyPy issue for the ValueError: site must subclass AdminSite exception has moved to https://github.com/pypy/pypy/issues/3750.

I've just opened #35418 to track adding the workaround.

Note: See TracTickets for help on using tickets.
Back to Top