Fix #3680: MaskErrors does not mask errors for subscriptions#4301
Fix #3680: MaskErrors does not mask errors for subscriptions#4301Ladol wants to merge 2 commits intostrawberry-graphql:mainfrom
MaskErrors does not mask errors for subscriptions#4301Conversation
…bscriptions Fixes an issue where schema extensions (like `MaskErrors`) were bypassed during WebSocket subscriptions. The extensions' `_process_result` hooks are now properly triggered for each yielded result in both `graphql-transport-ws` and `graphql-ws` protocols, ensuring errors are correctly formatted before being sent to the client.
Reviewer's GuideEnsures WebSocket subscription results are passed through schema extensions (e.g., MaskErrors) before being sent to clients, by invoking extension _process_result hooks for graphql-ws and graphql-transport-ws subscription payloads and adding regression tests and release notes. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
for more information, see https://pre-commit.ci
|
Thanks for adding the Here's a preview of the changelog: Fixes an issue where schema extensions (like DescriptionFixes an issue where schema extensions (such as Previously, standard Queries and Mutations would pass their results through the extension pipeline, but Subscriptions would send raw Migration guideNo migration required. Types of Changes
Checklist
Here's the tweet text: |
|
Thanks for adding the Here's a preview of the changelog: Fixes an issue where schema extensions (like DescriptionFixes an issue where schema extensions (such as Previously, standard Queries and Mutations would pass their results through the extension pipeline, but Subscriptions would send raw Migration guideNo migration required. Types of Changes
Checklist
Here's the tweet text: |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The extension processing logic is duplicated between
graphql_transport_ws(_process_extensions) andgraphql_ws.send_data_message; consider extracting a shared helper or aligning them through a common abstraction so the two protocols stay in sync as behavior evolves. - The current approach instantiates extensions (
ext()whenisinstance(ext, type)) on each result, which may diverge from how extensions are normally constructed and maintain state in the HTTP pipeline; it would be safer to reuse the same extension instances and lifecycle that are used for non-subscription operations. - In
send_nextyou only run extensions whenexecution_result.errorsis truthy, but other code paths may run_process_resultfor all results; consider matching the existing extension pipeline behavior so subscriptions are processed consistently with queries/mutations.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The extension processing logic is duplicated between `graphql_transport_ws` (`_process_extensions`) and `graphql_ws.send_data_message`; consider extracting a shared helper or aligning them through a common abstraction so the two protocols stay in sync as behavior evolves.
- The current approach instantiates extensions (`ext()` when `isinstance(ext, type)`) on each result, which may diverge from how extensions are normally constructed and maintain state in the HTTP pipeline; it would be safer to reuse the same extension instances and lifecycle that are used for non-subscription operations.
- In `send_next` you only run extensions when `execution_result.errors` is truthy, but other code paths may run `_process_result` for all results; consider matching the existing extension pipeline behavior so subscriptions are processed consistently with queries/mutations.
## Individual Comments
### Comment 1
<location path="strawberry/subscriptions/protocols/graphql_transport_ws/handlers.py" line_range="375-377" />
<code_context>
self.completed = False
self.task: asyncio.Task | None = None
+ def _process_extensions(self, execution_result: ExecutionResult) -> None:
+ """Run the execution result through any active schema extensions."""
+ if not execution_result.errors:
+ return
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The early-return on `execution_result.errors` doesn’t match the docstring and may skip useful extension hooks for successful results.
Right now, extensions won’t run for successful results, which contradicts the stated behavior and may break extensions that rely on seeing all results (e.g., logging/metrics/tracing). Consider either removing the `if not execution_result.errors` check or updating the docstring/contract to clarify that extensions only run on error cases.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| def _process_extensions(self, execution_result: ExecutionResult) -> None: | ||
| """Run the execution result through any active schema extensions.""" | ||
| if not execution_result.errors: |
There was a problem hiding this comment.
suggestion (bug_risk): The early-return on execution_result.errors doesn’t match the docstring and may skip useful extension hooks for successful results.
Right now, extensions won’t run for successful results, which contradicts the stated behavior and may break extensions that rely on seeing all results (e.g., logging/metrics/tracing). Consider either removing the if not execution_result.errors check or updating the docstring/contract to clarify that extensions only run on error cases.
Greptile SummaryThis PR fixes a real bug (issue #3680) where The approach works for
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant WSHandler
participant ExecutionEngine
participant ExtensionsRunner
participant MaskErrors
Note over Client,MaskErrors: Normal Query/Mutation path (working correctly)
Client->>WSHandler: HTTP POST /graphql
WSHandler->>ExtensionsRunner: on_operation() enter
WSHandler->>ExecutionEngine: execute()
ExecutionEngine-->>WSHandler: ExecutionResult
WSHandler->>ExtensionsRunner: on_operation() exit
ExtensionsRunner->>MaskErrors: _process_result(result) [via on_operation yield, with execution_context set]
MaskErrors-->>ExtensionsRunner: errors masked
WSHandler-->>Client: masked response
Note over Client,MaskErrors: Subscription path — this PR's fix
Client->>WSHandler: WebSocket subscribe
WSHandler->>ExecutionEngine: subscribe()
loop each yielded result
ExecutionEngine-->>WSHandler: ExecutionResult (raw)
WSHandler->>WSHandler: _process_extensions(result)
WSHandler->>MaskErrors: ext()._process_result(result) [NEW instance, no execution_context]
MaskErrors-->>WSHandler: errors masked (works for MaskErrors only)
WSHandler-->>Client: send_next / send_data_message
end
Last reviewed commit: 279388a |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def _process_extensions(self, execution_result: ExecutionResult) -> None: | ||
| """Run the execution result through any active schema extensions.""" | ||
| if not execution_result.errors: | ||
| return | ||
|
|
||
| extensions = getattr(self.handler.schema, "extensions", []) | ||
| for ext in extensions: | ||
| extension_instance = ext() if isinstance(ext, type) else ext | ||
| if hasattr(extension_instance, "_process_result"): | ||
| extension_instance._process_result(execution_result) |
There was a problem hiding this comment.
Freshly created instances never receive execution_context
The ext() call creates a brand-new extension instance, but the normal execution path (see schema.py line 591) explicitly sets extension.execution_context = execution_context on every instance after creation. This step is skipped here, so any extension whose _process_result accesses self.execution_context will raise an AttributeError at runtime.
MaskErrors._process_result happens to not use self.execution_context, which is why it works in the tests, but this is fragile for other extensions.
The correct approach is to reuse the already-configured extension instances from the current request's SchemaExtensionsRunner, or — at minimum — assign execution_context to each freshly created instance before calling _process_result.
def _process_extensions(self, execution_result: ExecutionResult) -> None:
"""Run the execution result through any active schema extensions."""
if not execution_result.errors:
return
extensions = getattr(self.handler.schema, "_async_extensions", [])
for extension_instance in extensions:
if hasattr(extension_instance, "_process_result"):
extension_instance._process_result(execution_result)| if execution_result.errors: | ||
| extensions = getattr(self.schema, "extensions", []) | ||
| for ext in extensions: | ||
| extension_instance = ext() if isinstance(ext, type) else ext | ||
| if hasattr(extension_instance, "_process_result"): | ||
| extension_instance._process_result(execution_result) |
There was a problem hiding this comment.
Same execution_context omission as in graphql_transport_ws
This inline block has the same problem: ext() creates a fresh extension instance with no execution_context, while the standard path sets extension.execution_context = execution_context before any lifecycle method is called. Extensions that access self.execution_context inside _process_result will fail with AttributeError.
Additionally, the logic is duplicated inline here rather than extracted to a shared helper (as was done in the graphql_transport_ws handler with _process_extensions), making future maintenance harder.
| if not execution_result.errors: | ||
| return |
There was a problem hiding this comment.
Early return silently skips non-error results for all extensions
The guard if not execution_result.errors: return prevents _process_result from ever being called on results that have no errors. While MaskErrors._process_result itself has the same guard internally, this outer early exit means any other extension whose _process_result needs to inspect or transform non-error results will be silently skipped.
The normal execution path (via on_operation) calls _process_result unconditionally and lets each extension decide what to do. Consider removing the guard here and letting each extension handle its own filtering, for consistency.
|
|
||
| @patch.object(MyExtension, "_process_result", create=True) | ||
| async def test_subscription_errors_trigger_extension_process_result( | ||
| mock: Mock, ws: WebSocketClient | ||
| ): | ||
| """Test that schema extensions are called to process results when a subscription yields an error.""" | ||
| await ws.send_message( | ||
| { | ||
| "id": "sub1", | ||
| "type": "subscribe", | ||
| "payload": { | ||
| "query": 'subscription { exception(message: "TEST EXC") }', | ||
| }, | ||
| } | ||
| ) | ||
|
|
||
| next_message: NextMessage = await ws.receive_json() | ||
|
|
||
| assert next_message["type"] == "next" | ||
| assert next_message["id"] == "sub1" | ||
| assert "errors" in next_message["payload"] | ||
|
|
||
| # Error intercepted and extension called |
There was a problem hiding this comment.
Test validates call count but not actual masking behaviour
The test patches _process_result with a no-op mock and asserts it was called once. This confirms the hook fires, but does not verify that error masking actually works end-to-end (e.g., that the error message is replaced with "Unexpected error." and that original exception details are not leaked to the client).
Consider adding a complementary integration test that uses a real MaskErrors extension and asserts the response contains the masked message rather than the raw exception text. This would catch regressions like the configuration-loss issue described in the handler comment.
Also note there is a missing blank line before the @patch.object decorator (PEP 8 E302).
| @patch.object(MyExtension, "_process_result", create=True) | |
| async def test_subscription_errors_trigger_extension_process_result( | |
| mock: Mock, ws: WebSocketClient | |
| ): | |
| """Test that schema extensions are called to process results when a subscription yields an error.""" | |
| await ws.send_message( | |
| { | |
| "id": "sub1", | |
| "type": "subscribe", | |
| "payload": { | |
| "query": 'subscription { exception(message: "TEST EXC") }', | |
| }, | |
| } | |
| ) | |
| next_message: NextMessage = await ws.receive_json() | |
| assert next_message["type"] == "next" | |
| assert next_message["id"] == "sub1" | |
| assert "errors" in next_message["payload"] | |
| # Error intercepted and extension called | |
| @patch.object(MyExtension, "_process_result", create=True) |
needs two blank lines after the previous test function body.
2 participants
Release type: patch
Fixes an issue where schema extensions (like
MaskErrors) were bypassed during WebSocket subscriptions. The extensions'_process_resulthooks are now properly triggered for each yielded result in bothgraphql-transport-wsandgraphql-wsprotocols, ensuring errors are correctly formatted before being sent to the client.Description
Fixes an issue where schema extensions (such as
MaskErrors) were being bypassed when streaming data over WebSockets.Previously, standard Queries and Mutations would pass their results through the extension pipeline, but Subscriptions would send raw
ExecutionResultobjects directly over the WebSocket. This caused internal/unmasked errors to leak to the client. This PR manually triggers_process_resulton active extensions right beforesend_nextandsend_data_messagedispatch the payload.Migration guide
No migration required.
Types of Changes
Checklist
Summary by Sourcery
Ensure WebSocket GraphQL subscriptions pass execution results through schema extensions so errors are consistently processed before being sent to clients.
Bug Fixes:
Documentation:
Tests: