Repository: apollographql/apollo-rs
Pull request #888 (2024-07-30 07:25:01)

fix(compiler): identify variables in operation directives
Fixes apollo-compiler rejecting valid queries, where a variable is used inside a directive applied directly to an operation. This is indeed allowed by spec.

Extended existing tests to check directives on fragment definitions as well, because I also found it a little surprising that they allow variables too.


Patch:
diff --git a/crates/apollo-compiler/src/validation/variable.rs b/crates/apollo-compiler/src/validation/variable.rs
index 0ce12b5..4aa3ca7 100644
--- a/crates/apollo-compiler/src/validation/variable.rs
+++ b/crates/apollo-compiler/src/validation/variable.rs
@@ -177,6 +177,12 @@ pub(crate) fn validate_unused_variables(
             )
         })
         .collect();
+
+    // You're allowed to do `query($var: Int!) @dir(arg: $var) {}`
+    for used in variables_in_directives(&operation.directives) {
+        unused_vars.remove(used);
+    }
+
     let walked = walk_selections(
         document,
         &operation.selection_set,


Test patch:
diff --git a/crates/apollo-compiler/test_data/diagnostics/0009_operation_with_undefined_variables_in_fragment.graphql b/crates/apollo-compiler/test_data/diagnostics/0009_operation_with_undefined_variables_in_fragment.graphql
index 6dca24f..a9f967f 100644
--- a/crates/apollo-compiler/test_data/diagnostics/0009_operation_with_undefined_variables_in_fragment.graphql
+++ b/crates/apollo-compiler/test_data/diagnostics/0009_operation_with_undefined_variables_in_fragment.graphql
@@ -1,11 +1,14 @@
-query ExampleQuery($variable: Int) {
+directive @track(label: String) on FRAGMENT_DEFINITION
+directive @report(label: String) on QUERY | MUTATION | SUBSCRIPTION
+
+query ExampleQuery($variable: Int) @report(label: $queryLabel) {
   topProducts(first: $variable) {
     name
   }
   ... subFrag
 }
 
-fragment subFrag on Query {
+fragment subFrag on Query @track(label: $productsLabel) {
   topProducts(first: $variable) {
     price(setPrice: $value)
   }
diff --git a/crates/apollo-compiler/test_data/diagnostics/0009_operation_with_undefined_variables_in_fragment.txt b/crates/apollo-compiler/test_data/diagnostics/0009_operation_with_undefined_variables_in_fragment.txt
index 5f9c09b..1ef0c53 100644
--- a/crates/apollo-compiler/test_data/diagnostics/0009_operation_with_undefined_variables_in_fragment.txt
+++ b/crates/apollo-compiler/test_data/diagnostics/0009_operation_with_undefined_variables_in_fragment.txt
@@ -1,7 +1,21 @@
+Error: variable `$queryLabel` is not defined
+   ╭─[0009_operation_with_undefined_variables_in_fragment.graphql:4:51]
+   │
+ 4 │ query ExampleQuery($variable: Int) @report(label: $queryLabel) {
+   │                                                   ─────┬─────  
+   │                                                        ╰─────── not found in this scope
+───╯
+Error: variable `$productsLabel` is not defined
+    ╭─[0009_operation_with_undefined_variables_in_fragment.graphql:11:41]
+    │
+ 11 │ fragment subFrag on Query @track(label: $productsLabel) {
+    │                                         ───────┬──────  
+    │                                                ╰──────── not found in this scope
+────╯
 Error: variable `$value` is not defined
-    ╭─[0009_operation_with_undefined_variables_in_fragment.graphql:10:21]
+    ╭─[0009_operation_with_undefined_variables_in_fragment.graphql:13:21]
     │
- 10 │     price(setPrice: $value)
+ 13 │     price(setPrice: $value)
     │                     ───┬──  
     │                        ╰──── not found in this scope
 ────╯
diff --git a/crates/apollo-compiler/test_data/ok/0042_used_variable_in_operation_directive.graphql b/crates/apollo-compiler/test_data/ok/0042_used_variable_in_operation_directive.graphql
new file mode 100644
index 0000000..a51c79c
--- /dev/null
+++ b/crates/apollo-compiler/test_data/ok/0042_used_variable_in_operation_directive.graphql
@@ -0,0 +1,12 @@
+directive @x(y: Int!) on QUERY | MUTATION | SUBSCRIPTION
+directive @z(f: Boolean) on FRAGMENT_DEFINITION
+
+type Query { field: Boolean }
+type Mutation { field: Boolean }
+type Subscription { field: Boolean }
+
+query Q ($a: Int!, $f: Boolean) @x(y: $a) { field ...f }
+mutation M ($b: Int!) @x(y: $b) { field }
+subscription S ($c: Int!) @x(y: $c) { field }
+
+fragment f on Query @z(f: $f) { field }
diff --git a/crates/apollo-compiler/test_data/ok/0042_used_variable_in_operation_directive.txt b/crates/apollo-compiler/test_data/ok/0042_used_variable_in_operation_directive.txt
new file mode 100644
index 0000000..80348fe
--- /dev/null
+++ b/crates/apollo-compiler/test_data/ok/0042_used_variable_in_operation_directive.txt
@@ -0,0 +1,401 @@
+Schema {
+    sources: {
+        1: SourceFile {
+            path: "built_in.graphql",
+            source_text: include_str!("built_in.graphql"),
+        },
+        43: SourceFile {
+            path: "0042_used_variable_in_operation_directive.graphql",
+            source_text: "directive @x(y: Int!) on QUERY | MUTATION | SUBSCRIPTION\ndirective @z(f: Boolean) on FRAGMENT_DEFINITION\n\ntype Query { field: Boolean }\ntype Mutation { field: Boolean }\ntype Subscription { field: Boolean }\n\nquery Q ($a: Int!, $f: Boolean) @x(y: $a) { field ...f }\nmutation M ($b: Int!) @x(y: $b) { field }\nsubscription S ($c: Int!) @x(y: $c) { field }\n\nfragment f on Query @z(f: $f) { field }\n",
+        },
+    },
+    schema_definition: SchemaDefinition {
+        description: None,
+        directives: [],
+        query: Some(
+            ComponentName {
+                origin: Definition,
+                name: "Query",
+            },
+        ),
+        mutation: Some(
+            ComponentName {
+                origin: Definition,
+                name: "Mutation",
+            },
+        ),
+        subscription: Some(
+            ComponentName {
+                origin: Definition,
+                name: "Subscription",
+            },
+        ),
+    },
+    directive_definitions: {
+        "skip": built_in_directive!("skip"),
+        "include": built_in_directive!("include"),
+        "deprecated": built_in_directive!("deprecated"),
+        "specifiedBy": built_in_directive!("specifiedBy"),
+        "x": 0..56 @43 DirectiveDefinition {
+            description: None,
+            name: "x",
+            arguments: [
+                13..20 @43 InputValueDefinition {
+                    description: None,
+                    name: "y",
+                    ty: 16..20 @43 NonNullNamed(
+                        "Int",
+                    ),
+                    default_value: None,
+                    directives: [],
+                },
+            ],
+            repeatable: false,
+            locations: [
+                "QUERY",
+                "MUTATION",
+                "SUBSCRIPTION",
+            ],
+        },
+        "z": 57..104 @43 DirectiveDefinition {
+            description: None,
+            name: "z",
+            arguments: [
+                70..80 @43 InputValueDefinition {
+                    description: None,
+                    name: "f",
+                    ty: 73..80 @43 Named(
+                        "Boolean",
+                    ),
+                    default_value: None,
+                    directives: [],
+                },
+            ],
+            repeatable: false,
+            locations: [
+                "FRAGMENT_DEFINITION",
+            ],
+        },
+    },
+    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(
+            106..135 @43 ObjectType {
+                description: None,
+                name: "Query",
+                implements_interfaces: {},
+                directives: [],
+                fields: {
+                    "field": Component {
+                        origin: Definition,
+                        node: 119..133 @43 FieldDefinition {
+                            description: None,
+                            name: "field",
+                            arguments: [],
+                            ty: Named(
+                                "Boolean",
+                            ),
+                            directives: [],
+                        },
+                    },
+                },
+            },
+        ),
+        "Mutation": Object(
+            136..168 @43 ObjectType {
+                description: None,
+                name: "Mutation",
+                implements_interfaces: {},
+                directives: [],
+                fields: {
+                    "field": Component {
+                        origin: Definition,
+                        node: 152..166 @43 FieldDefinition {
+                            description: None,
+                            name: "field",
+                            arguments: [],
+                            ty: Named(
+                                "Boolean",
+                            ),
+                            directives: [],
+                        },
+                    },
+                },
+            },
+        ),
+        "Subscription": Object(
+            169..205 @43 ObjectType {
+                description: None,
+                name: "Subscription",
+                implements_interfaces: {},
+                directives: [],
+                fields: {
+                    "field": Component {
+                        origin: Definition,
+                        node: 189..203 @43 FieldDefinition {
+                            description: None,
+                            name: "field",
+                            arguments: [],
+                            ty: Named(
+                                "Boolean",
+                            ),
+                            directives: [],
+                        },
+                    },
+                },
+            },
+        ),
+    },
+}
+ExecutableDocument {
+    sources: {
+        1: SourceFile {
+            path: "built_in.graphql",
+            source_text: include_str!("built_in.graphql"),
+        },
+        43: SourceFile {
+            path: "0042_used_variable_in_operation_directive.graphql",
+            source_text: "directive @x(y: Int!) on QUERY | MUTATION | SUBSCRIPTION\ndirective @z(f: Boolean) on FRAGMENT_DEFINITION\n\ntype Query { field: Boolean }\ntype Mutation { field: Boolean }\ntype Subscription { field: Boolean }\n\nquery Q ($a: Int!, $f: Boolean) @x(y: $a) { field ...f }\nmutation M ($b: Int!) @x(y: $b) { field }\nsubscription S ($c: Int!) @x(y: $c) { field }\n\nfragment f on Query @z(f: $f) { field }\n",
+        },
+    },
+    operations: OperationMap {
+        anonymous: None,
+        named: {
+            "Q": 207..263 @43 Operation {
+                operation_type: Query,
+                name: Some(
+                    "Q",
+                ),
+                variables: [
+                    216..224 @43 VariableDefinition {
+                        name: "a",
+                        ty: 220..224 @43 NonNullNamed(
+                            "Int",
+                        ),
+                        default_value: None,
+                        directives: [],
+                    },
+                    226..237 @43 VariableDefinition {
+                        name: "f",
+                        ty: 230..237 @43 Named(
+                            "Boolean",
+                        ),
+                        default_value: None,
+                        directives: [],
+                    },
+                ],
+                directives: [
+                    239..248 @43 Directive {
+                        name: "x",
+                        arguments: [
+                            242..247 @43 Argument {
+                                name: "y",
+                                value: 245..247 @43 Variable(
+                                    "a",
+                                ),
+                            },
+                        ],
+                    },
+                ],
+                selection_set: SelectionSet {
+                    ty: "Query",
+                    selections: [
+                        Field(
+                            251..256 @43 Field {
+                                definition: 119..133 @43 FieldDefinition {
+                                    description: None,
+                                    name: "field",
+                                    arguments: [],
+                                    ty: Named(
+                                        "Boolean",
+                                    ),
+                                    directives: [],
+                                },
+                                alias: None,
+                                name: "field",
+                                arguments: [],
+                                directives: [],
+                                selection_set: SelectionSet {
+                                    ty: "Boolean",
+                                    selections: [],
+                                },
+                            },
+                        ),
+                        FragmentSpread(
+                            257..261 @43 FragmentSpread {
+                                fragment_name: "f",
+                                directives: [],
+                            },
+                        ),
+                    ],
+                },
+            },
+            "M": 264..305 @43 Operation {
+                operation_type: Mutation,
+                name: Some(
+                    "M",
+                ),
+                variables: [
+                    276..284 @43 VariableDefinition {
+                        name: "b",
+                        ty: 280..284 @43 NonNullNamed(
+                            "Int",
+                        ),
+                        default_value: None,
+                        directives: [],
+                    },
+                ],
+                directives: [
+                    286..295 @43 Directive {
+                        name: "x",
+                        arguments: [
+                            289..294 @43 Argument {
+                                name: "y",
+                                value: 292..294 @43 Variable(
+                                    "b",
+                                ),
+                            },
+                        ],
+                    },
+                ],
+                selection_set: SelectionSet {
+                    ty: "Mutation",
+                    selections: [
+                        Field(
+                            298..303 @43 Field {
+                                definition: 152..166 @43 FieldDefinition {
+                                    description: None,
+                                    name: "field",
+                                    arguments: [],
+                                    ty: Named(
+                                        "Boolean",
+                                    ),
+                                    directives: [],
+                                },
+                                alias: None,
+                                name: "field",
+                                arguments: [],
+                                directives: [],
+                                selection_set: SelectionSet {
+                                    ty: "Boolean",
+                                    selections: [],
+                                },
+                            },
+                        ),
+                    ],
+                },
+            },
+            "S": 306..351 @43 Operation {
+                operation_type: Subscription,
+                name: Some(
+                    "S",
+                ),
+                variables: [
+                    322..330 @43 VariableDefinition {
+                        name: "c",
+                        ty: 326..330 @43 NonNullNamed(
+                            "Int",
+                        ),
+                        default_value: None,
+                        directives: [],
+                    },
+                ],
+                directives: [
+                    332..341 @43 Directive {
+                        name: "x",
+                        arguments: [
+                            335..340 @43 Argument {
+                                name: "y",
+                                value: 338..340 @43 Variable(
+                                    "c",
+                                ),
+                            },
+                        ],
+                    },
+                ],
+                selection_set: SelectionSet {
+                    ty: "Subscription",
+                    selections: [
+                        Field(
+                            344..349 @43 Field {
+                                definition: 189..203 @43 FieldDefinition {
+                                    description: None,
+                                    name: "field",
+                                    arguments: [],
+                                    ty: Named(
+                                        "Boolean",
+                                    ),
+                                    directives: [],
+                                },
+                                alias: None,
+                                name: "field",
+                                arguments: [],
+                                directives: [],
+                                selection_set: SelectionSet {
+                                    ty: "Boolean",
+                                    selections: [],
+                                },
+                            },
+                        ),
+                    ],
+                },
+            },
+        },
+    },
+    fragments: {
+        "f": 353..392 @43 Fragment {
+            name: "f",
+            directives: [
+                373..382 @43 Directive {
+                    name: "z",
+                    arguments: [
+                        376..381 @43 Argument {
+                            name: "f",
+                            value: 379..381 @43 Variable(
+                                "f",
+                            ),
+                        },
+                    ],
+                },
+            ],
+            selection_set: SelectionSet {
+                ty: "Query",
+                selections: [
+                    Field(
+                        385..390 @43 Field {
+                            definition: 119..133 @43 FieldDefinition {
+                                description: None,
+                                name: "field",
+                                arguments: [],
+                                ty: Named(
+                                    "Boolean",
+                                ),
+                                directives: [],
+                            },
+                            alias: None,
+                            name: "field",
+                            arguments: [],
+                            directives: [],
+                            selection_set: SelectionSet {
+                                ty: "Boolean",
+                                selections: [],
+                            },
+                        },
+                    ),
+                ],
+            },
+        },
+    },
+}
diff --git a/crates/apollo-compiler/test_data/ok/0115_interface_definition_with_extension_defines_field.txt b/crates/apollo-compiler/test_data/ok/0115_interface_definition_with_extension_defines_field.txt
index e99eb33..9bc9f02 100644
--- a/crates/apollo-compiler/test_data/ok/0115_interface_definition_with_extension_defines_field.txt
+++ b/crates/apollo-compiler/test_data/ok/0115_interface_definition_with_extension_defines_field.txt
@@ -4,7 +4,7 @@ Schema {
             path: "built_in.graphql",
             source_text: include_str!("built_in.graphql"),
         },
-        43: SourceFile {
+        44: SourceFile {
             path: "0115_interface_definition_with_extension_defines_field.graphql",
             source_text: "type Query {\n    foo: Node\n}\n\ninterface Node\n\nextend interface Node {\n    bar: String\n}\n",
         },
@@ -42,7 +42,7 @@ Schema {
         "Boolean": built_in_type!("Boolean"),
         "ID": built_in_type!("ID"),
         "Query": Object(
-            0..28 @43 ObjectType {
+            0..28 @44 ObjectType {
                 description: None,
                 name: "Query",
                 implements_interfaces: {},
@@ -50,7 +50,7 @@ Schema {
                 fields: {
                     "foo": Component {
                         origin: Definition,
-                        node: 17..26 @43 FieldDefinition {
+                        node: 17..26 @44 FieldDefinition {
                             description: None,
                             name: "foo",
                             arguments: [],
@@ -64,7 +64,7 @@ Schema {
             },
         ),
         "Node": Interface(
-            30..44 @43 InterfaceType {
+            30..44 @44 InterfaceType {
                 description: None,
                 name: "Node",
                 implements_interfaces: {},
@@ -74,11 +74,11 @@ Schema {
                         origin: Extension(
                             ExtensionId {
                                 arc: Some(
-                                    46..87 @43,
+                                    46..87 @44,
                                 ),
                             },
                         ),
-                        node: 74..85 @43 FieldDefinition {
+                        node: 74..85 @44 FieldDefinition {
                             description: None,
                             name: "bar",
                             arguments: [],
@@ -99,7 +99,7 @@ ExecutableDocument {
             path: "built_in.graphql",
             source_text: include_str!("built_in.graphql"),
         },
-        43: SourceFile {
+        44: SourceFile {
             path: "0115_interface_definition_with_extension_defines_field.graphql",
             source_text: "type Query {\n    foo: Node\n}\n\ninterface Node\n\nextend interface Node {\n    bar: String\n}\n",
         },
diff --git a/crates/apollo-compiler/test_data/serializer/diagnostics/0009_operation_with_undefined_variables_in_fragment.graphql b/crates/apollo-compiler/test_data/serializer/diagnostics/0009_operation_with_undefined_variables_in_fragment.graphql
index 748b5a8..647ea42 100644
--- a/crates/apollo-compiler/test_data/serializer/diagnostics/0009_operation_with_undefined_variables_in_fragment.graphql
+++ b/crates/apollo-compiler/test_data/serializer/diagnostics/0009_operation_with_undefined_variables_in_fragment.graphql
@@ -1,11 +1,15 @@
-query ExampleQuery($variable: Int) {
+directive @track(label: String) on FRAGMENT_DEFINITION
+
+directive @report(label: String) on QUERY | MUTATION | SUBSCRIPTION
+
+query ExampleQuery($variable: Int) @report(label: $queryLabel) {
   topProducts(first: $variable) {
     name
   }
   ...subFrag
 }
 
-fragment subFrag on Query {
+fragment subFrag on Query @track(label: $productsLabel) {
   topProducts(first: $variable) {
     price(setPrice: $value)
   }
diff --git a/crates/apollo-compiler/test_data/serializer/ok/0042_used_variable_in_operation_directive.graphql b/crates/apollo-compiler/test_data/serializer/ok/0042_used_variable_in_operation_directive.graphql
new file mode 100644
index 0000000..b6ea980
--- /dev/null
+++ b/crates/apollo-compiler/test_data/serializer/ok/0042_used_variable_in_operation_directive.graphql
@@ -0,0 +1,32 @@
+directive @x(y: Int!) on QUERY | MUTATION | SUBSCRIPTION
+
+directive @z(f: Boolean) on FRAGMENT_DEFINITION
+
+type Query {
+  field: Boolean
+}
+
+type Mutation {
+  field: Boolean
+}
+
+type Subscription {
+  field: Boolean
+}
+
+query Q($a: Int!, $f: Boolean) @x(y: $a) {
+  field
+  ...f
+}
+
+mutation M($b: Int!) @x(y: $b) {
+  field
+}
+
+subscription S($c: Int!) @x(y: $c) {
+  field
+}
+
+fragment f on Query @z(f: $f) {
+  field
+}