Return 400 on unknown operation name (version 4) #6652
Conversation
|
❌ Deploy Preview for apollo-server-docs failed.
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit a0c1bef:
|
| return e; | ||
| }); | ||
|
|
||
| if (resultErrors) { | ||
| await didEncounterErrors(resultErrors); | ||
| } | ||
|
|
||
| if (resultErrors?.length && !('data' in result)) { | ||
| return await sendErrorResponse(resultErrors); |
There was a problem hiding this comment.
this calls didEncounterErrors, so we don't want to do that twice.
also this doesn't call executionDidEnd? that seems wrong?
| @@ -75,6 +76,10 @@ function isBadUserInputGraphQLError(error: GraphQLError): boolean { | |||
| ); | |||
| } | |||
|
|
|||
| function isUnknownOperationNameError(error: GraphQLError): boolean { | |||
There was a problem hiding this comment.
We also need to catch the case where multiple operations are provided but no operation name is given: https://github.com/graphql/graphql-js/blob/ffe37cb7f501e65e445c30764a1b34f659562756/src/execution/execute.ts#L275
(I don't think we need to handle the third error in that function because IIUC this happens if there are no operations in the document, but I think that's a validation error: the parser requires there to be some top-level definition, and ExecutableDefinitionsRule requires all definitions to be operations or fragments. And NoUnusedFragmentsRule requires all fragments to be used (transitively) by an operation.)
That probably means that the name UnknownOperationNameError should change. Maybe to OperationResolveError or OperationResolutionError or something (going with our didResolveOperation hook).
Then um. We could just decide that if requestContext.operation is falsey, we turn any error we get out of execute into an OperationResolutionError, assuming our logic matches that in graphql-js... which I think is probably true.
BTW, if we do want to stick with error string matching, it raises the question of whether the error is precisely the same when execute is gateway or a custom executor... It looks like buildOperationContext in gateway happens to use the same error strings but that's a strong assumption...
(Of course hopefully one day graphql-js will just let us use its operation-resolving code directly, cc @IvanGoncharov.)
trevor-scheer
force-pushed
the
trevor/parse-validate-operationName-errors
branch
from
65fa6bf
to
2d7c5f6
Compare
|
@glasser good comments. Hopefully all addressed via 2d7c5f6. Your link (https://github.com/graphql/graphql-js/blob/ffe37cb7f501e65e445c30764a1b34f659562756/src/execution/execute.ts#L275) was helpful in deciding to un-fragile the approach a bit, hopefully? Now rather than string matching, I'm assuming there's one expected error from |
| extensions: error.extensions, | ||
| }); | ||
|
|
||
| await Promise.all( |
There was a problem hiding this comment.
maybe throw the error instead of repeating this (so we never call executionDidEnd twice with errors).
| // If we don't have an operation, there's no reason to go further. We know | ||
| // `result` will consist of one error (returned by `graphql-js`'s | ||
| // `buildExecutionContext`). | ||
| if (!requestContext.operation && result.errors?.length) { |
There was a problem hiding this comment.
maybe throw something different if !operation and !errors
|
BTW for posterity: this was a regression on version-4. |
Specifically handle the cases where an operation couldn't be resolved (either
due to an incorrect
operationNameor a missingoperationNamein thepresence of two operations).
In these cases, we know that
buildExecutionContextwill return a single errorand we know this is a client error. We can end execution and respond with a 400
along with a useful error code and the error from
graphql-js.