Improve extensions error handling #3217

DoctorJohn · 2023-11-10T13:49:31Z

This PR brings schema extension error handling in line with field extension and resolver error handling.

Description

This PR alters schema extension error handling so that any error raised from within schema
extensions will abort the operation and is added to the execution result.

Before, most errors (including Strawberry/graphql-core GraphQLErrors) would cause an internal server error.

Furthermore, these changes enable extensions (e.g. the one described in #3204) to abort operations early in a requests lifecycle which was not easily possible before.

Types of Changes

Issues Fixed or Closed by This PR

Checklist

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
I have tested the changes and verified that they work and don't break anything (as well as I can manage).

codecov · 2023-11-10T13:53:20Z

Codecov Report

Merging #3217 (55d82c1) into main (87f6b47) will increase coverage by 0.01%.
Report is 5 commits behind head on main.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3217      +/-   ##
==========================================
+ Coverage   96.48%   96.49%   +0.01%     
==========================================
  Files         509      509              
  Lines       32703    32862     +159     
  Branches     5404     5421      +17     
==========================================
+ Hits        31552    31711     +159     
  Misses        940      940              
  Partials      211      211

codspeed-hq · 2023-11-10T13:54:45Z

CodSpeed Performance Report

Merging #3217 will improve performances by 13.87%

_{Comparing DoctorJohn:improve-extensions-error-handling (55d82c1) with main (861cc0d)}

Summary

⚡ 1 improvements
✅ 12 untouched benchmarks

Benchmarks breakdown

	Benchmark	`main`	`DoctorJohn:improve-extensions-error-handling`	Change
⚡	`test_execute_basic`	11.2 ms	9.9 ms	+13.87%

erikwrede · 2023-11-10T19:24:30Z

I am in favor of addressing #3204 in the same go, as the changing of the current mechanism might create some breakage for existing web views which we could mitigate by integrating the changes required from #3204. Should only be a few lines after all.

DoctorJohn · 2023-11-10T19:35:07Z

I am in favor of addressing #3204 in the same go, as the changing of the current mechanism might create some breakage for existing web views which we could mitigate by integrating the changes required from #3204. Should only be a few lines after all.

Just to make sure we're on the same page: In its current state this PR does not change the current mechanism, since it's only removing an unnecessary "fallback" except block wrapping a single call to the graphql-core parse function.

Should only be a few lines after all

Yup, pretty straightforward changes. Making sure they're covered by tests this time.

botberry · 2023-11-15T01:31:16Z

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:

Starting with this release, any error raised from within schema
extensions will abort the operation and is returned to the client.

This corresponds to the way we already handle field extension errors
and resolver errors.

This is particular useful for schema extensions performing checks early
in the request lifecycle, for example:

class MaxQueryLengthExtension(SchemaExtension):
    MAX_QUERY_LENGTH = 8192

    async def on_operation(self):
        if len(self.execution_context.query) > self.MAX_QUERY_LENGTH:
            raise StrawberryGraphQLError(message="Query too large")
        yield

Here's the tweet text:

🆕 Release (next) is out! Thanks to @NucleonJohn for the PR 👏

Get it here 👉 https://strawberry.rocks/release/(next)

strawberry/schema/execute.py

DoctorJohn · 2023-11-15T01:34:08Z

strawberry/schema/execute.py

+                        # only return a sanitised version to the client.
+                        process_errors(result.errors, execution_context)
+
+    except (MissingQueryError, InvalidOperationTypeError) as e:


These two exceptions get special handling by our view integrations and enables them to return appropriate status codes. For context:

strawberry/strawberry/http/sync_base_view.py

Lines 198 to 209 in d6bd38d

try:

result = self.execute_operation(

request=request,

context=context,

root_value=root_value,

)

except InvalidOperationTypeError as e:

raise HTTPException(

400, e.as_http_error_reason(request_adapter.method)

) from e

except MissingQueryError as e:

raise HTTPException(400, "No GraphQL query found in the request") from e

I understand that this solves the problem for now. However, I doubt that this is a long term solution due to the specificity of re-raising only specific exceptions. We should try to abstract that into an exception -> status code or modified response or graphQL error "middleware/exception filter" as a follow up to this PR.

something like an OperationError? 😊

let's do in another PR/refactoring 😊

patrick91 · 2023-11-27T10:48:28Z

/pre-release

botberry · 2023-11-27T10:49:28Z

Pre-release

👋

Pre-release 0.220.0.dev.1709543239 [e3b40c0] has been released on PyPi! 🚀
You can try it by doing:

poetry add strawberry-graphql==0.220.0.dev.1709543239

imconnorngl · 2023-11-27T12:56:22Z

After testing this compared to the functionality of #3256. It seems like this PR is more verbose than I'd like in best case, as seen below

Before:

After:

DoctorJohn · 2023-11-27T20:17:02Z

After testing this compared to the functionality of #3256. It seems like this PR is more verbose than I'd like in best case

Thanks for testing the pre-release! This PR currently wraps all raised errors in a GraphQLError, even when the raised error aleady is a GraphQLError. Currently testing whether not doing that yields the expected result :)

