Repository: apollographql/apollo-rs (rust)

Issue:
Mandatory variable cannot be used for optional argument since apollo-compiler 0.9.0
## Description

Since the update to apollo-compiler 0.9.0, we have new validation errors that do not seem valid.

### Steps to reproduce

Considering the schema like:
```
type Query {
  category(
    "Queries with specified ID"
    id: String,

    "Queries with specified key"
    key: String): Category
}
```
The following query:
```
          query CategoryByKey($key: String!){
            category(key: $key) {
              id, key, name(locale:"en")
            }
          }
```
with the key as variable is not valid anymore.
It fails with `variable `key` cannot be used for argument `key` as their types mismatch`

Changing the query to:
```
          query CategoryByKey($key: String){
            category(key: $key) {
              id, key, name(locale:"en")
            }
          }
```

works.

### Expected result

Not 100% sure, but a mandatory variable can be used for an optional argument.


Pull request:
fix(compiler): non-nullable variables should be accepted for nullable args
fixes #558 

also noticed a glitch for `null` being provided for a non-nullable input values - those should also error.

Patch:
diff --git a/crates/apollo-compiler/src/validation/value.rs b/crates/apollo-compiler/src/validation/value.rs
index 6706cc3..16a77be 100644
--- 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
index df602cf..acea44d 100644
--- 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)]


Test patch:
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
index a4116c1..8b8eddf 100644
--- 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,8 +1483,8 @@
             file_id: FileId {
                 id: 99,
             },
-            offset: 5071,
-            length: 10,
+            offset: 4955,
+            length: 39,
         },
         labels: [
             Label {
@@ -1492,26 +1492,25 @@
                     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,8 +1522,8 @@
             file_id: FileId {
                 id: 99,
             },
-            offset: 5102,
-            length: 13,
+            offset: 4906,
+            length: 4,
         },
         labels: [
             Label {
@@ -1532,26 +1531,66 @@
                     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
new file mode 100644
index 0000000..96af26d
--- /dev/null
+++ 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
new file mode 100644
index 0000000..7aadec0
--- /dev/null
+++ b/crates/apollo-compiler/test_data/ok/0033_valid_variable_usage.txt
@@ -0,0 +1,105 @@
+- DOCUMENT@0..237
+    - OPERATION_DEFINITION@0..123
+        - OPERATION_TYPE@0..5
+            - query_KW@0..5 "query"
+        - WHITESPACE@5..6 " "
+        - NAME@6..23
+            - IDENT@6..23 "nullableStringArg"
+        - VARIABLE_DEFINITIONS@23..49
+            - L_PAREN@23..24 "("
+            - VARIABLE_DEFINITION@24..48
+                - VARIABLE@24..39
+                    - DOLLAR@24..25 "$"
+                    - NAME@25..39
+                        - IDENT@25..39 "nonNullableVar"
+                - COLON@39..40 ":"
+                - WHITESPACE@40..41 " "
+                - NON_NULL_TYPE@41..48
+                    - NAMED_TYPE@41..47
+                        - NAME@41..47
+                            - IDENT@41..47 "String"
+                    - BANG@47..48 "!"
+            - R_PAREN@48..49 ")"
+        - WHITESPACE@49..50 " "
+        - SELECTION_SET@50..123
+            - L_CURLY@50..51 "{"
+            - WHITESPACE@51..54 "\n  "
+            - FIELD@54..121
+                - NAME@54..63
+                    - IDENT@54..63 "arguments"
+                - WHITESPACE@63..64 " "
+                - SELECTION_SET@64..121
+                    - L_CURLY@64..65 "{"
+                    - WHITESPACE@65..70 "\n    "
+                    - FIELD@70..117
+                        - NAME@70..84
+                            - IDENT@70..84 "nullableString"
+                        - ARGUMENTS@84..117
+                            - L_PAREN@84..85 "("
+                            - ARGUMENT@85..116
+                                - NAME@85..99
+                                    - IDENT@85..99 "nullableString"
+                                - COLON@99..100 ":"
+                                - WHITESPACE@100..101 " "
+                                - VARIABLE@101..116
+                                    - DOLLAR@101..102 "$"
+                                    - NAME@102..116
+                                        - IDENT@102..116 "nonNullableVar"
+                            - R_PAREN@116..117 ")"
+                    - WHITESPACE@117..120 "\n  "
+                    - R_CURLY@120..121 "}"
+            - WHITESPACE@121..122 "\n"
+            - R_CURLY@122..123 "}"
+    - WHITESPACE@123..125 "\n\n"
+    - OBJECT_TYPE_DEFINITION@125..164
+        - type_KW@125..129 "type"
+        - WHITESPACE@129..130 " "
+        - NAME@130..135
+            - IDENT@130..135 "Query"
+        - WHITESPACE@135..136 " "
+        - FIELDS_DEFINITION@136..164
+            - L_CURLY@136..137 "{"
+            - WHITESPACE@137..142 "\n    "
+            - FIELD_DEFINITION@142..162
+                - NAME@142..151
+                    - IDENT@142..151 "arguments"
+                - COLON@151..152 ":"
+                - WHITESPACE@152..153 " "
+                - NAMED_TYPE@153..162
+                    - NAME@153..162
+                        - IDENT@153..162 "Arguments"
+            - WHITESPACE@162..163 "\n"
+            - R_CURLY@163..164 "}"
+    - WHITESPACE@164..166 "\n\n"
+    - OBJECT_TYPE_DEFINITION@166..235
+        - type_KW@166..170 "type"
+        - WHITESPACE@170..171 " "
+        - NAME@171..180
+            - IDENT@171..180 "Arguments"
+        - WHITESPACE@180..181 " "
+        - FIELDS_DEFINITION@181..235
+            - L_CURLY@181..182 "{"
+            - WHITESPACE@182..187 "\n    "
+            - FIELD_DEFINITION@187..233
+                - NAME@187..201
+                    - IDENT@187..201 "nullableString"
+                - ARGUMENTS_DEFINITION@201..225
+                    - L_PAREN@201..202 "("
+                    - INPUT_VALUE_DEFINITION@202..224
+                        - NAME@202..216
+                            - IDENT@202..216 "nullableString"
+                        - COLON@216..217 ":"
+                        - WHITESPACE@217..218 " "
+                        - NAMED_TYPE@218..224
+                            - NAME@218..224
+                                - IDENT@218..224 "String"
+                    - R_PAREN@224..225 ")"
+                - COLON@225..226 ":"
+                - WHITESPACE@226..227 " "
+                - NAMED_TYPE@227..233
+                    - NAME@227..233
+                        - IDENT@227..233 "String"
+            - WHITESPACE@233..234 "\n"
+            - R_CURLY@234..235 "}"
+    - WHITESPACE@235..237 "\n\n"
+recursion limit: 4096, high: 2