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

suggestion for fix for issue #2461 #2466

Merged
merged 5 commits into from
Sep 4, 2024

Conversation

cameron-simpson
Copy link
Contributor

@cameron-simpson cameron-simpson commented Oct 26, 2023

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

Fixes GH-2461

Copy link
Member

@szokeasaurusrex szokeasaurusrex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR! I have left one comment on your proposed changes

Additionally, it looks like some of our required CI checks are failing with this pull request -- if you can, please try to debug these. For the linter check, you should make sure to run the file you changed through our code formatter, black (I believe this happens automatically if you have the pre-commit hooks configured). There is also a test failure; please try to solve this if you can; otherwise, if you struggle to figure it out, let us know, and we can help.

if DJANGO_VERSION < (3, 1):
_asgi_middleware_mixin_factory = lambda _: object
else:
django_supports_async_middleware = DJANGO_VERSION >= (3, 1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use all caps variable name for constants like this one, i.e. DJANGO_SUPPORTS_ASYNC_MIDDLEWARE

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've applied black and adjusted the variable case.

I'm puzzling over the CI failure here:
https://github.com/getsentry/sentry-python/actions/runs/6660942801/job/18340882553

It looks like the version of pytest is too old for upcoming versions of pytest-asyncio. However, I don't see how this should happen only on my branch and not also on the main sentry codebase. I've merged recent commits from master into my branch, but none seems relevant to this test so I don't imagine the next CI run should be any better. But please push the button on it just for the exercise.

I'll keep looking a bit at this today. Is there a guide to running these tests locally?

@szokeasaurusrex
Copy link
Member

We have our contributing docs here, which contains some information about running our tests, but unfortunately the guide is not super detailed. If you can figure it out, I would suggest running the test suite with tox, which will allow you to run the tests with the same configuration as they are run in CI (in terms of Python version and installed dependencies). Otherwise, let me know and I will try to see if I can debug the test failures.

Also, now that the code formatting was fixed, I can see in our linting CI check that there is a typing issue. Please try to fix this as well. We use the mypy linter, which you should also be able to run locally.

@szokeasaurusrex
Copy link
Member

szokeasaurusrex commented Nov 6, 2023

@cameron-simpson It actually seems that you can ignore the failing AWS Lambda tests for now; we recently opened an issue (#2487) because these tests have been failing for all contributor PRs; it is a problem with our tests, not your PR.

@szokeasaurusrex
Copy link
Member

Hey @cameron-simpson, we fixed the issue with our AWS Lambda tests. They currently are failing, but that is because for security reasons, I need to add a label to your PR to indicate that I have verified that your code won't leak our AWS Lambda secrets before the tests are allowed to run. Since I need to add this label every time you update the code, I will add this label once all other non-AWS Lambda checks are passing.

It looks like our CI checks and Python 3.8 common tests are failing. Please fix these, and then I will allow the AWS Lambda tests to run

@antonpirker antonpirker added this to the Django update milestone Jun 11, 2024
@antonpirker antonpirker removed this from the Django update milestone Jun 20, 2024
…er().__init__

 - SentryWrappingMiddleware: revert debugging hack, set new django_supports_async_middleware = DJANGO_VERSION >= (3, 1), use for both _asgi_middleware_mixin_factory and the async_capable flag inside SentryWrappingMiddleware

 - integrations/django/middleware.py: black autoformat

 - integrations/django/middleware.py: use all caps for capability variable

 - sentry_sdk/integrations/django/middleware.py: annotate stub _asgi_middleware_mixin_factory to match the .asgi typing

Co-authored-by: Daniel Szoke <daniel.szoke@sentry.io>
Copy link

codecov bot commented Jul 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.71%. Comparing base (d13fe23) to head (36cf20d).
Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2466      +/-   ##
==========================================
- Coverage   79.77%   79.71%   -0.07%     
==========================================
  Files         133      133              
  Lines       14289    14290       +1     
  Branches     3003     3003              
==========================================
- Hits        11399    11391       -8     
- Misses       2067     2080      +13     
+ Partials      823      819       -4     
Files Coverage Δ
sentry_sdk/integrations/django/middleware.py 84.37% <100.00%> (+0.16%) ⬆️

... and 3 files with indirect coverage changes

@antonpirker antonpirker added the Trigger: tests using secrets PR code is safe; run CI label Jul 18, 2024
@github-actions github-actions bot removed the Trigger: tests using secrets PR code is safe; run CI label Jul 25, 2024
@sentrivana
Copy link
Contributor

We can merge this as soon as master is green

@sentrivana sentrivana enabled auto-merge (squash) September 4, 2024 07:51
@sentrivana sentrivana merged commit 16d05f4 into getsentry:master Sep 4, 2024
122 of 124 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SentryWrappingMiddleware.__init__ fails if super() is object
4 participants