Skip to content

Commit

Permalink
[validation] Allow safe divergence
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@leebyron
leebyron committed Nov 13, 2015
1 parent 228215a commit d71e063
Show file tree
Hide file tree
Showing 2 changed files with 151 additions and 22 deletions.
140 changes: 124 additions & 16 deletions src/validation/__tests__/OverlappingFieldsCanBeMerged.js
View file
Expand Up @@ -21,7 +21,7 @@ import {
import {
GraphQLSchema,
GraphQLObjectType,
GraphQLUnionType,
GraphQLInterfaceType,
GraphQLList,
GraphQLNonNull,
GraphQLInt,
Expand Down Expand Up @@ -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 {
Expand All @@ -116,6 +131,36 @@ describe('Validate: Overlapping fields can be merged', () => {
]);
});

it('different args, second adds an argument', () => {
expectFailsRule(OverlappingFieldsCanBeMerged, `
fragment conflictingArgs on Dog {
doesKnowCommand
doesKnowCommand(dogCommand: HEEL)
}
`, [
{ message: fieldsConflictMessage(
'doesKnowCommand',
'they have differing arguments'
),
locations: [ { line: 3, column: 9 }, { line: 4, column: 9 } ] }
]);
});

it('different args, second missing an argument', () => {
expectFailsRule(OverlappingFieldsCanBeMerged, `
fragment conflictingArgs on Dog {
doesKnowCommand(dogCommand: SIT)
doesKnowCommand
}
`, [
{ message: fieldsConflictMessage(
'doesKnowCommand',
'they have differing arguments'
),
locations: [ { line: 3, column: 9 }, { line: 4, column: 9 } ] }
]);
});

it('conflicting args', () => {
expectFailsRule(OverlappingFieldsCanBeMerged, `
fragment conflictingArgs on Dog {
Expand All @@ -131,6 +176,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 {
Expand Down Expand Up @@ -353,39 +413,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',
Expand Down Expand Up @@ -413,37 +501,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
}
Expand Down Expand Up @@ -505,7 +613,7 @@ describe('Validate: Overlapping fields can be merged', () => {
it('ignores unknown types', () => {
expectPassesRuleWithSchema(schema, OverlappingFieldsCanBeMerged, `
{
boxUnion {
someBox {
...on UnknownType {
scalar
}
Expand Down
33 changes: 27 additions & 6 deletions src/validation/rules/OverlappingFieldsCanBeMerged.js
View file
Expand Up @@ -27,6 +27,7 @@ import {
import type {
GraphQLType,
GraphQLNamedType,
GraphQLCompositeType,
GraphQLFieldDefinition
} from '../../type/definition';
import { typeFromAST } from '../../utilities/typeFromAST';
Expand Down Expand Up @@ -75,12 +76,32 @@ 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);
Expand Down Expand Up @@ -267,7 +288,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;
Expand Down

0 comments on commit d71e063

Please sign in to comment.