Incorrect HTTP code (200) on POST with empty string

[elbow-jason]

What happened:

When POSTing an empty string to the /query path the response comes back with a 200 HTTP code, but the json clearly indicates an error.

What I expected to happen:

When POSTing an empty string to the /query path the response comes back with a 400 or a 422 HTTP code to indicate that something was wrong with the request from the client.

An example:

$ curl -X POST http://127.0.0.1:8080/query -d "" -v
*   Trying 127.0.0.1...
* Connected to 127.0.0.1 (127.0.0.1) port 8080 (#0)
> POST /query HTTP/1.1
> Host: 127.0.0.1:8080
> User-Agent: curl/7.43.0
> Accept: */*
> Content-Length: 0
> Content-Type: application/x-www-form-urlencoded
>
< HTTP/1.1 200 OK
< Access-Control-Allow-Credentials: true
< Access-Control-Allow-Headers: Content-Type, Content-Length, Accept-Encoding, X-CSRF-Token,X-Auth-Token, Cache-Control, X-Requested-With
< Access-Control-Allow-Methods: POST, OPTIONS
< Access-Control-Allow-Origin: *
< Connection: close
< Date: Wed, 26 Jul 2017 08:04:40 GMT
< Content-Length: 71
< Content-Type: text/plain; charset=utf-8
<
* Closing connection 0
{"code":"ErrorInvalidRequest","message":"Invalid request encountered."}⏎

[manishrjain]

Would be good to check what GraphQL providers typically do in this case -- do they always send back 200, or do they change the http codes. From my understanding, they rely upon errors field: http://facebook.github.io/graphql/#sec-Data

[elbow-jason]

It looks like the convention in graphql is to ignore http codes and rely on the structure of the responses. This is due to some graphql servers being able to return partially successful results and errors simultaneously. In the facebook spec (the link above) they API is returning a map with two entries at it's root data for successful partial queries and errors for any reportable errors that occurred. It may be necessary to incorporate this scoping of data to data and errors to errors fields so that successful query results are clearly distinguishable from errors.

For example, I can imaging a scenario where I am storing my dgraph query errors in a dgraph itself (meta, right?) and I ask for code and message as fields in my query and the response is {"code":"ErrorInvalidRequest","message":"Invalid request encountered."}. The question is: Is this the good response to my query or an error?

[pawanrawal]

We could add data key to the response but I feel that just makes parsing a bit harder. For each block now you must access it through the data key.

[elbow-jason]

I agree, accessing all results through data is not ideal. But there must be something that differentiates a good response from a bad response. Without some method of differentiation we are left with a leaky abstraction. Perhaps the just the errors could be placed into errors and then document that it is not recommended to use a errors(func: ....) {} as a query?

[pawanrawal]

We can make sure that every time there is an error, the http status code is 400. Would that work?

[elbow-jason]

That would work very well!

[manishrjain]

Sorry, I disagree with @pawanrawal's assessment here. If GraphQL does 200 + {data, errors}, then we should do the same. They deliberately tried to stay away from REST's conception of using http codes to indicate issues, instead keeping http code OK, and returning a JSON response. So, we should stick to it. Over time, we want to be closer to how GraphQL does things than go further away from it.

[pawanrawal]

We now send an errors key in case there is an error. A successful response is nested under the data key. This is in accordance with the GraphQL spec.