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

wrong status code when schema parse error #554

Closed
mydearxym opened this issue May 21, 2018 · 5 comments
Closed

wrong status code when schema parse error #554

mydearxym opened this issue May 21, 2018 · 5 comments

Comments

@mydearxym
Copy link

mydearxym commented May 21, 2018

Environment

  • Elixir version : 1.6.4
  • Absinthe version : 1.4.2
  • Client Framework and version: Apollo-client (latest)

Expected behavior

should return a 200 status code with error filed contains parse error infos

Actual behavior

when i query with missing arg, the server return a 400 status code, which cause graphql-client treat this as Network-Error instead of GraphQL-Parse-Error

image

a 400 status code is returned
image

Relevant Schema/Middleware Code

.... 
field :github_signin, :token_info do
   arg(:code, non_null(:string))
     
   resolve(&Resolvers.Accounts.github_signin/3)
end
....
@benwilson512
Copy link
Contributor

The 400 status code was definitely what the spec required at some point in the past, but I haven't looked lately. Let me see if I can find the relevant bit.

@mydearxym
Copy link
Author

thanks @benwilson512, but i don't think there is relevant bit in the spec .

quote from this issue

As far as I can tell from the GraphQL spec there is no discussion of http response codes at all. The status codes of HTTP are strongly tied to rest principles. It's not clear to me whether GraphQL APIs should distinguish between 2XX and 4XX status codes.

Most notably, you can batch multiple mutations into the same request to the server. If one successfully creates a resource (201), one mutates a resource (200), one fails validation (400) and another the target object does not exists (404), what should the return value from the API be?

At the moment, GraphQL APIs seem to err on the side of 200 unless the server blows up, in which case 500.

related issue: GraphiQL expects 200 status for errors

@hubertlepicki
Copy link

hubertlepicki commented Jun 6, 2018

I have created a PR to resolve that... but I am not 100% convinced what we should be doing here.

I am leaning towards returning 200 OK unless the server blows up, mostly because of good point above about batch mutations.

The other thing is that HTTP is just a transport layer, and as such transport layer should be invisible for the applications that use it. GraphQL specification seems to not to mention HTTP status codes at all.

But HTTP status code used here has a meaning on a transport layer: "request-response cycle was successful".

When we return 400 on wrong requests we are also giving it protocol-layer meaning.

Now if we keep doing that, swapping GraphQL transport layers from say HTTP to websocket or something else might not be as easy, becausue our application code relies on status codes from transport layer.. that might not be there at all in case of protocol different than HTTP...

@mydearxym
Copy link
Author

mydearxym commented Jun 7, 2018

thanks @hubertlepicki
your thought about HTTP make sence, but currently return 200 is best for the front-end tooling. what you think ? @benwilson512 @bruce

@benwilson512
Copy link
Contributor

Will review the plug PR. Technically this is just a plug issue since Absinthe doesn't know anything about status codes, so closing this for now.

mydearxym added a commit to coderplanets/coderplanets_server that referenced this issue Jul 14, 2019
mydearxym added a commit to coderplanets/coderplanets_server that referenced this issue Jul 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants