dgraph-io/dgraph

Make HTTP endpoint compatible with GraphQL best practices

Closed this issue · 7 comments

paulftw commented

This is a Design Doc for Dgraph v1.1 - a breaking change to make HTTP API more compatible with GraphQL.

Further reading:

In v1.1 Dgraph HTTP endpoints must have following properties:

/query

  1. URL /query accepts HTTP POST with Content-Type application/json and POST body in JSON format:
{
  "query": "...",
  "variables": { "myVariable": "someValue", ... },
  "startTs": <number>,
  "debug": <true/false>,
}

  1. JSON field "query" is required, all other fields are optional.

  2. If the "application/graphql" Content-Type header is present, treat the HTTP POST body contents as the GraphQL query string and read all other fields from the query string e.g. /query?variables=...&startTs=...&debug=true

/mutate

  1. /mutate no longer accepts X-Dgraph-... custom headers. OPTIONS response no longer advertises the headers, if a POST request is received with any of these headers, their presence and values are ignored.

  2. As of dgraph v1.0.13 /mutate accepts X-Dgraph-CommitNow and X-Dgraph-MutationType. Mutation type can be inferred from the Content Type header and thus no replacement is necessary.
    CommitNow can be specified on the URL string: /mutate/<startTs>?commitNow=true

/commit

/commit does not use any of the X-Dgraph-... so no changes to its behavior are necessary.

paulftw commented

@manishrjain @danielmai I've thought about all the facts a bit more and added support for Dgraph transactions. Not sure if I added it in a clean way but in general one network request is better than two.

Let me know if you think this spec should be altered in any way

manishrjain commented

The endpoint naming shouldn't be changed, I'm not sure why that's part of the proposal. Until we're fully GraphQL compatible, we can't call it /graphql.

My understanding of this proposal was that we'll focus on the unnecessary HTTP headers and move them out to either query parameters or to the body, based on suggestions from GraphQL. But, the proposal is trying to build a unified theory of everything that we do currently. I don't see why such a dire change is needed. I'm going to mark this as wontfix until this proposal is simplified.

paulftw commented

@manishrjain sorry, seems like I've misinterpreted what goal we've agreed on.

Please take another look. I've removed all changes not related to the custom headers.

(Originally I wanted to minimize future API changes needed to make Dgraph API more GraphQL-like and one small step in that direction lead to another small step... before too long it grew into the big proposal I've submitted).

manishrjain commented

I think this proposal looks good. Maybe @srfrog can pick it up.

  • 👍1
mangalaman93 commented

Two questions -

  • /query endpoint, variables cannot be part of the URL in a way described. It has to be something like this /query?var1=a&var2=v....&startTs=...&debug=true. I am wondering how can a json be included in the URL for value of the variable variables.
  • There is inconsistency in the way startTs is provided in /mutate and /query endpoint. In /mutate, it is provided as part of the URL whereas in /query endpoint it is provided in the header. Is that expected?
paulftw commented

@mangalaman93
As for the /query endpoint the main reason to move vars to the URL was encoding issues, no matter what we do we'll need to URL encode var values (they can contain unicode chars, equal signs, question marks etc.).

One way to have JSON in the URL is to URL encode the whole JSON string:

?variables=%7Bvar1%3A%20%22foo%22%2C%20var2%3A%20%22%D0%AE%D0%BD%D0%B8%D0%BA%D0%BE%D0%B4%22%7D

is equivalent to

  variables = '{var1: "foo", var2: "Юникод"}'

Go server could json unmarshall the string into dictionary.

I'm not sure if var names in URL strings accept all the characters that GraphQL+- allows in variable names. If yes - your alternative would work, with one exception - user will not be allowed to use var names debug, startTs etc. More importantly if we decide to introduce a new URL param later it'd be a breaking change for all queries. Encoding whole JSON gives us a clean isolated namespace for vars.

Inconsistency in the way startTs - yes this is one of quite a few inconsistencies in the our HTTP endpoints (I think original version of this proposal has a large list of issues we could be fixing).

Manish wanted us to focus on headers only in this change.

manishrjain commented 
/query?startTs=X&var1=Y

If var1 == startTs, then there’s a conflict.

/query?variables={var1=Y ??

If variables → send application/json
If normal query → send json or application/graphql.

And the rest would be encoded in the payload, using the two means.

Current:
commit now is a header.
Mutation type is a header.

Proposed:
/query?startTs=X&debug=true
Mutation type → Content type header.
/mutate?startTs=X&commitNow=true
/commit?startTs=X&abort=true Body is JSON.