Skip to content

Commit

Permalink
fix(compiler): fix validation for fragment spread of interface withou…
Browse files Browse the repository at this point in the history 
…t implementers (#896)

* fix(compiler): add failing test for spread of empty interface

`Intf` has no implementations. As written in the spec, doing a `... on
Intf` fragment spread should never work, as the set of possible types is
empty and can never intersect with the parent type. However,
implementations like graphql-js and graphql-go have an early check,
accepting the fragment if the type condition is equal to the parent
type. This tests reproduces that.

We may want to align with graphql-js and graphql-go rather than the spec
here for compatibility? Though it's not something that's likely to
happen in the real world.

Ref graphql/graphql-spec#1109

* fix(compiler): always accept spreading ...Ty when parent type is Ty
  • Loading branch information
@goto-bus-stop
goto-bus-stop authored Aug 26, 2024
1 parent 55002fd commit d93c1fc
Show file tree
Hide file tree
Showing 7 changed files with 289 additions and 24 deletions.
7 changes: 7 additions & 0 deletions crates/apollo-compiler/src/validation/fragment.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,13 @@ fn validate_fragment_spread_type(
selection: &executable::Selection,
context: OperationValidationContext<'_>,
) {
// Treat a spread that's just literally on the parent type as always valid:
// by spec text, it shouldn't be, but graphql-{js,java,go} and others all do this.
// See https://github.com/graphql/graphql-spec/issues/1109
if type_condition == against_type {
return;
}

// Another diagnostic will be raised if the type condition was wrong.
// We reduce noise by silencing other issues with the fragment.
let Some(type_condition_definition) = schema.types.get(type_condition) else {
Expand Down
10 changes: 7 additions & 3 deletions crates/apollo-compiler/test_data/diagnostics/0091_recursive_interface_definition.graphql
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,17 @@ interface A implements B {
interface B implements A {
name: String
}
fragment recursive on A {
name
type Impl implements A & B {
name: String
}

type Query {
get: A
}

fragment recursive on A {
name
}

query {
get { ...recursive }
}
19 changes: 0 additions & 19 deletions crates/apollo-compiler/test_data/diagnostics/0091_recursive_interface_definition.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,23 +23,4 @@ Error: interface `B` declares that it implements `A`, but to do so it must also
│ │
│ ╰─────── B must also be implemented here
───╯
Error: fragment `recursive` with type condition `A` cannot be applied to `A`
╭─[0091_recursive_interface_definition.graphql:15:9]
│
1 │ ╭───▶ interface A implements B {
┆ ┆
3 │ ├───▶ }
│ │
│ ╰───────── type condition `A` is not assignable to this type
│
7 │ ╭─▶ fragment recursive on A {
┆ ┆
9 │ ├─▶ }
│ │
│ ╰─────── fragment declared with type condition `A` here
│
15 │ get { ...recursive }
│ ──────┬─────
│ ╰─────── fragment `recursive` cannot be applied
────╯

16 changes: 16 additions & 0 deletions crates/apollo-compiler/test_data/ok/0116_interface_without_implementations.graphql
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
type Query {
intf: Intf
}
interface Intf {
field: Int
}

query SelectDirectly {
intf { field }
}

query UsingInlineFragment {
intf {
... on Intf { field }
}
}
232 changes: 232 additions & 0 deletions crates/apollo-compiler/test_data/ok/0116_interface_without_implementations.txt
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,232 @@
Schema {
sources: {
1: SourceFile {
path: "built_in.graphql",
source_text: include_str!("built_in.graphql"),
},
45: SourceFile {
path: "0116_interface_without_implementations.graphql",
source_text: "type Query {\n intf: Intf\n}\ninterface Intf {\n field: Int\n}\n\nquery SelectDirectly {\n intf { field }\n}\n\nquery UsingInlineFragment {\n intf {\n ... on Intf { field }\n }\n}\n",
},
},
schema_definition: SchemaDefinition {
description: None,
directives: [],
query: Some(
ComponentName {
origin: Definition,
name: "Query",
},
),
mutation: None,
subscription: None,
},
directive_definitions: {
"skip": built_in_directive!("skip"),
"include": built_in_directive!("include"),
"deprecated": built_in_directive!("deprecated"),
"specifiedBy": built_in_directive!("specifiedBy"),
},
types: {
"__Schema": built_in_type!("__Schema"),
"__Type": built_in_type!("__Type"),
"__TypeKind": built_in_type!("__TypeKind"),
"__Field": built_in_type!("__Field"),
"__InputValue": built_in_type!("__InputValue"),
"__EnumValue": built_in_type!("__EnumValue"),
"__Directive": built_in_type!("__Directive"),
"__DirectiveLocation": built_in_type!("__DirectiveLocation"),
"Int": built_in_type!("Int"),
"Float": built_in_type!("Float"),
"String": built_in_type!("String"),
"Boolean": built_in_type!("Boolean"),
"ID": built_in_type!("ID"),
"Query": Object(
0..27 @45 ObjectType {
description: None,
name: "Query",
implements_interfaces: {},
directives: [],
fields: {
"intf": Component {
origin: Definition,
node: 15..25 @45 FieldDefinition {
description: None,
name: "intf",
arguments: [],
ty: Named(
"Intf",
),
directives: [],
},
},
},
},
),
"Intf": Interface(
28..59 @45 InterfaceType {
description: None,
name: "Intf",
implements_interfaces: {},
directives: [],
fields: {
"field": Component {
origin: Definition,
node: 47..57 @45 FieldDefinition {
description: None,
name: "field",
arguments: [],
ty: Named(
"Int",
),
directives: [],
},
},
},
},
),
},
}
ExecutableDocument {
sources: {
1: SourceFile {
path: "built_in.graphql",
source_text: include_str!("built_in.graphql"),
},
45: SourceFile {
path: "0116_interface_without_implementations.graphql",
source_text: "type Query {\n intf: Intf\n}\ninterface Intf {\n field: Int\n}\n\nquery SelectDirectly {\n intf { field }\n}\n\nquery UsingInlineFragment {\n intf {\n ... on Intf { field }\n }\n}\n",
},
},
operations: OperationMap {
anonymous: None,
named: {
"SelectDirectly": 61..102 @45 Operation {
operation_type: Query,
name: Some(
"SelectDirectly",
),
variables: [],
directives: [],
selection_set: SelectionSet {
ty: "Query",
selections: [
Field(
86..100 @45 Field {
definition: 15..25 @45 FieldDefinition {
description: None,
name: "intf",
arguments: [],
ty: Named(
"Intf",
),
directives: [],
},
alias: None,
name: "intf",
arguments: [],
directives: [],
selection_set: SelectionSet {
ty: "Intf",
selections: [
Field(
93..98 @45 Field {
definition: 47..57 @45 FieldDefinition {
description: None,
name: "field",
arguments: [],
ty: Named(
"Int",
),
directives: [],
},
alias: None,
name: "field",
arguments: [],
directives: [],
selection_set: SelectionSet {
ty: "Int",
selections: [],
},
},
),
],
},
},
),
],
},
},
"UsingInlineFragment": 104..172 @45 Operation {
operation_type: Query,
name: Some(
"UsingInlineFragment",
),
variables: [],
directives: [],
selection_set: SelectionSet {
ty: "Query",
selections: [
Field(
134..170 @45 Field {
definition: 15..25 @45 FieldDefinition {
description: None,
name: "intf",
arguments: [],
ty: Named(
"Intf",
),
directives: [],
},
alias: None,
name: "intf",
arguments: [],
directives: [],
selection_set: SelectionSet {
ty: "Intf",
selections: [
InlineFragment(
145..166 @45 InlineFragment {
type_condition: Some(
"Intf",
),
directives: [],
selection_set: SelectionSet {
ty: "Intf",
selections: [
Field(
159..164 @45 Field {
definition: 47..57 @45 FieldDefinition {
description: None,
name: "field",
arguments: [],
ty: Named(
"Int",
),
directives: [],
},
alias: None,
name: "field",
arguments: [],
directives: [],
selection_set: SelectionSet {
ty: "Int",
selections: [],
},
},
),
],
},
},
),
],
},
},
),
],
},
},
},
},
fragments: {},
}
8 changes: 6 additions & 2 deletions ...llo-compiler/test_data/serializer/diagnostics/0091_recursive_interface_definition.graphql
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,18 @@ interface B implements A {
name: String
}

fragment recursive on A {
name
type Impl implements A & B {
name: String
}

type Query {
get: A
}

fragment recursive on A {
name
}

query {
get {
...recursive
Expand Down
21 changes: 21 additions & 0 deletions ...es/apollo-compiler/test_data/serializer/ok/0116_interface_without_implementations.graphql
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
type Query {
intf: Intf
}

interface Intf {
field: Int
}

query SelectDirectly {
intf {
field
}
}

query UsingInlineFragment {
intf {
... on Intf {
field
}
}
}

0 comments on commit d93c1fc

Please sign in to comment.