AWS Lambda version on node 8.10 returns statusCode 400 on graphql error

[dacz]

GraphQL Lambda

Invalid graphql query returns correct graphql response with errors but the statusCode is 400 instead of 200.

• serverless platform
• node 8.10
• using async/await with babel and webpack

My lambda is simple enough (90% is the example from docs with callbackFilter).

I modified the callbackFilter as follows to sort out the error:

const callbackFilter = (error, output) => {
    output.statusCode = 200; // <<<< ADDED THIS ONE
    output.headers['Access-Control-Allow-Origin'] = '*';
    output.headers['Access-Control-Allow-Credentials'] = true;
    callback(error, output);
  };

The statusCode was returned even the error was undefined.

Desired outcome

Return statusCode 200 by default.

[abernix]

Are you saying that the query sent from the client was syntactically incorrect/malformed? Can you give an example of the query?

Keeping in mind that the logic behind the choosing HTTP status codes (outside of the context of GraphQL) is not isolated to the transport layer, are you suggesting that you would expect a 200 status code in the event of a syntax error encountered while parsing the query?

[dacz]

Hi @abernix, I think there are generally 2 cases:

1. the request is malformed (may even be valid JSON but for example does not contain query field). The status code returned can be decided on best practices of http reply values (400 Bad Request probably). This is not the case I'm describing in my issue.

2. the request is valid but the query is not valid with the schema. Example - querying field nonexistent on type that does not define this field. GraphQL returns the GraphQL error in the response, but imo the status code (by default) should be 200. The frontend developers will get the proper error and may handle it accordingly.
   This behaviour could be configurable - it could make sense to return 400 in production in general (but in any case schema is usually exposed by introspection so the malicious attach this way doesn't make much sense). But in lambda environment when your lambda returns 400, API GW returns 400 and no information/errors go to the developer.

[ojongerius]

@dacz I'd expect the body to be passed on by API GW, is this not the case?

If so, and the client feels lucky enough to not check the HTTP status of her request she would still be able to inspect the "errors" in the body, like she would for 200s.

[dacz]

@ojongerius I'm not looking for workaround, as I stated in the point 2. of my previous comment, I believe that GraphQL server (understand apollo graphql lambda) should return 200 and well-formed GraphQL request (with errors).

I believe that apollo lambda does this correctly on node 6.10, but AWS now supports node 8.10 with async and this is the case where the return status code is not correct.

The reason I posted it here is to know if this is desired behaviour of apollo lambda with node 8.10 or not and if not, flag it as a bug and community or apollo may fix it.

[o-alexandrov]

@dacz IMO, you should use graphql library for execution directly (example).
That way you would be able to configure the error codes yourself.

Instead of importing additional ~228.3KB apollo-server-lambda which in turn brings unnecessary graphiql and most probably much more (don't have time yet to explore) as you cannot do tree shaking with webpack on commonjs(2) modules yet.

P.S. authors should split the exporting of the modules into separate files, so that we could properly import exactly what we need.

[vespertilian]

@dacz

Just hit this issue. I agree that it would be nice if it was consistent. I don't think its a node version issue.

Below is the code block, I think is responsible, for returning the 400 vs 200 response code. It's seems to be because validation errors don't have data set by default. Also, I think this returns before the custom resolver you provide gets called, so you cannot even override the response code.

The other issue with it getting called without the resolver is that, say you are missing one field "name", but have other fields that are present, that would throw errors (maybe "age" < 18). You will only get the missing field error back with the first request, so you will have to call the api a second time to get the age error.

This is all a bit new to me, but if the best practice for graphql is that errors should be returned with 200. I feel that "required" validation errors should fall into the same category as any other custom validation errors and return the same response code. Maybe setting data to null would be a solution.

That said maybe there is a good reason for the 400 code.

Current

Beta 7

if (!isBatch) {
    const gqlResponse = responses[0];
    //This code is run on parse/validation errors and any other error that
    //doesn't reach GraphQL execution
    if (gqlResponse.errors && typeof gqlResponse.data === 'undefined') {
      throw new HttpQueryError(400, prettyJSONStringify(gqlResponse), true, {
        'Content-Type': 'application/json',
      });
    }
    return prettyJSONStringify(gqlResponse);
  }

Master

  if (!isBatch) {
    const gqlResponse = responses[0];
    if (gqlResponse.errors && typeof gqlResponse.data === 'undefined') {
      throw new HttpQueryError(400, JSON.stringify(gqlResponse), true, {
        'Content-Type': 'application/json',
      });
    }
    return JSON.stringify(gqlResponse);
  }

Maybe a solution

if (!isBatch) {
    const gqlResponse = responses[0];
    //This code is run on parse/validation errors and any other error that
    //doesn't reach GraphQL execution
    if (gqlResponse.errors && typeof gqlResponse.data === 'undefined') {
      // set data to null and return 200 just like other errors
      gqlResponse.data = null
      throw new HttpQueryError(200, prettyJSONStringify(gqlResponse), true, {
        'Content-Type': 'application/json',
      });
    }
    return prettyJSONStringify(gqlResponse);
  }

Happy to create a pull request for this if someone with merge rights is happy to accept the request.

[abernix]

Thanks for logging this originally. I'm going to close this issue since It hasn't received much traction in quite some time and I'm fairly sure that we have a good number of users utilizing Apollo Server on Lambda. We've also changed substantial portions of Apollo Server since then and accepted a lot of PRs which change the apollo-server-lambda package functionality.

If this is still a problem, feel free to ping me back and I (or someone) will be happy to re-open it, though please do be prepared to provide a run-able reproduction so we can get to the bottom of the issue quickly. (Also for Lambda, this would be even better if this reproduction was also a serverless project so it can be quickly deployed and tested.) Thanks!

[bionicles]

@abernix this is one of those super annoying gotchas which drives me nuts ... we have to dig into hidden parts of the apollo server repo to find the status code for errors, which makes it super hard to set up CORS properly in api gateway. just blew hours on this issue, please reopen and simply document the status codes, as they are critical for CORS configuration

[dacz]

@bionicles I agree with @o-alexandrov fully. When you need better control, it's better not to use Apollo server but use graphql package directly. In Apollo server there is not only graphiql but the scaffolding to connect to their commercial services, too.

4 months ago I did reference implementtion and found:

• much smaller build without Apollo
• faster starts, especially cold starts (aws lambda)
• faster execution

One thing that you should be aware of is that Apollo server caches the parsed queries in memory (with some LRU cache package as I remember). It's easy to implement it. And for serverless environment (aws lambda) you can go even way further - use elasticache (redis) as this cache and you get shared parsed query cache across multiple lambdas.

The "only" gotcha is cache invalidation, but when you go down the way to have this infrastructure (and you need heavy duty graphql server as lambda), it's not so hard to do so and you can fine tune your cache for your requirements (based on queries etc., frequency, etc).

The beautifiul benefit is that you get the solution that you can tweak the way you want. I had CORS problems, too and I had to make some weird workarounds to add my own header with request id for debugging.