diff --git a/juniper/CHANGELOG.md b/juniper/CHANGELOG.md index c725666dd..4314bcda1 100644 --- a/juniper/CHANGELOG.md +++ b/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 diff --git a/juniper/src/executor_tests/variables.rs b/juniper/src/executor_tests/variables.rs index a4feb6afb..3ed4a8e48 100644 --- a/juniper/src/executor_tests/variables.rs +++ b/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( diff --git a/juniper/src/validation/input_value.rs b/juniper/src/validation/input_value.rs index 0616fe40a..e65c5b9ab 100644 --- a/juniper/src/validation/input_value.rs +++ b/juniper/src/validation/input_value.rs @@ -55,6 +55,12 @@ fn validate_var_defs( 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)) { diff --git a/juniper/src/validation/rules/default_values_of_correct_type.rs b/juniper/src/validation/rules/default_values_of_correct_type.rs index 33adf1b63..9dae3f416 100644 --- a/juniper/src/validation/rules/default_values_of_correct_type.rs +++ b/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)], + )], ); } diff --git a/tests/integration/tests/non_null_variable_defaults.rs b/tests/integration/tests/non_null_variable_defaults.rs new file mode 100644 index 000000000..94aaef747 --- /dev/null +++ b/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; + +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![])), + ); +}