re: <@U0NT624JZ> &gt; One thing our custom graphql...

# graphql-kotlin

[Dariusz Kuc]

re: @rocketraman

One thing our custom graphql ktor layer has is an exception handler in our call handler i.e. the wrapper around the graphql server's execute call. This exception handle creates a standard GraphQLResponse for unhandled exceptions -- otherwise we found unexpected errors weren't handled properly by the client side (a 3rd party project using Elm). I haven't checked the PR but would we still have access to this extension point?

[Dariusz Kuc]

default logic is relying on the default GraphQLRequestHandler which should capture all execution exceptions -> https://github.com/ExpediaGroup/graphql-kotlin/blob/master/servers/graphql-kotlin-[…]/expediagroup/graphql/server/execution/GraphQLRequestHandler.kt

[Dariusz Kuc]

what sort of exceptions were you getting? i.e. server logic is

1. parse request
2. calculate context
3. execute/process request

[rocketraman]

Hmm I don't recall TBH, that was a while ago. I think the code you referenced is fine.

[Dariusz Kuc]

if there is a bad request you will get non-graphql response but that is expected

[rocketraman]

That might have been it.

[Dariusz Kuc]

we respond with Bad Request if it cannot be parsed -> https://github.com/ExpediaGroup/graphql-kotlin/blob/master/servers/graphql-kotlin-[…]src/main/kotlin/com/expediagroup/graphql/server/ktor/GraphQL.kt

[Dariusz Kuc]

which is matching https://github.com/graphql/graphql-over-http/blob/main/spec/GraphQLOverHTTP.md#status-codes

[rocketraman]

Why would an exception prevent the generation of a well-formed GraphQL response? I think the spec intends that for when the response generation itself fails.

[Dariusz Kuc]

yep thats the behavior -> if you got invalid request that we cannot process it will return HTTP 400,, if we can parse it then we return GraphQL response (which may contain errors) with HTTP 200

[rocketraman]

But that's my point -- why couldn't a valid GraphQL response be returned even if the request is invalid?

[Dariusz Kuc]

what would you return in that case?

[rocketraman]

A 200 response with a valid GraphQL response payload, with an error indicating the problem with the request.

[Dariusz Kuc]

from the spec

If the GraphQL response does not contain the {data} entry then the server MUST reply with a 4xx or 5xx status code as appropriate.

Note: The GraphQL specification indicates that the only situation in which the GraphQL response does not include the {data} entry is one in which the {errors} entry is populated.

If the GraphQL request is invalid (e.g. it is malformed, or does not pass validation) then the server SHOULD reply with 400 status code.

If the client is not permitted to issue the GraphQL request then the server SHOULD reply with 403, 401 or similar appropriate status code.

[Dariusz Kuc]

you could raise an issue/question against the spec

[rocketraman]

That section only applies if the response is still valid GraphQL. In other words, the body is content type application/graphql-response+json, along with the 4xx/5xx http status code. I don't believe that is what graphql-kotlin is doing.

[rocketraman]

This section only applies when the response body is to use the application/graphql-response+json media type.

[Dariusz Kuc]

Note: For compatibility with legacy servers, this specification allows the use of 4xx or 5xx status codes for failed requests where the response uses the application/json media type, but it is strongly discouraged. To use 4xx and 5xx status codes, please use the application/graphql-response+json media type.

[rocketraman]

Yep. But ktor-server is responding with bad request and no content as far as I can tell. Not even sure its using the "strongly discouraged" application/json type in that case.

[rocketraman]

Would you accept a PR here?

[Dariusz Kuc]

i'll defer to @Samuel Vazquez and other expedia folks but IMHO if the request is bad, i.e. it cannot be parsed to { "query": "some string", "variables": {}, "operationName": "someString" } then server should respond with BAD_REQUEST

we already ignore unknown fields so it would only fail IF incoming request does not specify query at all (or query field is not a string)

if the request can be parsed then we will attempt it to process it through graphql engine (which may throw validation errors)

[Samuel Vazquez]

I agree that it query is not able to be deserialized the server should response with 400, since that’s an issue with the transportation layer, as soon as string is able to be tokenized then any semantic error should be considered a GraphQL error and provide the error message on the GraphQL response payload