Question regarding well-formedness of GQL over http responses

[duzinkie]

Hello,

When discussing some issues that appeared when trying to integrate https://github.com/apollographql/apollo-kotlin and https://github.com/google/cronet-transport-for-okhttp (specifically: https://github.com/apollographql/apollo-kotlin/issues/5885) and in the course of this discussion, we encountered something that we're not sure about how to interpret and would appreciate a clarification.

Specifically, the current implementation of the GQL client in https://github.com/apollographql/apollo-kotlin was only reading a json response up to point it reaches the end of the "logical" end of a json payload, and technically if there's anything else past that send in the response, it would be ignored. In practical applications we're working with, there's indeed nothing past that, but we're wondering what should be a client behavior if that's not the case - should such a response then by rejected by a client due to not being "well-formed" given the specs are mentioning it must be:

When a server receives a well-formed GraphQL-over-HTTP request, it must return a well‐formed GraphQL response

https://graphql.github.io/graphql-over-http/draft/#sec-Response

[benjie]

From a client POV if you send a well formed request to a trusted server, then you can assume either the response will be well formed or you'll get a non-200 status code. Since you trust the response to be well formed, it is reasonable to parse to the end of the object and then ignore any trailing data, which is expected to be whitespace only. If you wanted to be super careful then you could scan the remaining data for any non-whitespace characters and, if found, raise a warning. One reason you might want to do this, is we've discussed using JSONL for certain things, such as incremental delivery and subscriptions, and that is just object after object. But it would have a different Content-Type (+jsonl rather than +json) so you'd know this was what to expect.

https://gist.github.com/benjie/f696f494878ddebb423c978ccb3a39df

[martinbonnin]

Thanks for the insight @benjie !

If you wanted to be super careful then you could scan the remaining data for any non-whitespace characters and, if found, raise a warning

The question is whether this would be a warning or error. The robustness principle would aim towards "warning" for maximum compatibility but I see this principle questioned more and more. I was initially team "warning" but I'm starting to err on the "error" side...

If some server sends a malformed response (i.e. using the wrong content-type or sending extra objects or garbage) then better know it sooner than later?

[benjie]

Anyone not using a streaming JSON parser would raise an error, since the data isn't valid JSON. So if you want to aim to be as "standard" as possible you could do that.

But I'd argue that so long as you're checking response types, trailing data after the JSON is so unlikely from a trusted server, and even if it happens it's really unlikely it's being done to invalidate the response, that you should be fine just raising a warning. But also, it's so incredibly unlikely (unless something is majorly misconfigured) that an error would also be fine. My concern with error is you'd have to wait for it to handle it, whereas logging a warning can be done after the response completes.

[martinbonnin]

My concern with error is you'd have to wait for it to handle it, whereas logging a warning can be done after the response completes.

Not sure I'm following that one. Any extra JSON token after the GraphQL response would trigger either the warning/error. I think this would happen at the same time? It's just a matter of semantics whether we want to log something (warning) or error the operation (error)

{
  "data": {...}
}
{ // We know here that we have unexpected trailing data. Log warning or error the operation

[benjie]

Imagine {…} followed by a megabyte of whitespace and then {. You could process the result once the } is returned and render before the request even completes.