Skip to content

[validation] Allow safe divergence #229

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 13, 2015
Merged

[validation] Allow safe divergence #229

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
140 changes: 124 additions & 16 deletions src/validation/__tests__/OverlappingFieldsCanBeMerged.js
View file
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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