Repository: graphql/graphql-js
Pull request #3722 (2022-09-02 13:50:38)

original `execute` should throw if defer/stream directives are present
Currently, when the defer/stream directives are not present in the schema, defer/stream will not be triggered, and so the directives will be ignored even when used with the original function.

But -- if the directives are in the schema, the original functions will sometimes throw (if the first result from `execute` is synchronous), but sometimes return maps with errors (when the first result from `execute` is async or when any (sync or async) `subscribe` payload has incremental results.

This PR just aligns the behavior in terms of throwing or returning a map with errors. We really shouldn't return a map with errors after execution has begun.

I think another viable alternative that potentially makes more sense is to have the original `execute` and `subscribe` functions ignore the directives. To enable the experimental behavior, you would need to have the directives in the schema and use the experimental functions.

@glasser @robrichard @IvanGoncharov @n1ru4l

netlify[bot]: ### <span aria-hidden="true">👷</span> Deploy Preview for *compassionate-pike-271cb3* processing.


|  Name | Link |
|---------------------------------|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 4f8f807a40186088606d6005d1aaed04d4e28682 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/63120a2f4aeb000008264067 |
github-actions[bot]: Hi @yaacovCR, I'm @github-actions bot happy to help you with this PR 👋

<details>
<summary> Supported commands </summary>

Please post this commands in separate comments and only one per comment:
* `@github-actions run-benchmark` - Run benchmark comparing base and merge commits for this PR
* `@github-actions publish-pr-on-npm` - Build package from this PR and publish it on NPM
</details>

yaacovCR: @glasser @robrichard @IvanGoncharov @n1ru4l
glasser: I like the idea of being more consistent, but maybe this is consistent in the wrong direction and it would be more informative to take the choice that is more likely to push the error to the client? Not sure.

> Currently, when the defer/stream directives are not present in the schema, defer/stream will not be triggered, and so the directives will be ignored even when used with the original function.

Just to be clear: this is a statement about execute specifically: such operations should still fail validation, no?


yaacovCR: @glasser correct, they should fail validation if not in schema.

In terms of this behavior being more consistent, but not ideal, maybe it would make sense in v16 to try something along that line, like providing a temporary validation rule that outlaws the use of these directives that could be used if you're not using the experimental version. Maybe providing an experimental validate We're only the original validate includes this rule?

Otherwise, the original execute could just ignore the directives. 
I think that both of those options would be better than what we have now, but at least with this PR, we are consistently growing. I don't think execute should return a map with errors once execution has begun. It might sound a bit dogmatic, but I think that way of thinking also exposes that the "error" that is occurring is not actually a field error and should be handled earlier or differently.
yaacovCR: @glasser et al My preferred alternative to this is now #3727

I am going to keep this open, however, until it is more officially rejected by the group.
yaacovCR: @robrichard @IvanGoncharov

It looks to me like this PR is essentially the behavior we were shooting for.

Rebased and ready to review.
yaacovCR: > This seems like it would cause confusion for any users who are using GraphQL execution indirectly via software like Apollo Server/GraphQL Yoga/graphql-helix/etc rather than calling `execute` directly. A user-level change (adding a line to their schema) tells them to change a function that they aren't actually calling directly.
> 
> I'm not saying I object to the basic concept of throwing an error in this case (though I'm a bit skeptical) but I do wonder if the error message could be more actionable in the common case where the person putting together the data (schema) is not the person calling the named function.

@glasser -- I made the error message a bit more generic, without reference to the exact method by which to enable experimental execution features. I am not sure if this is better, definitely open to suggestions for the exact language. 
IvanGoncharov: @yaacovCR Thanks for iterating on this.
If we will have issues with an error message we can definitely improve it in subsequent PRs.

Patch:
diff --git a/src/execution/execute.ts b/src/execution/execute.ts
index 052cb8da..ffdb3f53 100644
--- a/src/execution/execute.ts
+++ b/src/execution/execute.ts
@@ -263,6 +263,9 @@ export interface ExecutionArgs {
   subscribeFieldResolver?: Maybe<GraphQLFieldResolver<any, any>>;
 }
 
+const UNEXPECTED_EXPERIMENTAL_DIRECTIVES =
+  'The provided schema unexpectedly contains experimental directives (@defer or @stream). These directives may only be utilized if experimental execution features are explicitly enabled.';
+
 const UNEXPECTED_MULTIPLE_PAYLOADS =
   'Executing this GraphQL operation would unexpectedly produce multiple payloads (due to @defer or @stream directive)';
 
@@ -278,14 +281,20 @@ const UNEXPECTED_MULTIPLE_PAYLOADS =
  *
  * This function does not support incremental delivery (`@defer` and `@stream`).
  * If an operation which would defer or stream data is executed with this
