Don't override custom code extensions from scalar parseValue errors (#…

…7183)

Adding `BAD_USER_INPUT` is a nice default (and overrides the
inappropriate default of `INTERNAL_SERVER_ERROR`) but if somebody has
set a `code` already, we shouldn't override.

(Note that there's a separate issue where graphql-js throws out
extensions from the thrown error itself and only pays attention to
extensions on the error's originalError; we're trying to fix that in
graphql/graphql-js#3785 but this is orthogonal.)

Fixes #7178.

Co-authored-by: Trevor Scheer <trevor.scheer@gmail.com>

.changeset/tiny-deers-compare.md

@@ -0,0 +1,6 @@
+---
+'@apollo/server-integration-testsuite': patch
+'@apollo/server': patch
+---
+
+Apollo Server tries to detect if execution errors are variable coercion errors in order to give them a `code` extension of `BAD_USER_INPUT` rather than `INTERNAL_SERVER_ERROR`. Previously this would unconditionally set the `code`; now, it only sets the `code` if no `code` is already set, so that (for example) custom scalar `parseValue` methods can throw errors with specific `code`s. (Note that a separate graphql-js bug can lead to these extensions being lost; see /~https://github.com/graphql/graphql-js/pull/3785 for details.)

packages/integration-testsuite/src/apolloServerTests.ts

@@ -17,6 +17,7 @@ import {
   printSchema,
   FieldNode,
   GraphQLFormattedError,
+  GraphQLScalarType,
 } from 'graphql';
 
 // Note that by doing deep imports here we don't need to install React.
@@ -467,6 +468,106 @@ export function defineIntegrationTestSuiteApolloServerTests(
           );
           expect(result.errors[0].extensions.code).toBe('BAD_USER_INPUT');
         });
+
+        it('catches custom scalar parseValue and returns BAD_USER_INPUT', async () => {
+          const uri = await createServerAndGetUrl({
+            typeDefs: gql`
+              scalar CustomScalar
+              type Query {
+                hello(x: CustomScalar): String
+              }
+            `,
+            resolvers: {
+              CustomScalar: new GraphQLScalarType({
+                name: 'CustomScalar',
+                parseValue() {
+                  // Work-around for /~https://github.com/graphql/graphql-js/pull/3785
+                  // Once that's fixed, we can just directly throw this error.
+                  const e = new GraphQLError('Something bad happened', {
+                    extensions: { custom: 'foo' },
+                  });
+                  throw new GraphQLError(e.message, { originalError: e });
+                },
+              }),
+            },
+          });
+
+          const apolloFetch = createApolloFetch({ uri });
+
+          const result = await apolloFetch({
+            query: `query ($x:CustomScalar) {hello(x:$x)}`,
+            variables: { x: 'foo' },
+          });
+          expect(result).toMatchInlineSnapshot(`
+            {
+              "errors": [
+                {
+                  "extensions": {
+                    "code": "BAD_USER_INPUT",
+                    "custom": "foo",
+                  },
+                  "locations": [
+                    {
+                      "column": 8,
+                      "line": 1,
+                    },
+                  ],
+                  "message": "Variable "$x" got invalid value "foo"; Something bad happened",
+                },
+              ],
+            }
+          `);
+        });
+
+        it('catches custom scalar parseValue and preserves code', async () => {
+          const uri = await createServerAndGetUrl({
+            typeDefs: gql`
+              scalar CustomScalar
+              type Query {
+                hello(x: CustomScalar): String
+              }
+            `,
+            resolvers: {
+              CustomScalar: new GraphQLScalarType({
+                name: 'CustomScalar',
+                parseValue() {
+                  // Work-around for /~https://github.com/graphql/graphql-js/pull/3785
+                  // Once that's fixed, we can just directly throw this error.
+                  const e = new GraphQLError('Something bad happened', {
+                    extensions: { custom: 'foo', code: 'CUSTOMIZED' },
+                  });
+                  throw new GraphQLError(e.message, { originalError: e });
+                },
+              }),
+            },
+          });
+
+          const apolloFetch = createApolloFetch({ uri });
+
+          const result = await apolloFetch({
+            query: `query ($x:CustomScalar) {hello(x:$x)}`,
+            variables: { x: 'foo' },
+          });
+          expect(result).toMatchInlineSnapshot(`
+            {
+              "errors": [
+                {
+                  "extensions": {
+                    "code": "CUSTOMIZED",
+                    "custom": "foo",
+                  },
+                  "locations": [
+                    {
+                      "column": 8,
+                      "line": 1,
+                    },
+                  ],
+                  "message": "Variable "$x" got invalid value "foo"; Something bad happened",
+                },
+              ],
+            }
+          `);
+        });
       });
 
       describe('schema creation', () => {

packages/server/src/requestPipeline.ts

@@ -447,13 +447,18 @@ export async function processGraphQLRequest<TContext extends BaseContext>(
       // variables are required and get non-null values. If any of these things
       // lead to errors, we change them into UserInputError so that their code
       // doesn't end up being INTERNAL_SERVER_ERROR, since these are client
-      // errors.
+      // errors. (But if the error already has a code, perhaps because the
+      // original error was thrown from a custom scalar parseValue, we leave it
+      // alone. We check that here instead of as part of
+      // isBadUserInputGraphQLError since perhaps that function will one day be
+      // changed to something we can get directly from graphql-js, but the
+      // `code` check is AS-specific.)
       //
       // This is hacky! Hopefully graphql-js will give us a way to separate
       // variable resolution from execution later; see
       // /~https://github.com/graphql/graphql-js/issues/3169
       const resultErrors = result.errors?.map((e) => {
-        if (isBadUserInputGraphQLError(e)) {
+        if (isBadUserInputGraphQLError(e) && e.extensions?.code == null) {
           return new UserInputError(e);
         }
         return e;