Validation: improving overlapping fields quality (#386)

This improves the overlapping fields validation performance and improves error reporting quality by separating the concepts of checking fields "within" a single collection of fields from checking fields "between" two different collections of fields. This ensures for deeply overlapping fields that nested fields are not checked against each other repeatedly. Extending this concept further, fragment spreads are no longer expanded inline before looking for conflicts, instead the fields within a fragment are compared to the fields with the selection set which contained the referencing fragment spread.

e.g.

```graphql
{
  same: a
  same: b
  ...X
}

fragment X on T {
  same: c
  same: d
}
```

In the above example, the initial query body is checked "within" so `a` is compared to `b`. Also, the fragment `X` is checked "within" so `c` is compared to `d`. Because of the fragment spread, the query body and fragment `X` are checked "between" so that `a` and `b` are each compared to `c` and `d`. In this trivial example, no fewer checks are performed, but in the case where fragments are referenced multiple times, this reduces the overall number of checks (regardless of memoization).

**BREAKING**: This can change the order of fields reported when a conflict arises when fragment spreads are involved. If you are checking the precise output of errors (e.g. for unit tests), you may find existing errors change from `"a" and "c" are different fields` to `"c" and "a" are different fields`.

From a perf point of view, this is fairly minor as the memoization "PairSet" was already keeping these repeated checks from consuming time, however this will reduce the number of memoized hits because of the algorithm improvement.

From an error reporting point of view, this reports nearest-common-ancestor issues when found in a fragment that comes later in the validation process. I've added a test which fails with the existing impl and now passes, as well as changed a comment.

This also fixes an error where validation issues could be missed because of an over-eager memoization. I've also modified the `PairSet` to be aware of both forms of memoization, also represented by a previously failing test.

Author: leebyron

src/validation/__tests__/OverlappingFieldsCanBeMerged-test.js

@@ -247,10 +247,10 @@ describe('Validate: Overlapping fields can be merged', () => {
     `, [
       { message: fieldsConflictMessage('x', 'a and b are different fields'),
         locations: [ { line: 18, column: 9 }, { line: 21, column: 9 } ] },
-      { message: fieldsConflictMessage('x', 'a and c are different fields'),
-        locations: [ { line: 18, column: 9 }, { line: 14, column: 11 } ] },
-      { message: fieldsConflictMessage('x', 'b and c are different fields'),
-        locations: [ { line: 21, column: 9 }, { line: 14, column: 11 } ] }
+      { message: fieldsConflictMessage('x', 'c and a are different fields'),
+        locations: [ { line: 14, column: 11 }, { line: 18, column: 9 } ] },
+      { message: fieldsConflictMessage('x', 'c and b are different fields'),
+        locations: [ { line: 14, column: 11 }, { line: 21, column: 9 } ] }
     ]);
   });
 
@@ -363,6 +363,97 @@ describe('Validate: Overlapping fields can be merged', () => {
     ]);
   });
 
+  it('reports deep conflict to nearest common ancestor in fragments', () => {
+    expectFailsRule(OverlappingFieldsCanBeMerged, `
+      {
+        field {
+          ...F
+        }
+        field {
+          ...F
+        }
+      }
+      fragment F on T {
+        deepField {
+          deeperField {
+            x: a
+          }
+          deeperField {
+            x: b
+          }
+        },
+        deepField {
+          deeperField {
+            y
+          }
+        }
+      }
+    `, [
+      { message: fieldsConflictMessage(
+          'deeperField', [ [ 'x', 'a and b are different fields' ] ]
+        ),
+        locations: [
+          { line: 12, column: 11 },
+          { line: 13, column: 13 },
+          { line: 15, column: 11 },
+          { line: 16, column: 13 } ] },
+    ]);
+  });
+
+  it('reports deep conflict in nested fragments', () => {
+    expectFailsRule(OverlappingFieldsCanBeMerged, `
+      {
+        field {
+          ...F
+        }
+        field {
+          ...I
+        }
+      }
+      fragment F on T {
+        x: a
+        ...G
+      }
+      fragment G on T {
+        y: c
+      }
+      fragment I on T {
+        y: d
+        ...J
+      }
+      fragment J on T {
+        x: b
+      }
+    `, [
+      { message: fieldsConflictMessage(
+          'field', [ [ 'x', 'a and b are different fields' ],
+                     [ 'y', 'c and d are different fields' ] ]
+        ),
+        locations: [
+          { line: 3, column: 9 },
+          { line: 11, column: 9 },
+          { line: 15, column: 9 },
+          { line: 6, column: 9 },
+          { line: 22, column: 9 },
+          { line: 18, column: 9 } ] },
+    ]);
+  });
+
+  it('ignores unknown fragments', () => {
+    expectPassesRule(OverlappingFieldsCanBeMerged, `
+    {
+      field
+      ...Unknown
+      ...Known
+    }
+
+    fragment Known on T {
+      field
+      ...OtherUnknown
+    }
+    `);
+  });
+
   describe('return types must be unambiguous', () => {
 
     const SomeBox = new GraphQLInterfaceType({
@@ -537,6 +628,64 @@ describe('Validate: Overlapping fields can be merged', () => {
       ]);
     });
 
+    it('reports correctly when a non-exclusive follows an exclusive', () => {
+      expectFailsRuleWithSchema(schema, OverlappingFieldsCanBeMerged, `
+        {
+          someBox {
+            ... on IntBox {
+              deepBox {
+                ...X
+              }
+            }
+          }
+          someBox {
+            ... on StringBox {
+              deepBox {
+                ...Y
+              }
+            }
+          }
+          memoed: someBox {
+            ... on IntBox {
+              deepBox {
+                ...X
+              }
+            }
+          }
+          memoed: someBox {
+            ... on StringBox {
+              deepBox {
+                ...Y
+              }
+            }
+          }
+          other: someBox {
+            ...X
+          }
+          other: someBox {
+            ...Y
+          }
+        }
+        fragment X on SomeBox {
+          scalar
+        }
+        fragment Y on SomeBox {
+          scalar: unrelatedField
+        }
+      `, [
+        { message: fieldsConflictMessage(
+            'other',
+            [ [ 'scalar', 'scalar and unrelatedField are different fields' ] ]
+          ),
+          locations: [
+            { line: 31, column: 11 },
+            { line: 39, column: 11 },
+            { line: 34, column: 11 },
+            { line: 42, column: 11 },
+          ] }
+      ]);
+    });
+
     it('disallows differing return type nullability despite no overlap', () => {
       expectFailsRuleWithSchema(schema, OverlappingFieldsCanBeMerged, `
         {
@@ -725,15 +874,15 @@ describe('Validate: Overlapping fields can be merged', () => {
       `, [
         { message: fieldsConflictMessage(
             'edges',
-            [ [ 'node', [ [ 'id', 'id and name are different fields' ] ] ] ]
+            [ [ 'node', [ [ 'id', 'name and id are different fields' ] ] ] ]
           ),
           locations: [
-            { line: 14, column: 11 },
-            { line: 15, column: 13 },
-            { line: 16, column: 15 },
             { line: 5, column: 13 },
             { line: 6, column: 15 },
             { line: 7, column: 17 },
+            { line: 14, column: 11 },
+            { line: 15, column: 13 },
+            { line: 16, column: 15 },
           ] }
       ]);
     });