Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SentryWrappingMiddleware.__init__ fails if super() is object #2461

Closed
cameron-simpson opened this issue Oct 23, 2023 · 13 comments · Fixed by #2466
Closed

SentryWrappingMiddleware.__init__ fails if super() is object #2461

cameron-simpson opened this issue Oct 23, 2023 · 13 comments · Fixed by #2466
Labels
Integration: Django Triaged Has been looked at recently during old issue triage

Comments

@cameron-simpson
Copy link
Contributor

cameron-simpson commented Oct 23, 2023

How do you use Sentry?

Sentry Saas (sentry.io)

Version

1.32.0

Steps to Reproduce

Oh, this is in Django version 2.2.6, which I accept is pretty old. We've a local ticket to upgrade that but I've no idea when it will get attention.

1: Install sentry in a django app with probably no other middleware.

2: manage.py runserver

This raises a TypeError at

super(SentryWrappingMiddleware, self).__init__(get_response)

because it is passing get_response, and the super type __init__ method comes from object, which does not accept an additional parameter.

I've worked around this locally by hand commenting out the parameter.

Maybe there's some check which can be made of what super(SentryWrappingMiddleware, self).__init__ resolves to?

Expected Result

The dev server starts up as normal.

Actual Result

Starting development server at http://127.0.0.1:8000/
Quit the server with CONTROL-C.
Exception in thread django-main-thread:
Traceback (most recent call last):
  File "/Users/cameron/var/pyenv/versions/3.7.13/lib/python3.7/threading.py", line 926, in _bootstrap_inner
    self.run()
  File "/Users/cameron/them/preferredmedia/1627-django-sentry-setup/var/go.sh-venv/lib/python3.7/site-packages/sentry_sdk/integrations/threading.py", line 72, in run
    reraise(*_capture_exception())
  File "/Users/cameron/them/preferredmedia/1627-django-sentry-setup/var/go.sh-venv/lib/python3.7/site-packages/sentry_sdk/_compat.py", line 115, in reraise
    raise value
  File "/Users/cameron/them/preferredmedia/1627-django-sentry-setup/var/go.sh-venv/lib/python3.7/site-packages/sentry_sdk/integrations/threading.py", line 70, in run
    return old_run_func(self, *a, **kw)
  File "/Users/cameron/var/pyenv/versions/3.7.13/lib/python3.7/threading.py", line 870, in run
    self._target(*self._args, **self._kwargs)
  File "/Users/cameron/them/preferredmedia/1627-django-sentry-setup/var/go.sh-venv/lib/python3.7/site-packages/django/utils/autoreload.py", line 54, in wrapper
    fn(*args, **kwargs)
  File "/Users/cameron/them/preferredmedia/1627-django-sentry-setup/var/go.sh-venv/lib/python3.7/site-packages/django/core/management/commands/runserver.py", line 137, in inner_run
    handler = self.get_handler(*args, **options)
  File "/Users/cameron/them/preferredmedia/1627-django-sentry-setup/var/go.sh-venv/lib/python3.7/site-packages/django/contrib/staticfiles/management/commands/runserver.py", line 27, in get_handler
    handler = super().get_handler(*args, **options)
  File "/Users/cameron/them/preferredmedia/1627-django-sentry-setup/var/go.sh-venv/lib/python3.7/site-packages/django/core/management/commands/runserver.py", line 64, in get_handler
    return get_internal_wsgi_application()
  File "/Users/cameron/them/preferredmedia/1627-django-sentry-setup/var/go.sh-venv/lib/python3.7/site-packages/django/core/servers/basehttp.py", line 42, in get_internal_wsgi_application
    return get_wsgi_application()
  File "/Users/cameron/them/preferredmedia/1627-django-sentry-setup/var/go.sh-venv/lib/python3.7/site-packages/django/core/wsgi.py", line 13, in get_wsgi_application
    return WSGIHandler()
  File "/Users/cameron/them/preferredmedia/1627-django-sentry-setup/var/go.sh-venv/lib/python3.7/site-packages/django/core/handlers/wsgi.py", line 135, in __init__
    self.load_middleware()
  File "/Users/cameron/them/preferredmedia/1627-django-sentry-setup/var/go.sh-venv/lib/python3.7/site-packages/sentry_sdk/integrations/django/middleware.py", line 66, in sentry_patched_load_middleware
    return old_load_middleware(*args, **kwargs)
  File "/Users/cameron/them/preferredmedia/1627-django-sentry-setup/var/go.sh-venv/lib/python3.7/site-packages/django/core/handlers/base.py", line 37, in load_middleware
    mw_instance = middleware(handler)
  File "/Users/cameron/them/preferredmedia/1627-django-sentry-setup/var/go.sh-venv/lib/python3.7/site-packages/sentry_sdk/integrations/django/middleware.py", line 140, in __init__
    super(SentryWrappingMiddleware, self).__init__(get_response)
TypeError: object.__init__() takes exactly one argument (the instance to initialize)
@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 Oct 23, 2023
@cameron-simpson
Copy link
Contributor Author

