Skip to content

Commit 0699f51

Browse files
committed
RFC: Return type overlap validation
Implements graphql/graphql-spec#162 This alters the "Overlapping Fields Can Be Merged" validation rule to better express return type validation issues. The existing rule has presented some false-positives in some schema where interfaces with co-variant types are commonly used. The "same return type" check doesn't remove any ambiguity. Instead what that "same return type" check is attempting to prevent is spreading two fragments (or inline fragments) which have fields with return types where ambiguity would be introduced in the response. In order to curb false-positives, we later changed this rule such that if two fields were known to never apply simultaneously, then we would skip the remainder of the rule. ``` { ... on Person { foo: fullName } ... on Pet { foo: petName } } ``` However this can introduce false-negatives! ``` { ... on Person { foo: birthday { bar: year } } ... on Business { foo: location { bar: street } } } ``` In the above example, `data.foo.bar` could be of type `Int` or type `String`, it's ambiguous! This differing return type breaks some client model implementations (Fragment models) in addition to just being confusing.
1 parent c7e6a75 commit 0699f51

File tree

2 files changed

+261
-61
lines changed

2 files changed

+261
-61
lines changed

src/validation/__tests__/OverlappingFieldsCanBeMerged-test.js

Lines changed: 138 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -376,19 +376,25 @@ describe('Validate: Overlapping fields can be merged', () => {
376376
const StringBox = new GraphQLObjectType({
377377
name: 'StringBox',
378378
interfaces: [ SomeBox ],
379-
fields: {
379+
fields: () => ({
380380
scalar: { type: GraphQLString },
381381
unrelatedField: { type: GraphQLString },
382-
}
382+
listStringBox: { type: new GraphQLList(StringBox) },
383+
stringBox: { type: StringBox },
384+
intBox: { type: IntBox },
385+
})
383386
});
384387

385388
const IntBox = new GraphQLObjectType({
386389
name: 'IntBox',
387390
interfaces: [ SomeBox ],
388-
fields: {
391+
fields: () => ({
389392
scalar: { type: GraphQLInt },
390393
unrelatedField: { type: GraphQLString },
391-
}
394+
listStringBox: { type: new GraphQLList(StringBox) },
395+
stringBox: { type: StringBox },
396+
intBox: { type: IntBox },
397+
})
392398
});
393399

394400
const NonNullStringBox1 = new GraphQLInterfaceType({
@@ -455,7 +461,7 @@ describe('Validate: Overlapping fields can be merged', () => {
455461
connection: { type: Connection }
456462
})
457463
}),
458-
types: [ IntBox, NonNullStringBox1Impl, NonNullStringBox2Impl ]
464+
types: [ IntBox, StringBox, NonNullStringBox1Impl, NonNullStringBox2Impl ]
459465
});
460466

461467
it('conflicting return types which potentially overlap', () => {
@@ -477,21 +483,142 @@ describe('Validate: Overlapping fields can be merged', () => {
477483
`, [
478484
{ message: fieldsConflictMessage(
479485
'scalar',
480-
'they return differing types Int and String!'
486+
'they return conflicting types Int and String!'
481487
),
482488
locations: [ { line: 5, column: 15 }, { line: 8, column: 15 } ] }
483489
]);
484490
});
485491

486-
it('allows differing return types which cannot overlap', () => {
487-
// This is valid since an object cannot be both an IntBox and a StringBox.
488-
expectPassesRuleWithSchema(schema, OverlappingFieldsCanBeMerged, `
492+
it('disallows differing return types despite no overlap', () => {
493+
expectFailsRuleWithSchema(schema, OverlappingFieldsCanBeMerged, `
489494
{
490495
someBox {
491-
...on IntBox {
496+
... on IntBox {
497+
scalar
498+
}
499+
... on StringBox {
492500
scalar
493501
}
494-
...on StringBox {
502+
}
503+
}
504+
`, [
505+
{ message: fieldsConflictMessage(
506+
'scalar',
507+
'they return conflicting types Int and String'
508+
),
509+
locations: [ { line: 5, column: 15 }, { line: 8, column: 15 } ] }
510+
]);
511+
});
512+
513+
it('disallows differing return type nullability despite no overlap', () => {
514+
expectFailsRuleWithSchema(schema, OverlappingFieldsCanBeMerged, `
515+
{
516+
someBox {
517+
... on NonNullStringBox1 {
518+
scalar
519+
}
520+
... on StringBox {
521+
scalar
522+
}
523+
}
524+
}
525+
`, [
526+
{ message: fieldsConflictMessage(
527+
'scalar',
528+
'they return conflicting types String! and String'
529+
),
530+
locations: [ { line: 5, column: 15 }, { line: 8, column: 15 } ] }
531+
]);
532+
});
533+
534+
it('disallows differing return type list despite no overlap', () => {
535+
expectFailsRuleWithSchema(schema, OverlappingFieldsCanBeMerged, `
536+
{
537+
someBox {
538+
... on IntBox {
539+
box: listStringBox {
540+
scalar
541+
}
542+
}
543+
... on StringBox {
544+
box: stringBox {
545+
scalar
546+
}
547+
}
548+
}
549+
}
550+
`, [
551+
{ message: fieldsConflictMessage(
552+
'box',
553+
'they return conflicting types [StringBox] and StringBox'
554+
),
555+
locations: [ { line: 5, column: 15 }, { line: 10, column: 15 } ] }
556+
]);
557+
});
558+
559+
it('disallows differing subfields', () => {
560+
expectFailsRuleWithSchema(schema, OverlappingFieldsCanBeMerged, `
561+
{
562+
someBox {
563+
... on IntBox {
564+
box: stringBox {
565+
val: scalar
566+
val: unrelatedField
567+
}
568+
}
569+
... on StringBox {
570+
box: stringBox {
571+
val: scalar
572+
}
573+
}
574+
}
575+
}
576+
`, [
577+
{ message: fieldsConflictMessage(
578+
'val',
579+
'scalar and unrelatedField are different fields'
580+
),
581+
locations: [ { line: 6, column: 17 }, { line: 7, column: 17 } ] }
582+
]);
583+
});
584+
585+
it('disallows differing deep return types despite no overlap', () => {
586+
expectFailsRuleWithSchema(schema, OverlappingFieldsCanBeMerged, `
587+
{
588+
someBox {
589+
... on IntBox {
590+
box: stringBox {
591+
scalar
592+
}
593+
}
594+
... on StringBox {
595+
box: intBox {
596+
scalar
597+
}
598+
}
599+
}
600+
}
601+
`, [
602+
{ message: fieldsConflictMessage(
603+
'box',
604+
[ [ 'scalar', 'they return conflicting types String and Int' ] ]
605+
),
606+
locations: [
607+
{ line: 5, column: 15 },
608+
{ line: 6, column: 17 },
609+
{ line: 10, column: 15 },
610+
{ line: 11, column: 17 } ] }
611+
]);
612+
});
613+
614+
it('allows non-conflicting overlaping types', () => {
615+
expectPassesRuleWithSchema(schema, OverlappingFieldsCanBeMerged, `
616+
{
617+
someBox {
618+
... on IntBox {
619+
scalar: unrelatedField
620+
}
621+
... on StringBox {
495622
scalar
496623
}
497624
}

0 commit comments

Comments
 (0)