Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Defra Server GQL Endpoint Bugs Out Using GQL Clients #367

Closed
Tracked by #372
shahzadlone opened this issue Apr 20, 2022 · 5 comments · Fixed by #382
Closed
Tracked by #372

Defra Server GQL Endpoint Bugs Out Using GQL Clients #367

shahzadlone opened this issue Apr 20, 2022 · 5 comments · Fixed by #382
Assignees
Labels
area/api Related to the external API component bug Something isn't working

Comments

@shahzadlone
Copy link
Member

shahzadlone commented Apr 20, 2022

Tried these and only Altair Client worked for me. I ensured that it was using GET request for all of them.

  • Insomnia ❌
  • Postman ❌
  • GraphQL Playground ❌
  • GraphQL Editor ❌
  • Apollo Studio ❌
  • Altair Client ✅

On a little bit investigation found out that most of them (that didn't work) returned this error:

"Query is missing query or mutation statements"

from: query/graphql/planner/planner.go

@shahzadlone shahzadlone added the bug Something isn't working label Apr 20, 2022
@shahzadlone shahzadlone added this to the DefraDB v0.4 milestone Apr 20, 2022
@shahzadlone shahzadlone changed the title Server GQL Endpoint Bugs Out Using GQL Clients Defra Server GQL Endpoint Bugs Out Using GQL Clients Apr 20, 2022
@jsimnz
Copy link
Member

jsimnz commented Apr 22, 2022

@shahzadlone
Copy link
Member Author

The quick fix I did that fixes this for a few clients (tested Insomnia so far) is here in this branch (will leave it for reference): https://github.com/sourcenetwork/defradb/tree/lone/367/fix/handle-request-body-from-gql-clients

All the fix does is it parses the request body on a GET request. Probably not ideal, as discussed on our last call however, since we only use GET request right now, some of the GQL clients send the data in the request body when making a GET request.

This would be cleaned up / hopefully be moved to a POST request as a sub-task of: #372

@fredcarle

@shahzadlone shahzadlone mentioned this issue Apr 26, 2022
1 task
@shahzadlone shahzadlone added the area/api Related to the external API component label Apr 26, 2022
@shahzadlone
Copy link
Member Author

Also wanted to mention for example how the handler from https://github.com/graphql-go/handler works, obviously for POST request body not GET:

The handler will accept requests with
the parameters:

  • query: A string GraphQL document to be executed.

  • variables: The runtime values to use for any GraphQL query variables
    as a JSON object.

  • operationName: If the provided query contains multiple named
    operations, this specifies which operation should be executed. If not
    provided, an 400 error will be returned if the query contains multiple
    named operations.

GraphQL will first look for each parameter in the URL's query-string:

/graphql?query=query+getUser($id:ID){user(id:$id){name}}&variables={"id":"4"}

If not found in the query-string, it will look in the POST request body.
The handler will interpret it
depending on the provided Content-Type header.

  • application/json: the POST body will be parsed as a JSON
    object of parameters.

  • application/x-www-form-urlencoded: this POST body will be
    parsed as a url-encoded string of key-value pairs.

  • application/graphql: The POST body will be parsed as GraphQL
    query string, which provides the query parameter.

@fredcarle
Copy link
Collaborator

@shahzadlone if you want you can test it out with the HTTP API refactor branch https://github.com/sourcenetwork/defradb/tree/fredcarle/refactor/I372-http-api.

We now accept both Get and Post request for the graphql endpoint. On a Get we parse the query parameter and on a Post we parse the request body. We assume application/graphql for all content types. Should we handle application/json and application/x-www-form-urlencoded specifically?

@shahzadlone
Copy link
Member Author

shahzadlone commented Apr 27, 2022

I think application/json, and application/text should be handled but it should just work without specifically handling it ? Funnily most GQL client I played with use application/json or application/text, but I just tested your branch with Altair and Insomnia.

As I mentioned above the method most GQL servers (like graphql-go/handler I mentioned above) look for POST request body after also checking the query parameter. Since your branch only does this on the GET request Insomnia Client is broken (i.e. doesn't work with GET requests with DefraDB). On the contrast Altair is broken for POST requests. Hunch is Insomnia always sends GQL requests in the request body, and Altair always uses query parameters.

We are definitely following the GQL standard (the one @jsimnz posted: graphql.org/learn/serving-over-http) but the issue is different GQL clients do different things and obviously we can't tell them what to do LOL (maybe we can change some preferences within them? but I am using them in their default settings).

One solution might be to do what I did in that commit: 8e93274

Basically, support query parameters and request body with both POST and GET requests, super quick and fast fix. This might seem non-sensible at first LOL but at least we won't have to worry about which GQL clients work with POST requests and which work with GET. This way our usage documentation also becomes simpler i.e. "use POST or GET they behave the same".

Another reason to support my above suggestion would be when we open source, new users of DefraDB might already have committed to a certain GQL client which they don't want to switch from. The onboarding experience of things just working rather than tinkering around to find if to use GET or POST can be frustrating.

There are exceptions where this can be bad, if we wanted to differentiate and provide different functionality with GET and POST for example to only limit mutation requests to be over POST and query requests to be over GET. However I would strongly suggest we don't do that, as again we can't guarantee what 3rd party GQL client does what.

Any who open to discussion, for other possible solutions.

fredcarle added a commit that referenced this issue May 6, 2022
fredcarle added a commit that referenced this issue May 12, 2022
Resolves #372
Resolves #367

This PR restructures the api/http package with idiomatic patterns and a few useful utility functions.
shahzadlone pushed a commit to shahzadlone/defradb that referenced this issue Feb 23, 2024
shahzadlone pushed a commit to shahzadlone/defradb that referenced this issue Feb 23, 2024
Resolves sourcenetwork#372
Resolves sourcenetwork#367

This PR restructures the api/http package with idiomatic patterns and a few useful utility functions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Related to the external API component bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants