default variable fixes (#1376)

* Allow default values on non-`Null` variables in `DefaultValuesOfCorrectType` validation rule

* Link CHANGELOG entry to PR #1376

* Honor variable default values in `validate_input_values`

Author: jbellenger

juniper/CHANGELOG.md

@@ -64,6 +64,7 @@ All user visible changes to `juniper` crate will be documented in this file. Thi
 - Missing `@specifiedBy(url:)` directive in [SDL] generated by `RootNode::as_sdl()` and `RootNode::as_document()` methods. ([#1348])
 - Incorrect double escaping in `ScalarToken::String` `Display`ing. ([#1349])
 - Memory leak caused by incorrect error handling in `#[graphql_subscription]` macro expansion. ([#1371])
+- Incorrect rejection of default values on non-`Null` variables. ([#1376])
 
 [#864]: /../../issues/864
 [#1055]: /../../issues/1055
@@ -78,6 +79,7 @@ All user visible changes to `juniper` crate will be documented in this file. Thi
 [#1358]: /../../pull/1358
 [#1361]: /../../pull/1361
 [#1371]: /../../pull/1371
+[#1376]: /../../pull/1376
 [graphql/graphql-spec#525]: https://github.com/graphql/graphql-spec/pull/525
 [graphql/graphql-spec#687]: https://github.com/graphql/graphql-spec/issues/687
 [graphql/graphql-spec#805]: https://github.com/graphql/graphql-spec/pull/805

juniper/src/executor_tests/variables.rs

@@ -575,6 +575,61 @@ async fn allow_non_nullable_inputs_to_be_set_to_value_directly() {
     .await;
 }
 
+#[tokio::test]
+async fn default_used_for_non_nullable_variable_when_not_provided() {
+    run_variable_query(
+        r#"query q($value: String! = "fallback") { fieldWithNonNullableStringInput(input: $value) }"#,
+        graphql::vars! {},
+        |result| {
+            assert_eq!(
+                result.get_field_value("fieldWithNonNullableStringInput"),
+                Some(&graphql::value!(r#""fallback""#)),
+            );
+        },
+    )
+    .await;
+}
+
+#[tokio::test]
+async fn provided_value_overrides_non_nullable_variable_default() {
+    run_variable_query(
+        r#"query q($value: String! = "fallback") { fieldWithNonNullableStringInput(input: $value) }"#,
+        graphql::vars! {"value": "override"},
+        |result| {
+            assert_eq!(
+                result.get_field_value("fieldWithNonNullableStringInput"),
+                Some(&graphql::value!(r#""override""#)),
+            );
+        },
+    )
+    .await;
+}
+
+#[tokio::test]
+async fn does_not_allow_null_for_non_nullable_variable_with_default() {
+    let schema = RootNode::new(
+        TestType,
+        EmptyMutation::<()>::new(),
+        EmptySubscription::<()>::new(),
+    );
+
+    let query = r#"query q($value: String! = "fallback") { fieldWithNonNullableStringInput(input: $value) }"#;
+    let vars = graphql::vars! {"value": null};
+
+    let error = crate::execute(query, None, &schema, &vars, &())
+        .await
+        .unwrap_err();
+
+    assert_eq!(
+        error,
+        RuleError::new(
+            r#"Variable "$value" of required type "String!" was not provided."#,
+            &[SourcePosition::new(8, 0, 8)],
+        )
+        .into(),
+    );
+}
+
 #[tokio::test]
 async fn allow_lists_to_be_null() {
     run_variable_query(

juniper/src/validation/input_value.rs

@@ -55,6 +55,12 @@ fn validate_var_defs<S>(
         let raw_type_name = def.var_type.item.innermost_name();
         match schema.concrete_type_by_name(raw_type_name) {
             Some(t) if t.is_input() => {
+                // Spec §6.1.2: if no value is provided and a default value
+                // exists, the default value is used regardless of nullability.
+                if values.get(name.item).is_none() && def.default_value.is_some() {
+                    continue;
+                }
+
                 let ct = schema.make_type(&def.var_type.item);
 
                 if def.var_type.item.is_non_null() && is_absent_or_null(values.get(name.item)) {

juniper/src/validation/rules/default_values_of_correct_type.rs

@@ -28,20 +28,13 @@ where
             ref span,
         }) = var_def.default_value
         {
-            if var_def.var_type.item.is_non_null() {
+            let meta_type = ctx.schema.make_type(&var_def.var_type.item);
+
+            if let Some(err) = validate_literal_value(ctx.schema, &meta_type, var_value) {
                 ctx.report_error(
-                    &non_null_error_message(var_name.item, &var_def.var_type.item),
+                    &type_error_message(var_name.item, &var_def.var_type.item, err),
                     &[span.start],
-                )
-            } else {
-                let meta_type = ctx.schema.make_type(&var_def.var_type.item);
-
-                if let Some(err) = validate_literal_value(ctx.schema, &meta_type, var_value) {
-                    ctx.report_error(
-                        &type_error_message(var_name.item, &var_def.var_type.item, err),
-                        &[span.start],
-                    );
-                }
+                );
             }
         }
     }
@@ -58,16 +51,9 @@ fn type_error_message(
     )
 }
 
-fn non_null_error_message(arg_name: impl fmt::Display, type_name: impl fmt::Display) -> String {
-    format!(
-        "Argument \"{arg_name}\" has type \"{type_name}\" and is not nullable, \
-         so it can't have a default value",
-    )
-}
-
 #[cfg(test)]
 mod tests {
-    use super::{factory, non_null_error_message, type_error_message};
+    use super::{factory, type_error_message};
 
     use crate::{
         parser::SourcePosition,
@@ -117,24 +103,30 @@ mod tests {
     }
 
     #[test]
-    fn no_required_variables_with_default_values() {
+    fn required_variables_with_valid_default_values() {
+        expect_passes_rule::<_, _, DefaultScalarValue>(
+            factory,
+            r#"
+            query RequiredWithDefaults($a: Int! = 3, $b: String! = "default") {
+                dog { name }
+            }
+            "#,
+        );
+    }
+
+    #[test]
+    fn required_variables_with_null_default_value() {
         expect_fails_rule::<_, _, DefaultScalarValue>(
             factory,
             r#"
-            query UnreachableDefaultValues($a: Int! = 3, $b: String! = "default") {
+            query NullDefaultForRequired($a: Int! = null) {
                 dog { name }
             }
             "#,
-            &[
-                RuleError::new(
-                    &non_null_error_message("a", "Int!"),
-                    &[SourcePosition::new(55, 1, 54)],
-                ),
-                RuleError::new(
-                    &non_null_error_message("b", "String!"),
-                    &[SourcePosition::new(72, 1, 71)],
-                ),
-            ],
+            &[RuleError::new(
+                &type_error_message("a", "Int!", error::non_null("Int!")),
+                &[SourcePosition::new(53, 1, 52)],
+            )],
         );
     }
 

tests/integration/tests/non_null_variable_defaults.rs

@@ -0,0 +1,46 @@
+//! Checks that non-`Null` variables may carry a default value, per [§6.1.2]
+//! of the GraphQL spec.
+//! See [#1376](https://github.com/graphql-rust/juniper/pull/1376) for details.
+//!
+//! [§6.1.2]: https://spec.graphql.org/October2021/#sec-Coercing-Variable-Values
+
+use juniper::{
+    EmptyMutation, EmptySubscription, RootNode, graphql_object, graphql_value, graphql_vars,
+};
+
+pub struct Query;
+
+#[graphql_object]
+impl Query {
+    fn hello() -> &'static str {
+        "world"
+    }
+}
+
+type Schema = RootNode<Query, EmptyMutation, EmptySubscription>;
+
+const QUERY: &str = r#"
+    query ($var: Boolean! = true) {
+        __typename @skip(if: $var)
+    }
+"#;
+
+#[tokio::test]
+async fn default_applies_when_variable_not_provided() {
+    let schema = Schema::new(Query, EmptyMutation::new(), EmptySubscription::new());
+
+    assert_eq!(
+        juniper::execute(QUERY, None, &schema, &graphql_vars! {}, &()).await,
+        Ok((graphql_value!({}), vec![])),
+    );
+}
+
+#[tokio::test]
+async fn provided_variable_overrides_default() {
+    let schema = Schema::new(Query, EmptyMutation::new(), EmptySubscription::new());
+
+    assert_eq!(
+        juniper::execute(QUERY, None, &schema, &graphql_vars! {"var": false}, &()).await,
+        Ok((graphql_value!({"__typename": "Query"}), vec![])),
+    );
+}