getsentry / sentry-python

The official Python SDK for Sentry.io
https://sentry.io/for/python/
MIT License
1.81k stars 475 forks source link

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

Open cameron-simpson opened 8 months ago

cameron-simpson commented 8 months ago

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 https://github.com/getsentry/sentry-python/blob/085595b5f02931a3268c2de2a58b6986f3766d75/sentry_sdk/integrations/django/middleware.py#L140 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)
cameron-simpson commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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

cameron-simpson commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

@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 commented 8 months ago

On 31Oct2023 04:00, Daniel Szoke @.***> wrote:

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

I did submit a PR! - Cameron

cameron-simpson commented 8 months ago

@szokeasaurusrex , the PR link is here: https://github.com/getsentry/sentry-python/pull/2466

szokeasaurusrex commented 8 months ago

@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