From e3be3281531202f0d99ae33bdeeb8355268e4814 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Thu, 8 Apr 2021 12:13:18 -0700 Subject: [PATCH] apollo-server-core: use UserInputError for variable coercion errors (#5091) This particular error can be trivially triggered by clients, so it doesn't make sense to use `INTERNAL_SERVER_ERROR` for it. It seems like a good fit for `BAD_USER_INPUT`, which previously was only used if you explicitly throw a `UserInputError` in your app. Fixes #3498. --- CHANGELOG.md | 1 + .../apollo-server-core/src/requestPipeline.ts | 30 ++++++++++++++++--- packages/apollo-server-errors/src/index.ts | 4 ++- .../src/ApolloServer.ts | 21 +++++++++++++ 4 files changed, 51 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d57a2a020b7..63a45ca4b65 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ The version headers in this history reflect the versions of Apollo Server itself - `apollo-server-cache-redis`: New `BaseRedisCache` class which takes an `ioredis`-compatible Redis client as an argument. The existing classes `RedisCache` and `RedisClusterCache` (which pass their arguments to `ioredis` constructors) are now implemented in terms of this class. This allows you to use any of the `ioredis` constructor forms rather than just the ones recognized by our classes. This also fixes a long-standing bug where the Redis cache implementations returned a number from `delete()`; it now returns a number, matching what the `KeyValueCache` interface and the TypeScript types expect. [PR #5034](https://github.com/apollographql/apollo-server/pull/5034) [PR #5088](https://github.com/apollographql/apollo-server/pull/5088) [Issue #4870](https://github.com/apollographql/apollo-server/issues/4870) [Issue #5006](https://github.com/apollographql/apollo-server/issues/5006) - `apollo-server-core`: Fix type for `formatResponse` function. It never is called with a `null` argument, and is allowed to return `null`. [Issue #5009](https://github.com/apollographql/apollo-server/issues/5009) [PR #5089](https://github.com/apollographql/apollo-server/pull/5089) - `apollo-server-lambda`: Fix regression in v2.21.2 where thrown errors were replaced by throwing the JS Error class itself. [PR #5085](https://github.com/apollographql/apollo-server/pull/5085) +- `apollo-server-core`: If a client sends a variable of the wrong type, this is now reported as an error with an `extensions.code` of `BAD_USER_INPUT` rather than `INTERNAL_SERVER_ERROR`. [PR #5091](https://github.com/apollographql/apollo-server/pull/5091) [Issue #3498](https://github.com/apollographql/apollo-server/issues/3498) ## v2.22.2 diff --git a/packages/apollo-server-core/src/requestPipeline.ts b/packages/apollo-server-core/src/requestPipeline.ts index d1a43dfdb9b..d448b4d505a 100644 --- a/packages/apollo-server-core/src/requestPipeline.ts +++ b/packages/apollo-server-core/src/requestPipeline.ts @@ -10,6 +10,7 @@ import { validate as graphqlValidate, parse as graphqlParse, execute as graphqlExecute, + Kind, } from 'graphql'; import { GraphQLExtension, @@ -31,6 +32,7 @@ import { PersistedQueryNotSupportedError, PersistedQueryNotFoundError, formatApolloErrors, + UserInputError, } from 'apollo-server-errors'; import { GraphQLRequest, @@ -451,16 +453,36 @@ export async function processGraphQLRequest( requestContext as GraphQLRequestContextExecutionDidStart, ); - if (result.errors) { - await didEncounterErrors(result.errors); + // The first thing that execution does is coerce the request's variables + // to the types declared in the operation, which can lead to errors if + // they are of the wrong type. We change any such errors into + // UserInputError so that their code doesn't end up being + // INTERNAL_SERVER_ERROR, since these are client errors. + const resultErrors = result.errors?.map((e) => { + if ( + e.nodes?.length === 1 && + e.nodes[0].kind === Kind.VARIABLE_DEFINITION && + e.message.startsWith( + `Variable "$${e.nodes[0].variable.name.value}" got invalid value `, + ) + ) { + return fromGraphQLError(e, { + errorClass: UserInputError, + }); + } + return e; + }); + + if (resultErrors) { + await didEncounterErrors(resultErrors); } response = { ...result, - errors: result.errors ? formatErrors(result.errors) : undefined, + errors: resultErrors ? formatErrors(resultErrors) : undefined, }; - executionDispatcher.reverseInvokeHookSync("executionDidEnd"); + executionDispatcher.reverseInvokeHookSync('executionDidEnd'); } catch (executionError) { executionDispatcher.reverseInvokeHookSync("executionDidEnd", executionError); return await sendErrorResponse(executionError); diff --git a/packages/apollo-server-errors/src/index.ts b/packages/apollo-server-errors/src/index.ts index 8f2fb3ecb4f..6eeea005bd4 100644 --- a/packages/apollo-server-errors/src/index.ts +++ b/packages/apollo-server-errors/src/index.ts @@ -134,7 +134,9 @@ export function toApolloError( export interface ErrorOptions { code?: string; - errorClass?: typeof ApolloError; + // This declaration means it takes any "class" that has a constructor that + // takes a single string, and should be invoked via the `new` operator. + errorClass?: new (message: string) => ApolloError; } export function fromGraphQLError(error: GraphQLError, options?: ErrorOptions) { diff --git a/packages/apollo-server-integration-testsuite/src/ApolloServer.ts b/packages/apollo-server-integration-testsuite/src/ApolloServer.ts index 706af759511..e7fa2a03c6e 100644 --- a/packages/apollo-server-integration-testsuite/src/ApolloServer.ts +++ b/packages/apollo-server-integration-testsuite/src/ApolloServer.ts @@ -348,6 +348,27 @@ export function testApolloServer( }); }); + it('variable coercion errors', async () => { + const { url: uri } = await createApolloServer({ + typeDefs: gql` + type Query { + hello(x: String): String + } + `, + }); + + const apolloFetch = createApolloFetch({ uri }); + + const result = await apolloFetch({ + query: `query ($x:String) {hello(x:$x)}`, + variables: { x: 2 }, + }); + expect(result.data).toBeUndefined(); + expect(result.errors).toBeDefined(); + expect(result.errors[0].message).toMatch(/got invalid value 2; Expected type String/); + expect(result.errors[0].extensions.code).toBe('BAD_USER_INPUT'); + }); + describe('schema creation', () => { it('accepts typeDefs and resolvers', async () => { const typeDefs = gql`