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(integrations): Do not patch execute #4026

Merged
merged 10 commits into from
Feb 11, 2025

Conversation

sentrivana
Copy link
Contributor

@sentrivana sentrivana commented Feb 10, 2025

Looks like the new release of strawberry (0.259.0) is not compatible with our integration. Fix it.

Context

New Strawberry version removes the execute and execute_sync functions that we were monkeypatching in favor of integrating the code directly in Schema.execute and Schema.execute_sync.

We were previously patching execute instead of Schema.execute that's calling it because that way we had access to a populated execution_context which contains data that we wanted to put on the event via an event processor.

We have access to the execution_context directly in the extension hooks Strawberry provides, so we now add the event processor there instead of monkeypatching anything.

This should also work for older Strawberry versions, so shouldn't be necessary to keep the old implementation around for compat.

Closes #4037

Copy link

codecov bot commented Feb 10, 2025

Codecov Report

Attention: Patch coverage is 65.00000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 80.24%. Comparing base (0cda7d9) to head (d577e4b).
Report is 1 commits behind head on master.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
sentry_sdk/integrations/strawberry.py 75.00% 4 Missing ⚠️
sentry_sdk/integrations/graphene.py 0.00% 2 Missing ⚠️
sentry_sdk/integrations/ariadne.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4026      +/-   ##
==========================================
- Coverage   80.24%   80.24%   -0.01%     
==========================================
  Files         139      139              
  Lines       15424    15403      -21     
  Branches     2608     2605       -3     
==========================================
- Hits        12377    12360      -17     
+ Misses       2203     2202       -1     
+ Partials      844      841       -3     
Files with missing lines Coverage Δ
sentry_sdk/integrations/gql.py 78.94% <100.00%> (ø)
sentry_sdk/integrations/ariadne.py 87.50% <0.00%> (ø)
sentry_sdk/integrations/graphene.py 82.35% <0.00%> (ø)
sentry_sdk/integrations/strawberry.py 85.57% <75.00%> (+0.43%) ⬆️

@patrick91
Copy link
Contributor

@sentrivana thanks! I totally forgot that sentry was using those files 😊

@sentrivana sentrivana changed the title fix(integrations): Fix Strawberry fix(integrations): Do not patch execute Feb 11, 2025
@sentrivana
Copy link
Contributor Author

@patrick91 No worries, not sure TBH why we were patching execute in the first place. Will switch to injecting data in the on_operation hook now, which feels nicer. :)

@sentrivana sentrivana marked this pull request as ready for review February 11, 2025 13:55
@sentrivana
Copy link
Contributor Author

For everyone following, this will be fixed in the next SDK release later this week.

@@ -27,16 +27,17 @@
raise DidNotEnable("strawberry-graphql integration requires Python 3.8 or newer")

try:
import strawberry.schema.schema as strawberry_schema # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

so glad this is gone :D

@sentrivana sentrivana merged commit 3217cca into master Feb 11, 2025
150 of 151 checks passed
@sentrivana sentrivana deleted the ivana/fix-strawberry-new-version branch February 11, 2025 14:48
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.

Strawberry integration doesn't work with newest strawberry-graphql release
3 participants