Fix/nested fragments for require-id-when-available (#962)

* 🚸 NEW: tests for nested fragments * 🐛 FIX: nested fragment for rule: require-id-when-available * improve rule improve report message add failing test fix test add test fragment spread inside inline fragments n level fix test add another tests simplify code fix `Intl.ListFormat` typing * add changeset * update snapshots Co-authored-by: Dimitri POSTOLOV <dmytropostolov@gmail.com>
dimaMachina · Feb 18, 2022 · 58b5bfd · 58b5bfd
1 parent b86425f
commit 58b5bfd
Show file tree

Hide file tree

Showing 5 changed files with 269 additions and 47 deletions.
diff --git a/.changeset/neat-eagles-tap.md b/.changeset/neat-eagles-tap.md
@@ -0,0 +1,5 @@
+---
+'@graphql-eslint/eslint-plugin': minor
+---
+
+fix: false negative case when nested fragments used in `require-id-when-available` rule
diff --git a/packages/plugin/src/rules/require-id-when-available.ts b/packages/plugin/src/rules/require-id-when-available.ts
@@ -1,15 +1,24 @@
-import { getLocation, requireGraphQLSchemaFromContext, requireSiblingsOperations } from '../utils';
+import { requireGraphQLSchemaFromContext, requireSiblingsOperations } from '../utils';
 import { GraphQLESLintRule } from '../types';
-import { GraphQLInterfaceType, GraphQLObjectType, Kind, SelectionNode, SelectionSetNode } from 'graphql';
+import { GraphQLInterfaceType, GraphQLObjectType, Kind, SelectionSetNode } from 'graphql';
 import { asArray } from '@graphql-tools/utils';
 import { getBaseType, GraphQLESTreeNode } from '../estree-parser';
 
 export type RequireIdWhenAvailableRuleConfig = { fieldName: string | string[] };
 
 const RULE_ID = 'require-id-when-available';
-const MESSAGE_ID = 'REQUIRE_ID_WHEN_AVAILABLE';
 const DEFAULT_ID_FIELD_NAME = 'id';
 
+declare namespace Intl {
+  class ListFormat {
+    constructor(locales: string, options: any);
+
+    public format: (items: [string]) => string;
+  }
+}
+
+const englishJoinWords = words => new Intl.ListFormat('en-US', { type: 'disjunction' }).format(words);
+
 const rule: GraphQLESLintRule<[RequireIdWhenAvailableRuleConfig], true> = {
   meta: {
     type: 'suggestion',
@@ -59,10 +68,7 @@ const rule: GraphQLESLintRule<[RequireIdWhenAvailableRuleConfig], true> = {
       recommended: true,
     },
     messages: {
-      [MESSAGE_ID]: [
-        `Field {{ fieldName }} must be selected when it's available on a type. Please make sure to include it in your selection set!`,
-        `If you are using fragments, make sure that all used fragments {{ checkedFragments }}specifies the field {{ fieldName }}.`,
-      ].join('\n'),
+      [RULE_ID]: `Field{{ pluralSuffix }} {{ fieldName }} must be selected when it's available on a type.\nInclude it in your selection set{{ addition }}.`,
     },
     schema: {
       definitions: {
@@ -95,11 +101,9 @@ const rule: GraphQLESLintRule<[RequireIdWhenAvailableRuleConfig], true> = {
     const { fieldName = DEFAULT_ID_FIELD_NAME } = context.options[0] || {};
     const idNames = asArray(fieldName);
 
-    const isFound = (s: GraphQLESTreeNode<SelectionNode> | SelectionNode) =>
-      s.kind === Kind.FIELD && idNames.includes(s.name.value);
-
-    // Skip check selections in FragmentDefinition
-    const selector = 'OperationDefinition SelectionSet[parent.kind!=OperationDefinition]';
+    // Check selections only in OperationDefinition,
+    // skip selections of OperationDefinition and InlineFragment
+    const selector = 'OperationDefinition SelectionSet[parent.kind!=/(^OperationDefinition|InlineFragment)$/]';
 
     return {
       [selector](node: GraphQLESTreeNode<SelectionSetNode, true>) {
@@ -121,39 +125,56 @@ const rule: GraphQLESLintRule<[RequireIdWhenAvailableRuleConfig], true> = {
         }
         const checkedFragmentSpreads = new Set<string>();
 
-        for (const selection of node.selections) {
-          if (isFound(selection)) {
-            return;
-          }
-          if (selection.kind === Kind.INLINE_FRAGMENT && selection.selectionSet.selections.some(isFound)) {
-            return;
-          }
-          if (selection.kind === Kind.FRAGMENT_SPREAD) {
-            const [foundSpread] = siblings.getFragment(selection.name.value);
-            if (foundSpread) {
-              checkedFragmentSpreads.add(foundSpread.document.name.value);
-              if (foundSpread.document.selectionSet.selections.some(isFound)) {
-                return;
+        function checkSelections(selections): boolean {
+          let hasIdField = false;
+          for (const selection of selections) {
+            if (hasIdField) {
+              return true;
+            }
+
+            if (selection.kind === Kind.FIELD) {
+              hasIdField = idNames.includes(selection.name.value);
+              continue;
+            }
+
+            if (selection.kind === Kind.FRAGMENT_SPREAD) {
+              const [foundSpread] = siblings.getFragment(selection.name.value);
+              if (foundSpread) {
+                const fragmentSpread = foundSpread.document;
+                checkedFragmentSpreads.add(fragmentSpread.name.value);
+                hasIdField = checkSelections(fragmentSpread.selectionSet.selections);
               }
+              continue;
+            }
+
+            if (selection.kind === Kind.INLINE_FRAGMENT) {
+              hasIdField = checkSelections(selection.selectionSet.selections);
             }
           }
+          return hasIdField;
         }
 
-        const { parent } = node as any;
-        const hasIdFieldInInterfaceSelectionSet =
-          parent?.kind === Kind.INLINE_FRAGMENT &&
-          parent.parent?.kind === Kind.SELECTION_SET &&
-          parent.parent.selections.some(isFound);
-        if (hasIdFieldInInterfaceSelectionSet) {
+        const idFound = checkSelections(node.selections);
+        if (idFound) {
           return;
         }
 
+        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: getLocation(node.loc),
-          messageId: MESSAGE_ID,
+          loc: node.loc.start,
+          messageId: RULE_ID,
           data: {
-            checkedFragments: checkedFragmentSpreads.size === 0 ? '' : `(${[...checkedFragmentSpreads].join(', ')}) `,
-            fieldName: idNames.map(name => `"${name}"`).join(' or '),
+            pluralSuffix,
+            fieldName,
+            addition,
           },
         });
       },

diff --git a/packages/plugin/tests/__snapshots__/examples.spec.ts.snap b/packages/plugin/tests/__snapshots__/examples.spec.ts.snap
@@ -209,8 +209,8 @@ Array [
         ruleId: @graphql-eslint/no-anonymous-operations,
       },
       Object {
-        message: Field "id" must be selected when it's available on a type. Please make sure to include it in your selection set!
-If you are using fragments, make sure that all used fragments specifies the field "id".,
+        message: Field \`id\` must be selected when it's available on a type.
+Include it in your selection set.,
         ruleId: @graphql-eslint/require-id-when-available,
       },
     ],

diff --git a/packages/plugin/tests/__snapshots__/require-id-when-available.spec.ts.snap b/packages/plugin/tests/__snapshots__/require-id-when-available.spec.ts.snap
@@ -4,22 +4,36 @@ exports[` 1`] = `
 ❌ Error 1/1
 
 > 1 | { hasId { name } }
-    |         ^ Field "id" must be selected when it's available on a type. Please make sure to include it in your selection set!
-If you are using fragments, make sure that all used fragments specifies the field "id".
+    |         ^ Field \`id\` must be selected when it's available on a type.
+Include it in your selection set.
 `;
 
 exports[` 2`] = `
 ❌ Error 1/1
 
 > 1 | { hasId { id } }
-    |         ^ Field "name" must be selected when it's available on a type. Please make sure to include it in your selection set!
-If you are using fragments, make sure that all used fragments specifies the field "name".
+    |         ^ Field \`name\` must be selected when it's available on a type.
+Include it in your selection set.
 `;
 
 exports[` 3`] = `
 ❌ Error 1/1
 
 > 1 | { hasId { name } }
-    |         ^ Field "id" or "_id" must be selected when it's available on a type. Please make sure to include it in your selection set!
-If you are using fragments, make sure that all used fragments specifies the field "id" or "_id".
+    |         ^ Fields \`id\` or \`_id\` must be selected when it's available on a type.
+Include it in your selection set.
+`;
+
+exports[` 4`] = `
+❌ Error 1/1
+
+  1 |
+  2 |         query User {
+> 3 |           user {
+    |                ^ Field \`id\` must be selected when it's available on a type.
+Include it in your selection set or add to used fragments \`UserFullFields\`, \`UserMediumFields\`, or \`UserLightFields\`.
+  4 |             ...UserFullFields
+  5 |           }
+  6 |         }
+  7 |       
 `;