Error extensions improvements #6759

glasser · 2022-08-02T17:45:34Z

This PR does a few things:

(a) Moves error.extensions.exception.stacktrace to
error.extensions.stacktrace.

(b) Removes the rest of error.extensions.exception. This contained
enumerable properties of the original error, which could lead to
surprising information leakage.

(c) Documents that change and provides a formatError hook
in the migration guide to maintain AS3 behavior.

(d) Provide an unwrapResolverError function in @apollo/server/errors
to help get back the original error in the resolver error case, which is
a helpful part of the documented formatError recommendations
(since the old error formatting put stuff from that unwrapped error
on the exception extension).

(e) Gets rid of the declare module which made
error.extensions.exception.code/error.extensions.exception.stacktrace
have a non-unknown value. Note that this declaration (added in 3.5.0) was
actually inaccurate as code really goes directly on extensions rather than
on exception. We could have instead preserved the declaration
and adapted it to the new location of stacktrace and the correct
location of code.

Pro: declare module is a bit scary and doesn't always merge well if
you have more than one of them (eg, if you end up with both AS3 and AS4
in your TypeScript build: AS3 had a different declare module where
code was nested under exception).
Con: End users who catch our errors can't get "correct" typing for
error.extensions.code or error.extensions.stacktrace.
Pro: That's only "correct" if you assume all GraphQLErrors use
extensions compatible with the definition; it might be better to export
a helper function that takes a GraphQLError and returns the
code/exception if it looks like it has the right shape. And nobody seems
to have even noticed that the declaration has been wrong for almost a
year, so how much value is it providing?

(f) Renames the (new in v4) constructor option
includeStackTracesInErrorResponses to
includeStacktraceInErrorResponses, to match the extension name.

(g) Removes a test around error handling a particular yup style of errors
that is probably not relevant any more.

This is to some degree part of #6719 because we're concerned about the
effect of the declare module on the Gateway transition.

changeset-bot · 2022-08-02T17:45:37Z

🦋 Changeset detected

Latest commit: 7618722

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@apollo/server-integration-testsuite	Patch
@apollo/server	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

netlify · 2022-08-02T17:45:42Z

✅ Deploy Preview for apollo-server-docs ready!

Name	Link
🔨 Latest commit	`7618722`
🔍 Latest deploy log	https://app.netlify.com/sites/apollo-server-docs/deploys/62ed8b428250b00008bf754b
😎 Deploy Preview	https://deploy-preview-6759--apollo-server-docs.netlify.app/migration
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

codesandbox-ci · 2022-08-02T17:46:16Z

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 7618722:

Sandbox	Source
Apollo Server Typescript	Configuration
Apollo Server	Configuration

IvanGoncharov

Yes, declaration merging is problematic so if you think it will cause an issue removing it is the right call.
One possible alternative would be to do a breaking change and just move stacktrace at the top level of extensions.

glasser · 2022-08-03T13:51:46Z

@IvanGoncharov out of curiosity, why does that help? (It does seem reasonable though, esp since we already moved code out.)

glasser · 2022-08-03T18:41:45Z

BTW, we need to add the change of code's location to the migration guide (can be done as part of this PR).

glasser · 2022-08-04T05:32:53Z

@IvanGoncharov OK, I've made a few more changes along these lines.

IvanGoncharov · 2022-08-04T17:11:38Z

packages/server/src/__tests__/errors.test.ts

-      expect(error.extensions.code).toEqual(code);
-      // stacktrace should exist under exception
-      expect(error.extensions.exception.stacktrace).toBeDefined();
+      expect(error.extensions?.key).toEqual(key);


extensions are always present, you shouldn't need optional chaining here.

@IvanGoncharov It's actually optional on GraphQLFormattedError, which is what this is.

IvanGoncharov · 2022-08-04T17:22:30Z

packages/server/src/errorNormalize.ts

