Fully replace 'ApolloError' with 'GraphQLError' #6355

IvanGoncharov · 2022-04-25T17:01:07Z

No description provided.

codesandbox-ci · 2022-04-25T17:01:54Z

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 250e9f2:

Sandbox	Source
Apollo Server Typescript	Configuration
Apollo Server	Configuration

IvanGoncharov · 2022-04-25T17:08:52Z

Big step toward #6053

packages/server/src/errors.ts

glasser

Generally looks good! I'd like to see a bit more in the PR description (like say if there's any behavior lost by removing ApolloError, what things might come next, etc).

packages/server/src/errors.ts

packages/server/src/__tests__/integration/apolloServerTests.ts

glasser

looking great!

packages/server/src/errors.ts

glasser · 2022-05-31T18:41:38Z

packages/server/src/errors.ts

-        // XXX: This cast is pretty sketchy, as other error types can be thrown!
-        return enrichError(err as Partial<GraphQLError>, debug);
+      if (includeStackTracesInErrorResponses) {
+        return enrichError(err);


This seems like the same as before and good logic but while reviewing this confusing code I think it could be made more clear.

Let's rename err to formattingError and call out explicitly "includeStackTracesInErrorResponses is used in development so it will be helpful to show errors thrown by formatError hooks in that mode". Though honestly maybe we should wrap the error in some sort of "error thrown in formatError hook" error?

Implemented your feedback.

Though honestly maybe we should wrap the error in some sort of "error thrown in formatError hook" error?

Not sure I need to think about it.
Maybe it's overkill but maybe not.

glasser · 2022-05-31T18:44:59Z

packages/server/src/errors.ts


-    Object.defineProperty(this, 'name', { value: 'BadRequestError' });
+    this.name = 'BadRequestError';
  }
 }

 // This function should not throw.
 export function formatApolloErrors(


Can you add a docstring that explains what this function actually does? I think it now is way more coherent then it used to be and the docstring might actually make sense!

IIUC it's:

Convert non-Error to Error

Convert non-GraphQLError to GraphQLError

Add a default extensions.code

This strange block that I don't 100% get the motivation of and would be great if it had a clear description: if (originalError != null && !(originalError instanceof GraphQLError))

Copy stack trace onto an extension if requested

Convert to GraphQLFormatterError

Pass through formatError if provided (doing some error handling if that throws)

Also before merging let's make sure the PR description has at least a brief list of all the changes that are made. Then we can flesh that out into the migration guide draft (which should be merged from #6453 soon) in a follow-up PR.

Also I think you said this but probably the function should get a better name since there are no more Apollo Errors. normalizeAndFormatErrors?

I like "enrich" more than "normalize" since we only add stuff and don't remove stuff.
formatError can remove stuff but it's more of "formating" part of this function.
enrichAndFormatErrors?

The normalize part to me is how we convert non-Error and non-GraphQLError to GraphQLError... but I see what you mean.

packages/server/src/requestPipeline.ts

glasser · 2022-06-01T00:17:05Z

packages/server/src/errors.ts

    try {
-      return makePrintable(formatter(error));
+      return formatError(enrichError(error), error);


Interesting note: the change to two-arg formatError actually makes the fact that formatError is integrated into this function rather than just a second map over its output more crucial.
(ie, I was going to propose that (perhaps in a follow-up PR) we just remove this option and have the callers call formatError instead (or totally drop formatError and just have callers use didEncounterErrors from the plugin API) but this new argument makes the current structure make more sense.)

@glasser

Based on @glasser review suggestion

IvanGoncharov · 2022-06-02T16:30:12Z

Just saw the code example in this article.
The new signature for API exactly matches provided code sample:
https://blog.escape.tech/graphql-error-handling-a-security-pov/#how-to-mask-sensitive-information-and-stack-traces

So it's independent proof that the new API is more intuitive.

glasser · 2022-06-02T16:34:05Z

packages/server/src/__tests__/integration/apolloServerTests.ts

+        expect(result.errors[0].extensions.code).toEqual('UNAUTHENTICATED');
+        expect(result.errors[0].extensions.ext1).toEqual('myExt');
+        expect(result.errors[0].extensions.exception).toBeDefined();
+        expect(result.errors[0].extensions.exception.ext1).toBeUndefined();


should we also check result.errors[0].extensions.exception.extensions is undefined? basically we want to make sure it isn't there twice. (Or we could just do an inline snapshot on result tbh)

glasser · 2022-06-02T16:40:48Z

(Approved though after merge this PR should be updated with a description of the changes.)

Implement fix proposed in apollographql#6558 but implement it on top of AS4 Also contain some cleanup and refactoring related to apollographql#6355

@trevor-scheer

* migration: start of an editing pass * More progress on an editing pass * make it build * Reorganize and rework migration some more * Apply suggestions from code review Co-authored-by: Rose M Koron <32436232+rkoron007@users.noreply.github.com> * migration: result of auditing the change in exported API * migration: Add description of changes from #6355 * Reorganize error handling changes * migration: gql tag * Apply suggestions from code review Co-authored-by: Rose M Koron <32436232+rkoron007@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Rose M Koron <32436232+rkoron007@users.noreply.github.com> * Tweak gql section including re-adding triple-quotes because GH suggestions don't like them. And I guess trimming trailing newlines which I guess my editor does and which got introduced recently? * Simplify some bullets and add body-parser/cors section back * A few @trevor-scheer related changes * Apply suggestions from code review Co-authored-by: Rose M Koron <32436232+rkoron007@users.noreply.github.com> * Follow-up to the previous one * gateway * as shown above * context Co-authored-by: Rose M Koron <32436232+rkoron007@users.noreply.github.com> Co-authored-by: Ivan Goncharov <ivan.goncharov.ua@gmail.com>

…oError This is part of apollographql/apollo-server#6057 (which is itself part of apollographql/apollo-server#6719). We are trying to break the dependency of Gateway on Server so that (among other things) it is easier to have a single version of Gateway that works with both the current AS3 and the upcoming AS4. In AS4, we are removing the ApolloError class and its subclasses. Instead, we will just use GraphQLError directly. See: https://www.apollographql.com/docs/apollo-server/v4/migration#apolloerror https://www.apollographql.com/docs/apollo-server/v4/migration#built-in-error-classes apollographql/apollo-server#6355 apollographql/apollo-server#6705 This commit changes RemoteGraphQLDataSource to throw GraphQLError instead of ApolloError. The `code` extension will still be the same. (The `name` field of the thrown Error will no longer be eg `AuthenticationError`, though; this does not affect the error as serialized in GraphQL.) This is technically slightly backwards-incompatible (eg, the method errorFromResponse is public and now returns GraphQLError instead of the tighter ApolloError) but this doesn't seem likely to affect many users. We can adjust based on feedback if necessary.

This is part of apollographql/apollo-server#6057 (which is itself part of apollographql/apollo-server#6719). We are trying to break the dependency of Gateway on Server so that (among other things) it is easier to have a single version of Gateway that works with both the current AS3 and the upcoming AS4. In AS4, we are removing the ApolloError class and its subclasses. Instead, we will just use GraphQLError directly. See: https://www.apollographql.com/docs/apollo-server/v4/migration#apolloerror https://www.apollographql.com/docs/apollo-server/v4/migration#built-in-error-classes apollographql/apollo-server#6355 apollographql/apollo-server#6705 This commit changes RemoteGraphQLDataSource to throw GraphQLError instead of ApolloError. The `code` extension will still be the same. (The `name` field of the thrown Error will no longer be eg `AuthenticationError`, though; this does not affect the error as serialized in GraphQL.) This is technically slightly backwards-incompatible (eg, the method errorFromResponse is public and now returns GraphQLError instead of the tighter ApolloError) but this doesn't seem likely to affect many users. We can adjust based on feedback if necessary.

…2028) This is part of apollographql/apollo-server#6057 (which is itself part of apollographql/apollo-server#6719). We are trying to break the dependency of Gateway on Server so that (among other things) it is easier to have a single version of Gateway that works with both the current AS3 and the upcoming AS4. In AS4, we are removing the ApolloError class and its subclasses. Instead, we will just use GraphQLError directly. See: https://www.apollographql.com/docs/apollo-server/v4/migration#apolloerror https://www.apollographql.com/docs/apollo-server/v4/migration#built-in-error-classes apollographql/apollo-server#6355 apollographql/apollo-server#6705 This commit changes RemoteGraphQLDataSource to throw GraphQLError instead of ApolloError. The `code` extension will still be the same. (The `name` field of the thrown Error will no longer be eg `AuthenticationError`, though; this does not affect the error as serialized in GraphQL.) This is technically slightly backwards-incompatible (eg, the method errorFromResponse is public and now returns GraphQLError instead of the tighter ApolloError) but this doesn't seem likely to affect many users. We can adjust based on feedback if necessary.

* gateway: remove dependency on apollo-server-caching (#2029) The point of apollo-server-caching is to provide an abstraction over multiple cache backends. Gateway is not using that abstraction; it's just using one particular implementation, which is a wrapper around an old version of lru-cache. As part of apollographql/apollo-server#6057 and apollographql/apollo-server#6719 we want to remove dependencies on Apollo Server from Apollo Gateway. Technically we don't really need to remove this dependency (since apollo-server-caching doesn't depend on anything else in AS) but apollo-server-caching won't be updated any more (in fact, even in AS3 it has already been replaced by `@apollo/utils.keyvaluecache`), so let's do it. While we're at it, we make a few other improvements: - Ever since #440, the queryPlanStore field is always set, so we can remove some conditionals around it. - Instead of using the old lru-cache@6 wrapped by the apollo-server-caching package, we use the newer lru-cache@7 (which improves the algorithm internally and changes the names of methods a bit). - The get and set methods on InMemoryLRUCache were only async because they implement the abstract KeyValueCache interface: the implementations didn't actually do anything async. So we no longer need to await them or include a giant comment about how we're not awaiting them. * gateway RemoteGraphQLDataSource: throw GraphQLError, not ApolloError (#2028) This is part of apollographql/apollo-server#6057 (which is itself part of apollographql/apollo-server#6719). We are trying to break the dependency of Gateway on Server so that (among other things) it is easier to have a single version of Gateway that works with both the current AS3 and the upcoming AS4. In AS4, we are removing the ApolloError class and its subclasses. Instead, we will just use GraphQLError directly. See: https://www.apollographql.com/docs/apollo-server/v4/migration#apolloerror https://www.apollographql.com/docs/apollo-server/v4/migration#built-in-error-classes apollographql/apollo-server#6355 apollographql/apollo-server#6705 This commit changes RemoteGraphQLDataSource to throw GraphQLError instead of ApolloError. The `code` extension will still be the same. (The `name` field of the thrown Error will no longer be eg `AuthenticationError`, though; this does not affect the error as serialized in GraphQL.) This is technically slightly backwards-incompatible (eg, the method errorFromResponse is public and now returns GraphQLError instead of the tighter ApolloError) but this doesn't seem likely to affect many users. We can adjust based on feedback if necessary. * Adjust #2028 for graphql@15.8 compatibility

IvanGoncharov requested review from glasser and trevor-scheer April 25, 2022 17:01

trevor-scheer reviewed Apr 25, 2022

View reviewed changes

packages/server/src/errors.ts Outdated Show resolved Hide resolved

packages/server/src/errors.ts Outdated Show resolved Hide resolved

packages/server/src/errors.ts Outdated Show resolved Hide resolved

glasser reviewed Apr 27, 2022

View reviewed changes

packages/server/src/errors.ts Outdated Show resolved Hide resolved

packages/server/src/errors.ts Show resolved Hide resolved

packages/server/src/__tests__/integration/apolloServerTests.ts Show resolved Hide resolved

IvanGoncharov force-pushed the removeApolloError branch 2 times, most recently from 8606699 to 847a5db Compare May 9, 2022 16:51

glasser reviewed May 31, 2022

View reviewed changes

packages/server/src/requestPipeline.ts Outdated Show resolved Hide resolved

glasser reviewed Jun 1, 2022

View reviewed changes

IvanGoncharov added 11 commits June 2, 2022 17:42

Make ApolloError a subclass of GraphQLError

90a9bbf

Obscure error to simply be ApolloError

9a0eac4

Fully replace 'ApolloError' with 'GraphQLError'

5415403

GraphQLError now has 'toJSON' that provide all those fields

905b4a2

Remove toApolloError

8a27f39

Fix type of return value for formatApolloErrors

8966618

Update graphql to 16.5.0

ad03f0a

Rename formatApolloErrors options to match rest of source code

afda3fe

Rewrite formatApolloErrors

a581ec0

Fix tests

5ec6eb6

Fix prettier

79640b8

IvanGoncharov force-pushed the removeApolloError branch from 14369e5 to 79640b8 Compare June 2, 2022 14:58

IvanGoncharov added 4 commits June 2, 2022 18:18

remove fromGraphQLError + change signature for Error constructor

77b7592

Add missing 'code' to "obscure error"

fcedd6a

Make handling of errors thrown by formatError more explicit

17140ac

Based on @glasser review suggestion

added test to ensure we providing error code even for "obscure error"

972b05b

IvanGoncharov force-pushed the removeApolloError branch from 6ad20f1 to 972b05b Compare June 2, 2022 16:11

Bring back integration test that tested how we handle error extensions

e2c684d

glasser reviewed Jun 2, 2022

View reviewed changes

glasser approved these changes Jun 2, 2022

View reviewed changes

Address review comment

250e9f2

IvanGoncharov merged commit 1f63af9 into apollographql:version-4 Jun 2, 2022

IvanGoncharov deleted the removeApolloError branch June 2, 2022 16:47

glasser mentioned this pull request Jun 2, 2022

Rethink ApolloError #6053

Closed

IvanGoncharov mentioned this pull request Jun 14, 2022

Add error code to parse/validate errors passed to 'didEncounterErrors' #6572

Merged

IvanGoncharov added a commit to IvanGoncharov/apollo-server that referenced this pull request Jul 18, 2022

migration: Add description of changes from apollographql#6355

741e525

IvanGoncharov added a commit to IvanGoncharov/apollo-server that referenced this pull request Jul 18, 2022

migration: Add description of changes from apollographql#6355

2b0b51f

glasser pushed a commit that referenced this pull request Jul 20, 2022

migration: Add description of changes from #6355

0320f2e

glasser mentioned this pull request Aug 2, 2022

gateway RemoteGraphQLDataSource: throw GraphQLError, not ApolloError apollographql/federation#2028

Merged

trevor-scheer mentioned this pull request Oct 18, 2022

fix: add getter for field Symbol.toStringTag whose absence causes Typ… #5663

Closed

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

Fully replace 'ApolloError' with 'GraphQLError' #6355

Fully replace 'ApolloError' with 'GraphQLError' #6355

IvanGoncharov commented Apr 25, 2022

codesandbox-ci bot commented Apr 25, 2022 •

edited

IvanGoncharov commented Apr 25, 2022

glasser left a comment

glasser left a comment

glasser May 31, 2022

IvanGoncharov Jun 2, 2022 •

edited

glasser May 31, 2022

glasser May 31, 2022

glasser May 31, 2022

IvanGoncharov Jun 2, 2022

glasser Jun 2, 2022

glasser Jun 1, 2022

IvanGoncharov commented Jun 2, 2022

glasser Jun 2, 2022

glasser commented Jun 2, 2022

Fully replace 'ApolloError' with 'GraphQLError' #6355

Fully replace 'ApolloError' with 'GraphQLError' #6355

Conversation

IvanGoncharov commented Apr 25, 2022

codesandbox-ci bot commented Apr 25, 2022 • edited

IvanGoncharov commented Apr 25, 2022

glasser left a comment

Choose a reason for hiding this comment

glasser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanGoncharov Jun 2, 2022 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanGoncharov commented Jun 2, 2022

Choose a reason for hiding this comment

glasser commented Jun 2, 2022

codesandbox-ci bot commented Apr 25, 2022 •

edited

IvanGoncharov Jun 2, 2022 •

edited