DoctorJohn · 2023-11-30T18:13:56Z

/pre-release

patrick91 · 2023-11-30T18:15:13Z

/pre-release

patrick91 · 2024-03-04T09:06:34Z

/pre-release

patrick91 · 2024-04-14T07:51:10Z

I've merged main into this, but something has changed 😊 I'll take a proper look at this in the following days

patrick91 · 2024-04-15T16:08:27Z

tests/schema/extensions/test_extensions.py

@@ -721,7 +725,7 @@ def ping(self) -> str:
 @pytest.mark.parametrize(
    ("failing_hook", "expected_hooks"),
    (
-        ("on_operation_start", set()),
+        ("on_operation_start", {2, 3, 4, 5, 6, 7}),


@erikwrede do you think this makes sense?

we run all the hooks even if the execution stops?

mmh, looking at the current failure it looks like what we were trying to do is not working as I though

we can't stop an operation from an extension if we catch all exceptions, unless I'm missing something

strawberry/extensions/context.py

patrick91

Thank you so much for this, sorry it took so long to do this 😊

DoctorJohn added 2 commits November 10, 2023 14:14

Clarify names of test cases

8793d4a

Remove unnecessary except block

5a88374

DoctorJohn force-pushed the improve-extensions-error-handling branch 3 times, most recently from 32331a0 to cbebf5e Compare November 14, 2023 01:36

DoctorJohn added 2 commits November 14, 2023 02:39

Handle exceptions raised within extension hooks

1e34746

Adjust tests depending on previous behaviour

0379a33

DoctorJohn force-pushed the improve-extensions-error-handling branch from cbebf5e to 0379a33 Compare November 14, 2023 01:39

DoctorJohn added 3 commits November 14, 2023 02:45

Make ruff happy :)

a953610

Lint

1f360f5

Add release file

37e4d9e

DoctorJohn marked this pull request as ready for review November 15, 2023 01:30

botberry added bot:has-release-file bot:release-type-minor labels Nov 15, 2023

DoctorJohn commented Nov 15, 2023

View reviewed changes

strawberry/schema/execute.py Show resolved Hide resolved

DoctorJohn commented Nov 15, 2023

View reviewed changes

imconnorngl and others added 2 commits November 27, 2023 11:44

chore: add tests for syntax errors on parser cache

56e375f

Update test

a58e58b

patrick91 mentioned this pull request Nov 27, 2023

Fix ParserCache Syntax Errors #3256

Closed

11 tasks

Add # pragma: no cover

4efd893

Fix graphql errors were unnecessarily wrapped

4c41b17

strawberry-graphql deleted a comment from botberry Nov 30, 2023

Merge branch 'main' into improve-extensions-error-handling

e3b40c0

patrick91 added 3 commits April 13, 2024 18:23

Merge branch 'main' into improve-extensions-error-handling

4a2e016

Restore test

9ef1f75

Restore

8d2eae4

Pending exceptions

8769d88

patrick91 reviewed Apr 15, 2024

View reviewed changes

strawberry/extensions/context.py Outdated Show resolved Hide resolved

patrick91 added 5 commits April 15, 2024 18:09

Restore code

ef8a20c

Quit after one exception

6dd8acf

Fix type

feee3d1

Remove pending exception

55e4bae

Fix test

55d82c1

patrick91 approved these changes Apr 17, 2024

View reviewed changes

patrick91 merged commit 63dfc89 into strawberry-graphql:main Apr 17, 2024
64 checks passed

DoctorJohn deleted the improve-extensions-error-handling branch November 20, 2024 16:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve extensions error handling #3217

Improve extensions error handling #3217

DoctorJohn commented Nov 10, 2023 •

edited

Loading

codecov bot commented Nov 10, 2023 •

edited

Loading

codspeed-hq bot commented Nov 10, 2023 •

edited

Loading

erikwrede commented Nov 10, 2023

DoctorJohn commented Nov 10, 2023 •

edited

Loading

botberry commented Nov 15, 2023 •

edited

Loading

DoctorJohn Nov 15, 2023

erikwrede Nov 15, 2023

patrick91 Apr 13, 2024

patrick91 Apr 17, 2024

patrick91 commented Nov 27, 2023

botberry commented Nov 27, 2023 •

edited

Loading

imconnorngl commented Nov 27, 2023

DoctorJohn commented Nov 27, 2023

DoctorJohn commented Nov 30, 2023

patrick91 commented Nov 30, 2023

patrick91 commented Mar 4, 2024

patrick91 commented Apr 14, 2024

patrick91 Apr 15, 2024

patrick91 Apr 15, 2024

patrick91 left a comment

	try:
	result = self.execute_operation(
	request=request,
	context=context,
	root_value=root_value,
	)
	except InvalidOperationTypeError as e:
	raise HTTPException(
	400, e.as_http_error_reason(request_adapter.method)
	) from e
	except MissingQueryError as e:
	raise HTTPException(400, "No GraphQL query found in the request") from e

Improve extensions error handling #3217

Improve extensions error handling #3217

Conversation

DoctorJohn commented Nov 10, 2023 • edited Loading

Description

Types of Changes

Issues Fixed or Closed by This PR

Checklist

codecov bot commented Nov 10, 2023 • edited Loading

Codecov Report

codspeed-hq bot commented Nov 10, 2023 • edited Loading

CodSpeed Performance Report

Merging #3217 will improve performances by 13.87%

Summary

Benchmarks breakdown

erikwrede commented Nov 10, 2023

DoctorJohn commented Nov 10, 2023 • edited Loading

botberry commented Nov 15, 2023 • edited Loading

DoctorJohn Nov 15, 2023

Choose a reason for hiding this comment

erikwrede Nov 15, 2023

Choose a reason for hiding this comment

patrick91 Apr 13, 2024

Choose a reason for hiding this comment

patrick91 Apr 17, 2024

Choose a reason for hiding this comment

patrick91 commented Nov 27, 2023

botberry commented Nov 27, 2023 • edited Loading

Pre-release

imconnorngl commented Nov 27, 2023

DoctorJohn commented Nov 27, 2023

DoctorJohn commented Nov 30, 2023

patrick91 commented Nov 30, 2023

patrick91 commented Mar 4, 2024

patrick91 commented Apr 14, 2024

patrick91 Apr 15, 2024

Choose a reason for hiding this comment

patrick91 Apr 15, 2024

Choose a reason for hiding this comment

patrick91 left a comment

Choose a reason for hiding this comment

DoctorJohn commented Nov 10, 2023 •

edited

Loading

codecov bot commented Nov 10, 2023 •

edited

Loading

codspeed-hq bot commented Nov 10, 2023 •

edited

Loading

DoctorJohn commented Nov 10, 2023 •

edited

Loading

botberry commented Nov 15, 2023 •

edited

Loading

botberry commented Nov 27, 2023 •

edited

Loading