fix: invalid queries return their real error code

backend/src/server/errorCodes.test.ts

@@ -0,0 +1,108 @@
+// The error codes that a client really gets from the server: the real Apollo
+// Server with the real schema, depth limit and formatError, run without HTTP.
+// These tests need no database: an invalid query is rejected before any
+// resolver runs, and the one resolver that runs gets a database that fails.
+// formatError.test.ts checks the function on its own; this file checks that it
+// works together with Apollo, which wraps its own errors in a way a hand-made
+// test error does not copy.
+
+import type { ApolloServer } from '@apollo/server'
+import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest'
+import type { PrismaClient } from '../generated/prisma/client.js'
+import { createApolloServer } from './app.js'
+import type { Config } from './config.js'
+import { type Context, createContext } from './context.js'
+import { MAX_QUERY_DEPTH } from './depthLimit.js'
+import { INTERNAL_ERROR_MESSAGE } from './formatError.js'
+
+const config: Config = {
+  port: 3001,
+  imageBaseUrl: 'http://images.test/images',
+  corsOrigins: [],
+}
+
+/** A database that fails on every call, like one that is down. */
+const brokenDatabase = new Proxy(
+  {},
+  {
+    get: () => ({
+      findMany: () => Promise.reject(new Error('password=secret at db:5432')),
+    }),
+  },
+) as unknown as PrismaClient
+
+describe('the error codes the server gives', () => {
+  let server: ApolloServer<Context>
+
+  beforeAll(async () => {
+    server = createApolloServer()
+    await server.start()
+  })
+
+  afterAll(async () => {
+    await server.stop()
+  })
+
+  async function errorsOf(request: {
+    query: string
+    variables?: Record<string, unknown>
+    operationName?: string
+  }) {
+    const response = await server.executeOperation(request, {
+      contextValue: createContext(brokenDatabase, config),
+    })
+    if (response.body.kind !== 'single') throw new Error('Expected one result')
+    return response.body.singleResult.errors ?? []
+  }
+
+  it('gives an unknown field GRAPHQL_VALIDATION_FAILED', async () => {
+    const [error] = await errorsOf({ query: '{ nope }' })
+    expect(error?.extensions?.['code']).toBe('GRAPHQL_VALIDATION_FAILED')
+    expect(error?.message).toMatch(/Cannot query field "nope"/)
+  })
+
+  it('gives a syntax error GRAPHQL_PARSE_FAILED', async () => {
+    const [error] = await errorsOf({ query: '{ apparatus ' })
+    expect(error?.extensions?.['code']).toBe('GRAPHQL_PARSE_FAILED')
+    expect(error?.message).toMatch(/Syntax Error/)
+  })
+
+  it('gives a query that is too deep GRAPHQL_VALIDATION_FAILED', async () => {
+    // apparatus → groups → apparatus → … : each pair adds two levels.
+    let query = 'id'
+    for (let i = 0; i < MAX_QUERY_DEPTH; i++) {
+      query = i % 2 === 0 ? `apparatus { ${query} }` : `groups { ${query} }`
+    }
+    const [error] = await errorsOf({ query: `{ apparatus { ${query} } }` })
+    expect(error?.extensions?.['code']).toBe('GRAPHQL_VALIDATION_FAILED')
+    expect(error?.message).toMatch(/levels deep/)
+  })
+
+  it('gives a missing or invalid variable BAD_USER_INPUT', async () => {
+    const query = 'query ($id: ID!) { element(id: $id) { id } }'
+    const missing = await errorsOf({ query })
+    const invalid = await errorsOf({ query, variables: { id: { a: 1 } } })
+    for (const [error] of [missing, invalid]) {
+      expect(error?.extensions?.['code']).toBe('BAD_USER_INPUT')
+      expect(error?.message).toMatch(/\$id/)
+    }
+  })
+
+  it('gives an unknown operationName OPERATION_RESOLUTION_FAILURE', async () => {
+    const [error] = await errorsOf({
+      query: 'query A { apparatus { id } }',
+      operationName: 'B',
+    })
+    expect(error?.extensions?.['code']).toBe('OPERATION_RESOLUTION_FAILURE')
+  })
+
+  it('still hides an unexpected error in a resolver', async () => {
+    const log = vi.spyOn(console, 'error').mockImplementation(() => {})
+    const [error] = await errorsOf({ query: '{ apparatus { id } }' })
+    log.mockRestore()
+
+    expect(error?.extensions?.['code']).toBe('INTERNAL_SERVER_ERROR')
+    expect(error?.message).toBe(INTERNAL_ERROR_MESSAGE)
+    expect(JSON.stringify(error)).not.toContain('secret')
+  })
+})