- * function, it will throw or resolve to an object containing an error instead.
+ * function, it will throw or return a rejected promise.
  * Use `experimentalExecuteIncrementally` if you want to support incremental
  * delivery.
  */
 export function execute(args: ExecutionArgs): PromiseOrValue<ExecutionResult> {
+  if (args.schema.getDirective('defer') || args.schema.getDirective('stream')) {
+    throw new Error(UNEXPECTED_EXPERIMENTAL_DIRECTIVES);
+  }
+
   const result = experimentalExecuteIncrementally(args);
   if (!isPromise(result)) {
     if ('initialResult' in result) {
+      // This can happen if the operation contains @defer or @stream directives
+      // and is not validated prior to execution
       throw new Error(UNEXPECTED_MULTIPLE_PAYLOADS);
     }
     return result;
@@ -293,9 +302,9 @@ export function execute(args: ExecutionArgs): PromiseOrValue<ExecutionResult> {
 
   return result.then((incrementalResult) => {
     if ('initialResult' in incrementalResult) {
-      return {
-        errors: [new GraphQLError(UNEXPECTED_MULTIPLE_PAYLOADS)],
-      };
+      // This can happen if the operation contains @defer or @stream directives
+      // and is not validated prior to execution
+      throw new Error(UNEXPECTED_MULTIPLE_PAYLOADS);
     }
     return incrementalResult;
   });


Test patch:
diff --git a/src/execution/__tests__/defer-test.ts b/src/execution/__tests__/defer-test.ts
index 417be985..5cad95bb 100644
--- a/src/execution/__tests__/defer-test.ts
+++ b/src/execution/__tests__/defer-test.ts
@@ -723,21 +723,14 @@ describe('Execute: defer directive', () => {
       ... @defer { hero { id } }
     }
   `;
-    expectJSON(
-      await expectPromise(
-        execute({
-          schema,
-          document: parse(doc),
-          rootValue: {},
-        }),
-      ).toResolve(),
-    ).toDeepEqual({
-      errors: [
-        {
-          message:
-            'Executing this GraphQL operation would unexpectedly produce multiple payloads (due to @defer or @stream directive)',
-        },
-      ],
-    });
+    await expectPromise(
+      execute({
+        schema,
+        document: parse(doc),
+        rootValue: {},
+      }),
+    ).toRejectWith(
+      'Executing this GraphQL operation would unexpectedly produce multiple payloads (due to @defer or @stream directive)',
+    );
   });
 });
diff --git a/src/execution/__tests__/executor-test.ts b/src/execution/__tests__/executor-test.ts
index a945b4d0..5e25dddb 100644
--- a/src/execution/__tests__/executor-test.ts
+++ b/src/execution/__tests__/executor-test.ts
@@ -17,6 +17,10 @@ import {
   GraphQLScalarType,
   GraphQLUnionType,
 } from '../../type/definition.js';
+import {
+  GraphQLDeferDirective,
+  GraphQLStreamDirective,
+} from '../../type/directives.js';
 import {
   GraphQLBoolean,
   GraphQLInt,
@@ -914,6 +918,40 @@ describe('Execute: Handles basic execution tasks', () => {
     expect(result).to.deep.equal({ data: { a: 'b' } });
   });
 
+  it('errors when using original execute with schemas including experimental @defer directive', () => {
+    const schema = new GraphQLSchema({
+      query: new GraphQLObjectType({
+        name: 'Q',
+        fields: {
+          a: { type: GraphQLString },
+        },
+      }),
+      directives: [GraphQLDeferDirective],
+    });
+    const document = parse('query Q { a }');
+
+    expect(() => execute({ schema, document })).to.throw(
+      'The provided schema unexpectedly contains experimental directives (@defer or @stream). These directives may only be utilized if experimental execution features are explicitly enabled.',
+    );
+  });
+
+  it('errors when using original execute with schemas including experimental @stream directive', () => {
+    const schema = new GraphQLSchema({
+      query: new GraphQLObjectType({
+        name: 'Q',
+        fields: {
+          a: { type: GraphQLString },
+        },
+      }),
+      directives: [GraphQLStreamDirective],
+    });
+    const document = parse('query Q { a }');
+
+    expect(() => execute({ schema, document })).to.throw(
+      'The provided schema unexpectedly contains experimental directives (@defer or @stream). These directives may only be utilized if experimental execution features are explicitly enabled.',
+    );
+  });
+
   it('resolves to an error if schema does not support operation', () => {
     const schema = new GraphQLSchema({ assumeValid: true });