require-id-when-available check nested selections in fragments (#974)

* fix `require-id-when-available`, check also selections of fragment spread

* include fragment name in report message

* should work as expected

* update snapshots

Author: dimaMachina

.changeset/neat-eagles-tap.md

@@ -1,5 +1,5 @@
 ---
-'@graphql-eslint/eslint-plugin': minor
+'@graphql-eslint/eslint-plugin': patch
 ---
 
 fix: false negative case when nested fragments used in `require-id-when-available` rule

.changeset/nice-pandas-confess.md

@@ -0,0 +1,5 @@
+---
+'@graphql-eslint/eslint-plugin': patch
+---
+
+fix: check nested selections in fragments in `require-id-when-available` rule

packages/plugin/src/rules/require-id-when-available.ts

@@ -1,7 +1,18 @@
-import { ARRAY_DEFAULT_OPTIONS, requireGraphQLSchemaFromContext, requireSiblingsOperations } from '../utils';
-import { GraphQLESLintRule, OmitRecursively } from '../types';
-import { GraphQLInterfaceType, GraphQLObjectType, Kind, SelectionSetNode } from 'graphql';
+import {
+  ASTNode,
+  GraphQLInterfaceType,
+  GraphQLObjectType,
+  GraphQLOutputType,
+  Kind,
+  SelectionSetNode,
+  TypeInfo,
+  visit,
+  visitWithTypeInfo,
+} from 'graphql';
+import type * as ESTree from 'estree';
 import { asArray } from '@graphql-tools/utils';
+import { ARRAY_DEFAULT_OPTIONS, requireGraphQLSchemaFromContext, requireSiblingsOperations } from '../utils';
+import { GraphQLESLintRule, OmitRecursively, ReportDescriptor } from '../types';
 import { getBaseType, GraphQLESTreeNode } from '../estree-parser';
 
 export type RequireIdWhenAvailableRuleConfig = { fieldName: string | string[] };
@@ -22,6 +33,7 @@ const englishJoinWords = words => new Intl.ListFormat('en-US', { type: 'disjunct
 const rule: GraphQLESLintRule<[RequireIdWhenAvailableRuleConfig], true> = {
   meta: {
     type: 'suggestion',
+    // eslint-disable-next-line eslint-plugin/require-meta-has-suggestions
     hasSuggestions: true,
     docs: {
       category: 'Operations',
@@ -93,82 +105,132 @@ const rule: GraphQLESLintRule<[RequireIdWhenAvailableRuleConfig], true> = {
     },
   },
   create(context) {
-    requireGraphQLSchemaFromContext(RULE_ID, context);
+    const schema = requireGraphQLSchemaFromContext(RULE_ID, context);
     const siblings = requireSiblingsOperations(RULE_ID, context);
     const { fieldName = DEFAULT_ID_FIELD_NAME } = context.options[0] || {};
     const idNames = asArray(fieldName);
 
     // Check selections only in OperationDefinition,
     // skip selections of OperationDefinition and InlineFragment
     const selector = 'OperationDefinition SelectionSet[parent.kind!=/(^OperationDefinition|InlineFragment)$/]';
+    const typeInfo = new TypeInfo(schema);
 
-    return {
-      [selector](node: GraphQLESTreeNode<SelectionSetNode, true>) {
-        const typeInfo = node.typeInfo();
-        if (!typeInfo.gqlType) {
-          return;
-        }
-        const rawType = getBaseType(typeInfo.gqlType);
-        const isObjectType = rawType instanceof GraphQLObjectType;
-        const isInterfaceType = rawType instanceof GraphQLInterfaceType;
-        if (!isObjectType && !isInterfaceType) {
-          return;
+    function checkFragments(node: GraphQLESTreeNode<SelectionSetNode>): void {
+      for (const selection of node.selections) {
+        if (selection.kind !== Kind.FRAGMENT_SPREAD) {
+          continue;
         }
 
-        const fields = rawType.getFields();
-        const hasIdFieldInType = idNames.some(name => fields[name]);
-        if (!hasIdFieldInType) {
-          return;
+        const [foundSpread] = siblings.getFragment(selection.name.value);
+        if (!foundSpread) {
+          continue;
         }
-        const checkedFragmentSpreads = new Set<string>();
 
-        const hasIdField = ({ selections }: OmitRecursively<SelectionSetNode, 'loc'>): boolean =>
-          selections.some(selection => {
-            if (selection.kind === Kind.FIELD) {
-              return idNames.includes(selection.name.value);
+        const checkedFragmentSpreads = new Set<string>();
+        const visitor = visitWithTypeInfo(typeInfo, {
+          SelectionSet(node, key, parent: ASTNode) {
+            if (parent.kind === Kind.FRAGMENT_DEFINITION) {
+              checkedFragmentSpreads.add(parent.name.value);
+            } else if (parent.kind !== Kind.INLINE_FRAGMENT) {
+              checkSelections(node, typeInfo.getType(), selection.loc.start, parent, checkedFragmentSpreads);
             }
+          },
+        });
 
-            if (selection.kind === Kind.INLINE_FRAGMENT) {
-              return hasIdField(selection.selectionSet);
+        visit(foundSpread.document, visitor);
+      }
+    }
+
+    function checkSelections(
+      node: OmitRecursively<SelectionSetNode, 'loc'>,
+      type: GraphQLOutputType,
+      // Fragment can be placed in separate file
+      // Provide actual fragment spread location instead of location in fragment
+      loc: ESTree.Position,
+      // Can't access to node.parent in GraphQL AST.Node, so pass as argument
+      parent: any,
+      checkedFragmentSpreads = new Set<string>()
+    ): void {
+      const rawType = getBaseType(type);
+      const isObjectType = rawType instanceof GraphQLObjectType;
+      const isInterfaceType = rawType instanceof GraphQLInterfaceType;
+
+      if (!isObjectType && !isInterfaceType) {
+        return;
+      }
+      const fields = rawType.getFields();
+      const hasIdFieldInType = idNames.some(name => fields[name]);
+
+      if (!hasIdFieldInType) {
+        return;
+      }
+
+      function hasIdField({ selections }: typeof node): boolean {
+        return selections.some(selection => {
+          if (selection.kind === Kind.FIELD) {
+            return idNames.includes(selection.name.value);
+          }
+
+          if (selection.kind === Kind.INLINE_FRAGMENT) {
+            return hasIdField(selection.selectionSet);
+          }
+
+          if (selection.kind === Kind.FRAGMENT_SPREAD) {
+            const [foundSpread] = siblings.getFragment(selection.name.value);
+            if (foundSpread) {
+              const fragmentSpread = foundSpread.document;
+              checkedFragmentSpreads.add(fragmentSpread.name.value);
+              return hasIdField(fragmentSpread.selectionSet);
             }
+          }
+          return false;
+        });
+      }
 
-            if (selection.kind === Kind.FRAGMENT_SPREAD) {
-              const [foundSpread] = siblings.getFragment(selection.name.value);
-              if (foundSpread) {
-                const fragmentSpread = foundSpread.document;
-                checkedFragmentSpreads.add(fragmentSpread.name.value);
-                return hasIdField(fragmentSpread.selectionSet);
-              }
-            }
-            return false;
-          });
+      const hasId = hasIdField(node);
 
-        if (hasIdField(node)) {
-          return;
-        }
+      checkFragments(node as GraphQLESTreeNode<SelectionSetNode>);
 
-        const pluralSuffix = idNames.length > 1 ? 's' : '';
-        const fieldName = englishJoinWords(idNames.map(name => `\`${name}\``));
-        const addition =
-          checkedFragmentSpreads.size === 0
-            ? ''
-            : ` or add to used fragment${checkedFragmentSpreads.size > 1 ? 's' : ''} ${englishJoinWords(
-                [...checkedFragmentSpreads].map(name => `\`${name}\``)
-              )}`;
-
-        context.report({
-          loc: node.loc.start,
-          messageId: RULE_ID,
-          data: {
-            pluralSuffix,
-            fieldName,
-            addition,
-          },
-          suggest: idNames.map(idName => ({
-            desc: `Add \`${idName}\` selection`,
-            fix: fixer => fixer.insertTextBefore((node as any).selections[0], `${idName} `),
-          })),
-        });
+      if (hasId) {
+        return;
+      }
+
+      const pluralSuffix = idNames.length > 1 ? 's' : '';
+      const fieldName = englishJoinWords(idNames.map(name => `\`${(parent.alias || parent.name).value}.${name}\``));
+
+      const addition =
+        checkedFragmentSpreads.size === 0
+          ? ''
+          : ` or add to used fragment${checkedFragmentSpreads.size > 1 ? 's' : ''} ${englishJoinWords(
+              [...checkedFragmentSpreads].map(name => `\`${name}\``)
+            )}`;
+
+      const problem: ReportDescriptor = {
+        loc,
+        messageId: RULE_ID,
+        data: {
+          pluralSuffix,
+          fieldName,
+          addition,
+        },
+      };
+
+      // Don't provide suggestions for selections in fragments as fragment can be in a separate file
+      if ('type' in node) {
+        problem.suggest = idNames.map(idName => ({
+          desc: `Add \`${idName}\` selection`,
+          fix: fixer => fixer.insertTextBefore((node as any).selections[0], `${idName} `),
+        }));
+      }
+      context.report(problem);
+    }
+
+    return {
+      [selector](node: GraphQLESTreeNode<SelectionSetNode, true>) {
+        const typeInfo = node.typeInfo();
+        if (typeInfo.gqlType) {
+          checkSelections(node, typeInfo.gqlType, node.loc.start, (node as any).parent);
+        }
       },
     };
   },

