Required fields and fragments

CHANGELOG.md

@@ -1,5 +1,13 @@
 # Change log
 ### vNEXT
+- BREAKING: The `required-fields` rule has been significantly changed to make it a completely reliable method of ensuring an `id` field (or any other field name) is always requested when available. [PR #199](https://github.com/apollographql/eslint-plugin-graphql/pull/199) Here is the behavior, let's say we are requiring field `id`:
+    - On any field whose return type defines a field called `id`, the selection set must directly contain `id`.
+    - In any named fragment declaration whose type defines a field called `id`, the selection set must directly contain `id`.
+    - An inline fragment whose type defines a field called `id` must contain `id` in its selection set unless its parent is also an inline fragment that contains the field `id`.
+    - Here's a specific case which is _no longer valid_:
+        - `query { greetings { hello ... on Greetings { id } } }`
+        - This must now be written as `query { greetings { id hello ... on Greetings { id } } }`
+    - This is a more conservative approach than before, driven by the fact that it's quite hard to ensure that a combination of inline fragments actually covers all of the possible types of a selection set.
 - Fix breaking change in `graphql@^14.0.0` that renamed `ProvidedNonNullArguments` to `ProvidedRequiredArguments` [#192](https://github.com/apollographql/eslint-plugin-graphql/pull/192)
 - Update dependencies to graphql-tools 4 and eslint 5.9 [#193](https://github.com/apollographql/eslint-plugin-graphql/pull/193)
 

src/customGraphQLValidationRules.js

@@ -1,60 +1,128 @@
-import { GraphQLError, getNamedType } from 'graphql';
+import { GraphQLError, getNamedType } from "graphql";
 
 export function OperationsMustHaveNames(context) {
   return {
     OperationDefinition(node) {
       if (!node.name) {
         context.reportError(
-          new GraphQLError("All operations must be named", [ node ])
+          new GraphQLError("All operations must be named", [node])
         );
       }
-    },
+    }
   };
 }
 
-function getFieldWasRequestedOnNode(node, field, recursing = false) {
+function getFieldWasRequestedOnNode(node, field) {
   return node.selectionSet.selections.some(n => {
-    // If it's an inline fragment, we need to look deeper
-    if (n.kind === 'InlineFragment' && !recursing) {
-      return getFieldWasRequestedOnNode(n, field, true);
-    }
-    if (n.kind === 'FragmentSpread') {
-      // We don't know if the field was requested in this case, so default to not erroring.
-      return true;
-    }
-    return n.name.value === field;
+    return n.kind === "Field" && n.name.value === field;
   });
 }
 
+function fieldAvailableOnType(type, field) {
+  return (
+    (type && type._fields && type._fields[field]) ||
+    (type.ofType && fieldAvailableOnType(type.ofType, field))
+  );
+}
+
 export function RequiredFields(context, options) {
+  const { requiredFields } = options;
+
   return {
-    Field(node) {
-      const def = context.getFieldDef();
-      if (!def) {
-        return;
-      }
-      const { requiredFields } = options;
+    FragmentDefinition(node) {
       requiredFields.forEach(field => {
-        const fieldAvaliableOnType = def.type && def.type._fields && def.type._fields[field];
+        const type = context.getType();
 
-        function recursivelyCheckOnType(ofType, field) {
-          return (ofType._fields && ofType._fields[field]) || (ofType.ofType && recursivelyCheckOnType(ofType.ofType, field));
+        if (fieldAvailableOnType(type, field)) {
+          const fieldWasRequested = getFieldWasRequestedOnNode(node, field);
+          if (!fieldWasRequested) {
+            context.reportError(
+              new GraphQLError(
+                `'${field}' field required on 'fragment ${node.name.value} on ${
+                  node.typeCondition.name.value
+                }'`,
+                [node]
+              )
+            );
+          }
         }
+      });
+    },
+
+    // Every inline fragment must have the required field specified inside
+    // itself or in some parent selection set.
+    InlineFragment(node, key, parent, path, ancestors) {
+      requiredFields.forEach(field => {
+        const type = context.getType();
+
+        if (fieldAvailableOnType(type, field)) {
+          // First, check the selection set on this inline fragment
+          if (node.selectionSet && getFieldWasRequestedOnNode(node, field)) {
+            return true;
+          }
 
-        let fieldAvaliableOnOfType = false;
-        if (def.type && def.type.ofType) {
-          fieldAvaliableOnOfType = recursivelyCheckOnType(def.type.ofType, field);
+          const ancestorClone = [...ancestors];
+
+          let nearestField;
+          let nextAncestor;
+
+          // Now, walk up the ancestors, until you see a field.
+          while (!nearestField) {
+            nextAncestor = ancestorClone.pop();
+
+            if (
+              nextAncestor.selectionSet &&
+              getFieldWasRequestedOnNode(nextAncestor, field)
+            ) {
+              return true;
+            }
+
+            if (nextAncestor.kind === "Field") {
+              nearestField = nextAncestor;
+            }
+          }
+
+          // If we never found a field, the query is malformed
+          if (!nearestField) {
+            throw new Error(
+              "Inline fragment found inside document without a parent field."
+            );
+          }
+
+          // We found a field, but we never saw the field we were looking for in
+          // the intermediate selection sets.
+          context.reportError(
+            new GraphQLError(
+              `'${field}' field required on '... on ${
+                node.typeCondition.name.value
+              }'`,
+              [node]
+            )
+          );
         }
-        if (fieldAvaliableOnType || fieldAvaliableOnOfType) {
+      });
+    },
+
+    // Every field that can have the field directly on it, should. It's not
+    // enough to have some child fragment to include the field, since we don't
+    // know if that fragment covers all of the possible type options.
+    Field(node) {
+      const def = context.getFieldDef();
+
+      requiredFields.forEach(field => {
+        if (fieldAvailableOnType(def.type, field)) {
           const fieldWasRequested = getFieldWasRequestedOnNode(node, field);
           if (!fieldWasRequested) {
             context.reportError(
-              new GraphQLError(`'${field}' field required on '${node.name.value}'`, [node])
+              new GraphQLError(
+                `'${field}' field required on '${node.name.value}'`,
+                [node]
+              )
             );
           }
         }
       });
-    },
+    }
   };
 }
 
@@ -64,11 +132,14 @@ export function typeNamesShouldBeCapitalized(context) {
       const typeName = node.name.value;
       if (typeName[0] == typeName[0].toLowerCase()) {
         context.reportError(
-          new GraphQLError("All type names should start with a capital letter", [ node ])
+          new GraphQLError(
+            "All type names should start with a capital letter",
+            [node]
+          )
         );
       }
     }
-  }
+  };
 }
 
 // Mostly taken from https://github.com/graphql/graphql-js/blob/063148de039b02670a760b8d3dfaf2a04a467169/src/utilities/findDeprecatedUsages.js
@@ -81,11 +152,13 @@ export function noDeprecatedFields(context) {
         const parentType = context.getParentType();
         if (parentType) {
           const reason = fieldDef.deprecationReason;
-          context.reportError(new GraphQLError(
-            `The field ${parentType.name}.${fieldDef.name} is deprecated.` +
-            (reason ? ' ' + reason : ''),
-            [ node ]
-          ));
+          context.reportError(
+            new GraphQLError(
+              `The field ${parentType.name}.${fieldDef.name} is deprecated.` +
+                (reason ? " " + reason : ""),
+              [node]
+            )
+          );
         }
       }
     },
@@ -97,13 +170,15 @@ export function noDeprecatedFields(context) {
         const type = getNamedType(context.getInputType());
         if (type) {
           const reason = enumVal.deprecationReason;
-          context.reportError(new GraphQLError(
-            `The enum value ${type.name}.${enumVal.name} is deprecated.` +
-            (reason ? ' ' + reason : ''),
-            [ node ]
-          ));
+          context.reportError(
+            new GraphQLError(
+              `The enum value ${type.name}.${enumVal.name} is deprecated.` +
+                (reason ? " " + reason : ""),
+              [node]
+            )
+          );
         }
       }
     }
-  }
+  };
 }

test/__fixtures__/required-fields-valid-id.graphql

@@ -1 +1 @@
-query { greetings { id, hello, foo } }
+query { greetings { id, hello, hi } }

test/schema.graphql

@@ -30,6 +30,8 @@ type Query {
   sum(a: Int!, b: Int!): Int!
   greetings: Greetings
   stories: [Story!]!
+  greetingOrStory: GreetingOrStory
+  nodes: [Node]
 }
 
 type AllFilmsObj {
@@ -70,3 +72,19 @@ type Comment {
 type PublicUser {
   fullName: String
 }
+
+union GreetingOrStory = Greetings | Story
+
+interface Node {
+  id: ID!
+}
+
+type NodeA implements Node {
+  id: ID!
+  fieldA: String
+}
+
+type NodeB implements Node{
+  id: ID!
+  fieldB: String
+}

test/validationRules/required-fields.js

@@ -7,8 +7,11 @@ const requiredFieldsTestCases = {
   pass: [
     "const x = gql`query { allFilms { films { title } } }`",
     "const x = gql`query { stories { id comments { text } } }`",
-    "const x = gql`query { greetings { id, hello, foo } }`",
-    "const x = gql`query { greetings { hello ... on Greetings { id } } }`"
+    "const x = gql`query { greetings { id, hello, hi } }`",
+    "const x = gql`query { greetings { id ... on Greetings { hello } } }`",
+    "const x = gql`fragment Name on Greetings { id hello }`",
+    "const x = gql`fragment Id on Node { id ... on NodeA { fieldA } }`",
+    "const x = gql`query { nodes { id ... on NodeA { fieldA } } }`",
   ],
   fail: [
     {
@@ -31,14 +34,52 @@ const requiredFieldsTestCases = {
     },
     {
       code:
-        "const x = gql`query { greetings { hello ... on Greetings { foo } } }`",
+        "const x = gql`query { greetings { hello ... on Greetings { hello } } }`",
       errors: [
         {
           message: `'id' field required on 'greetings'`,
           type: "TaggedTemplateExpression"
         }
       ]
-    }
+    },
+    {
+      code: 'const x = gql`query { greetings { hello ...GreetingsFragment} }`',
+      errors: [
+        {
+          message: `'id' field required on 'greetings'`,
+          type: 'TaggedTemplateExpression',
+        },
+      ],
+    },
+    {
+      code: 'const x = gql`fragment Name on Greetings { hello }`',
+      errors: [
+        {
+          message: `'id' field required on 'fragment Name on Greetings'`,
+          type: 'TaggedTemplateExpression',
+        },
+      ],
+    },
+    {
+      code:
+        "const x = gql`query { greetingOrStory { ... on Greetings { id } ... on Story { comments { text } } } }`",
+      errors: [
+        {
+          message: `'id' field required on '... on Story'`,
+          type: "TaggedTemplateExpression"
+        }
+      ]
+    },
+    {
+      code:
+        "const x = gql`query { nodes { ... on NodeA { id fieldA } ... on NodeB { id fieldB }}}`",
+      errors: [
+        {
+          message: `'id' field required on 'nodes'`,
+          type: "TaggedTemplateExpression"
+        }
+      ]
+    },
   ]
 };
 
@@ -52,7 +93,7 @@ let options = [
   }
 ];
 
-ruleTester.run("testing required-fields rule", rules["required-fields"], {
+ruleTester.run("testing required-fields rule with env", rules["required-fields"], {
   valid: requiredFieldsTestCases.pass.map(code => ({
     options,
     parserOptions,
@@ -74,7 +115,7 @@ options = [
     requiredFields: ["id"]
   }
 ];
-ruleTester.run("testing required-fields rule", rules["required-fields"], {
+ruleTester.run("testing required-fields rule without env", rules["required-fields"], {
   valid: requiredFieldsTestCases.pass.map(code => ({
     options,
     parserOptions,