LazyObject defines attribute that don't exist on wrapped object

[Andrey Fedoseev]

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

[Sergey Fedoseev]

​PR

[Theofilos Alexiou]

​PR

[Mariusz Felisiak <felisiak.mariusz@…>]

In 97d7990a:

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

Thanks Sergey Fedoseev for the initial patch.

[Mariusz Felisiak <felisiak.mariusz@…>]

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@…>

[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

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

[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?

[Mariusz Felisiak]

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+.

[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?

[Dylan Young]

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?

[Dylan Young]

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))

[Dylan Young]

Introduced by 97d7990abde3fe4b525ae83958fd0b52d6a1d13f

Confirmed 1d071ec1aa8fa414bb96b41f7be8a1bd01079815 does not exhibit the error.

[Dylan Young]

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?

[Carl Friedrich Bolz-Tereick]

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!

[Collin Anderson]

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.

[Lily Foote]

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.