extensions: add setParsed and capture error in extensions #2659

evans · 2019-05-10T01:45:28Z

Currently we don't set the parsed document in extensions immediately,
which means that apollo-engine-reporting is forced to return a full signature
instead of the normalized signature. This leads to cardinality
explosion. This ensures that we report with a normalized signature and
report errors that occur in the requestPipeline.

TODO:

Update CHANGELOG.md with your change (include reference to issue & this PR)
Rebase your changes on master so that they can be merged easily

jbaxleyiii

@evans this looks great! Thanks for finding this edge case with the operation registry (or other custom usages)

evans · 2019-05-10T02:48:14Z

Currently this leaves out the operation name, which we should add to the setParsedDocument

Currently we don't set the parsed document in extensions immediately, which means that apollo-engine-reporting is forced to return a full signature instead of the normalized signature. This leads to cardinality explosion. This ensures that we report with a normalized signature and report errors that occur in the requestPipeline.

evans · 2019-05-10T17:52:27Z

Tested ^ on apigateway

abernix

I'd like to avoid adding a very broad, dual-purpose setter method like this to the API surface area if we can avoid it, so I'd appreciate your thoughts on the following, which I think demonstrates how this could be done using existing information that's already available to the request pipeline.

(Also worth noting that didEncounterErrors and the setting of documentAST pre-didResolveOperation are, I think, two separate and unrelated concerns?)

abernix · 2019-05-13T12:15:38Z

CHANGELOG.md

@@ -1,5 +1,7 @@
 # Changelog

