https://kotlinlang.org logo
#graphql-kotlin
Title
# graphql-kotlin
d

Dariusz Kuc

02/14/2023, 10:23 PM
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?
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
what sort of exceptions were you getting? i.e. server logic is
Copy code
1. parse request
2. calculate context
3. execute/process request
r

rocketraman

02/14/2023, 11:03 PM
Hmm I don't recall TBH, that was a while ago. I think the code you referenced is fine.
d

Dariusz Kuc

02/14/2023, 11:04 PM
if there is a bad request you will get non-graphql response but that is expected
r

rocketraman

02/14/2023, 11:04 PM
That might have been it.
r

rocketraman

02/14/2023, 11:11 PM
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.
d

Dariusz Kuc

02/14/2023, 11:12 PM
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
r

rocketraman

02/14/2023, 11:12 PM
But that's my point -- why couldn't a valid GraphQL response be returned even if the request is invalid?
d

Dariusz Kuc

02/14/2023, 11:13 PM
what would you return in that case?
r

rocketraman

02/14/2023, 11:13 PM
A 200 response with a valid GraphQL response payload, with an error indicating the problem with the request.
d

Dariusz Kuc

02/14/2023, 11:15 PM
from the spec
Copy code
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.
you could raise an issue/question against the spec
r

rocketraman

02/14/2023, 11:16 PM
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.
This section only applies when the response body is to use the application/graphql-response+json media type.
d

Dariusz Kuc

02/14/2023, 11:18 PM
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.
r

rocketraman

02/14/2023, 11:22 PM
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.
Would you accept a PR here?
d

Dariusz Kuc

02/15/2023, 1:24 AM
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)
s

Samuel Vazquez

02/15/2023, 1:44 AM
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
6 Views