fix(compiler): non-nullable variables should be accepted for nullable…

… args (#565) * fix(compiler): non-nullable variables should be accepted for nullable arguments
apollographql · May 23, 2023 · 0c7df4a · 0c7df4a
1 parent e260dd9
commit 0c7df4a
Show file tree

Hide file tree

Showing 5 changed files with 207 additions and 43 deletions.
diff --git a/crates/apollo-compiler/src/validation/value.rs b/crates/apollo-compiler/src/validation/value.rs
@@ -142,13 +142,17 @@ pub fn value_of_correct_type(
                         if var_def.ty().name() != type_def.name() {
                             diagnostics.push(unsupported_type!(db, val.clone(), ty));
                         } else if let Some(default_value) = var_def.default_value() {
-                            value_of_correct_type(
-                                db,
-                                var_def.ty(),
-                                default_value,
-                                var_defs.clone(),
-                                diagnostics,
-                            )
+                            if var_def.ty().is_non_null() && default_value.is_null() {
+                                diagnostics.push(unsupported_type!(db, default_value, var_def.ty()))
+                            } else {
+                                value_of_correct_type(
+                                    db,
+                                    var_def.ty(),
+                                    default_value,
+                                    var_defs.clone(),
+                                    diagnostics,
+                                )
+                            }
                         }
                     }
                 }
@@ -223,10 +227,17 @@ pub fn value_of_correct_type(
                     input_obj.fields().for_each(|f| {
                         let ty = f.ty();
                         let is_missing = !obj.iter().any(|(name, ..)| f.name() == name.src());
+                        let is_null = obj
+                            .iter()
+                            .any(|(name, value)| f.name() == name.src() && value.is_null());
 
-                        // If no default value is provided and the input object
-                        // field’s type is non-null, an error should be raised
-                        if (ty.is_non_null() && f.default_value().is_none()) && is_missing {
+                        // If the input object field type is non_null, and no
+                        // default value is provided, or if the value provided
+                        // is null or missing entirely, an error should be
+                        // raised.
+                        if (ty.is_non_null() && f.default_value().is_none())
+                            && (is_missing || is_null)
+                        {
                             let mut diagnostic = ApolloDiagnostic::new(
                                 db,
                                 val.loc().into(),

diff --git a/crates/apollo-compiler/src/validation/variable.rs b/crates/apollo-compiler/src/validation/variable.rs
@@ -231,9 +231,7 @@ fn are_types_compatible(variable_ty: &hir::Type, location_ty: &hir::Type) -> boo
     match (location_ty, variable_ty) {
         // 1. If location_ty is a non-null type:
         // 1.a. If variable_ty is NOT a non-null type, return false.
-        (hir::Type::NonNull { .. }, hir::Type::Named { .. } | hir::Type::List { .. }) => {
-            return false
-        }
+        (hir::Type::NonNull { .. }, hir::Type::Named { .. } | hir::Type::List { .. }) => false,
         // 1.b. Let nullable_location_ty be the unwrapped nullable type of location_ty.
         // 1.c. Let nullable_variable_type be the unwrapped nullable type of variable_ty.
         // 1.d. Return AreTypesCompatible(nullable_variable_ty, nullable_location_ty).
@@ -246,7 +244,7 @@ fn are_types_compatible(variable_ty: &hir::Type, location_ty: &hir::Type) -> boo
                 ty: nullable_variable_ty,
                 ..
             },
-        ) => return are_types_compatible(nullable_variable_ty, nullable_location_ty),
+        ) => are_types_compatible(nullable_variable_ty, nullable_location_ty),
         // 2. Otherwise, if variable_ty is a non-null type:
         // 2.a. Let nullable_variable_ty be the nullable type of variable_ty.
         // 2.b. Return are_types_compatible(nullable_variable_ty, location_ty).
@@ -259,9 +257,7 @@ fn are_types_compatible(variable_ty: &hir::Type, location_ty: &hir::Type) -> boo
         ) => are_types_compatible(nullable_variable_ty, location_ty),
         // 3.Otherwise, if location_ty is a list type:
         // 3.a. If variable_ty is NOT a list type, return false.
-        (hir::Type::List { .. }, hir::Type::Named { .. } | hir::Type::NonNull { .. }) => {
-            return false
-        }
+        (hir::Type::List { .. }, hir::Type::Named { .. } | hir::Type::NonNull { .. }) => false,
         // 3.b.Let item_location_ty be the unwrapped item type of location_ty.
         // 3.c. Let item_variable_ty be the unwrapped item type of variable_ty.
         // 3.d. Return AreTypesCompatible(item_variable_ty, item_location_ty).
@@ -274,15 +270,14 @@ fn are_types_compatible(variable_ty: &hir::Type, location_ty: &hir::Type) -> boo
                 ty: item_variable_ty,
                 ..
             },
-        ) => return are_types_compatible(item_location_ty, item_variable_ty),
+        ) => are_types_compatible(item_location_ty, item_variable_ty),
         // 4. Otherwise, if variable_ty is a list type, return false.
         (hir::Type::Named { .. }, hir::Type::List { .. }) => false,
         // 5. Return true if variable_ty and location_ty are identical, otherwise false.
         (hir::Type::Named { name: loc_name, .. }, hir::Type::Named { name: var_name, .. }) => {
-            return var_name == loc_name
+            var_name == loc_name
         }
-    };
-    false
+    }
 }
 
 #[cfg(test)]

diff --git a/crates/apollo-compiler/test_data/diagnostics/0102_invalid_string_values.txt b/crates/apollo-compiler/test_data/diagnostics/0102_invalid_string_values.txt
@@ -1483,35 +1483,34 @@
             file_id: FileId {
                 id: 99,
             },
-            offset: 5071,
-            length: 10,
+            offset: 4955,
+            length: 39,
         },
         labels: [
             Label {
                 location: DiagnosticLocation {
                     file_id: FileId {
                         id: 99,
                     },
-                    offset: 4895,
-                    length: 15,
+                    offset: 4955,
+                    length: 39,
                 },
-                text: "variable `a` of type `Int!` is declared here",
+                text: "missing value for argument `requiredField`",
             },
             Label {
                 location: DiagnosticLocation {
                     file_id: FileId {
                         id: 99,
                     },
-                    offset: 5071,
-                    length: 10,
+                    offset: 882,
+                    length: 7,
                 },
-                text: "argument `intArg` of type `Int` is declared here",
+                text: "argument defined here",
             },
         ],
         help: None,
-        data: DisallowedVariableUsage {
-            var_name: "a",
-            arg_name: "intArg",
+        data: RequiredArgument {
+            name: "requiredField",
         },
     },
     ApolloDiagnostic {
@@ -1523,35 +1522,75 @@
             file_id: FileId {
                 id: 99,
             },
-            offset: 5102,
-            length: 13,
+            offset: 4906,
+            length: 4,
         },
         labels: [
             Label {
                 location: DiagnosticLocation {
                     file_id: FileId {
                         id: 99,
                     },
-                    offset: 4914,
-                    length: 18,
+                    offset: 4899,
+                    length: 3,
                 },
-                text: "variable `b` of type `String!` is declared here",
+                text: "field declared here as Int type",
             },
             Label {
                 location: DiagnosticLocation {
                     file_id: FileId {
                         id: 99,
                     },
-                    offset: 5102,
-                    length: 13,
+                    offset: 4906,
+                    length: 4,
                 },
-                text: "argument `stringArg` of type `String` is declared here",
+                text: "argument declared here is of Null type",
             },
         ],
         help: None,