backend/src/server/formatError.test.ts

@@ -1,11 +1,20 @@
+import { ApolloServerErrorCode } from '@apollo/server/errors'
 import { GraphQLError } from 'graphql'
 import { afterEach, describe, expect, it, vi } from 'vitest'
 import {
   BadUserInputError,
+  InvalidItemsError,
+  LimitReachedError,
+  NameTakenError,
+  NotFoundError,
   NotImplementedError,
   UnauthenticatedError,
 } from '../errors.js'
-import { formatError } from './formatError.js'
+import {
+  APOLLO_CLIENT_ERROR_CODES,
+  INTERNAL_ERROR_MESSAGE,
+  formatError,
+} from './formatError.js'
 
 /** What Apollo hands to formatError for an error thrown in a resolver. */
 const thrownInResolver = (error: Error) =>
@@ -15,6 +24,17 @@ const thrownInResolver = (error: Error) =>
     extensions: { code: 'INTERNAL_SERVER_ERROR' },
   })
 
+/**
+ * What Apollo hands to formatError for an error of its own (a syntax error, an
+ * invalid query, a bad variable): it wraps the GraphQL error in another one, so
+ * `originalError` is set. This is the shape that was once hidden by mistake.
+ */
+const raisedByApollo = (message: string, code: string) =>
+  new GraphQLError(message, {
+    originalError: new GraphQLError(message),
+    extensions: { code },
+  })
+
 describe('formatError', () => {
   afterEach(() => {
     vi.restoreAllMocks()
@@ -24,6 +44,13 @@ describe('formatError', () => {
     [new BadUserInputError('first must be 1–100'), 'BAD_USER_INPUT'],
     [new UnauthenticatedError(), 'UNAUTHENTICATED'],
     [new NotImplementedError('login'), 'NOT_IMPLEMENTED'],
+    [
+      new InvalidItemsError('A routine on vault has only one vault.'),
+      'INVALID_ITEMS',
+    ],
+    [new NotFoundError('The gymnast'), 'NOT_FOUND'],
+    [new NameTakenError(), 'NAME_TAKEN'],
+    [new LimitReachedError('A coach can have 200 gymnasts.'), 'LIMIT_REACHED'],
   ])('gives %s its own code and keeps the message', (error, code) => {
     const error2 = thrownInResolver(error)
     const result = formatError(error2.toJSON(), error2)
@@ -39,7 +66,7 @@ describe('formatError', () => {
     )
     const result = formatError(error.toJSON(), error)
 
-    expect(result.message).toBe('Internal server error')
+    expect(result.message).toBe(INTERNAL_ERROR_MESSAGE)
     expect(result.extensions).toEqual({ code: 'INTERNAL_SERVER_ERROR' })
     expect(JSON.stringify(result)).not.toContain('secret')
     expect(log).toHaveBeenCalledOnce()
@@ -56,15 +83,31 @@ describe('formatError', () => {
     })
     for (const error of [withOtherCode, withoutCode]) {
       const result = formatError(error.toJSON(), error)
-      expect(result.message).toBe('Internal server error')
+      expect(result.message).toBe(INTERNAL_ERROR_MESSAGE)
       expect(result.extensions).toEqual({ code: 'INTERNAL_SERVER_ERROR' })
     }
   })
 
-  it('leaves the errors from GraphQL itself as they are', () => {
-    const error = new GraphQLError('The query is 9 levels deep.', {
-      extensions: { code: 'GRAPHQL_VALIDATION_FAILED' },
-    })
-    expect(formatError(error.toJSON(), error)).toEqual(error.toJSON())
+  it.each([...APOLLO_CLIENT_ERROR_CODES])(
+    'lets an error with the Apollo code %s through, with its message',
+    (code) => {
+      const log = vi.spyOn(console, 'error').mockImplementation(() => {})
+      const error = raisedByApollo('Something the client can fix.', code)
+      const result = formatError(error.toJSON(), error)
+
+      expect(result).toEqual(error.toJSON())
+      expect(result.extensions?.['code']).toBe(code)
+      expect(log).not.toHaveBeenCalled()
+    },
+  )
+
+  it("has decided on every code in Apollo's enum", () => {
+    // A new code in a later Apollo version fails here, so that someone decides
+    // whether the client or the server is to blame for it.
+    for (const code of Object.values(ApolloServerErrorCode)) {
+      expect(APOLLO_CLIENT_ERROR_CODES.has(code)).toBe(
+        code !== ApolloServerErrorCode.INTERNAL_SERVER_ERROR,
+      )
+    }
   })
 })

backend/src/server/formatError.ts

@@ -1,14 +1,47 @@
 // How errors look to the client (see "Error handling" in docs/API.md).
 //  - The errors we throw on purpose (BadUserInputError and the others in
 //    src/errors.ts) get their code in `extensions.code`.
-//  - Errors from GraphQL itself (an invalid query, a query that is too deep)
-//    keep their own code, for example GRAPHQL_VALIDATION_FAILED.
+//  - Errors that Apollo Server or GraphQL gives a client error code (an
+//    invalid query, a syntax error, a bad variable, a query that is too deep)
+//    keep their own code and message, for example GRAPHQL_VALIDATION_FAILED.
 //  - Anything else is a bug or a failure on our side: the client gets a
 //    general message and no details, and the real error goes to the log.
 
+import { ApolloServerErrorCode } from '@apollo/server/errors'
 import type { GraphQLFormattedError } from 'graphql'
 import { isClientError } from '../errors.js'
 
+/**
+ * The codes that Apollo Server gives to errors that are the client's doing:
+ * something wrong with the request, not with the server. Every one is listed by
+ * name, so it is clear what is let through. INTERNAL_SERVER_ERROR is left out on
+ * purpose: it is the code of an unexpected error, which is hidden.
+ *
+ * formatError.test.ts checks that each code in Apollo's enum is either here or
+ * INTERNAL_SERVER_ERROR, so a new code in a later Apollo version fails the test
+ * and has to be decided on.
+ */
+export const APOLLO_CLIENT_ERROR_CODES: ReadonlySet<string> = new Set<string>([
+  // The operation has a syntax error.
+  ApolloServerErrorCode.GRAPHQL_PARSE_FAILED,
+  // The operation does not fit the schema, or breaks a validation rule such as
+  // the depth limit (src/server/depthLimit.ts).
+  ApolloServerErrorCode.GRAPHQL_VALIDATION_FAILED,
+  // A variable or an argument has an invalid value.
+  ApolloServerErrorCode.BAD_USER_INPUT,
+  // The request was wrong before it could be read as a GraphQL operation.
+  ApolloServerErrorCode.BAD_REQUEST,
+  // The operation is valid, but the server cannot tell which one to run (for
+  // example an unknown operationName).
+  ApolloServerErrorCode.OPERATION_RESOLUTION_FAILURE,
+  // Automatic persisted queries: the hash is unknown, or the feature is off.
+  ApolloServerErrorCode.PERSISTED_QUERY_NOT_FOUND,
+  ApolloServerErrorCode.PERSISTED_QUERY_NOT_SUPPORTED,
+])
+
+/** What the client reads when an unexpected error is hidden. */
+export const INTERNAL_ERROR_MESSAGE = 'Internal server error'
+
 /**
  * The error that a resolver threw. GraphQL wraps it in its own error and keeps
  * it as `originalError`. This reads that field directly and does not use
@@ -20,29 +53,42 @@ function unwrapResolverError(error: unknown): unknown {
   return original ?? error
 }
 
+/** Apollo Server or GraphQL itself says the client sent something wrong. */
+function hasApolloClientCode(formatted: GraphQLFormattedError): boolean {
+  const code = formatted.extensions?.['code']
+  return typeof code === 'string' && APOLLO_CLIENT_ERROR_CODES.has(code)
+}
+
+/** The answer for an unexpected error: no details, and the code that says so. */
+function hiddenError(formatted: GraphQLFormattedError): GraphQLFormattedError {
+  return {
+    message: INTERNAL_ERROR_MESSAGE,
+    extensions: { code: ApolloServerErrorCode.INTERNAL_SERVER_ERROR },
+    ...(formatted.path ? { path: formatted.path } : {}),
+  }
+}
+
 export function formatError(
   formatted: GraphQLFormattedError,
   error: unknown,
 ): GraphQLFormattedError {
   const original = unwrapResolverError(error)
+
+  // 1. An error we threw on purpose.
   if (isClientError(original)) {
     return { ...formatted, extensions: { code: original.code } }
   }
-  // An error that wraps something we did not throw on purpose is always masked,
-  // whatever code it carries. A bare error without a code is masked too.
-  const wrapsUnknownError = original !== error
-  const code = formatted.extensions?.['code']
-  if (
-    wrapsUnknownError ||
-    code === undefined ||
-    code === 'INTERNAL_SERVER_ERROR'
-  ) {
-    console.error('Internal server error:', original)
-    return {
-      message: 'Internal server error',
-      extensions: { code: 'INTERNAL_SERVER_ERROR' },
-      ...(formatted.path ? { path: formatted.path } : {}),
-    }
+
+  // 2. An error with one of Apollo's client error codes. Apollo wraps these in
+  //    another error, so `original` can differ from `error` here: only the code
+  //    says whether the client or the server is to blame. A resolver that
+  //    throws an unexpected error never gets one of these codes (it gets
+  //    INTERNAL_SERVER_ERROR), so such an error is still hidden below.
+  if (hasApolloClientCode(formatted)) {
+    return formatted
   }
-  return formatted
+
+  // 3. Anything else, with whatever code or none: hide it and log the real one.
+  console.error('Internal server error:', original)
+  return hiddenError(formatted)
 }

docs/AI_USAGE.md

@@ -371,3 +371,21 @@
 - **Review:** The team reviewed the theme changes and confirmed the colors
   should come from the existing CSS variables rather than new palette values.
 
+## Error codes of invalid queries (`backend/src/server/formatError.ts`)
+
+- **Tool:** Claude Code
+- **What the team did:** Wrote issue #93 (invalid queries came back as
+  `INTERNAL_SERVER_ERROR`) and decided the rule: a mistake in the request keeps
+  its real code and message, and an unexpected error is still hidden. Chose a
+  list of allowed codes over hiding only `INTERNAL_SERVER_ERROR`, because an
+  unknown error must not leak by default, and asked that every Apollo code is
+  listed by name.
+- **What Claude did:** Ran `formatError` against a real Apollo Server and found
+  that a missing variable and an unknown `operationName` were hidden too, not
+  only syntax and validation errors. Compared the Apollo documentation and
+  other guides, wrote the named list `APOLLO_CLIENT_ERROR_CODES`, the tests
+  (a real server without a database, one test per own error class, and a test
+  that fails when a new Apollo code has not been decided on) and the text in
+  `docs/API.md`. The old unit test used an error without `originalError`,
+  which is not how Apollo sends them, so it passed while the server test failed;
+  the new tests use the real shape, and turning the list off makes them fail.