Invoke `didEncounterErrors` for known errors during pre-parse. #3614

abernix · 2019-12-16T16:29:02Z

Roughly, these "pre-parse" errors come in two forms: When the query was entirely omitted, or APQ (automated persisted query) errors.

Arguably, there may be other forms of pre-parse errors, but those would be entirely unexpected (or based on external factors) and potentially out of scope of the didEncounterErrors life-cycle hook. Such errors would still be caught by runHttpQuery.ts, but we wouldn't have a guarantee that any plugins have been initialized. This is unlikely to improve until we re-work the entirety of the request pipeline in Apollo Server 3.x.

While these errors were in-fact causing errors at runtime that were being delivered to the client, those errors were not being delivered to the plugin's didEncounterErrors hooks, causing reduced visibility by reporting tools which utilize that life-cycle hook.

This commit changes the parent class of InvaidGraphQLRequestError from Error to GraphQLError in order to allow such an error to be received by didEncounterErrors, which requires GraphQLErrors. The GraphQLError itself is still an instanceof Error, so any existing code that leverages such a condition should still work as expected, and I suspect that most other conditions that could be checked on either an Error or GraphQLError should also still remain the same. Despite any absolute certainty here, I'd call this a net-win for reporting errors more reliably to the hook on which they're expected to be received!

Previously a missing test.