Well, things are more complex than I'd thought. super(...) is returning me the Htmx middleware. If I put in a breakpoint just before the failing call I see this:

> /Users/cameron/them/preferredmedia/1627-django-sentry-setup/var/go.sh-venv/lib/python3.7/site-packages/sentry_sdk/integrations/django/middleware.py(141)__init__()
-> super(SentryWrappingMiddleware, self).__init__() ## (get_response)
(Pdb) sup=super(SentryWrappingMiddleware, self)
(Pdb) sup
<super: <class 'HtmxMiddleware'>, <HtmxMiddleware object>>
(Pdb) init=super(SentryWrappingMiddleware, self).__init__
(Pdb) init
<method-wrapper '__init__' of HtmxMiddleware object at 0x11316bd10>
(Pdb) HtmxMiddleware.__init__
*** NameError: name 'HtmxMiddleware' is not defined
(Pdb) help(init)
*** No help for '(init)'
(Pdb) ~
*** SyntaxError: invalid syntax
(Pdb) get_response
<function BaseHandler._get_response at 0x113038050>
(Pdb) init(get_response)
*** TypeError: object.__init__() takes exactly one argument (the instance to initialize)

which still evokesthe TypeError if I call the resolved super(....).__init__ with a get_response parameter. Even though the Htmx init accepts that parameter and does not itself seem to call any further __init__ :-(

This may be harder for you to reproduce than I'd thought.

@sentrivana
Copy link
Contributor

Hey @cameron-simpson, thanks for reporting this. Django 2.2 tests are part of our pipeline so it's unlikely this is a general problem with the integration not working at all, it's indeed more likely that there is some custom middleware stuff going on. I'll put this in our backlog.

Could you let us know what your MIDDLEWARE and INSTALLED_APPS look like in settings.py?

@cameron-simpson
Copy link
Contributor Author

Listed below. And now you point me there, I wrote AuditLogMiddleware myself, and my ignorance was great. I might put some debugging in there...

MIDDLEWARE ('django.middleware.security.SecurityMiddleware', 'preferredmedia.common.middleware.NoMediaICWhiteNoiseMiddleware', 'django.contrib.sessions.middleware.SessionMiddleware', 'django.middleware.cache.UpdateCacheMiddleware', 'django.middleware.common.CommonMiddleware', 'django.middleware.cache.FetchFromCacheMiddleware', 'django.middleware.csrf.CsrfViewMiddleware', 'django.contrib.auth.middleware.AuthenticationMiddleware', 'django.contrib.messages.middleware.MessageMiddleware', 'django.middleware.clickjacking.XFrameOptionsMiddleware', 'django.contrib.admindocs.middleware.XViewMiddleware', 'preferredmedia.common.middleware.AuditLogMiddleware', 'django_htmx.middleware.HtmxMiddleware')
INSTALLED_APPS ('django.contrib.admin', 'django.contrib.auth', 'django.contrib.contenttypes', 'django.contrib.sessions', 'django.contrib.messages', 'django.contrib.staticfiles', 'django.contrib.admindocs', 'django.contrib.sitemaps', 'ixc_django_docker.celery', 'django_celery_results', 'djcelery_email', 'compressor', 'django_extensions', 'django_nose', 'post_office', 'django.contrib.admindocs', 'django.forms', 'preferredmedia.filemaker', 'preferredmedia.ingest', 'preferredmedia.pre_ingest', 'preferredmedia.common', 'preferredmedia.library_htmx', 'preferredmedia.project_utils', 'preferredmedia.taskqueue', 'preferredmedia.users', 'rest_framework', 'generic_relations', 'django_dbconn_retry', 'orderable', 'ordered_model', 'ajax_select', 'django_admin_lightweight_date_hierarchy', 'import_export', 'admin_auto_filters', 'django_admin_inline_paginator', 'django_elasticsearch_dsl_drf', 'django_elasticsearch_dsl', 'django_htmx', 'template_update_get', 'ixc_whitenoise')

@cameron-simpson
Copy link
Contributor Author

Hmm, removing django_htmx.middleware.HtmxMiddleware from the middleware removes the exception.

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 Oct 26, 2023
@cameron-simpson
Copy link
Contributor Author

I'm poking around in SentryWrappingMiddleware.__init__ from sentry_sdk/integrations/django/middleware.py:131 and am a bit puzzled.

The MRO of this SentryWrappingMiddleware is (django_htmx.middleware.HtmxMiddleware,object) which seems as expected. But self isn't an instance of SentryWrappingMiddleware, it's a plain old HtmxMiddleware instance.

In the if self.async_capable: branch we're calling super(SentryWrappingMiddleware, self).__init__(get_response).

However the docs for super() require that the second argument (self) is an instance of the first argument (SentryWrappingMiddleware), which it is not. Clearly this isn't checked, but I think we're off in "undefined behaviour" land.

I'm suspecting that this is breaking the lookup for __init__ because SentryWrappingMiddleware isn't found in the MRO of self because it's not a SentryWrappingMiddleware instance and its MRO is only (HtmxMiddleware,object). So I'm imagining that it's landing on the last class (object) because it doesn't find SentryWrappingMiddleware as the stopping point.

And so it calls object.__init__(get_response) which of course doesn't work.

I don't see why this doesn't explode on the other middleware classes we've got in play at my end, but maybe the root cause is how SentryWrappingMiddleware.__init__ is being called with a bare HtmxMiddleware instance?

Or maybe the behaviour of super() has changed; I'm using Python 3.7 here (yes more tech debt).

@cameron-simpson
Copy link
Contributor Author

Belay that: I'm being deceived by this code at the bottom of the middleware.py module:

    for attr in (
        "__name__",
        "__module__",
        "__qualname__",
    ):
        if hasattr(middleware, attr):
            setattr(SentryWrappingMiddleware, attr, getattr(middleware, attr))

such that my debug statements are printing the name of the wrapped class. An isinstance() test shows I do have a SentryWrappingMiddleware instance.

@cameron-simpson
Copy link
Contributor Author

Oh, here we go. Possible diagnosis.

We define SentryWrappingMiddleware like this:

    class SentryWrappingMiddleware(
        _asgi_middleware_mixin_factory(_check_middleware_span)  # type: ignore
    ):

and we define _asgi_middleware_mixin_factory like this at the top of the middleware.py file:

if DJANGO_VERSION < (3, 1):
    _asgi_middleware_mixin_factory = lambda _: object
else:
    from .asgi import _asgi_middleware_mixin_factory

such that the MRO of SentryWrappingMiddleware will just be object for Django < 3.1.

I warrant that HtmxMiddleware is the only middleware I've got advertising itself with async_capable = True (I will try to verify this), which is what triggers the super() miscall in SentryWrappingMiddleware.__init__.

Maybe htmx isn't tested with old Djangos.

@cameron-simpson
Copy link
Contributor Author

Suggested fix:

It appears the async support for middleware came in with Django 3.1 (hence the 3.1 test quoted in the previous comment).

What if the start of SentryWrappingMiddleware:

    class SentryWrappingMiddleware(
        _asgi_middleware_mixin_factory(_check_middleware_span)  # type: ignore
    ):
        async_capable = getattr(middleware, "async_capable", False)

also required Django 3.1 for async_capable to be true?

@antonpirker
Copy link
Member

Hey @cameron-simpson thanks for all the additional information! As this is for quite old Django versions, it is not on top of our priority list. But if anyone else is having this issue please upvote this issue so we know that there is demand!

@szokeasaurusrex
Copy link
Member

szokeasaurusrex commented Oct 31, 2023

@cameron-simpson We also welcome community contributions, so if you would like to try to fix the problem yourself, please feel free to open a PR

@cameron-simpson
Copy link
Contributor Author

cameron-simpson commented Nov 1, 2023 via email

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 Nov 1, 2023
@cameron-simpson
Copy link
Contributor Author

@szokeasaurusrex , the PR link is here: #2466

@szokeasaurusrex
Copy link
Member

@szokeasaurusrex , the PR link is here: #2466

Thank you, I see it now -- I had missed the PR when scrolling through the issue earlier. Will take a look as soon as I have time

@getsantry getsantry bot removed the status in GitHub Issues with 👀 Nov 2, 2023
@szokeasaurusrex szokeasaurusrex self-assigned this Nov 2, 2023
@antonpirker antonpirker added this to the Django update milestone Jun 7, 2024
@antonpirker antonpirker removed this from the Django update milestone Jun 20, 2024
@szokeasaurusrex szokeasaurusrex removed their assignment Jul 2, 2024
@antonpirker antonpirker added the Triaged Has been looked at recently during old issue triage label Jul 15, 2024
sentrivana pushed a commit that referenced this issue Sep 4, 2024
…ject

As described in issue #2461, the SentryWrappingMiddleware MRO is just object if Django < 3.1 (when async middleware became a thing), but the async_capable check inside the class only looks for the async_capable attribute inside the middleware class.

This PR makes that check also conditional on Django >= 3.1.

Otherwise the code calls super(.....).__init__(get_response) and for Django < 3.1 this only finds object.__init__, not the wrapped middleware __init__.

---

Co-authored-by: Daniel Szoke <daniel.szoke@sentry.io>
arjennienhuis pushed a commit to arjennienhuis/sentry-python that referenced this issue Sep 30, 2024
…ject

As described in issue getsentry#2461, the SentryWrappingMiddleware MRO is just object if Django < 3.1 (when async middleware became a thing), but the async_capable check inside the class only looks for the async_capable attribute inside the middleware class.

This PR makes that check also conditional on Django >= 3.1.

Otherwise the code calls super(.....).__init__(get_response) and for Django < 3.1 this only finds object.__init__, not the wrapped middleware __init__.

---

Co-authored-by: Daniel Szoke <daniel.szoke@sentry.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Integration: Django Triaged Has been looked at recently during old issue triage
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants