Repository: apollographql/apollo-rs
Pull request #883 (2024-07-19 14:45:19)

fix(compiler): fix validation for undefined variables nested in values
Previously, an operation like this would pass validation:
```graphql
query {
  field(listArg: [$undefinedVar])
}
```
It turns out this is not very common as we haven't seen this cause a problem
on hundreds of thousands of operations. But it does happen :)

This moves the UndefinedVariable rule to value validation: we are already
recursing into the value here, and we're actually also already checking
that the variable exists, so this is a simpler place to do it than in
the `variable.rs` module.

This fixes the immediate problem. There are two things that I may be able to
address here before we merge it time willing...
- [ ] We are validating the type of variables passed directly to arguments in `variable.rs`, but the type of a variable used inside a nested value inside `value.rs`, and both ways are different? Is this correct according to spec or should it be changed?
- [ ] There is a TODO in this code that seems valid and this might be a nice time to do it


Patch:
diff --git a/crates/apollo-compiler/src/validation/value.rs b/crates/apollo-compiler/src/validation/value.rs
index 9b8c21a..f2bebe9 100644
--- a/crates/apollo-compiler/src/validation/value.rs
+++ b/crates/apollo-compiler/src/validation/value.rs
@@ -132,34 +132,42 @@ pub(crate) fn value_of_correct_type(
                 unsupported_type(diagnostics, arg_value, ty);
             }
         }
-        ast::Value::Variable(var_name) => match &type_definition {
-            schema::ExtendedType::Scalar(_)
-            | schema::ExtendedType::Enum(_)
-            | schema::ExtendedType::InputObject(_) => {
-                let var_def = var_defs.iter().find(|v| v.name == *var_name);
-                if let Some(var_def) = var_def {
-                    // we don't have the actual variable values here, so just
-                    // compare if two Types are the same
-                    // TODO(@goto-bus-stop) This should use the is_assignable_to check
-                    if var_def.ty.inner_named_type() != ty.inner_named_type() {
-                        unsupported_type(diagnostics, arg_value, ty);
-                    } else if let Some(default_value) = &var_def.default_value {
-                        if var_def.ty.is_non_null() && default_value.is_null() {
-                            unsupported_type(diagnostics, default_value, &var_def.ty)
-                        } else {
-                            value_of_correct_type(
-                                diagnostics,
-                                schema,
-                                &var_def.ty,
-                                default_value,
-                                var_defs,
-                            )
+        ast::Value::Variable(var_name) => {
+            if let Some(var_def) = var_defs.iter().find(|v| v.name == *var_name) {
+                match &type_definition {
+                    schema::ExtendedType::Scalar(_)
+                    | schema::ExtendedType::Enum(_)
+                    | schema::ExtendedType::InputObject(_) => {
+                        // we don't have the actual variable values here, so just
+                        // compare if two Types are the same
+                        // TODO(@goto-bus-stop) This should use the is_assignable_to check
+                        if var_def.ty.inner_named_type() != ty.inner_named_type() {
+                            unsupported_type(diagnostics, arg_value, ty);
+                        } else if let Some(default_value) = &var_def.default_value {
+                            if var_def.ty.is_non_null() && default_value.is_null() {
+                                unsupported_type(diagnostics, default_value, &var_def.ty)
+                            } else {
+                                value_of_correct_type(
+                                    diagnostics,
+                                    schema,
+                                    &var_def.ty,
+                                    default_value,
+                                    var_defs,
+                                )
+                            }
                         }
                     }
+                    _ => unsupported_type(diagnostics, arg_value, ty),
                 }
+            } else {
+                diagnostics.push(
+                    arg_value.location(),
+                    DiagnosticData::UndefinedVariable {
+                        name: var_name.clone(),
+                    },
+                );
             }
-            _ => unsupported_type(diagnostics, arg_value, ty),
-        },
+        }
         ast::Value::Enum(value) => match &type_definition {
             schema::ExtendedType::Scalar(scalar) if !scalar.is_built_in() => {
                 // Accept enum values as input for custom scalars
diff --git a/crates/apollo-compiler/src/validation/variable.rs b/crates/apollo-compiler/src/validation/variable.rs
index 0f9bbf7..c6d9082 100644
--- a/crates/apollo-compiler/src/validation/variable.rs
+++ b/crates/apollo-compiler/src/validation/variable.rs
@@ -240,17 +240,10 @@ pub(crate) fn validate_variable_usage(
                 return Err(());
             }
         } else {
-            diagnostics.push(
-                argument.value.location(),
-                DiagnosticData::UndefinedVariable {
-                    name: var_name.clone(),
-                },
-            );
-            return Err(());
+            // If the variable is not defined, we raise an error in `value.rs`
         }
     }
-    // It's super confusing to produce a diagnostic here if either the
-    // location_ty or variable_ty is missing, so just return Ok(());
+
     Ok(())
 }
 


Test patch:
diff --git a/crates/apollo-compiler/test_data/diagnostics/0007_operation_with_undefined_variables.graphql b/crates/apollo-compiler/test_data/diagnostics/0007_operation_with_undefined_variables.graphql
index fcdc43d..ec51822 100644
--- a/crates/apollo-compiler/test_data/diagnostics/0007_operation_with_undefined_variables.graphql
+++ b/crates/apollo-compiler/test_data/diagnostics/0007_operation_with_undefined_variables.graphql
@@ -1,13 +1,25 @@
 query ExampleQuery {
-  topProducts(first: $undefinedVariable) {
+  topProducts(
+    first: $undefinedVariable
+    filter: {
+      offset: $offset
+      keywords: ["a", $keyword]
+    }
+  ) {
     name
   }
 }
 
+input Filter {
+  keywords: [String!]
+  offset: Int
+  limit: Int
+}
+
 type Query {
-  topProducts(first: Int): Product,
+  topProducts(first: Int, filter: Filter): Product,
 }
 
 type Product {
   name: String
-}
\ No newline at end of file
+}
diff --git a/crates/apollo-compiler/test_data/diagnostics/0007_operation_with_undefined_variables.txt b/crates/apollo-compiler/test_data/diagnostics/0007_operation_with_undefined_variables.txt
index bdd945f..d05c572 100644
--- a/crates/apollo-compiler/test_data/diagnostics/0007_operation_with_undefined_variables.txt
+++ b/crates/apollo-compiler/test_data/diagnostics/0007_operation_with_undefined_variables.txt
@@ -1,8 +1,22 @@
 Error: variable `$undefinedVariable` is not defined
-   ╭─[0007_operation_with_undefined_variables.graphql:2:22]
+   ╭─[0007_operation_with_undefined_variables.graphql:3:12]
    │
- 2 │   topProducts(first: $undefinedVariable) {
-   │                      ─────────┬────────  
-   │                               ╰────────── not found in this scope
+ 3 │     first: $undefinedVariable
+   │            ─────────┬────────  
+   │                     ╰────────── not found in this scope
+───╯
+Error: variable `$offset` is not defined
+   ╭─[0007_operation_with_undefined_variables.graphql:5:7]
+   │
+ 5 │       offset: $offset
+   │       ───────┬───────  
+   │              ╰───────── not found in this scope
+───╯
+Error: variable `$keyword` is not defined
+   ╭─[0007_operation_with_undefined_variables.graphql:6:23]
+   │
+ 6 │       keywords: ["a", $keyword]
+   │                       ────┬───  
+   │                           ╰───── not found in this scope
 ───╯
 
diff --git a/crates/apollo-compiler/test_data/diagnostics/0101_mismatched_variable_usage.graphql b/crates/apollo-compiler/test_data/diagnostics/0101_mismatched_variable_usage.graphql
index 54a0bcd..d94030c 100644
--- a/crates/apollo-compiler/test_data/diagnostics/0101_mismatched_variable_usage.graphql
+++ b/crates/apollo-compiler/test_data/diagnostics/0101_mismatched_variable_usage.graphql
@@ -46,6 +46,12 @@ query doubleNestedFragmentNonNullIntArgField($intArg: Int) {
     }
 }
 
+query intCannotGoIntoBooleanList($intArg: Int) {
+    arguments {
+	nonNullBooleanListField(nonNullBooleanListArg: [$intArg])
+    }
+}
+
 type Query {
     arguments: Arguments
 }
diff --git a/crates/apollo-compiler/test_data/diagnostics/0101_mismatched_variable_usage.txt b/crates/apollo-compiler/test_data/diagnostics/0101_mismatched_variable_usage.txt
index 892fbc2..6231ef2 100644
--- a/crates/apollo-compiler/test_data/diagnostics/0101_mismatched_variable_usage.txt
+++ b/crates/apollo-compiler/test_data/diagnostics/0101_mismatched_variable_usage.txt
@@ -64,4 +64,15 @@ Error: variable `$intArg` of type `Int` cannot be used for argument `nonNullIntA
     │                                              ──────┬─────  
     │                                                    ╰─────── variable `$intArg` of type `Int` is declared here
 ────╯
+Error: expected value of type Boolean, found a variable
+    ╭─[0101_mismatched_variable_usage.graphql:51:50]
+    │
+ 51 │     nonNullBooleanListField(nonNullBooleanListArg: [$intArg])
+    │                                                     ───┬───  
+    │                                                        ╰───── provided value is a variable
+    │ 
+ 63 │     nonNullBooleanListField(nonNullBooleanListArg: [Boolean]!): Boolean
+    │                                                    ─────┬────  
+    │                                                         ╰────── expected type declared here as Boolean
+────╯
 
diff --git a/crates/apollo-compiler/test_data/diagnostics/0111_const_value.graphql b/crates/apollo-compiler/test_data/diagnostics/0111_const_value.graphql
index aa7c3e8..3a893e8 100644
--- a/crates/apollo-compiler/test_data/diagnostics/0111_const_value.graphql
+++ b/crates/apollo-compiler/test_data/diagnostics/0111_const_value.graphql
@@ -1,6 +1,6 @@
 query(
     $var1: Boolean!
-    $var2: Boolean! = $var
+    $var2: Boolean! = $var1
 ) {
     f1 @include(if: $var1)
     f2 @include(if: $var2)
diff --git a/crates/apollo-compiler/test_data/diagnostics/0111_const_value.txt b/crates/apollo-compiler/test_data/diagnostics/0111_const_value.txt
index 8151c30..59c5d16 100644
--- a/crates/apollo-compiler/test_data/diagnostics/0111_const_value.txt
+++ b/crates/apollo-compiler/test_data/diagnostics/0111_const_value.txt
@@ -1,7 +1,7 @@
 Error: syntax error: unexpected variable value in a Const context
    ╭─[0111_const_value.graphql:3:23]
    │
- 3 │     $var2: Boolean! = $var
+ 3 │     $var2: Boolean! = $var1
    │                       ┬  
    │                       ╰── unexpected variable value in a Const context
 ───╯
diff --git a/crates/apollo-compiler/test_data/serializer/diagnostics/0007_operation_with_undefined_variables.graphql b/crates/apollo-compiler/test_data/serializer/diagnostics/0007_operation_with_undefined_variables.graphql
index 8d99f59..5edd764 100644
--- a/crates/apollo-compiler/test_data/serializer/diagnostics/0007_operation_with_undefined_variables.graphql
+++ b/crates/apollo-compiler/test_data/serializer/diagnostics/0007_operation_with_undefined_variables.graphql
@@ -1,11 +1,17 @@
 query ExampleQuery {
-  topProducts(first: $undefinedVariable) {
+  topProducts(first: $undefinedVariable, filter: {offset: $offset, keywords: ["a", $keyword]}) {
     name
   }
 }
 
+input Filter {
+  keywords: [String!]
+  offset: Int
+  limit: Int
+}
+
 type Query {
-  topProducts(first: Int): Product
+  topProducts(first: Int, filter: Filter): Product
 }
 
 type Product {
diff --git a/crates/apollo-compiler/test_data/serializer/diagnostics/0101_mismatched_variable_usage.graphql b/crates/apollo-compiler/test_data/serializer/diagnostics/0101_mismatched_variable_usage.graphql
index 9714550..e3f87e5 100644
--- a/crates/apollo-compiler/test_data/serializer/diagnostics/0101_mismatched_variable_usage.graphql
+++ b/crates/apollo-compiler/test_data/serializer/diagnostics/0101_mismatched_variable_usage.graphql
@@ -46,6 +46,12 @@ query doubleNestedFragmentNonNullIntArgField($intArg: Int) {
   }
 }
 
+query intCannotGoIntoBooleanList($intArg: Int) {
+  arguments {
+    nonNullBooleanListField(nonNullBooleanListArg: [$intArg])
+  }
+}
+
 type Query {
   arguments: Arguments
 }
diff --git a/crates/apollo-compiler/test_data/serializer/diagnostics/0111_const_value.graphql b/crates/apollo-compiler/test_data/serializer/diagnostics/0111_const_value.graphql
index dd1e72c..5526d4d 100644
--- a/crates/apollo-compiler/test_data/serializer/diagnostics/0111_const_value.graphql
+++ b/crates/apollo-compiler/test_data/serializer/diagnostics/0111_const_value.graphql
@@ -1,4 +1,4 @@
-query($var1: Boolean!, $var2: Boolean! = $var) {
+query($var1: Boolean!, $var2: Boolean! = $var1) {
   f1 @include(if: $var1)
   f2 @include(if: $var2)
 }