+- extensions: add setParsed to ensure we get an operation signature and capture errors in extensions when they occur in unexpected locations [PR#2659](https://github.com/apollographql/apollo-server/pull/2659)


This CHANGELOG.md entry doesn't match up with the method that was implemented (setParsed vs. setParsedDocumentAndOperationName).

😬 that's an oversight. Good catch!

abernix · 2019-05-13T12:16:19Z

packages/apollo-server-core/src/requestPipeline.ts

+    if (requestContext.document) {
+      extensionStack.setParsedDocumentAndOperationName(
+        requestContext.document,
+        request.operationName,


The request.operationName is already available to implementors of graphql-extensions's requestDidStart method via the operationName parameter. The apollo-engine-reporting extension just didn't take advantage of that, I think we can avoid implementing this new setParsedDocumentAndOperationName method and instead allow the requestDidStart within apollo-engine-reporting to take care of that.

Ah yeah! It does. Love this

abernix · 2019-05-13T12:57:30Z

packages/apollo-server-core/src/requestPipeline.ts

@@ -229,6 +234,10 @@ export async function processGraphQLRequest<TContext>(

      try {
        requestContext.document = parse(query, config.parseOptions);
+        extensionStack.setParsedDocumentAndOperationName(


Rather than introducing a new public method to the extensions API, could we just access requestContext.document within the requestDidEnd callback of apollo-engine-reporting if its own documentAST isn't already set?

Yeah we can do that. I had originally thought that it would be a better solution! However, I wasn't willing to modify the type of the EndHandler returned from the requestDidStart function, since it breaks the types. With typescript inheritance/overrides, the functions need to be typed in the subclass as well, so I'd worry about breaking current implementations.

apollo-server/packages/graphql-extensions/src/index.ts

Line 26 in 4a2459e

export type EndHandler = (...errors: Array<Error>) => void;

I don't think we need to necessarily change that type. How do you feel about just using working within the scope of where we declare the requestDidStart 's EndHandler in apollo-engine-reporting/../extension.ts?

i.e. somewhere here:

apollo-server/packages/apollo-engine-reporting/src/extension.ts

Lines 232 to 237 in 7fb5a29

this.trace.fullQueryCacheHit = !!o.requestContext.metrics

.responseCacheHit;

const operationName = this.operationName || '';

let signature;

if (this.documentAST) {

We could just add:

const documentAST = this.documentAST || o.requestContext.document;

And then use documentAST rather than this.documentAST below it.

abernix · 2019-05-13T12:58:01Z

packages/apollo-server-core/src/requestPipeline.ts

@@ -372,6 +381,11 @@ export async function processGraphQLRequest<TContext>(
    }

    return sendResponse(response);
+  } catch (error) {
+    // an error was thrown during the parse, validate, execute phases, so we


This comment is not really correct since the parse and execute phases are entirely wrapped in try and catch calls and would never be reported to didEncounterErrors through this hook. Do we want them to?

That's right! I think the didEncounterErrors was really meant as a didEncounterExecutionErrors originally. The only usage is:

apollo-server/packages/apollo-server-core/src/requestPipeline.ts

Lines 336 to 338 in 4a2459e

if (result.errors) {

extensionStack.didEncounterErrors(result.errors);

}

The implementation in this PR is certainly a hammer, so I'd be happy to scope down didEncounterErrors to surrounding the calls out to non-extension user land, ie plugins, execution, and possibly context creation(would be nice to know about db connection failures)

I can certainly see how all errors might be useful. However, if we keep this in place, we just need to also change some other things to account for it (i.e. the new rewriteError #2618, which currently expects GraphQLErrors).

abernix · 2019-05-13T13:17:59Z

packages/graphql-extensions/src/index.ts

@@ -99,6 +103,17 @@ export class GraphQLExtensionStack<TContext = any> {
      ext => ext.parsingDidStart && ext.parsingDidStart(o),
    );
  }
+  public setParsedDocumentAndOperationName(


I don't think the name of this method name really needs to have parsed in it.

Sure! setOperationDocument?

Maybe just setDocumentAndOperationName if we need to keep it?

abernix

Thanks for the follow-up. I can tweak tomorrow, barring any final discussions.

abernix · 2019-05-13T20:12:23Z

packages/apollo-server-core/src/requestPipeline.ts

@@ -229,6 +234,10 @@ export async function processGraphQLRequest<TContext>(

      try {
        requestContext.document = parse(query, config.parseOptions);
+        extensionStack.setParsedDocumentAndOperationName(


I don't think we need to necessarily change that type. How do you feel about just using working within the scope of where we declare the requestDidStart 's EndHandler in apollo-engine-reporting/../extension.ts?

i.e. somewhere here:

apollo-server/packages/apollo-engine-reporting/src/extension.ts

Lines 232 to 237 in 7fb5a29

this.trace.fullQueryCacheHit = !!o.requestContext.metrics

.responseCacheHit;

const operationName = this.operationName || '';

let signature;

if (this.documentAST) {

We could just add:

const documentAST = this.documentAST || o.requestContext.document;

And then use documentAST rather than this.documentAST below it.

abernix · 2019-05-13T20:13:31Z

packages/apollo-server-core/src/requestPipeline.ts

@@ -372,6 +381,11 @@ export async function processGraphQLRequest<TContext>(
    }

    return sendResponse(response);
+  } catch (error) {
+    // an error was thrown during the parse, validate, execute phases, so we


I can certainly see how all errors might be useful. However, if we keep this in place, we just need to also change some other things to account for it (i.e. the new rewriteError #2618, which currently expects GraphQLErrors).

abernix · 2019-05-13T20:14:26Z

packages/graphql-extensions/src/index.ts

@@ -99,6 +103,17 @@ export class GraphQLExtensionStack<TContext = any> {
      ext => ext.parsingDidStart && ext.parsingDidStart(o),
    );
  }
+  public setParsedDocumentAndOperationName(


Maybe just setDocumentAndOperationName if we need to keep it?

This aims to accomplish one of the bits which was included within #2659 via a technique that scopes the errors more precisely. While I do think we should be passing all errors, the fix in #2659 was specifically aimed at a particular error-case which surfaced and we should consider a more refined approach to capturing those errors in the right places in future API iterations.

The work in #2659 was meant to ensure that the `document` and `operationName` were set at the appropriate locations within the lifecycle events for the `graphql-extensions` module. The change in #2659 introduced a new life-cycle method, but we could also calculate this information without adding to the API surface area using data which was already exposed to the extensions API. Even though `graphql-extensions` is no longer really being supported, avoiding growth on the API surface area of `graphql-extensions` is still important for any API. Ref: #2659

This aims to accomplish one of the bits which was included within #2659 via a technique that scopes the errors more precisely. While I do think we should be passing all errors, the fix in #2659 was specifically aimed at a particular error-case which surfaced and we should consider a more refined approach to capturing those errors in the right places in future API iterations.

The work in #2659 was meant to ensure that the `document` and `operationName` were set at the appropriate locations within the lifecycle events for the `graphql-extensions` module. The change in #2659 introduced a new life-cycle method, but we could also calculate this information without adding to the API surface area using data which was already exposed to the extensions API. Even though `graphql-extensions` is no longer really being supported, avoiding growth on the API surface area of `graphql-extensions` is still important for any API. Ref: #2659

This is another take on #2659 which has been otherwise excluded from the 2.6.0 release. The work in #2659 was meant to ensure that the `document` and `operationName` were set at the appropriate locations within the lifecycle events for the `graphql-extensions` module, specifically when an error had been thrown within `didResolveOperation` of the new request pipeline. While the solution in #2659 certainly works, it introduced a new life-cycle method for details which we could also calculate without adding to the API surface area by using information which was already exposed. Even though `graphql-extensions` is no longer really being supported, avoiding growth on the API surface area of `graphql-extensions` is still important for any API! Ref: #2659

… errors (#2659)" This reverts commit 2a6af9c. Superseded by: #2711 and Superseded by: #2710

evans requested review from abernix and martijnwalraven as code owners May 10, 2019 01:45

evans requested review from pcarrier and jbaxleyiii May 10, 2019 01:48

evans force-pushed the evans/set-parsed-document branch 2 times, most recently from 591e0d6 to bd51b22 Compare May 10, 2019 01:53

jbaxleyiii approved these changes May 10, 2019

View reviewed changes

evans force-pushed the evans/set-parsed-document branch from bd51b22 to d937166 Compare May 10, 2019 02:24

evans added 3 commits May 10, 2019 10:46

a-e-r: add setParsedDocument

8624ba6

extensions: include operation name as well as parsed document

4a2459e

evans force-pushed the evans/set-parsed-document branch from 0fab957 to 4a2459e Compare May 10, 2019 17:46

evans merged commit 2a6af9c into master May 10, 2019

evans deleted the evans/set-parsed-document branch May 10, 2019 17:52

abernix reviewed May 13, 2019

View reviewed changes

abernix mentioned this pull request May 22, 2019

engine-reporting: Handle errors within didResolveOperation life-cycle hook. #2710

Merged

abernix mentioned this pull request May 22, 2019

engine-reporting: Capture operationName and document within errors. #2711

Merged

abernix added a commit that referenced this pull request May 23, 2019

Revert "extensions: add setParsedDocumentAndOperationName and capture…

7db109e

… errors (#2659)" This reverts commit 2a6af9c. Superseded by: #2711 and Superseded by: #2710

github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

extensions: add setParsed and capture error in extensions #2659

extensions: add setParsed and capture error in extensions #2659

evans commented May 10, 2019

jbaxleyiii left a comment

evans commented May 10, 2019

evans commented May 10, 2019

abernix left a comment

abernix May 13, 2019

evans May 13, 2019

abernix May 13, 2019

evans May 13, 2019

abernix May 13, 2019

evans May 13, 2019

abernix May 13, 2019

abernix May 13, 2019

evans May 13, 2019 •

edited

Loading

abernix May 13, 2019

abernix May 13, 2019

evans May 13, 2019

abernix May 13, 2019

abernix left a comment

abernix May 13, 2019

abernix May 13, 2019

abernix May 13, 2019

		@@ -1,5 +1,7 @@
		# Changelog

		- extensions: add setParsed to ensure we get an operation signature and capture errors in extensions when they occur in unexpected locations [PR#2659](https://github.com/apollographql/apollo-server/pull/2659)

	this.trace.fullQueryCacheHit = !!o.requestContext.metrics
	.responseCacheHit;

	const operationName = this.operationName \|\| '';
	let signature;
	if (this.documentAST) {

	if (result.errors) {
	extensionStack.didEncounterErrors(result.errors);
	}

extensions: add setParsed and capture error in extensions #2659

extensions: add setParsed and capture error in extensions #2659

Conversation

evans commented May 10, 2019

jbaxleyiii left a comment

Choose a reason for hiding this comment

evans commented May 10, 2019

evans commented May 10, 2019

abernix left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evans May 13, 2019 • edited Loading

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

abernix left a 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

evans May 13, 2019 •

edited

Loading