AS3 → AS4: Difficulty controlling HTTP response when throwing errors from `requestDidStart`

[markandrus]

Issue Description

AS3 Behavior

In Apollo v3, we could throw an HttpQueryError from a plugin's requestDidStart method in order to control the HTTP server's response code. I used this to build an authentication plugin that can respond with 403 Forbidden for unauthenticated and unauthorized requests. A mock version of this can be found in the v3 folder of my reproduction. It looks roughly like this:

class AuthPlugin {
  requestDidStart () {
    // NOTE: In reality, we'd only throw if a request is unauthenticated and/or unauthorized.
    throw new HttpQueryError(403, 'Unauthorized')
  }
}

Issuing a request to Apollo Server with this plugin installed results in a 403 HTTP status code ✅ and response body equal to "Unauthorized": ✅

Unauthorized

AS4 Behavior

In Apollo v4, HttpQueryError is removed, and so I think we should throw a GraphQLError from the plugin's requestDidStart method instead. In theory, we can set http inside of extensions with whatever status and headers we want. Unfortunately, this does not seem to work, due to this section of ApolloServer.ts. A mock version of this can be found in the v4 folder of my reproduction. It looks roughly like this:

class AuthPlugin {
  requestDidStart () {
    // NOTE: In reality, we'd only throw if a request is unauthenticated and/or unauthorized.
    throw new GraphQLError('Unauthorized', {
      extensions: {
        http: { status: 403 }
      }
    })
  }
}

Issuing a request to Apollo Server with this plugin installed results in a 500 HTTP status code ❌ and a JSON response body with an Internal Server Error inside: ❌

{
  "errors": [
    {
      "message": "Internal server error",
      "extensions": {
        "code": "INTERNAL_SERVER_ERROR",
        "stacktrace": [
          "Error: Internal server error",
          "    at internalExecuteOperation (/Users/mark/src/mark/apollo-v4-issue/node_modules/@apollo/server/dist/cjs/ApolloServer.js:624:15)",
          "    at processTicksAndRejections (node:internal/process/task_queues:96:5)",
          "    at async runHttpQuery (/Users/mark/src/mark/apollo-v4-issue/node_modules/@apollo/server/dist/cjs/runHttpQuery.js:135:29)",
          "    at async runPotentiallyBatchedHttpQuery (/Users/mark/src/mark/apollo-v4-issue/node_modules/@apollo/server/dist/cjs/httpBatching.js:37:16)",
          "    at async ApolloServer.executeHTTPGraphQLRequest (/Users/mark/src/mark/apollo-v4-issue/node_modules/@apollo/server/dist/cjs/ApolloServer.js:525:20)"
        ]
      }
    }
  ]
}

Partial AS4 Workaround

It appears that we can still control the status code if we re-throw from a plugin's unexpectedErrorProcessingRequest method. I have checked in a workaround in under the v4-workaround folder of my reproduction. Note that I take care to only re-throw my own error inside of unexpectedErrorProcessingRequest. It looks roughly like this:

class AuthPlugin {
  requestDidStart () {
    // NOTE: In reality, we'd only throw if a request is unauthenticated and/or unauthorized.
    throw new AuthPluginError()
  }

  unexpectedErrorProcessingRequest ({ error }) {
    // NOTE: Only re-throw instances of our own error.
    if (error instanceof AuthPluginError) {
      throw error
    }
  }
}

Issuing a request to Apollo Server with this plugin installed results in a 403 HTTP status code ✅ and a JSON response body with an Internal Server Error inside: ❌

{
  "errors": [
    {
      "message": "Unauthorized",
      "extensions": {
        "code": "UNAUTHORIZED",
        "stacktrace": [
          "GraphQLError: Unauthorized",
          "    at new AuthPluginError (/Users/mark/src/mark/apollo-v4-issue/v4-workaround/index.js:10:5)",
          "    at AuthPlugin.requestDidStart (/Users/mark/src/mark/apollo-v4-issue/v4-workaround/index.js:21:11)",
          "    at /Users/mark/src/mark/apollo-v4-issue/node_modules/@apollo/server/dist/cjs/requestPipeline.js:27:97",
          "    at Array.map (<anonymous>)",
          "    at processGraphQLRequest (/Users/mark/src/mark/apollo-v4-issue/node_modules/@apollo/server/dist/cjs/requestPipeline.js:27:67)",
          "    at internalExecuteOperation (/Users/mark/src/mark/apollo-v4-issue/node_modules/@apollo/server/dist/cjs/ApolloServer.js:615:69)",
          "    at runHttpQuery (/Users/mark/src/mark/apollo-v4-issue/node_modules/@apollo/server/dist/cjs/runHttpQuery.js:135:82)",
          "    at runPotentiallyBatchedHttpQuery (/Users/mark/src/mark/apollo-v4-issue/node_modules/@apollo/server/dist/cjs/httpBatching.js:37:57)",
          "    at ApolloServer.executeHTTPGraphQLRequest (/Users/mark/src/mark/apollo-v4-issue/node_modules/@apollo/server/dist/cjs/ApolloServer.js:525:79)",
          "    at processTicksAndRejections (node:internal/process/task_queues:96:5)"
        ]
      }
    }
  ]
}

Even with the workaround I shared, there is a behavior change between Apollo v3 and v4. How can we recover the behavior of v3? Thanks in advance 👋

Link to Reproduction

https://github.com/markandrus/apollo-v4-issue

Reproduction Steps

Clone my reproduction, run npm install, and the npm test. You can run the v4 and v4-workaround versions independently by cd-ing into them and running npm test.

[dannyan2014]

Thanks for the workaround.
Came here to check for this very issue. We do the auth check in requestDidStart like the OP with apollo server v3.
AuthenticationError is thrown whenever there was an error.
Now with v4, it looks like a custom error can only be thrown from inside a resolver by setting code.
Any custom error thrown inside requestDidStart is changed to a generic INTERNAL_SERVER_ERROR.
We'd like to have this feature back.

[glasser]

A goal in AS4 is to give every error a single place to be handled by plugins. For requestDidStart, we consider errors to be thrown as "unexpected" and handle them in unexpectedErrorProcessingRequest. We can't handle them in the more "usual" place of didEncounterErrors because those callbacks are on the object returned by requestDidStart.

We theoretically could make this into yet another special case with its own top-level requestDidStartDidThrow plugin event but that felt like unnecessary proliferation.

Would doing this check in your context function rather than in a plugin work for you? We do now explicitly support throwing from context functions (in a way that only sort of worked in AS3). Admittedly this doesn't have the same UX as "just put another plugin in the array" but it seems like a reasonable place to do per-request processing.

[markandrus]

For requestDidStart, we consider errors to be thrown as "unexpected" and handle them in unexpectedErrorProcessingRequest. … We theoretically could make this into yet another special case with its own top-level requestDidStartDidThrow plugin event but that felt like unnecessary proliferation.

Makes sense 👍

Would doing this check in your context function rather than in a plugin work for you?

Yes! Thank you for this suggestion 👍 I have converted my plugin to instead be a function I invoke inside of context.

Admittedly this doesn't have the same UX as "just put another plugin in the array" but it seems like a reasonable place to do per-request processing.

I'm not generally a plugin author (we just have a piece for handling auth) and so I don't have strong opinions on the UX. For my use case, the function call inside of context is actually safer, since I can ensure I actually set a context variable with the result of my auth check (whereas the plugin relied on mutation).

So this works well for me. Thanks again.