[validation] Allow safe divergence

As pointed out in #53, our validation is more conservative than it needs to be in some cases. Particularly, two fields which could not overlap are still treated as a potential conflict.

This change loosens this rule, allowing fields which can never both apply in a frame of execution to diverge.

Author: leebyron

src/validation/__tests__/OverlappingFieldsCanBeMerged.js

@@ -21,7 +21,7 @@ import {
 import {
   GraphQLSchema,
   GraphQLObjectType,
-  GraphQLUnionType,
+  GraphQLInterfaceType,
   GraphQLList,
   GraphQLNonNull,
   GraphQLInt,
@@ -101,6 +101,21 @@ describe('Validate: Overlapping fields can be merged', () => {
     ]);
   });
 
+  it('Same aliases allowed on non-overlapping fields', () => {
+    // This is valid since no object can be both a "Dog" and a "Cat", thus
+    // these fields can never overlap.
+    expectPassesRule(OverlappingFieldsCanBeMerged, `
+      fragment sameAliasesWithDifferentFieldTargets on Pet {
+        ... on Dog {
+          name
+        }
+        ... on Cat {
+          name: nickname
+        }
+      }
+    `);
+  });
+
   it('Alias masking direct field access', () => {
     expectFailsRule(OverlappingFieldsCanBeMerged, `
       fragment aliasMaskingDirectFieldAccess on Dog {
@@ -131,6 +146,21 @@ describe('Validate: Overlapping fields can be merged', () => {
     ]);
   });
 
+  it('allows different args where no conflict is possible', () => {
+    // This is valid since no object can be both a "Dog" and a "Cat", thus
+    // these fields can never overlap.
+    expectPassesRule(OverlappingFieldsCanBeMerged, `
+      fragment conflictingArgs on Pet {
+        ... on Dog {
+          name(surname: true)
+        }
+        ... on Cat {
+          name
+        }
+      }
+    `);
+  });
+
   it('conflicting directives', () => {
     expectFailsRule(OverlappingFieldsCanBeMerged, `
       fragment conflictingDirectiveArgs on Dog {
@@ -353,39 +383,67 @@ describe('Validate: Overlapping fields can be merged', () => {
 
   describe('return types must be unambiguous', () => {
 
+    var SomeBox = new GraphQLInterfaceType({
+      name: 'SomeBox',
+      resolveType: () => StringBox,
+      fields: {
+        unrelatedField: { type: GraphQLString }
+      }
+    });
+
+    /* eslint-disable no-unused-vars */
     var StringBox = new GraphQLObjectType({
       name: 'StringBox',
+      interfaces: [ SomeBox ],
       fields: {
-        scalar: { type: GraphQLString }
+        scalar: { type: GraphQLString },
+        unrelatedField: { type: GraphQLString },
       }
     });
 
     var IntBox = new GraphQLObjectType({
       name: 'IntBox',
+      interfaces: [ SomeBox ],
       fields: {
-        scalar: { type: GraphQLInt }
+        scalar: { type: GraphQLInt },
+        unrelatedField: { type: GraphQLString },
       }
     });
 
-    var NonNullStringBox1 = new GraphQLObjectType({
+    var NonNullStringBox1 = new GraphQLInterfaceType({
       name: 'NonNullStringBox1',
+      resolveType: () => StringBox,
       fields: {
         scalar: { type: new GraphQLNonNull(GraphQLString) }
       }
     });
 
-    var NonNullStringBox2 = new GraphQLObjectType({
+    var NonNullStringBox1Impl = new GraphQLObjectType({
+      name: 'NonNullStringBox1Impl',
+      interfaces: [ SomeBox, NonNullStringBox1 ],
+      fields: {
+        scalar: { type: new GraphQLNonNull(GraphQLString) },
+        unrelatedField: { type: GraphQLString },
+      }
+    });
+
+    var NonNullStringBox2 = new GraphQLInterfaceType({
       name: 'NonNullStringBox2',
+      resolveType: () => StringBox,
       fields: {
         scalar: { type: new GraphQLNonNull(GraphQLString) }
       }
     });
 
-    var BoxUnion = new GraphQLUnionType({
-      name: 'BoxUnion',
-      resolveType: () => StringBox,
-      types: [ StringBox, IntBox, NonNullStringBox1, NonNullStringBox2 ]
+    var NonNullStringBox2Impl = new GraphQLObjectType({
+      name: 'NonNullStringBox2Impl',
+      interfaces: [ SomeBox, NonNullStringBox2 ],
+      fields: {
+        scalar: { type: new GraphQLNonNull(GraphQLString) },
+        unrelatedField: { type: GraphQLString },
+      }
     });
+    /* eslint-enable no-unused-vars */
 
     var Connection = new GraphQLObjectType({
       name: 'Connection',
@@ -413,37 +471,57 @@ describe('Validate: Overlapping fields can be merged', () => {
       query: new GraphQLObjectType({
         name: 'QueryRoot',
         fields: () => ({
-          boxUnion: { type: BoxUnion },
+          someBox: { type: SomeBox },
           connection: { type: Connection }
         })
       })
     });
 
-    it('conflicting scalar return types', () => {
+    it('conflicting return types which potentially overlap', () => {
+      // This is invalid since an object could potentially be both the Object
+      // type IntBox and the interface type NonNullStringBox1. While that
+      // condition does not exist in the current schema, the schema could
+      // expand in the future to allow this. Thus it is invalid.
       expectFailsRuleWithSchema(schema, OverlappingFieldsCanBeMerged, `
         {
-          boxUnion {
+          someBox {
             ...on IntBox {
               scalar
             }
-            ...on StringBox {
+            ...on NonNullStringBox1 {
               scalar
             }
           }
         }
       `, [
         { message: fieldsConflictMessage(
             'scalar',
-            'they return differing types Int and String'
+            'they return differing types Int and String!'
           ),
           locations: [ { line: 5, column: 15 }, { line: 8, column: 15 } ] }
       ]);
     });
 
+    it('allows differing return types which cannot overlap', () => {
+      // This is valid since an object cannot be both an IntBox and a StringBox.
+      expectPassesRuleWithSchema(schema, OverlappingFieldsCanBeMerged, `
+        {
+          someBox {
+            ...on IntBox {
+              scalar
+            }
+            ...on StringBox {
+              scalar
+            }
+          }
+        }
+      `);
+    });
+
     it('same wrapped scalar return types', () => {
       expectPassesRuleWithSchema(schema, OverlappingFieldsCanBeMerged, `
         {
-          boxUnion {
+          someBox {
             ...on NonNullStringBox1 {
               scalar
             }
@@ -505,7 +583,7 @@ describe('Validate: Overlapping fields can be merged', () => {
     it('ignores unknown types', () => {
       expectPassesRuleWithSchema(schema, OverlappingFieldsCanBeMerged, `
         {
-          boxUnion {
+          someBox {
             ...on UnknownType {
               scalar
             }

src/validation/rules/OverlappingFieldsCanBeMerged.js

@@ -27,6 +27,7 @@ import {
 import type {
   GraphQLType,
   GraphQLNamedType,
+  GraphQLCompositeType,
   GraphQLFieldDefinition
 } from '../../type/definition';
 import { typeFromAST } from '../../utilities/typeFromAST';
@@ -75,16 +76,29 @@ export function OverlappingFieldsCanBeMerged(context: ValidationContext): any {
 
   function findConflict(
     responseName: string,
-    pair1: [Field, GraphQLFieldDefinition],
-    pair2: [Field, GraphQLFieldDefinition]
+    pair1: [ GraphQLCompositeType, Field, GraphQLFieldDefinition ],
+    pair2: [ GraphQLCompositeType, Field, GraphQLFieldDefinition ]
   ): ?Conflict {
-    var [ ast1, def1 ] = pair1;
-    var [ ast2, def2 ] = pair2;
+    var [ parentType1, ast1, def1 ] = pair1;
+    var [ parentType2, ast2, def2 ] = pair2;
     if (ast1 === ast2 || comparedSet.has(ast1, ast2)) {
       return;
     }
     comparedSet.add(ast1, ast2);
 
+    // If the statically known parent types could not possibly apply at the same
+    // time, then it is safe to permit them to diverge as they will not present
+    // any ambiguity by differing.
+    // It is known that two parent types could never overlap if they are
+    // different Object types. Interface or Union types might overlap - if not
+    // in the current state of the schema, then perhaps in some future version,
+    // thus may not safely diverge.
+    if (parentType1 !== parentType2 &&
+        parentType1 instanceof GraphQLObjectType &&
+        parentType2 instanceof GraphQLObjectType) {
+      return;
+    }
+
     var name1 = ast1.name.value;
     var name2 = ast2.name.value;
     if (name1 !== name2) {
@@ -267,7 +281,7 @@ function collectFieldASTsAndDefs(
         if (!_astAndDefs[responseName]) {
           _astAndDefs[responseName] = [];
         }
-        _astAndDefs[responseName].push([ selection, fieldDef ]);
+        _astAndDefs[responseName].push([ parentType, selection, fieldDef ]);
         break;
       case INLINE_FRAGMENT:
         var typeCondition = selection.typeCondition;