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

fix(django): Add sync_capable to SentryWrappingMiddleware #3510

Merged
merged 3 commits into from
Sep 11, 2024

Conversation

szokeasaurusrex
Copy link
Member

@szokeasaurusrex szokeasaurusrex commented Sep 9, 2024

Apparently, not having the sync_capable on the SentryWrappingMiddleware sometimes breaks things when we have an async-only middleware, such as ServeStatic.

Fixes #3506

Copy link

codecov bot commented Sep 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.26%. Comparing base (53897ff) to head (2c6e450).
Report is 1 commits behind head on master.

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3510      +/-   ##
==========================================
- Coverage   84.26%   84.26%   -0.01%     
==========================================
  Files         136      136              
  Lines       13894    13895       +1     
  Branches     2938     2938              
==========================================
  Hits        11708    11708              
  Misses       1458     1458              
- Partials      728      729       +1     
Files with missing lines Coverage Δ
sentry_sdk/integrations/django/middleware.py 91.11% <100.00%> (+0.09%) ⬆️

... and 1 file with indirect coverage changes

@szokeasaurusrex
Copy link
Member Author

@sentrivana or @antonpirker, do you think we need a test for this? I am not sure how exactly we would test this change

@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/django-sync_capable branch from 11698e4 to 1f19214 Compare September 9, 2024 16:48
@sentrivana
Copy link
Contributor

Nice!

do you think we need a test for this? I am not sure how exactly we would test this change

Do we already have any middleware tests? It'd be great to have a test case that sets up a middleware, then inits the SDK, and then checks whether the wrapped middleware has all the expected attributes, including sync_capable.

If there's already a similar test case, we can just add this to it/repurpose that. If we don't have this, that's a blind spot we should fix. But if it's too much effort, you can just create a ticket and eventually we'll get to it during maintenance.

@szokeasaurusrex
Copy link
Member Author

@sentrivana @antonpirker, I added a test. Would appreciate if one of you could check that it looks reasonable

@szokeasaurusrex szokeasaurusrex enabled auto-merge (squash) September 11, 2024 10:11
@szokeasaurusrex szokeasaurusrex merged commit a581542 into master Sep 11, 2024
123 checks passed
@szokeasaurusrex szokeasaurusrex deleted the szokeasaurusrex/django-sync_capable branch September 11, 2024 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sentry SDK does not properly adapt async Django middleware
2 participants