diff --git a/src/validation/__tests__/OverlappingFieldsCanBeMerged.js b/src/validation/__tests__/OverlappingFieldsCanBeMerged.js index 2ea6ef04dd..7d163c0129 100644 --- a/src/validation/__tests__/OverlappingFieldsCanBeMerged.js +++ b/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,20 +471,24 @@ 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 } } @@ -434,16 +496,32 @@ describe('Validate: Overlapping fields can be merged', () => { `, [ { 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 } diff --git a/src/validation/rules/OverlappingFieldsCanBeMerged.js b/src/validation/rules/OverlappingFieldsCanBeMerged.js index f6c41965a8..049f11a98d 100644 --- a/src/validation/rules/OverlappingFieldsCanBeMerged.js +++ b/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,37 @@ export function OverlappingFieldsCanBeMerged(context: ValidationContext): any { function findConflict( responseName: string, - pair1: [Field, GraphQLFieldDefinition], - pair2: [Field, GraphQLFieldDefinition] + field1: [ GraphQLCompositeType, Field, GraphQLFieldDefinition ], + field2: [ GraphQLCompositeType, Field, GraphQLFieldDefinition ] ): ?Conflict { - var [ ast1, def1 ] = pair1; - var [ ast2, def2 ] = pair2; - if (ast1 === ast2 || comparedSet.has(ast1, ast2)) { + var [ parentType1, ast1, def1 ] = field1; + var [ parentType2, ast2, def2 ] = field2; + + // Not a pair. + if (ast1 === ast2) { + return; + } + + // 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; + } + + // Memoize, do not report the same issue twice. + if (comparedSet.has(ast1, ast2)) { return; } comparedSet.add(ast1, ast2); + var name1 = ast1.name.value; var name2 = ast2.name.value; if (name1 !== name2) { @@ -267,7 +289,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;