fix: on_operation hooks unable to modify result on error. #3629
Conversation
Reviewer's Guide by SourceryThis pull request addresses an issue where on_operation hooks were unable to modify the result when an error occurred. The main changes involve restructuring the error handling logic in the execute function to allow for proper error processing and result modification. File-Level Changes
Tips
|
for more information, see https://pre-commit.ci
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @nrbnlulu - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues found
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| "message": "Unexpected error.", | ||
| "path": ["hiddenError"], | ||
| } | ||
| ] | ||
| def test_mask_some_errors(): |
There was a problem hiding this comment.
suggestion (testing): Consider adding an async version of test_mask_some_errors
To maintain consistency and ensure both sync and async paths are tested equally, it would be beneficial to add an async version of the test_mask_some_errors test case.
def test_mask_some_errors():
# ... existing code ...
import pytest
@pytest.mark.asyncio
async def test_mask_some_errors_async():
# ... async version of the test ...
strawberry/schema/execute.py
Outdated
| async with extensions_runner.operation(): | ||
| # Note: In graphql-core the schema would be validated here but in | ||
| # Strawberry we are validating it at initialisation time instead | ||
| try: |
There was a problem hiding this comment.
issue (complexity): Consider restructuring the code to reduce nesting and improve readability.
The restructuring has increased code complexity by adding an extra level of nesting. While this change might have been intended to ensure proper closure of the context manager, it can be achieved without increasing the nesting level. Consider using a try-finally block to maintain the original structure while ensuring the context manager is always exited properly:
async def execute(...) -> ExecutionResult | PreExecutionError:
try:
async with extensions_runner.operation():
# Note: In graphql-core the schema would be validated here but in
# Strawberry we are validating it at initialisation time instead
if errors := await _parse_and_validate_async(
execution_context, extensions_runner
):
return await _handle_execution_result(
execution_context, errors, extensions_runner, process_errors
)
assert execution_context.graphql_document
result = None
async with extensions_runner.executing():
if not execution_context.result:
result = await await_maybe(
original_execute(
schema,
execution_context.graphql_document,
root_value=execution_context.root_value,
middleware=middleware_manager,
variable_values=execution_context.variables,
operation_name=execution_context.operation_name,
context_value=execution_context.context,
execution_context_class=execution_context_class,
)
)
else:
result = execution_context.result
# return results after all the operation completed.
return await _handle_execution_result(
execution_context, result, extensions_runner, process_errors
)
except (MissingQueryError, InvalidOperationTypeError) as e:
raise e
except Exception as exc:
return await _handle_execution_result(
execution_context,
PreExecutionError(data=None, errors=[_coerce_error(exc)]),
extensions_runner,
process_errors,
)
finally:
await extensions_runner.operation().__aexit__(None, None, None)This approach maintains the original structure, reduces nesting, and ensures proper closure of the context manager in all cases, including when exceptions occur.
|
Thanks for adding the Here's a preview of the changelog: This release fixes an issue that prevented extensions to receive the result from Here's the tweet text: |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3629 +/- ##
=======================================
Coverage 96.75% 96.76%
=======================================
Files 521 521
Lines 33715 33732 +17
Branches 5622 5627 +5
=======================================
+ Hits 32622 32640 +18
+ Misses 861 860 -1
Partials 232 232 |
# Conflicts: # strawberry/schema/execute.py
CodSpeed Performance ReportMerging #3629 will not alter performanceComparing Summary
|
for more information, see https://pre-commit.ci
# Conflicts: # strawberry/schema/execute.py
for more information, see https://pre-commit.ci
Description
Types of Changes
Issues Fixed or Closed by This PR
fix: #3625
Checklist
Summary by Sourcery
Refactor the execute function to ensure on_operation hooks can modify the result on error and add a test to verify error masking in asynchronous operations.
Bug Fixes:
Tests: