Fix MaskErrors for synchronous pre-execution errors - #3968
Conversation
Reviewer's GuideRefactor the MaskErrors extension by extracting common error masking logic into a new helper and applying it consistently to both pre-execution and execution errors in on_operation, along with adding a corresponding release note. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
I've still got to properly test this, but I thought I'd open a draft pull request to get some initial feedback |
|
Look alright! But I'll do a proper review once we get tests :D |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3968 +/- ##
==========================================
+ Coverage 94.40% 94.41% +0.01%
==========================================
Files 528 528
Lines 34371 34368 -3
Branches 1803 1801 -2
==========================================
+ Hits 32449 32450 +1
+ Misses 1630 1627 -3
+ Partials 292 291 -1 🚀 New features to boost your workflow:
|
Yeah, need to look into how to set them up for the extension as there weren't any originally |
|
Hey @dextermb are you still working on this PR? I can provide a test if needed - I'm just not sure how i can contribute to another author branch :) Test: But i found another code smell in https://github.com/strawberry-graphql/strawberry/blob/main/strawberry/schema/schema.py#L602. This returns an |
Hi, I haven't had chance to loop back around to this. Happy to continue to contribute. If you want to chat off GitHub my Discord username is "dmb". Work wise, I'll likely have more time on this in the coming month(s). |
|
Thanks for adding the Below is the changelog that will be used for the release. This release fixes an issue where Synchronous execution now masks pre-execution errors consistently with This release was contributed by @dextermb in #3968 Additional contributors: @patrick91 |
a847ce4 to
f8af92a
Compare
|
@Speedy1991 can you check this? :D @dextermb I've updated this a bit, hope that's ok! |
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tests/schema/extensions/test_mask_errors.py" line_range="24-29" />
<code_context>
+ def test_field(self) -> str:
+ return "TestField"
+
+ schema = strawberry.Schema(
+ query=Query,
+ extensions=[ValidationCache, MaskErrors],
+ )
+
+ result = schema.execute_sync(query)
+
+ assert result.errors is not None
</code_context>
<issue_to_address>
**issue (testing):** Add an explicit regression test that exercises ValidationCache on a cache hit (same failing query executed twice).
This test only runs each invalid query once, so it never exercises the cached-validation path. To cover the regression scenario (including when `ValidationCache` returns a cached result), please add a test that runs the same invalid query twice against the same schema, e.g.:
```python
query = "query { missingField }"
result1 = schema.execute_sync(query)
result2 = schema.execute_sync(query)
for result in (result1, result2):
assert result.errors is not None
assert [e.message for e in result.errors] == ["Unexpected error."]
```
This validates masking on both the initial and cached validation results.
</issue_to_address>
### Comment 2
<location path="tests/schema/extensions/test_mask_errors.py" line_range="29-32" />
<code_context>
+ extensions=[ValidationCache, MaskErrors],
+ )
+
+ result = schema.execute_sync(query)
+
+ assert result.errors is not None
+ assert [error.message for error in result.errors] == ["Unexpected error."]
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Also assert that `result.data` is `None` to prove the error is truly pre-execution.
Since these tests target pre-execution errors, add an assertion that `result.data is None` to verify no resolvers ran. This will cause the test to fail if execution starts happening even though the error message remains masked.
```suggestion
result = schema.execute_sync(query)
assert result.data is None
assert result.errors is not None
assert [error.message for error in result.errors] == ["Unexpected error."]
```
</issue_to_address>
### Comment 3
<location path="RELEASE.md" line_range="14-15" />
<code_context>
+ error details.
+---
+
+This release fixes `MaskErrors` leaking parsing and validation error details
+during synchronous execution.
+
+Synchronous execution now masks pre-execution errors consistently with
</code_context>
<issue_to_address>
**suggestion (typo):** Consider rephrasing this sentence to improve grammatical clarity around "fixes `MaskErrors` leaking".
The phrase "fixes `MaskErrors` leaking parsing and validation error details" is grammatically awkward. Consider, for example: "This release fixes an issue where `MaskErrors` leaked parsing and validation error details during synchronous execution."
```suggestion
This release fixes an issue where `MaskErrors` leaked parsing and validation error details during synchronous execution.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Greptile SummaryThis PR extends error masking to synchronous parsing and validation failures. The main changes are:
Confidence Score: 5/5This looks safe to merge after a small test coverage cleanup.
tests/schema/extensions/test_mask_errors.py Important Files Changed
Reviews (1): Last reviewed commit: "Align sync pre-execution result lifecycl..." | Re-trigger Greptile |
|
This PR was published as 0.323.2. Thank you for contributing! |
Description
This fixes
MaskErrorsso parsing and validation errors are also masked during synchronous execution, including whenValidationCachesupplies the validation result. 🍓The sync path previously returned its pre-execution result from inside the operation lifecycle. After-yield extension hooks could update
execution_context.pre_execution_errors, but the already-created return object retained the original errors.This update:
MaskErrorsprocesspre_execution_errorswhen no execution result existsFixes #3844.
Type of change
Validation
uv run pytest tests/schema/extensions/test_mask_errors.py -q— 9 passeduv run pytest tests/schema/extensions/ -q— 174 passed, 10 skippeduv run mypy --config-file mypy.ini— 240 source files cleanuv run ruff check .— passeduv run ruff format --check .— 647 files formattedSummary by Sourcery
Fix masking of pre-execution errors during synchronous execution so error details are consistently hidden, and add regression coverage and release notes for this behavior.
Bug Fixes:
Enhancements:
Documentation:
Tests: