regression in http status setting for certain errors (v3 to v4)

[jessemyers]

Issue Description

We are in the process of upgrading our system from v3 to v4 and see a regression in several unit tests for HTTP status codes of invalid inputs. The particular error is of the form:

Variable "$foo" of required type "ID!" was not provided

I've traced these calls into requestPipeline.ts and do not see any mechanism for setting HTTP status after the pipeline reaches a specific point. We have other test cases that did not regress; all of these were caught earlier in the pipeline (e.g. here).

The problematic behaviors occurs on lines 474-477:

const { formattedErrors, httpFromErrors } = resultErrors
    ? formatErrors(resultErrors)
    : { formattedErrors: undefined, httpFromErrors: newHTTPGraphQLHead() };
mergeHTTPGraphQLHead(requestContext.response.http, httpFromErrors);

If there are errors (eg. if resultErrors exists), the formatErrors function delegates to the normalizeAndFormatErrors function, which always creates a new instance for httpFromErrors.

Subsequently the mergeHTTPGraphQLHead function only sets the target.status to a non-200 HTTP code if this httpFromErrors value defines a status, which it does not because it is a fresh instance and takes no input from the resultErrors.

I have not taken the time to dig into the older software, but this seems like an obvious regression; either this kind of error should be caught earlier in the pipeline (where it can be handled differently) or this part of the pipeline should be able to set the HTTP status based on the error content.

Link to Reproduction

n/a

Reproduction Steps

Our setup is: application => internal framework => @nestjs/graphql => @nestjs/apollo => @apollo/server.

I get the value and importance of a reproducible test case; I also estimate that it would take several days to strip down this flow into something that neither leaked internal systems and directly used apollo (vs @nestjs). I'm hopefully that my line-by-line breakdown of the issue suffices; I doubt we can take the time to produce a palatable reproduction.

[glasser]

@trevor-scheer and I looked into this a bit and yes, there's an unintended regression here. He actually noticed this a bit ago and filed a PR to the project that implements an HTTP-level test suite for this.

Looks like I introduced this in #6502 where I deleted a block of code with this comment and replaced it with similar code in the sendErrorResponse function:

    // This code is run on parse/validation errors and any other error that
    // doesn't reach GraphQL execution

Unfortunately what I missed was that this code was also run on code that did reach GraphQL execution if the GraphQL executor didn't produce a data field in its result, which I believe is precisely the "variable coercion fails" case — and that case doesn't go through the sendErrorResponse function, so this change lost an important case.

I think @trevor-scheer is going to take it from here in figuring out what the right response is (and perhaps validating my vague memory that the variable coercion case is the only one where execute runs and doesn't return perhaps-null data). Fixing this might count as backwards-incompatible so we might want to make the fix opt-in until v5 (which we probably will release in the next month or two to drop Node v14 support — this would likely be the only other backwards-incompatible change).

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
For general questions, we recommend using StackOverflow or our discord server.