@@ -66,17 +67,18 @@ export function normalizeAndFormatErrors(

      if (originalErrorEnumerableEntries.length > 0) {
        extensions.exception = {
-          ...extensions.exception,
+          ...ensureObject(extensions.exception),
          ...Object.fromEntries(originalErrorEnumerableEntries),


It's increasing the scope of this PR, but maybe we can remove the behavior of passing all enumerable properties as extensions.exception.
This practice can be considered as information leaking, similar to "did you mean".
Now that we gave formatError access to the original error, users can peek and choose what properties they want to put into extensions.
So basically, I propose to just get rid of extensions.exception.

IvanGoncharov · 2022-08-04T17:26:59Z

@IvanGoncharov out of curiosity, why does that help? (It does seem reasonable though, esp since we already moved code out.)

My suggestion was based on the idea of completely removing exception.
I focused solely on stacktrace because I forgot that exception also contains top-level properties from the thrown error.
So the main idea is to just remove exception completely.

glasser · 2022-08-04T23:18:38Z

@IvanGoncharov I liked your suggestion and took it.

I updated the PR description, which is worth re-reading.

One thing I found is that documenting that the second argument to formatError is the originally thrown error wasn't quite accurate. That's because in the most common case (errors thrown in resolvers), the originally thrown error will be wrapped in a GraphQLError by the graphql-js locatedError function. I tried to adjust the migration docs to say that the argument might be the original error or you might have to look on the originalError field but that got really weird to explain.

So instead I added a special case to the invocation of formatError. If the error it is formatting is a GraphQLError with a path and an originalError, it passes error.originalError as the second argument instead. This provides easier access to the actually original error. If you need access to error.path (or error.locations), it's right there on the first GraphQLFormattedError argument.

The one downside I see here is that if you wanted access to nodes (or source) you're out of luck. That's a bit annoying.

Another option is to change formatError to take an options argument instead of two arguments, and then provide three arguments:

formatError({
  defaultFormattedError: GraphQLFormattedError,
  thrownError: Error,
  locatedError: GraphQLError | null,
})

If error is the actual error provided to normalizeAndFormatErrors, then:

defaultFormattedError = enrichError(error)
thrownError = error instanceof GraphQLError && error.path && error.originalError
          ? error.originalError
          : error
locatedError = error instanceof GraphQLError && error.path ? error : null

Maybe this is more complicated than it's worth and we should just explain to users that they need to sometimes (but not always!) look at error.originalError in their formatError... but I don't love that either.

IvanGoncharov · 2022-08-05T16:06:19Z

docs/source/migration.mdx

+  error: unknown,
+) {
+  const exception: Record<string, unknown> = {
+    ...(typeof error === 'object' ? error : null),


better to do instanceof Error check here.
We need to protect against strings and arrays (pass you current check).

Also to work with originalError it can be

...(error instanceof Error ? error.originalError ?? error : null),

I think I'm not super-worried about things like strings and arrays here... if you pass something ultra weird, then your code ends up weird. I just want to make sure TS builds.

IvanGoncharov · 2022-08-05T16:29:23Z

packages/server/src/errorNormalize.ts

+        // there is no path, this error didn't come from a resolver.)
+        error instanceof GraphQLError && error.path && error.originalError
+          ? error.originalError
+          : error,


passing originalError makes sense even if path is absent.
example errors thrown from coerceValue/serialize since they also wrapped.
So I recommend dropping the path check here.

IvanGoncharov · 2022-08-05T16:41:29Z

@glasser I think the cleanest semantic would be that formatError gets the most top-level error, and the first argument is serialized version of it and the second argument is a raw error.
Meaning that if you need an original error, you need to look into the chain of originalError yourself.
So your example in migration guide would have this code but using second argument:
https://github.com/apollographql/apollo-server/pull/6759/files#diff-b519aadbfa6ea02ae453a6c785bcf1038f7844c43549e9bcb2eed2944df55b48L61-L73

glasser · 2022-08-05T20:25:25Z

@IvanGoncharov going with your suggestion, plus a new unwrapResolverError function that can be helpful.

trevor-scheer · 2022-08-05T20:57:29Z

packages/integration-testsuite/src/apolloServerTests.ts

@@ -843,7 +843,7 @@ export function defineIntegrationTestSuiteApolloServerTests(
            },
          },
          introspection: true,
-          includeStackTracesInErrorResponses: true,
+          includeStacktraceInErrorResponses: true,


Does having this set to true have any effect on this test? Seems like the object matcher below should be looking for a stack trace, else maybe we just unset this?

Or am I misunderstanding something here maybe?

I think I'm going to delete this test. Reviewing the initial PR that introduced it (#1288) I don't understand what was interesting about this random yup package and suspect it is related to this flattening code which we have deleted. The PR description refers to schema-stitching stuff too. Since the error code is so much simpler now I doubt we have to test one random style of error.

trevor-scheer · 2022-08-05T21:15:44Z

packages/server/src/errors/index.ts

+ * (like parse errors) are not unwrapped by this function.
+ */
+export function unwrapResolverError(error: unknown): unknown {
+  if (error instanceof GraphQLError && error.path && error.originalError) {


Not sure I understand why we also need the error.path check here.

Unrelated: I didn't find anything interesting on this path but maybe you will. Pretty sure the incoming unknown makes any attempt at refining the return type pointless. Might only be useful in a context where you have some type info about the incoming error argument.

export function unwrapResolverError<T = unknown>(error: T): T | Error { if (error instanceof GraphQLError && error.path && error.originalError) { return error.originalError; } return error; }

@trevor-scheer Well, that's what makes this "unwrapResolverError" and not just "unwrapError".

Look at, say, SyntaxError — it's a GraphQLError defined in our code that wraps a GraphQLError received from graphql-js, adding code as an extension. I don't think it's helpful for a formatError to strip off our SyntaxError, is it?

Got it, I was only seeing this as a type-level refinement and didn't see why we needed it to access originalError.

This PR does a few things: (a) Moves `error.extensions.exception.stacktrace` to `error.extensions.stacktrace`. (b) Removes the rest of `error.extensions.exception`. This contained enumerable properties of the original error, which could lead to surprising information leakage. (c) Documents that change and provides a `formatError` hook in the migration guide to maintain AS3 behavior. (d) Provide an `unwrapResolverError` function in `@apollo/server/errors` to help get back the original error in the resolver error case, which is a helpful part of the documented `formatError` recommendations (since the old error formatting put stuff from that unwrapped error on the `exception` extension). (e) Gets rid of the `declare module` which made `error.extensions.exception.code`/`error.extensions.exception.stacktrace` have a non-unknown value. Note that this declaration (added in 3.5.0) was actually inaccurate as `code` really goes directly on `extensions` rather than on `exception`. We could have instead preserved the declaration and adapted it to the new location of `stacktrace` and the correct location of `code`. - Pro: `declare module` is a bit scary and doesn't always merge well if you have more than one of them (eg, if you end up with both AS3 and AS4 in your TypeScript build: AS3 had a different `declare module` where `code` was nested under `exception`). - Con: End users who catch our errors can't get "correct" typing for `error.extensions.code` or `error.extensions.stacktrace`. - Pro: That's only "correct" if you assume all GraphQLErrors use extensions compatible with the definition; it might be better to export a helper function that takes a `GraphQLError` and returns the code/exception if it looks like it has the right shape. And nobody seems to have even noticed that the declaration has been wrong for almost a year, so how much value is it providing? (f) Renames the (new in v4) constructor option includeStackTracesInErrorResponses to includeStacktraceInErrorResponses, to match the extension name. This is to some degree part of #6719 because we're concerned about the effect of the `declare module` on the Gateway transition.

Until now, the refactored AS4 did not support Apollo Gateway (or any implementation of the AS3 `gateway` option). That's because `GraphQLRequestContext` is part of the API between Apollo Gateway and Apollo Server, and that type has changed in some minor but incompatible ways in AS4. (Additionally, clashes between `declare module` declarations in AS3 and AS4 caused issue, but we removed those declarations from this branch in PRs #6764 and #6759.) This commit restores gateway support. It does this by having AS4 produce an AS3-style request context object. It uses a new `@apollo/server-gateway-interface` package to define the appropriate types for connecting to Gateway. (Note: this package will be code reviewed in this PR in this repo, but once it's approved, it will move to the apollo-utils repo and be released as non-alpha there. Once we've released AS4.0.0 we can move that package back here, but trying to do some prereleases and some non-prereleases on the same branch seems challenging.) This PR removes the top-level `executor` function, which is redundant with the `gateway` option. (Internally, the relevant field is now named `gatewayExecutor`.) Some types had been parametrized by `TContext`, because in AS3, `GraphQLExecutor` (now `GatewayExecutor`) appeared to take a `<TContext>`. However, even though the type itself took a generic argument, its main use in the return from `gateway.load` implicitly hardcoded the default `TContext`. So we are doubling down on that and only allowing `GraphQLExecutor` to use AS3's default `TContext`, the quite flexible `Record<string, any>`. Most of the way toward #6719.

Until now, the refactored AS4 did not support Apollo Gateway (or any implementation of the AS3 `gateway` option). That's because `GraphQLRequestContext` is part of the API between Apollo Gateway and Apollo Server, and that type has changed in some minor but incompatible ways in AS4. (Additionally, clashes between `declare module` declarations in AS3 and AS4 caused issue, but we removed those declarations from this branch in PRs #6764 and #6759.) This commit restores gateway support. It does this by having AS4 produce an AS3-style request context object. It uses a new `@apollo/server-gateway-interface` package to define the appropriate types for connecting to Gateway. (Note: this package will be code reviewed in this PR in this repo, but once it's approved, it will move to the apollo-utils repo and be released as non-alpha there. Once we've released AS4.0.0 we can move that package back here, but trying to do some prereleases and some non-prereleases on the same branch seems challenging.) This PR removes the top-level `executor` function, which is redundant with the `gateway` option. (Internally, the relevant field is now named `gatewayExecutor`.) Some types had been parametrized by `TContext`, because in AS3, `GraphQLExecutor` (now `GatewayExecutor`) appeared to take a `<TContext>`. However, even though the type itself took a generic argument, its main use in the return from `gateway.load` implicitly hardcoded the default `TContext`. So we are doubling down on that and only allowing `GraphQLExecutor` to use AS3's default `TContext`, the quite flexible `Record<string, any>`. Most of the way toward #6719. Co-authored-by: Trevor Scheer <trevor.scheer@gmail.com>

glasser requested a review from IvanGoncharov August 2, 2022 17:45

IvanGoncharov reviewed Aug 3, 2022

View reviewed changes

glasser force-pushed the glasser/no-declare-module-for-error-extensions branch 2 times, most recently from 2e87966 to aa31257 Compare August 4, 2022 05:32

glasser changed the title ~~Remove declare module for error extensions~~ Error extensions improvements Aug 4, 2022

IvanGoncharov reviewed Aug 4, 2022

View reviewed changes

glasser force-pushed the glasser/no-declare-module-for-error-extensions branch 5 times, most recently from 833b461 to 407ef56 Compare August 4, 2022 23:06

IvanGoncharov reviewed Aug 5, 2022

View reviewed changes

glasser force-pushed the glasser/no-declare-module-for-error-extensions branch from 407ef56 to 26bd3f1 Compare August 5, 2022 20:24

glasser requested a review from trevor-scheer August 5, 2022 20:24

glasser force-pushed the glasser/no-declare-module-for-error-extensions branch 3 times, most recently from 1f4193d to c4238de Compare August 5, 2022 20:28

trevor-scheer approved these changes Aug 5, 2022

View reviewed changes

glasser force-pushed the glasser/no-declare-module-for-error-extensions branch from c4238de to 7618722 Compare August 5, 2022 21:27

glasser merged commit 6ef6a09 into version-4 Aug 5, 2022

glasser deleted the glasser/no-declare-module-for-error-extensions branch August 5, 2022 21:29

glasser mentioned this pull request Aug 8, 2022

Gateway support via new @apollo/server-gateway-interface package #6771

Merged

github-actions bot mentioned this pull request Aug 9, 2022

Version Packages (alpha) #6786

Merged

glasser mentioned this pull request Aug 13, 2022

Add and update the docs for "Defining a Schema" and "Fetching Data" #6776

Merged

github-actions bot mentioned this pull request Oct 10, 2022

Version Packages #7017

Merged

github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Error extensions improvements #6759

Error extensions improvements #6759

glasser commented Aug 2, 2022 •

edited

Loading

changeset-bot bot commented Aug 2, 2022 •

edited

Loading

netlify bot commented Aug 2, 2022 •

edited

Loading

codesandbox-ci bot commented Aug 2, 2022 •

edited

Loading

IvanGoncharov left a comment

glasser commented Aug 3, 2022

glasser commented Aug 3, 2022

glasser commented Aug 4, 2022

IvanGoncharov Aug 4, 2022

glasser Aug 4, 2022

IvanGoncharov Aug 4, 2022 •

edited

Loading

IvanGoncharov commented Aug 4, 2022

glasser commented Aug 4, 2022

IvanGoncharov Aug 5, 2022

IvanGoncharov Aug 5, 2022

glasser Aug 5, 2022

IvanGoncharov Aug 5, 2022

IvanGoncharov commented Aug 5, 2022

glasser commented Aug 5, 2022

trevor-scheer Aug 5, 2022

glasser Aug 5, 2022

trevor-scheer Aug 5, 2022

glasser Aug 5, 2022

trevor-scheer Aug 5, 2022

Error extensions improvements #6759

Error extensions improvements #6759

Conversation

glasser commented Aug 2, 2022 • edited Loading

changeset-bot bot commented Aug 2, 2022 • edited Loading

🦋 Changeset detected

netlify bot commented Aug 2, 2022 • edited Loading

✅ Deploy Preview for apollo-server-docs ready!

codesandbox-ci bot commented Aug 2, 2022 • edited Loading

IvanGoncharov left a comment

Choose a reason for hiding this comment

glasser commented Aug 3, 2022

glasser commented Aug 3, 2022

glasser commented Aug 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanGoncharov Aug 4, 2022 • edited Loading

Choose a reason for hiding this comment

IvanGoncharov commented Aug 4, 2022

glasser commented Aug 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanGoncharov commented Aug 5, 2022

glasser commented Aug 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glasser commented Aug 2, 2022 •

edited

Loading

changeset-bot bot commented Aug 2, 2022 •

edited

Loading

netlify bot commented Aug 2, 2022 •

edited

Loading

codesandbox-ci bot commented Aug 2, 2022 •

edited

Loading

IvanGoncharov Aug 4, 2022 •

edited

Loading