Throw errors to be handled by koa error handler rather than formatting and sending them to the client with a 200

[madole]

Throw errors to be handled by koa error handler rather than formatting and sending them to the client with a 200.

That way the client can respond to failed requests based on HTTP status codes rather than checking for stack in the returned data

[chentsulin]

You will get status 400 or 500 if error be thrown: https://github.com/chentsulin/koa-graphql/blob/master/src/index.js#L198 https://github.com/chentsulin/koa-graphql/blob/master/src/index.js#L206 https://github.com/chentsulin/koa-graphql/blob/master/src/index.js#L244 https://github.com/chentsulin/koa-graphql/blob/master/src/index.js#L267

[kaareal]

I am seeing the same thing getting an response with { data: {...}, errors: [{...}] } in both koa-graphql and express-graphql, with status code 200. This seems to happen when there is a bug in the schema e.g. reference issues. I am quite new to graphql and I am wondering if this is by design => that its correct to return 200 when some data could be delivered ? I can write up test if that has any interest.

[chentsulin]

@kaareal It's great to have a reproduce example if you can write it up.

It's weird because only those four lines of code assign errors to the result, and they also assign 400, 500 status to the response:

https://github.com/chentsulin/koa-graphql/blob/master/src/index.js#L198-L199 https://github.com/chentsulin/koa-graphql/blob/master/src/index.js#L206-L207 https://github.com/chentsulin/koa-graphql/blob/master/src/index.js#L244-L245 https://github.com/chentsulin/koa-graphql/blob/master/src/index.js#L267-L268

[kaareal]

after getting ready to write a test i noticed in your test files, there is already a test covering this https://github.com/kaareal/koa-graphql/blob/master/src/__tests__/http-test.js#L905 so i am thinking this might be desired, though i don't understand the reasoning.

[madole]

I'd like to understand the reasoning too, I'd prefer if any errors chucked a ~500/~400.

[chentsulin]

@leebyron @stubailo do you know the reason why we provide status 200 when error be thrown in schema?

[leebyron]

If any data exists, then 200 is appropriate. That is some errors were transient or partial and there is still data to be provided. If there is no data then an error code is appropriate, perhaps for a syntax error, validation error, or the rare query error which manages to bubble up and null out data

[stubailo]

At the end of the day the HTTP transport is not specified so can be application specific. It depends on what you are intending to use the http code for - is it caching? Displaying errors in the UI?

[kaareal]

Make sense. Thanks @leebyron, @chentsulin

I still not 100% sure why that unit test, doesn't return data:null => and therefor a http error, it seems to me there is no data in this example.

[miuan]

Hi all,

(not sure if I am in right tread)

I have some decorators for resolver as admin,owner,user example usage:

const UsersResolvers : IResolvers = {
  Query : {
    allUsers: protect.to.admin(allUsers),
    User: protect.to.owner(User, protect.by.id),
  },
  Mutation : {
    signIn: protect.to.public(signIn),
    createUser: protect.to.admin(createUser),
    updateUser: protect.to.owner(updateUser, protect.by.id),
  }

admin for example looks like

const _admin = (protectedFn) => async (root, data, ctx) => {
        // admin throw exception when is  the loged user not admin
    await adminCheck(ctx);
    return await protectedFn(root, data, ctx);
}

but when exception happen, I do not return any data and code result.data == null in line https://github.com/chentsulin/koa-graphql/blob/master/src/index.js#L275 response with status 500 ( for me is ok:-) ) but the apollo-client and graphiql they always expect 200 and thats the problem...

so I would like ask if exist better way how to do protection in decorator, to have response status 200 and still tell the client, he is not allowed to these data?

[iki]

Actually, GraphQL specifies passing errors via errors field in the response and not using application transport protocol level, which may or may not be HTTP.

This concept can be simplified as always send 200 with graphql json response, or 500 if server blows up and is shared for example by facebook relay, graphcool, apollo-server, and graphene on server side, and expected for example by apollo-client on client-side.

Sending other error codes with graphql json response may result in graphql clients assuming server error and not trying to parse json in body, so the passed errors field is lost and error is replaced with generic http status error. This happens e.g. with apollo-client.

It would be nice to either unify on single concept, or make it configurable.

Here, the forced 500 on missing data in response makes server to ignore any error status provided previously by user via ctx.throw(status, error). Imo it would help e.g. to only set the status to 500 if it wasn't set previously.

[felipap]

Any updates on this?

[chentsulin]

Now we have exactly same behavior copied from express-graphql and it seems to be spec-compliant. So I'm going to close this issue.