-        data: DisallowedVariableUsage {
-            var_name: "b",
-            arg_name: "stringArg",
+        data: UnsupportedValueType {
+            value: "Null",
+            ty: "Int",
+        },
+    },
+    ApolloDiagnostic {
+        cache: {
+            0: "built_in_types.graphql",
+            99: "0102_invalid_string_values.graphql",
+        },
+        location: DiagnosticLocation {
+            file_id: FileId {
+                id: 99,
+            },
+            offset: 4928,
+            length: 4,
+        },
+        labels: [
+            Label {
+                location: DiagnosticLocation {
+                    file_id: FileId {
+                        id: 99,
+                    },
+                    offset: 4918,
+                    length: 6,
+                },
+                text: "field declared here as String type",
+            },
+            Label {
+                location: DiagnosticLocation {
+                    file_id: FileId {
+                        id: 99,
+                    },
+                    offset: 4928,
+                    length: 4,
+                },
+                text: "argument declared here is of Null type",
+            },
+        ],
+        help: None,
+        data: UnsupportedValueType {
+            value: "Null",
+            ty: "String",
         },
     },
     ApolloDiagnostic {

diff --git a/crates/apollo-compiler/test_data/ok/0033_valid_variable_usage.graphql b/crates/apollo-compiler/test_data/ok/0033_valid_variable_usage.graphql
@@ -0,0 +1,14 @@
+query nullableStringArg($nonNullableVar: String!) {
+  arguments {
+    nullableString(nullableString: $nonNullableVar)
+  }
+}
+
+type Query {
+    arguments: Arguments
+}
+
+type Arguments {
+    nullableString(nullableString: String): String
+}
+
diff --git a/crates/apollo-compiler/test_data/ok/0033_valid_variable_usage.txt b/crates/apollo-compiler/test_data/ok/0033_valid_variable_usage.txt
@@ -0,0 +1,105 @@
+- [email protected]
+    - [email protected]
+        - [email protected]
+            - [email protected] "query"
+        - [email protected] " "
+        - [email protected]
+            - [email protected] "nullableStringArg"
+        - [email protected]
+            - [email protected] "("
+            - [email protected]
+                - [email protected]
+                    - [email protected] "$"
+                    - [email protected]
+                        - [email protected] "nonNullableVar"
+                - [email protected] ":"
+                - [email protected] " "
+                - [email protected]
+                    - [email protected]
+                        - [email protected]
+                            - [email protected] "String"
+                    - [email protected] "!"
+            - [email protected] ")"
+        - [email protected] " "
+        - [email protected]
+            - [email protected] "{"
+            - [email protected] "\n  "
+            - [email protected]
+                - [email protected]
+                    - [email protected] "arguments"
+                - [email protected] " "
+                - [email protected]
+                    - [email protected] "{"
+                    - [email protected] "\n    "
+                    - [email protected]
+                        - [email protected]
+                            - [email protected] "nullableString"
+                        - [email protected]
+                            - [email protected] "("
+                            - [email protected]
+                                - [email protected]
+                                    - [email protected] "nullableString"
+                                - [email protected] ":"
+                                - [email protected] " "
+                                - [email protected]
+                                    - [email protected] "$"
+                                    - [email protected]
+                                        - [email protected] "nonNullableVar"
+                            - [email protected] ")"
+                    - [email protected] "\n  "
+                    - [email protected] "}"
+            - [email protected] "\n"
+            - [email protected] "}"
+    - [email protected] "\n\n"
+    - [email protected]
+        - [email protected] "type"
+        - [email protected] " "
+        - [email protected]
+            - [email protected] "Query"
+        - [email protected] " "
+        - [email protected]
+            - [email protected] "{"
+            - [email protected] "\n    "
+            - [email protected]
+                - [email protected]
+                    - [email protected] "arguments"
+                - [email protected] ":"
+                - [email protected] " "
+                - [email protected]
+                    - [email protected]
+                        - [email protected] "Arguments"
+            - [email protected] "\n"
+            - [email protected] "}"
+    - [email protected] "\n\n"
+    - [email protected]
+        - [email protected] "type"
+        - [email protected] " "
+        - [email protected]
+            - [email protected] "Arguments"
+        - [email protected] " "
+        - [email protected]
+            - [email protected] "{"
+            - [email protected] "\n    "
+            - [email protected]
+                - [email protected]
+                    - [email protected] "nullableString"
+                - [email protected]
+                    - [email protected] "("
+                    - [email protected]
+                        - [email protected]
+                            - [email protected] "nullableString"
+                        - [email protected] ":"
+                        - [email protected] " "
+                        - [email protected]
+                            - [email protected]
+                                - [email protected] "String"
+                    - [email protected] ")"
+                - [email protected] ":"
+                - [email protected] " "
+                - [email protected]
+                    - [email protected]
+                        - [email protected] "String"
+            - [email protected] "\n"
+            - [email protected] "}"
+    - [email protected] "\n\n"
+recursion limit: 4096, high: 2