graphql-kotlin
graphql-kotlin copied to clipboard
ExpediaGroup
fix: move exception handling into ktor statuspages
:pencil: Description
Currently, the Ktor version of the GraphQL server swallows a base exception, which disallows any custom error responses. This PR moves exception handling to statuspages, which is the recommended error handler.
Note: This is a breaking change for people relying on the current functionality to handle errors
:link: Related Issues
https://github.com/ExpediaGroup/graphql-kotlin/issues/1920
Hey @curtiscook, I no longer have maintainer permissions on this repo so tagging @ExpediaGroup/graphql-kotlin-committers and @tapaderster for review.
But the code change looks good to me 👍🏻
I believe by changing error handling behavior you will force compatibility with GraphQL over HTTP spec to the end users and the lib will no longer be able to give you those guarantees.
I'll defer to @samuelAndalon and @tapaderster but IMHO the Spring and Ktor servers should provide users guarantees about the compliance with the GraphQL over HTTP spec.
I'll defer to @samuelAndalon and @tapaderster but IMHO the Spring and Ktor servers should provide users guarantees about the compliance with the GraphQL over HTTP spec.
Per https://github.com/graphql/graphql-over-http/blob/main/spec/GraphQLOverHTTP.md:
A server MAY forbid individual requests by a client to any endpoint for any reason, for example to require authentication or payment; when doing so it SHOULD use the relevant 4xx or 5xx status code. This decision SHOULD NOT be based on the contents of a well-formed GraphQL-over-HTTP request.
The current implementation of catching a base exception and returning a 400 swallows errors unrelated to the graphql request e.g. 401, 403, and 5xx errors all become 400s. This PR aims to move the error handling into a more appropriate place, so these sorts of errors can be properly processed.
If you're uncomfortable with this solution, one alternative could be to drop the internal visibility modifier so that it would be possible to override this error handling. As of right now, you have to rewrite the module to extend error handling.
Changes look good to me. @samuelAndalon since this is a breaking change do you want to merge lib updates first and cut another v7 version before starting on the v8-alpha?
lets do a release with this, and update dependencies in next major version, hopefully by end of this month graphql-java will release its next major version
@curtiscook could you address the ktlint errors
/home/runner/work/graphql-kotlin/graphql-kotlin/servers/graphql-kotlin-ktor-server/src/main/kotlin/com/expediagroup/graphql/server/ktor/GraphQLStatusPages.kt:34:60 Missing newline before "}"
/home/runner/work/graphql-kotlin/graphql-kotlin/servers/graphql-kotlin-ktor-server/src/main/kotlin/com/expediagroup/graphql/server/ktor/GraphQLStatusPages.kt:34:62 Missing newline before "}"
Will merge it after https://github.com/ExpediaGroup/graphql-kotlin/pull/1951