…vements). Previously, a number of APQ errors did not have tests for errors which could arise at runtime. These include: - When the APQ `version` is not a version that we support. (Currently, the only supported `version` is `1`. - When the APQ `version` is entirely omitted. (It must be `1`.) - When the provided APQ `sha256Hash` doesn't match the provided `query` during APQ registration. I'm adding these tests which assert the existing logic without making any changes to the logic itself prior to making a number of changes to said logic which will ensure that these (again, existing) errors are _also_ propagated to the `didEncounterErrors` hook (They currently are NOT!).

Roughly, these "pre-parse" errors come in two forms: When the query was entirely omitted, or APQ (automated persisted query) errors. Arguably, there is a third form of pre-parse errors, but it would be unexpected and potentially out of scope of the `didEncounterErrors` life-cycle hook. Such an error would still be caught by `runHttpQuery.ts`, but we wouldn't have a guarantee that our plugins have been initialized. This is unlikely to improve until we re-work the entirety of the request pipeline in Apollo Server 3.x. While these errors were in-fact causing errors at runtime that were being delivered to the client, those errors were not being delivered to the plugin's `didEncounterErrors` hooks, causing reduced visibility by reporting tools which utilize that life-cycle hook. This commit changes the parent class of `InvaidGraphQLRequestError` from `Error` to `GraphQLError` in order to allow such an error to be received by `didEncounterErrors`, which requires `GraphQLError`s. The `GraphQLError` itself is still an `instanceof Error`, so any existing code that leverages such a condition should still work as expected, and I suspect that most other conditions that could be checked on either an `Error` or `GraphQLError` should also still remain the same. Despite any uncertain here, I'd call this a net-win for reporting errors more reliably to the hook on which they're expected to be received.

trevor-scheer · 2019-12-16T17:09:24Z

packages/apollo-server-integration-testsuite/src/index.ts

+
+        // Only the first request should have resulted in an error, not the
+        // second.
+        expect(didEncounterErrors).toHaveBeenCalledTimes(1);


This expectation is duplicated immediately above

Intentionally! Added comment to clarify: 21aa570.

Let me know if it makes sense?

Hmm, in retrospect, you're right that this additional assertion may be confusing and unnecssary. Maybe it would make more sense if I moved the first expecttation to be along-side the expect(didEncounterErrors).toBeCalledWith( on line 1381?

See 66c32f8

@trevor-scheer

Ref: https://github.com/apollographql/apollo-server/pull/3614/files#r358355794 Cc: @trevor-scheer

trevor-scheer

This LGTM! The additions to existing test cases (and new ones) are excellent! Thanks for taking the time to flesh those out further 👍

…ounterErrors

abernix · 2019-12-16T17:29:58Z

packages/apollo-server-types/src/index.ts

@@ -93,7 +93,7 @@ export interface GraphQLRequestContext<TContext = Record<string, any>> {

 export type ValidationRule = (context: ValidationContext) => ASTVisitor;

-export class InvalidGraphQLRequestError extends Error {}
+export class InvalidGraphQLRequestError extends GraphQLError {}


This changes the parent class of InvaidGraphQLRequestError from Error to GraphQLError in order to allow such an error to be received by didEncounterErrors, which requires GraphQLErrors.

The GraphQLError itself is still an instanceof Error, so any existing code that leverages such a condition should still work as expected, and I suspect that most other conditions that could be checked on either an Error or GraphQLError should also still remain the same. Despite any uncertain here, I'd call this a net-win for reporting errors more reliably to the hook on which they're expected to be received.

…ounterErrors

…rting. The fix in #3614 changed behavior which was not surfacing errors to the extensions and request pipeline plugins when those errors occurred during APQ negotiation. However, it failed to consider - nor were there any tests - which ensured that the `apollo-engine-reporting`'s mechanism didn't receive an error earlier in the request pipeline than previously allowed. This applies a fix which special-cases the APQ error in this case, and avoids reporting it to Apollo Graph Manager (which is the same behavior as before).

To preserve the same behavior as before #3614.

…rting. The fix in #3614 changed behavior which was not surfacing errors to the extensions and request pipeline plugins when those errors occurred during APQ negotiation. However, it failed to consider - nor were there any tests - which ensured that the `apollo-engine-reporting`'s mechanism didn't receive an error earlier in the request pipeline than previously allowed. This applies a fix which special-cases the APQ error in this case, and avoids reporting it to Apollo Graph Manager (which is the same behavior as before).

To preserve the same behavior as before #3614.

The fix in #3614 changed behavior which was not surfacing errors to the extensions and request pipeline plugins when those errors occurred during APQ negotiation. However, it failed to consider - nor were there any tests - which ensured that the `apollo-engine-reporting`'s mechanism didn't receive an error earlier in the request pipeline than previously allowed. This applies a fix which special-cases the APQ error in this case, and avoids reporting it to Apollo Graph Manager (which is the same behavior as before). We also introduce a test for exercising this regression, though it's certainly a test which makes sure that the A-E-R functionality works, not something that tests that errors are not thrown before the expected time in a more general "extensions"-based sense.

Previously, prior to the new plugin API, the Apollo Engine Reporting mechanism was implemented using `graphql-extensions`, the API for which didn't invoke `requestDidStart` until _after_ APQ had been negotiated. The new plugin API starts its `requestDidStart` _before_ APQ validation and various other assertions which weren't included in the `requestDidStart` life-cycle, even if they perhaps should be in terms of error reporting. The new plugin API is able to properly capture such errors within its `didEncounterErrors` lifecycle hook (thanks to #3614, which intentionally captures these failures so plugin authors can accurately react to them), however, for behavioral consistency reasons, we will still special-case those errors and maintain the legacy behavior to avoid a breaking change. We can reconsider this in a future version of Apollo Engine Reporting (AS3, perhaps!). Ref: #3614 Ref: #3627 Ref: #3638

This fixes the failing tests which correctly surfaced on the last commit. Previously, prior to the new plugin API, the Apollo Engine Reporting mechanism was implemented using `graphql-extensions`, the API for which didn't invoke `requestDidStart` until _after_ APQ had been negotiated. The new plugin API starts its `requestDidStart` _before_ APQ validation and various other assertions which weren't included in the `requestDidStart` life-cycle, even if they perhaps should be in terms of error reporting. The new plugin API is able to properly capture such errors within its `didEncounterErrors` lifecycle hook (thanks to #3614, which intentionally captures these failures so plugin authors can accurately react to them), however, for behavioral consistency reasons, we will still special-case those errors and maintain the legacy behavior to avoid a breaking change. We can reconsider this in a future version of Apollo Engine Reporting (AS3, perhaps!). Ref: #3614 Ref: #3627 Ref: #3638

abernix added 3 commits December 16, 2019 17:44

tests: Assert didEncounterErrors is invoked for execution errors.

9ab2827

Previously a missing test.

abernix requested review from queerviolet and trevor-scheer December 16, 2019 16:29

trevor-scheer reviewed Dec 16, 2019

View reviewed changes

Add comment which better explains intent for duplicated assertion.

21aa570

Ref: https://github.com/apollographql/apollo-server/pull/3614/files#r358355794 Cc: @trevor-scheer

trevor-scheer approved these changes Dec 16, 2019

View reviewed changes

abernix added 3 commits December 16, 2019 19:12

Merge branch 'master' into abernix/deliver-pre-parse-errors-to-didEnc…

975dc4b

…ounterErrors

Follow-up clarity to 21aa570.

66c32f8

Add CHANGELOG.md for #3614.

76da502

abernix commented Dec 16, 2019

View reviewed changes

abernix added 2 commits December 16, 2019 19:32

Merge branch 'master' into abernix/deliver-pre-parse-errors-to-didEnc…

88a1988

…ounterErrors

Merge branch 'master' into abernix/deliver-pre-parse-errors-to-didEnc…

323beca

…ounterErrors

abernix changed the title ~~Invoke didEncounterErrors for known errors during pre-parse phases.~~ Invoke didEncounterErrors for known errors during pre-parse. Dec 17, 2019

abernix merged commit dcbbc34 into master Dec 17, 2019

abernix deleted the abernix/deliver-pre-parse-errors-to-didEncounterErrors branch December 17, 2019 11:58

abernix added this to the Release 2.9.14 milestone Dec 17, 2019

eugene1g mentioned this pull request Dec 23, 2019

apollo-server-core 2.9.14 breaks APQ usage #3627

Closed

abernix added a commit that referenced this pull request Dec 26, 2019

Also special case other pre-requestDidStart errors.

834b529

To preserve the same behavior as before #3614.

abernix mentioned this pull request Dec 26, 2019

Fix regression caused by #3614 when using APQ with Graph Manager reporting. #3638

Merged

abernix added a commit that referenced this pull request Dec 27, 2019

Also special case other pre-requestDidStart errors.

92e8e44

To preserve the same behavior as before #3614.

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Invoke `didEncounterErrors` for known errors during pre-parse. #3614

Invoke `didEncounterErrors` for known errors during pre-parse. #3614

abernix commented Dec 16, 2019

trevor-scheer Dec 16, 2019

abernix Dec 16, 2019

abernix Dec 16, 2019

abernix Dec 16, 2019

trevor-scheer left a comment

abernix Dec 16, 2019

Invoke didEncounterErrors for known errors during pre-parse. #3614

Invoke didEncounterErrors for known errors during pre-parse. #3614

Conversation

abernix commented Dec 16, 2019

trevor-scheer Dec 16, 2019

Choose a reason for hiding this comment

abernix Dec 16, 2019

Choose a reason for hiding this comment

abernix Dec 16, 2019

Choose a reason for hiding this comment

abernix Dec 16, 2019

Choose a reason for hiding this comment

trevor-scheer left a comment

Choose a reason for hiding this comment

abernix Dec 16, 2019

Choose a reason for hiding this comment

Invoke `didEncounterErrors` for known errors during pre-parse. #3614

Invoke `didEncounterErrors` for known errors during pre-parse. #3614