Forbid @Skip and @include directives in subscription root selection (graphql#3974)

This is an implementation of
graphql/graphql-spec#860

The spec calls for a separate `CollectSubscriptionFields` algorithm due
to executing without `variableValues`; but I figured that maintenance
would be easier with the algorithms synchronized.

Please feel free to make any changes you need to this PR; it's just to
get the ball rolling.

Related GraphQL WG action item (from 2021!):
graphql/graphql-wg#695

Author: benjie

src/execution/collectFields.ts

@@ -2,6 +2,7 @@ import { AccumulatorMap } from '../jsutils/AccumulatorMap.js';
 import type { ObjMap } from '../jsutils/ObjMap.js';
 
 import type {
+  DirectiveNode,
   FieldNode,
   FragmentDefinitionNode,
   FragmentSpreadNode,
@@ -22,7 +23,10 @@ import { typeFromAST } from '../utilities/typeFromAST.js';
 
 import type { GraphQLVariableSignature } from './getVariableSignature.js';
 import type { VariableValues } from './values.js';
-import { getDirectiveValues, getFragmentVariableValues } from './values.js';
+import {
+  experimentalGetArgumentValues,
+  getFragmentVariableValues,
+} from './values.js';
 
 export interface FieldDetails {
   node: FieldNode;
@@ -45,6 +49,8 @@ interface CollectFieldsContext {
   runtimeType: GraphQLObjectType;
   visitedFragmentNames: Set<string>;
   hideSuggestions: boolean;
+  forbiddenDirectiveInstances: Array<DirectiveNode>;
+  forbidSkipAndInclude: boolean;
 }
 
 /**
@@ -64,7 +70,11 @@ export function collectFields(
   runtimeType: GraphQLObjectType,
   selectionSet: SelectionSetNode,
   hideSuggestions: boolean,
-): GroupedFieldSet {
+  forbidSkipAndInclude = false,
+): {
+  groupedFieldSet: GroupedFieldSet;
+  forbiddenDirectiveInstances: ReadonlyArray<DirectiveNode>;
+} {
   const groupedFieldSet = new AccumulatorMap<string, FieldDetails>();
   const context: CollectFieldsContext = {
     schema,
@@ -73,10 +83,15 @@ export function collectFields(
     runtimeType,
     visitedFragmentNames: new Set(),
     hideSuggestions,
+    forbiddenDirectiveInstances: [],
+    forbidSkipAndInclude,
   };
 
   collectFieldsImpl(context, selectionSet, groupedFieldSet);
-  return groupedFieldSet;
+  return {
+    groupedFieldSet,
+    forbiddenDirectiveInstances: context.forbiddenDirectiveInstances,
+  };
 }
 
 /**
@@ -105,6 +120,8 @@ export function collectSubfields(
     runtimeType: returnType,
     visitedFragmentNames: new Set(),
     hideSuggestions,
+    forbiddenDirectiveInstances: [],
+    forbidSkipAndInclude: false,
   };
   const subGroupedFieldSet = new AccumulatorMap<string, FieldDetails>();
 
@@ -143,7 +160,12 @@ function collectFieldsImpl(
     switch (selection.kind) {
       case Kind.FIELD: {
         if (
-          !shouldIncludeNode(selection, variableValues, fragmentVariableValues)
+          !shouldIncludeNode(
+            context,
+            selection,
+            variableValues,
+            fragmentVariableValues,
+          )
         ) {
           continue;
         }
@@ -156,6 +178,7 @@ function collectFieldsImpl(
       case Kind.INLINE_FRAGMENT: {
         if (
           !shouldIncludeNode(
+            context,
             selection,
             variableValues,
             fragmentVariableValues,
@@ -179,7 +202,12 @@ function collectFieldsImpl(
 
         if (
           visitedFragmentNames.has(fragName) ||
-          !shouldIncludeNode(selection, variableValues, fragmentVariableValues)
+          !shouldIncludeNode(
+            context,
+            selection,
+            variableValues,
+            fragmentVariableValues,
+          )
         ) {
           continue;
         }
@@ -222,26 +250,47 @@ function collectFieldsImpl(
  * directives, where `@skip` has higher precedence than `@include`.
  */
 function shouldIncludeNode(
+  context: CollectFieldsContext,
   node: FragmentSpreadNode | FieldNode | InlineFragmentNode,
   variableValues: VariableValues,
   fragmentVariableValues: VariableValues | undefined,
 ): boolean {
-  const skip = getDirectiveValues(
-    GraphQLSkipDirective,
-    node,
-    variableValues,
-    fragmentVariableValues,
+  const skipDirectiveNode = node.directives?.find(
+    (directive) => directive.name.value === GraphQLSkipDirective.name,
   );
+  if (skipDirectiveNode && context.forbidSkipAndInclude) {
+    context.forbiddenDirectiveInstances.push(skipDirectiveNode);
+    return false;
+  }
+  const skip = skipDirectiveNode
+    ? experimentalGetArgumentValues(
+        skipDirectiveNode,
+        GraphQLSkipDirective.args,
+        variableValues,
+        fragmentVariableValues,
+        context.hideSuggestions,
+      )
+    : undefined;
   if (skip?.if === true) {
     return false;
   }
 
-  const include = getDirectiveValues(
-    GraphQLIncludeDirective,
-    node,
-    variableValues,
-    fragmentVariableValues,
+  const includeDirectiveNode = node.directives?.find(
+    (directive) => directive.name.value === GraphQLIncludeDirective.name,
   );
+  if (includeDirectiveNode && context.forbidSkipAndInclude) {
+    context.forbiddenDirectiveInstances.push(includeDirectiveNode);
+    return false;
+  }
+  const include = includeDirectiveNode
+    ? experimentalGetArgumentValues(
+        includeDirectiveNode,
+        GraphQLIncludeDirective.args,
+        variableValues,
+        fragmentVariableValues,
+        context.hideSuggestions,
+      )
+    : undefined;
   if (include?.if === false) {
     return false;
   }

src/execution/execute.ts

@@ -283,7 +283,7 @@ export function executeQueryOrMutationOrSubscriptionEvent(
       );
     }
 
-    const groupedFieldSet = collectFields(
+    const { groupedFieldSet } = collectFields(
       schema,
       fragments,
       variableValues,
@@ -1622,7 +1622,7 @@ function executeSubscription(
     );
   }
 
-  const groupedFieldSet = collectFields(
+  const { groupedFieldSet } = collectFields(
     schema,
     fragments,
     variableValues,

src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts

@@ -286,6 +286,48 @@ describe('Validate: Subscriptions with single field', () => {
     ]);
   });
 
+  it('fails with @skip or @include directive', () => {
+    expectErrors(`
+      subscription RequiredRuntimeValidation($bool: Boolean!) {
+        newMessage @include(if: $bool) {
+          body
+          sender
+        }
+        disallowedSecondRootField @skip(if: $bool)
+      }
+    `).toDeepEqual([
+      {
+        message:
+          'Subscription "RequiredRuntimeValidation" must not use `@skip` or `@include` directives in the top level selection.',
+        locations: [
+          { line: 3, column: 20 },
+          { line: 7, column: 35 },
+        ],
+      },
+    ]);
+  });
+
+  it('fails with @skip or @include directive in anonymous subscription', () => {
+    expectErrors(`
+      subscription ($bool: Boolean!) {
+        newMessage @include(if: $bool) {
+          body
+          sender
+        }
+        disallowedSecondRootField @skip(if: $bool)
+      }
+    `).toDeepEqual([
+      {
+        message:
+          'Anonymous Subscription must not use `@skip` or `@include` directives in the top level selection.',
+        locations: [
+          { line: 3, column: 20 },
+          { line: 7, column: 35 },
+        ],
+      },
+    ]);
+  });
+
   it('skips if not subscription type', () => {
     const emptySchema = buildSchema(`
       type Query {

src/validation/rules/SingleFieldSubscriptionsRule.ts

@@ -23,7 +23,8 @@ function toNodes(fieldDetailsList: FieldDetailsList): ReadonlyArray<FieldNode> {
  * Subscriptions must only include a non-introspection field.
  *
  * A GraphQL subscription is valid only if it contains a single root field and
- * that root field is not an introspection field.
+ * that root field is not an introspection field. `@skip` and `@include`
+ * directives are forbidden.
  *
  * See https://spec.graphql.org/draft/#sec-Single-root-field
  */
@@ -45,14 +46,27 @@ export function SingleFieldSubscriptionsRule(
               fragments[definition.name.value] = { definition };
             }
           }
-          const groupedFieldSet = collectFields(
-            schema,
-            fragments,
-            variableValues,
-            subscriptionType,
-            node.selectionSet,
-            context.hideSuggestions,
-          );
+          const { groupedFieldSet, forbiddenDirectiveInstances } =
+            collectFields(
+              schema,
+              fragments,
+              variableValues,
+              subscriptionType,
+              node.selectionSet,
+              context.hideSuggestions,
+              true,
+            );
+          if (forbiddenDirectiveInstances.length > 0) {
+            context.reportError(
+              new GraphQLError(
+                operationName != null
+                  ? `Subscription "${operationName}" must not use \`@skip\` or \`@include\` directives in the top level selection.`
+                  : 'Anonymous Subscription must not use `@skip` or `@include` directives in the top level selection.',
+                { nodes: forbiddenDirectiveInstances },
+              ),
+            );
+            return;
+          }
           if (groupedFieldSet.size > 1) {
             const fieldDetailsLists = [...groupedFieldSet.values()];
             const extraFieldDetailsLists = fieldDetailsLists.slice(1);