packages/plugin/src/rules/selection-set-depth.ts

@@ -101,7 +101,7 @@ const rule: GraphQLESLintRule<[SelectionSetDepthRuleConfig]> = {
       ) {
         try {
           const rawNode = node.rawNode();
-          const fragmentsInUse = siblings ? siblings.getFragmentsInUse(rawNode, true) : [];
+          const fragmentsInUse = siblings ? siblings.getFragmentsInUse(rawNode) : [];
           const document: DocumentNode = {
             kind: Kind.DOCUMENT,
             definitions: [rawNode, ...fragmentsInUse],

packages/plugin/src/sibling-operations.ts

@@ -1,7 +1,6 @@
 import { resolve } from 'path';
 import {
   FragmentDefinitionNode,
-  FragmentSpreadNode,
   Kind,
   OperationDefinitionNode,
   SelectionSetNode,
@@ -18,15 +17,15 @@ export type OperationSource = { filePath: string; document: OperationDefinitionN
 
 export type SiblingOperations = {
   available: boolean;
-  getOperations(): OperationSource[];
-  getFragments(): FragmentSource[];
   getFragment(fragmentName: string): FragmentSource[];
+  getFragments(): FragmentSource[];
   getFragmentByType(typeName: string): FragmentSource[];
   getFragmentsInUse(
     baseOperation: OperationDefinitionNode | FragmentDefinitionNode | SelectionSetNode,
-    recursive: boolean
+    recursive?: boolean
   ): FragmentDefinitionNode[];
   getOperation(operationName: string): OperationSource[];
+  getOperations(): OperationSource[];
   getOperationByType(operationType: OperationTypeNode): OperationSource[];
 };
 
@@ -91,13 +90,13 @@ export function getSiblingOperations(options: ParserOptions, gqlConfig: GraphQLC
 
     return {
       available: false,
-      getFragments: noopWarn,
-      getOperations: noopWarn,
       getFragment: noopWarn,
+      getFragments: noopWarn,
       getFragmentByType: noopWarn,
+      getFragmentsInUse: noopWarn,
       getOperation: noopWarn,
+      getOperations: noopWarn,
       getOperationByType: noopWarn,
-      getFragmentsInUse: noopWarn,
     };
   }
 
@@ -113,11 +112,11 @@ export function getSiblingOperations(options: ParserOptions, gqlConfig: GraphQLC
         const result: FragmentSource[] = [];
 
         for (const source of siblings) {
-          for (const definition of source.document.definitions || []) {
+          for (const definition of source.document.definitions) {
             if (definition.kind === Kind.FRAGMENT_DEFINITION) {
               result.push({
                 filePath: source.location,
-                document: definition as FragmentDefinitionNode,
+                document: definition,
               });
             }
           }
@@ -134,11 +133,11 @@ export function getSiblingOperations(options: ParserOptions, gqlConfig: GraphQLC
         const result: OperationSource[] = [];
 
         for (const source of siblings) {
-          for (const definition of source.document.definitions || []) {
+          for (const definition of source.document.definitions) {
             if (definition.kind === Kind.OPERATION_DEFINITION) {
               result.push({
                 filePath: source.location,
-                document: definition as OperationDefinitionNode,
+                document: definition,
               });
             }
           }
@@ -152,25 +151,22 @@ export function getSiblingOperations(options: ParserOptions, gqlConfig: GraphQLC
 
     const collectFragments = (
       selectable: SelectionSetNode | OperationDefinitionNode | FragmentDefinitionNode,
-      recursive = true,
-      collected: Map<string, FragmentDefinitionNode> = new Map()
+      recursive,
+      collected = new Map<string, FragmentDefinitionNode>()
     ) => {
       visit(selectable, {
-        FragmentSpread(spread: FragmentSpreadNode) {
-          const name = spread.name.value;
-          const fragmentInfo = getFragment(name);
+        FragmentSpread(spread) {
+          const fragmentName = spread.name.value;
+          const [fragment] = getFragment(fragmentName);
 
-          if (fragmentInfo.length === 0) {
+          if (!fragment) {
             logger.warn(
-              `Unable to locate fragment named "${name}", please make sure it's loaded using "parserOptions.operations"`
+              `Unable to locate fragment named "${fragmentName}", please make sure it's loaded using "parserOptions.operations"`
             );
             return;
           }
-          const fragment = fragmentInfo[0];
-          const alreadyVisited = collected.has(name);
-
-          if (!alreadyVisited) {
-            collected.set(name, fragment.document);
+          if (!collected.has(fragmentName)) {
+            collected.set(fragmentName, fragment.document);
             if (recursive) {
               collectFragments(fragment.document, recursive, collected);
             }
@@ -182,13 +178,13 @@ export function getSiblingOperations(options: ParserOptions, gqlConfig: GraphQLC
 
     siblingOperations = {
       available: true,
-      getFragments,
-      getOperations,
       getFragment,
+      getFragments,
       getFragmentByType: typeName => getFragments().filter(f => f.document.typeCondition?.name?.value === typeName),
+      getFragmentsInUse: (selectable, recursive = true) => Array.from(collectFragments(selectable, recursive).values()),
       getOperation: name => getOperations().filter(o => o.document.name?.value === name),
+      getOperations,
       getOperationByType: type => getOperations().filter(o => o.document.operation === type),
-      getFragmentsInUse: (selectable, recursive = true) => Array.from(collectFragments(selectable, recursive).values()),
     };
 
     siblingOperationsCache.set(siblings, siblingOperations);