From dd166a3f28b961ec4da398827d600ebdbaf1dfdb Mon Sep 17 00:00:00 2001 From: BuildTools <19464008+zaksabeast@users.noreply.github.com> Date: Fri, 20 Nov 2020 20:07:13 -0600 Subject: [PATCH 1/6] Add descriptive value validation errors --- juniper/src/executor_tests/enums.rs | 2 +- juniper/src/executor_tests/variables.rs | 4 +- juniper/src/types/utilities.rs | 164 +++++++++++++++--- .../rules/arguments_of_correct_type.rs | 101 ++++++----- .../rules/default_values_of_correct_type.rs | 51 ++++-- 5 files changed, 238 insertions(+), 84 deletions(-) diff --git a/juniper/src/executor_tests/enums.rs b/juniper/src/executor_tests/enums.rs index bdd7ddcf5..ca50d2ba1 100644 --- a/juniper/src/executor_tests/enums.rs +++ b/juniper/src/executor_tests/enums.rs @@ -99,7 +99,7 @@ async fn does_not_accept_string_literals() { assert_eq!( error, ValidationError(vec![RuleError::new( - r#"Invalid value for argument "color", expected type "Color!""#, + r#"Error for argument "color": Invalid value ""RED"" for enum "Color""#, &[SourcePosition::new(18, 0, 18)], )]) ); diff --git a/juniper/src/executor_tests/variables.rs b/juniper/src/executor_tests/variables.rs index 76f73e014..09ff860e0 100644 --- a/juniper/src/executor_tests/variables.rs +++ b/juniper/src/executor_tests/variables.rs @@ -1038,7 +1038,7 @@ async fn does_not_allow_missing_required_field() { assert_eq!( error, ValidationError(vec![RuleError::new( - r#"Invalid value for argument "arg", expected type "ExampleInputObject!""#, + r#"Error for argument "arg": "ExampleInputObject" is missing fields: "b""#, &[SourcePosition::new(20, 0, 20)], )]) ); @@ -1062,7 +1062,7 @@ async fn does_not_allow_null_in_required_field() { assert_eq!( error, ValidationError(vec![RuleError::new( - r#"Invalid value for argument "arg", expected type "ExampleInputObject!""#, + r#"Error for argument "arg": Error on "ExampleInputObject" field "b": Type "Int!" is not nullable"#, &[SourcePosition::new(20, 0, 20)], )]) ); diff --git a/juniper/src/types/utilities.rs b/juniper/src/types/utilities.rs index 74271e77d..a1cda091c 100644 --- a/juniper/src/types/utilities.rs +++ b/juniper/src/types/utilities.rs @@ -1,34 +1,132 @@ use crate::{ ast::InputValue, schema::{ - meta::{EnumMeta, InputObjectMeta, MetaType}, + meta::{Argument, EnumMeta, InputObjectMeta, MetaType}, model::{SchemaType, TypeType}, }, value::ScalarValue, }; -use std::collections::HashSet; +use std::{collections::HashSet, fmt::Display, iter::Iterator}; -pub fn is_valid_literal_value( +pub fn non_null_error_message(arg_type: T) -> String +where + T: Display, +{ + format!("Type \"{}\" is not nullable", arg_type) +} + +pub fn enum_error_message(arg_value: T, arg_type: U) -> String +where + T: Display, + U: Display, +{ + format!("Invalid value \"{}\" for enum \"{}\"", arg_value, arg_type) +} + +pub fn type_error_message(arg_value: T, arg_type: U) -> String +where + T: Display, + U: Display, +{ + format!("Invalid value \"{}\" for type \"{}\"", arg_value, arg_type) +} + +pub fn parser_error_message(arg_type: T) -> String +where + T: Display, +{ + format!("Parser error for \"{}\"", arg_type) +} + +pub fn input_object_error_message(arg_type: T) -> String +where + T: Display, +{ + format!("\"{}\" is not an input object", arg_type) +} + +pub fn field_error_message(arg_type: T, field_name: U, error_message: &str) -> String +where + T: Display, + U: Display, +{ + format!( + "Error on \"{}\" field \"{}\": {}", + arg_type, field_name, error_message + ) +} + +pub fn missing_field_error_message(arg_type: T, missing_fields: U) -> String +where + T: Display, + U: Display, +{ + format!("\"{}\" is missing fields: {}", arg_type, missing_fields) +} + +pub fn unknown_field_error_message(arg_type: T, field_name: U) -> String +where + T: Display, + U: Display, +{ + format!( + "Field \"{}\" does not exist on type \"{}\"", + field_name, arg_type + ) +} + +/// Returns an error string if the field is invalid +fn validate_object_field( + schema: &SchemaType, + object_type: &TypeType, + object_fields: &Vec>, + field_value: &InputValue, + field_key: &str, +) -> Option +where + S: ScalarValue, +{ + let field_type = object_fields + .iter() + .filter(|f| f.name == field_key) + .map(|f| schema.make_type(&f.arg_type)) + .next(); + + if let Some(field_arg_type) = field_type { + let error_message = validate_literal_value(schema, &field_arg_type, field_value); + + if let Some(error_message) = error_message { + Some(field_error_message(object_type, field_key, &error_message)) + } else { + None + } + } else { + Some(unknown_field_error_message(object_type, field_key)) + } +} + +/// Returns an error string if the value is invalid +pub fn validate_literal_value( schema: &SchemaType, arg_type: &TypeType, arg_value: &InputValue, -) -> bool +) -> Option where S: ScalarValue, { match *arg_type { TypeType::NonNull(ref inner) => { if arg_value.is_null() { - false + Some(non_null_error_message(arg_type)) } else { - is_valid_literal_value(schema, inner, arg_value) + validate_literal_value(schema, inner, arg_value) } } TypeType::List(ref inner) => match *arg_value { InputValue::List(ref items) => items .iter() - .all(|i| is_valid_literal_value(schema, inner, &i.item)), - ref v => is_valid_literal_value(schema, inner, v), + .find_map(|i| validate_literal_value(schema, inner, &i.item)), + ref v => validate_literal_value(schema, inner, v), }, TypeType::Concrete(t) => { // Even though InputValue::String can be parsed into an enum, they @@ -36,19 +134,23 @@ where if let (&InputValue::Scalar(_), Some(&MetaType::Enum(EnumMeta { .. }))) = (arg_value, arg_type.to_concrete()) { - return false; + return Some(enum_error_message(arg_value, arg_type)); } match *arg_value { - InputValue::Null | InputValue::Variable(_) => true, + InputValue::Null | InputValue::Variable(_) => None, ref v @ InputValue::Scalar(_) | ref v @ InputValue::Enum(_) => { if let Some(parse_fn) = t.input_value_parse_fn() { - parse_fn(v) + if parse_fn(v) { + None + } else { + Some(type_error_message(arg_value, arg_type)) + } } else { - false + Some(parser_error_message(arg_type)) } } - InputValue::List(_) => false, + InputValue::List(_) => Some("Input lists are not literals".to_owned()), InputValue::Object(ref obj) => { if let MetaType::InputObject(InputObjectMeta { ref input_fields, .. @@ -65,23 +167,33 @@ where }) .collect::>(); - let all_types_ok = obj.iter().all(|&(ref key, ref value)| { + let error_message = obj.iter().find_map(|&(ref key, ref value)| { remaining_required_fields.remove(&key.item); - if let Some(ref arg_type) = input_fields - .iter() - .filter(|f| f.name == key.item) - .map(|f| schema.make_type(&f.arg_type)) - .next() - { - is_valid_literal_value(schema, arg_type, &value.item) - } else { - false - } + validate_object_field( + schema, + arg_type, + input_fields, + &value.item, + &key.item, + ) }); - all_types_ok && remaining_required_fields.is_empty() + if error_message.is_some() { + return error_message; + } + + if remaining_required_fields.is_empty() { + None + } else { + let missing_fields = remaining_required_fields + .into_iter() + .map(|s| format!("\"{}\"", &**s)) + .collect::>() + .join(", "); + Some(missing_field_error_message(arg_type, missing_fields)) + } } else { - false + Some(input_object_error_message(arg_type)) } } } diff --git a/juniper/src/validation/rules/arguments_of_correct_type.rs b/juniper/src/validation/rules/arguments_of_correct_type.rs index 495ca4f7c..552431b02 100644 --- a/juniper/src/validation/rules/arguments_of_correct_type.rs +++ b/juniper/src/validation/rules/arguments_of_correct_type.rs @@ -2,7 +2,7 @@ use crate::{ ast::{Directive, Field, InputValue}, parser::Spanning, schema::meta::Argument, - types::utilities::is_valid_literal_value, + types::utilities::validate_literal_value, validation::{ValidatorContext, Visitor}, value::ScalarValue, }; @@ -56,10 +56,11 @@ where .and_then(|args| args.iter().find(|a| a.name == arg_name.item)) { let meta_type = ctx.schema.make_type(&argument_meta.arg_type); + let validity_error = validate_literal_value(ctx.schema, &meta_type, &arg_value.item); - if !is_valid_literal_value(ctx.schema, &meta_type, &arg_value.item) { + if let Some(validity_error) = validity_error { ctx.report_error( - &error_message(arg_name.item, &format!("{}", argument_meta.arg_type)), + &error_message(arg_name.item, &validity_error), &[arg_value.start], ); } @@ -67,11 +68,8 @@ where } } -fn error_message(arg_name: &str, type_name: &str) -> String { - format!( - "Invalid value for argument \"{}\", expected type \"{}\"", - arg_name, type_name - ) +fn error_message(arg_name: &str, error_message: &str) -> String { + format!("Error for argument \"{}\": {}", arg_name, error_message) } #[cfg(test)] @@ -80,6 +78,10 @@ mod tests { use crate::{ parser::SourcePosition, + types::utilities::{ + enum_error_message, field_error_message, missing_field_error_message, + non_null_error_message, type_error_message, unknown_field_error_message, + }, validation::{expect_fails_rule, expect_passes_rule, RuleError}, value::DefaultScalarValue, }; @@ -110,7 +112,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("nonNullIntArg", "Int!"), + &error_message("nonNullIntArg", &non_null_error_message("Int!")), &[SourcePosition::new(97, 3, 50)], )], ); @@ -240,7 +242,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("stringArg", "String"), + &error_message("stringArg", &type_error_message("1", "String")), &[SourcePosition::new(89, 3, 42)], )], ); @@ -258,7 +260,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("stringArg", "String"), + &error_message("stringArg", &type_error_message("1", "String")), &[SourcePosition::new(89, 3, 42)], )], ); @@ -276,7 +278,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("stringArg", "String"), + &error_message("stringArg", &type_error_message("true", "String")), &[SourcePosition::new(89, 3, 42)], )], ); @@ -294,7 +296,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("stringArg", "String"), + &error_message("stringArg", &type_error_message("BAR", "String")), &[SourcePosition::new(89, 3, 42)], )], ); @@ -312,7 +314,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("intArg", "Int"), + &error_message("intArg", &type_error_message("\"3\"", "Int")), &[SourcePosition::new(83, 3, 36)], )], ); @@ -330,7 +332,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("intArg", "Int"), + &error_message("intArg", &type_error_message("FOO", "Int")), &[SourcePosition::new(83, 3, 36)], )], ); @@ -348,7 +350,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("intArg", "Int"), + &error_message("intArg", &type_error_message("3", "Int")), &[SourcePosition::new(83, 3, 36)], )], ); @@ -366,7 +368,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("intArg", "Int"), + &error_message("intArg", &type_error_message("3.333", "Int")), &[SourcePosition::new(83, 3, 36)], )], ); @@ -384,7 +386,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("floatArg", "Float"), + &error_message("floatArg", &type_error_message("\"3.333\"", "Float")), &[SourcePosition::new(87, 3, 40)], )], ); @@ -402,7 +404,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("floatArg", "Float"), + &error_message("floatArg", &type_error_message("true", "Float")), &[SourcePosition::new(87, 3, 40)], )], ); @@ -420,7 +422,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("floatArg", "Float"), + &error_message("floatArg", &type_error_message("FOO", "Float")), &[SourcePosition::new(87, 3, 40)], )], ); @@ -438,7 +440,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("booleanArg", "Boolean"), + &error_message("booleanArg", &type_error_message("2", "Boolean")), &[SourcePosition::new(91, 3, 44)], )], ); @@ -456,7 +458,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("booleanArg", "Boolean"), + &error_message("booleanArg", &type_error_message("1", "Boolean")), &[SourcePosition::new(91, 3, 44)], )], ); @@ -474,7 +476,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("booleanArg", "Boolean"), + &error_message("booleanArg", &type_error_message("\"true\"", "Boolean")), &[SourcePosition::new(91, 3, 44)], )], ); @@ -492,7 +494,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("booleanArg", "Boolean"), + &error_message("booleanArg", &type_error_message("TRUE", "Boolean")), &[SourcePosition::new(91, 3, 44)], )], ); @@ -510,7 +512,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("idArg", "ID"), + &error_message("idArg", &type_error_message("1", "ID")), &[SourcePosition::new(81, 3, 34)], )], ); @@ -528,7 +530,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("idArg", "ID"), + &error_message("idArg", &type_error_message("true", "ID")), &[SourcePosition::new(81, 3, 34)], )], ); @@ -546,7 +548,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("idArg", "ID"), + &error_message("idArg", &type_error_message("SOMETHING", "ID")), &[SourcePosition::new(81, 3, 34)], )], ); @@ -564,7 +566,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("dogCommand", "DogCommand"), + &error_message("dogCommand", &enum_error_message("2", "DogCommand")), &[SourcePosition::new(79, 3, 44)], )], ); @@ -582,7 +584,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("dogCommand", "DogCommand"), + &error_message("dogCommand", &enum_error_message("1", "DogCommand")), &[SourcePosition::new(79, 3, 44)], )], ); @@ -600,7 +602,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("dogCommand", "DogCommand"), + &error_message("dogCommand", &enum_error_message("\"SIT\"", "DogCommand")), &[SourcePosition::new(79, 3, 44)], )], ); @@ -618,7 +620,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("dogCommand", "DogCommand"), + &error_message("dogCommand", &enum_error_message("true", "DogCommand")), &[SourcePosition::new(79, 3, 44)], )], ); @@ -636,7 +638,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("dogCommand", "DogCommand"), + &error_message("dogCommand", &type_error_message("JUGGLE", "DogCommand")), &[SourcePosition::new(79, 3, 44)], )], ); @@ -654,7 +656,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("dogCommand", "DogCommand"), + &error_message("dogCommand", &type_error_message("sit", "DogCommand")), &[SourcePosition::new(79, 3, 44)], )], ); @@ -714,7 +716,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("stringListArg", "[String]"), + &error_message("stringListArg", &type_error_message("2", "String")), &[SourcePosition::new(97, 3, 50)], )], ); @@ -732,7 +734,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("stringListArg", "[String]"), + &error_message("stringListArg", &type_error_message("1", "String")), &[SourcePosition::new(97, 3, 50)], )], ); @@ -891,11 +893,11 @@ mod tests { "#, &[ RuleError::new( - &error_message("req2", "Int!"), + &error_message("req2", &type_error_message("\"two\"", "Int")), &[SourcePosition::new(82, 3, 35)], ), RuleError::new( - &error_message("req1", "Int!"), + &error_message("req1", &type_error_message("\"one\"", "Int")), &[SourcePosition::new(95, 3, 48)], ), ], @@ -914,7 +916,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("req1", "Int!"), + &error_message("req1", &type_error_message("\"one\"", "Int")), &[SourcePosition::new(82, 3, 35)], )], ); @@ -1028,7 +1030,10 @@ mod tests { } "#, &[RuleError::new( - &error_message("complexArg", "ComplexInput"), + &error_message( + "complexArg", + &missing_field_error_message("ComplexInput", "\"requiredField\""), + ), &[SourcePosition::new(91, 3, 44)], )], ); @@ -1049,7 +1054,14 @@ mod tests { } "#, &[RuleError::new( - &error_message("complexArg", "ComplexInput"), + &error_message( + "complexArg", + &field_error_message( + "ComplexInput", + "stringListField", + &type_error_message("2", "String"), + ), + ), &[SourcePosition::new(91, 3, 44)], )], ); @@ -1070,7 +1082,10 @@ mod tests { } "#, &[RuleError::new( - &error_message("complexArg", "ComplexInput"), + &error_message( + "complexArg", + &unknown_field_error_message("ComplexInput", "unknownField"), + ), &[SourcePosition::new(91, 3, 44)], )], ); @@ -1106,11 +1121,11 @@ mod tests { "#, &[ RuleError::new( - &error_message("if", "Boolean!"), + &error_message("if", &type_error_message("\"yes\"", "Boolean")), &[SourcePosition::new(38, 2, 27)], ), RuleError::new( - &error_message("if", "Boolean!"), + &error_message("if", &type_error_message("ENUM", "Boolean")), &[SourcePosition::new(74, 3, 27)], ), ], 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 d3f1ab6a9..6cd8d2686 100644 --- a/juniper/src/validation/rules/default_values_of_correct_type.rs +++ b/juniper/src/validation/rules/default_values_of_correct_type.rs @@ -1,7 +1,7 @@ use crate::{ ast::VariableDefinition, parser::Spanning, - types::utilities::is_valid_literal_value, + types::utilities::validate_literal_value, validation::{ValidatorContext, Visitor}, value::ScalarValue, }; @@ -35,9 +35,15 @@ where } else { let meta_type = ctx.schema.make_type(&var_def.var_type.item); - if !is_valid_literal_value(ctx.schema, &meta_type, var_value) { + if let Some(error_message) = + validate_literal_value(ctx.schema, &meta_type, var_value) + { ctx.report_error( - &type_error_message(var_name.item, &format!("{}", var_def.var_type.item)), + &type_error_message( + var_name.item, + &format!("{}", var_def.var_type.item), + &error_message, + ), &[*start], ); } @@ -46,16 +52,16 @@ where } } -fn type_error_message(arg_name: &str, type_name: &str) -> String { +fn type_error_message(arg_name: &str, type_name: &str, reason: &str) -> String { format!( - "Invalid default value for argument \"{}\", expected type \"{}\"", - arg_name, type_name + "Invalid default value for argument \"{}\", expected type \"{}\". Reason: {}", + arg_name, type_name, reason ) } fn non_null_error_message(arg_name: &str, type_name: &str) -> String { format!( - "Argument \"{}\" has type \"{}\" and is not nullable, so it't can't have a default value", + "Argument \"{}\" has type \"{}\" and is not nullable, so it can't have a default value", arg_name, type_name ) } @@ -66,6 +72,7 @@ mod tests { use crate::{ parser::SourcePosition, + types::utilities, validation::{expect_fails_rule, expect_passes_rule, RuleError}, value::DefaultScalarValue, }; @@ -147,15 +154,27 @@ mod tests { "#, &[ RuleError::new( - &type_error_message("a", "Int"), + &type_error_message( + "a", + "Int", + &utilities::type_error_message("\"one\"", "Int"), + ), &[SourcePosition::new(61, 2, 22)], ), RuleError::new( - &type_error_message("b", "String"), + &type_error_message( + "b", + "String", + &utilities::type_error_message("4", "String"), + ), &[SourcePosition::new(93, 3, 25)], ), RuleError::new( - &type_error_message("c", "ComplexInput"), + &type_error_message( + "c", + "ComplexInput", + &utilities::type_error_message("\"notverycomplex\"", "ComplexInput"), + ), &[SourcePosition::new(127, 4, 31)], ), ], @@ -172,7 +191,11 @@ mod tests { } "#, &[RuleError::new( - &type_error_message("a", "ComplexInput"), + &type_error_message( + "a", + "ComplexInput", + &utilities::missing_field_error_message("ComplexInput", "\"requiredField\""), + ), &[SourcePosition::new(57, 1, 56)], )], ); @@ -188,7 +211,11 @@ mod tests { } "#, &[RuleError::new( - &type_error_message("a", "[String]"), + &type_error_message( + "a", + "[String]", + &utilities::type_error_message("2", "String"), + ), &[SourcePosition::new(44, 1, 43)], )], ); From ad5ee43d67ad06ba4c7ae731c01c94c25adeb4b0 Mon Sep 17 00:00:00 2001 From: tyranron Date: Thu, 30 Nov 2023 18:51:41 +0100 Subject: [PATCH 2/6] Reorganize --- juniper/src/executor_tests/enums.rs | 2 +- juniper/src/executor_tests/variables.rs | 7 +- juniper/src/types/utilities.rs | 87 +++++++------- .../rules/arguments_of_correct_type.rs | 83 +++++++------ .../rules/default_values_of_correct_type.rs | 110 ++++++++---------- 5 files changed, 136 insertions(+), 153 deletions(-) diff --git a/juniper/src/executor_tests/enums.rs b/juniper/src/executor_tests/enums.rs index d705f84ce..1b1d730a8 100644 --- a/juniper/src/executor_tests/enums.rs +++ b/juniper/src/executor_tests/enums.rs @@ -100,7 +100,7 @@ async fn does_not_accept_string_literals() { assert_eq!( error, ValidationError(vec![RuleError::new( - r#"Error for argument "color": Invalid value ""RED"" for enum "Color""#, + r#"Invalid value for argument "color", reason: Invalid value ""RED"" for enum "Color""#, &[SourcePosition::new(18, 0, 18)], )]) ); diff --git a/juniper/src/executor_tests/variables.rs b/juniper/src/executor_tests/variables.rs index 54e8e77b8..c0ad27a9a 100644 --- a/juniper/src/executor_tests/variables.rs +++ b/juniper/src/executor_tests/variables.rs @@ -916,7 +916,8 @@ async fn does_not_allow_missing_required_field() { assert_eq!( error, ValidationError(vec![RuleError::new( - r#"Error for argument "arg": "ExampleInputObject" is missing fields: "b""#, + "Invalid value for argument \"arg\", \ + reason: \"ExampleInputObject\" is missing fields: \"b\"", &[SourcePosition::new(20, 0, 20)], )]), ); @@ -940,7 +941,9 @@ async fn does_not_allow_null_in_required_field() { assert_eq!( error, ValidationError(vec![RuleError::new( - r#"Error for argument "arg": Error on "ExampleInputObject" field "b": Type "Int!" is not nullable"#, + "Invalid value for argument \"arg\", \ + reason: Error on \"ExampleInputObject\" field \"b\": \ + \"null\" specified for not nullable type \"Int!\"", &[SourcePosition::new(20, 0, 20)], )]), ); diff --git a/juniper/src/types/utilities.rs b/juniper/src/types/utilities.rs index 51c294596..29ca1b679 100644 --- a/juniper/src/types/utilities.rs +++ b/juniper/src/types/utilities.rs @@ -1,4 +1,4 @@ -use std::{collections::HashSet, fmt::Display, iter::Iterator}; +use std::{collections::HashSet, iter::Iterator}; use crate::{ ast::InputValue, @@ -9,46 +9,45 @@ use crate::{ value::ScalarValue, }; -pub(crate) fn non_null_error_message(arg_type: impl Display) -> String { - format!("Type \"{arg_type}\" is not nullable") -} +/// Common error messages used in validation and execution of GraphQL operations +pub(crate) mod error { + use std::fmt::Display; -pub(crate) fn enum_error_message(arg_value: impl Display, arg_type: impl Display) -> String { - format!("Invalid value \"{arg_value}\" for enum \"{arg_type}\"") -} + pub(crate) fn non_null(arg_type: impl Display) -> String { + format!("\"null\" specified for not nullable type \"{arg_type}\"") + } -pub(crate) fn type_error_message(arg_value: impl Display, arg_type: impl Display) -> String { - format!("Invalid value \"{arg_value}\" for type \"{arg_type}\"") -} + pub(crate) fn enum_value(arg_value: impl Display, arg_type: impl Display) -> String { + format!("Invalid value \"{arg_value}\" for enum \"{arg_type}\"") + } -pub(crate) fn parser_error_message(arg_type: impl Display) -> String { - format!("Parser error for \"{arg_type}\"") -} + pub(crate) fn type_value(arg_value: impl Display, arg_type: impl Display) -> String { + format!("Invalid value \"{arg_value}\" for type \"{arg_type}\"") + } -pub(crate) fn input_object_error_message(arg_type: impl Display) -> String { - format!("\"{arg_type}\" is not an input object") -} + pub(crate) fn parser(arg_type: impl Display, msg: impl Display) -> String { + format!("Parser error for \"{arg_type}\": {msg}") + } -pub(crate) fn field_error_message( - arg_type: impl Display, - field_name: impl Display, - error_message: impl Display, -) -> String { - format!("Error on \"{arg_type}\" field \"{field_name}\": {error_message}") -} + pub(crate) fn not_input_object(arg_type: impl Display) -> String { + format!("\"{arg_type}\" is not an input object") + } -pub(crate) fn missing_field_error_message( - arg_type: impl Display, - missing_fields: impl Display, -) -> String { - format!("\"{arg_type}\" is missing fields: {missing_fields}") -} + pub(crate) fn field( + arg_type: impl Display, + field_name: impl Display, + error_message: impl Display, + ) -> String { + format!("Error on \"{arg_type}\" field \"{field_name}\": {error_message}") + } -pub(crate) fn unknown_field_error_message( - arg_type: impl Display, - field_name: impl Display, -) -> String { - format!("Field \"{field_name}\" does not exist on type \"{arg_type}\"") + pub(crate) fn missing_fields(arg_type: impl Display, missing_fields: impl Display) -> String { + format!("\"{arg_type}\" is missing fields: {missing_fields}") + } + + pub(crate) fn unknown_field(arg_type: impl Display, field_name: impl Display) -> String { + format!("Field \"{field_name}\" does not exist on type \"{arg_type}\"") + } } /// Returns an error string if the field is invalid @@ -71,13 +70,9 @@ where if let Some(field_arg_type) = field_type { let error_message = validate_literal_value(schema, &field_arg_type, field_value); - if let Some(error_message) = error_message { - Some(field_error_message(object_type, field_key, &error_message)) - } else { - None - } + error_message.map(|m| error::field(object_type, field_key, m)) } else { - Some(unknown_field_error_message(object_type, field_key)) + Some(error::unknown_field(object_type, field_key)) } } @@ -93,7 +88,7 @@ where match *arg_type { TypeType::NonNull(ref inner) => { if arg_value.is_null() { - Some(non_null_error_message(arg_type)) + Some(error::non_null(arg_type)) } else { validate_literal_value(schema, inner, arg_value) } @@ -125,7 +120,7 @@ where if let (&InputValue::Scalar(_), Some(&MetaType::Enum(EnumMeta { .. }))) = (arg_value, arg_type.to_concrete()) { - return Some(enum_error_message(arg_value, arg_type)); + return Some(error::enum_value(arg_value, arg_type)); } match *arg_value { @@ -136,10 +131,10 @@ where // TODO: reuse error? None } else { - Some(type_error_message(arg_value, arg_type)) + Some(error::type_value(arg_value, arg_type)) } } else { - Some(parser_error_message(arg_type)) + Some(error::parser(arg_type, "Not an input type")) } } InputValue::List(_) => Some("Input lists are not literals".to_owned()), @@ -179,10 +174,10 @@ where .map(|s| format!("\"{}\"", &**s)) .collect::>() .join(", "); - Some(missing_field_error_message(arg_type, missing_fields)) + Some(error::missing_fields(arg_type, missing_fields)) } } else { - Some(input_object_error_message(arg_type)) + Some(error::not_input_object(arg_type)) } } } diff --git a/juniper/src/validation/rules/arguments_of_correct_type.rs b/juniper/src/validation/rules/arguments_of_correct_type.rs index 5fc820d8f..55b71b715 100644 --- a/juniper/src/validation/rules/arguments_of_correct_type.rs +++ b/juniper/src/validation/rules/arguments_of_correct_type.rs @@ -66,7 +66,7 @@ where } fn error_message(arg_name: impl fmt::Display, msg: impl fmt::Display) -> String { - format!("Invalid value for argument \"{arg_name}\": {msg}") + format!("Invalid value for argument \"{arg_name}\", reason: {msg}") } #[cfg(test)] @@ -75,10 +75,7 @@ mod tests { use crate::{ parser::SourcePosition, - types::utilities::{ - enum_error_message, field_error_message, missing_field_error_message, - non_null_error_message, type_error_message, unknown_field_error_message, - }, + types::utilities::error, validation::{expect_fails_rule, expect_passes_rule, RuleError}, value::DefaultScalarValue, }; @@ -123,7 +120,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("nonNullIntArg", &non_null_error_message("Int!")), + &error_message("nonNullIntArg", error::non_null("Int!")), &[SourcePosition::new(97, 3, 50)], )], ); @@ -139,9 +136,9 @@ mod tests { nonNullStringListArgField(nonNullStringListArg: null) } } - "#, + "#, &[RuleError::new( - &error_message("nonNullStringListArg", "[String!]!"), + &error_message("nonNullStringListArg", error::non_null("[String!]!")), &[SourcePosition::new(111, 3, 64)], )], ); @@ -271,7 +268,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("stringArg", &type_error_message("1", "String")), + &error_message("stringArg", error::type_value("1", "String")), &[SourcePosition::new(89, 3, 42)], )], ); @@ -289,7 +286,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("stringArg", &type_error_message("1", "String")), + &error_message("stringArg", error::type_value("1", "String")), &[SourcePosition::new(89, 3, 42)], )], ); @@ -307,7 +304,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("stringArg", &type_error_message("true", "String")), + &error_message("stringArg", error::type_value("true", "String")), &[SourcePosition::new(89, 3, 42)], )], ); @@ -325,7 +322,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("stringArg", &type_error_message("BAR", "String")), + &error_message("stringArg", error::type_value("BAR", "String")), &[SourcePosition::new(89, 3, 42)], )], ); @@ -343,7 +340,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("intArg", &type_error_message("\"3\"", "Int")), + &error_message("intArg", error::type_value("\"3\"", "Int")), &[SourcePosition::new(83, 3, 36)], )], ); @@ -361,7 +358,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("intArg", &type_error_message("FOO", "Int")), + &error_message("intArg", error::type_value("FOO", "Int")), &[SourcePosition::new(83, 3, 36)], )], ); @@ -379,7 +376,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("intArg", &type_error_message("3", "Int")), + &error_message("intArg", error::type_value("3", "Int")), &[SourcePosition::new(83, 3, 36)], )], ); @@ -397,7 +394,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("intArg", &type_error_message("3.333", "Int")), + &error_message("intArg", error::type_value("3.333", "Int")), &[SourcePosition::new(83, 3, 36)], )], ); @@ -415,7 +412,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("floatArg", &type_error_message("\"3.333\"", "Float")), + &error_message("floatArg", error::type_value("\"3.333\"", "Float")), &[SourcePosition::new(87, 3, 40)], )], ); @@ -433,7 +430,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("floatArg", &type_error_message("true", "Float")), + &error_message("floatArg", error::type_value("true", "Float")), &[SourcePosition::new(87, 3, 40)], )], ); @@ -451,7 +448,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("floatArg", &type_error_message("FOO", "Float")), + &error_message("floatArg", error::type_value("FOO", "Float")), &[SourcePosition::new(87, 3, 40)], )], ); @@ -469,7 +466,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("booleanArg", &type_error_message("2", "Boolean")), + &error_message("booleanArg", error::type_value("2", "Boolean")), &[SourcePosition::new(91, 3, 44)], )], ); @@ -487,7 +484,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("booleanArg", &type_error_message("1", "Boolean")), + &error_message("booleanArg", error::type_value("1", "Boolean")), &[SourcePosition::new(91, 3, 44)], )], ); @@ -505,7 +502,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("booleanArg", &type_error_message("\"true\"", "Boolean")), + &error_message("booleanArg", error::type_value("\"true\"", "Boolean")), &[SourcePosition::new(91, 3, 44)], )], ); @@ -523,7 +520,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("booleanArg", &type_error_message("TRUE", "Boolean")), + &error_message("booleanArg", error::type_value("TRUE", "Boolean")), &[SourcePosition::new(91, 3, 44)], )], ); @@ -541,7 +538,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("idArg", &type_error_message("1", "ID")), + &error_message("idArg", error::type_value("1", "ID")), &[SourcePosition::new(81, 3, 34)], )], ); @@ -559,7 +556,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("idArg", &type_error_message("true", "ID")), + &error_message("idArg", error::type_value("true", "ID")), &[SourcePosition::new(81, 3, 34)], )], ); @@ -577,7 +574,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("idArg", &type_error_message("SOMETHING", "ID")), + &error_message("idArg", error::type_value("SOMETHING", "ID")), &[SourcePosition::new(81, 3, 34)], )], ); @@ -595,7 +592,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("dogCommand", &enum_error_message("2", "DogCommand")), + &error_message("dogCommand", error::enum_value("2", "DogCommand")), &[SourcePosition::new(79, 3, 44)], )], ); @@ -613,7 +610,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("dogCommand", &enum_error_message("1", "DogCommand")), + &error_message("dogCommand", error::enum_value("1", "DogCommand")), &[SourcePosition::new(79, 3, 44)], )], ); @@ -631,7 +628,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("dogCommand", &enum_error_message("\"SIT\"", "DogCommand")), + &error_message("dogCommand", error::enum_value("\"SIT\"", "DogCommand")), &[SourcePosition::new(79, 3, 44)], )], ); @@ -649,7 +646,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("dogCommand", &enum_error_message("true", "DogCommand")), + &error_message("dogCommand", error::enum_value("true", "DogCommand")), &[SourcePosition::new(79, 3, 44)], )], ); @@ -667,7 +664,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("dogCommand", &type_error_message("JUGGLE", "DogCommand")), + &error_message("dogCommand", error::type_value("JUGGLE", "DogCommand")), &[SourcePosition::new(79, 3, 44)], )], ); @@ -685,7 +682,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("dogCommand", &type_error_message("sit", "DogCommand")), + &error_message("dogCommand", error::type_value("sit", "DogCommand")), &[SourcePosition::new(79, 3, 44)], )], ); @@ -745,7 +742,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("stringListArg", &type_error_message("2", "String")), + &error_message("stringListArg", error::type_value("2", "String")), &[SourcePosition::new(97, 3, 50)], )], ); @@ -763,7 +760,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("stringListArg", &type_error_message("1", "String")), + &error_message("stringListArg", error::type_value("1", "String")), &[SourcePosition::new(97, 3, 50)], )], ); @@ -922,11 +919,11 @@ mod tests { "#, &[ RuleError::new( - &error_message("req2", &type_error_message("\"two\"", "Int")), + &error_message("req2", error::type_value("\"two\"", "Int")), &[SourcePosition::new(82, 3, 35)], ), RuleError::new( - &error_message("req1", &type_error_message("\"one\"", "Int")), + &error_message("req1", error::type_value("\"one\"", "Int")), &[SourcePosition::new(95, 3, 48)], ), ], @@ -945,7 +942,7 @@ mod tests { } "#, &[RuleError::new( - &error_message("req1", &type_error_message("\"one\"", "Int")), + &error_message("req1", error::type_value("\"one\"", "Int")), &[SourcePosition::new(82, 3, 35)], )], ); @@ -1061,7 +1058,7 @@ mod tests { &[RuleError::new( &error_message( "complexArg", - &missing_field_error_message("ComplexInput", "\"requiredField\""), + error::missing_fields("ComplexInput", "\"requiredField\""), ), &[SourcePosition::new(91, 3, 44)], )], @@ -1085,10 +1082,10 @@ mod tests { &[RuleError::new( &error_message( "complexArg", - &field_error_message( + error::field( "ComplexInput", "stringListField", - &type_error_message("2", "String"), + error::type_value("2", "String"), ), ), &[SourcePosition::new(91, 3, 44)], @@ -1113,7 +1110,7 @@ mod tests { &[RuleError::new( &error_message( "complexArg", - &unknown_field_error_message("ComplexInput", "unknownField"), + error::unknown_field("ComplexInput", "unknownField"), ), &[SourcePosition::new(91, 3, 44)], )], @@ -1150,11 +1147,11 @@ mod tests { "#, &[ RuleError::new( - &error_message("if", &type_error_message("\"yes\"", "Boolean")), + &error_message("if", error::type_value("\"yes\"", "Boolean")), &[SourcePosition::new(38, 2, 27)], ), RuleError::new( - &error_message("if", &type_error_message("ENUM", "Boolean")), + &error_message("if", error::type_value("ENUM", "Boolean")), &[SourcePosition::new(74, 3, 27)], ), ], 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 b2028a5e2..bc3458972 100644 --- a/juniper/src/validation/rules/default_values_of_correct_type.rs +++ b/juniper/src/validation/rules/default_values_of_correct_type.rs @@ -71,7 +71,7 @@ mod tests { use crate::{ parser::SourcePosition, - types::utilities, + types::utilities::error, validation::{expect_fails_rule, expect_passes_rule, RuleError}, value::DefaultScalarValue, }; @@ -81,10 +81,10 @@ mod tests { expect_passes_rule::<_, _, DefaultScalarValue>( factory, r#" - query NullableValues($a: Int, $b: String, $c: ComplexInput) { - dog { name } - } - "#, + query NullableValues($a: Int, $b: String, $c: ComplexInput) { + dog { name } + } + "#, ); } @@ -93,10 +93,10 @@ mod tests { expect_passes_rule::<_, _, DefaultScalarValue>( factory, r#" - query RequiredValues($a: Int!, $b: String!) { - dog { name } - } - "#, + query RequiredValues($a: Int!, $b: String!) { + dog { name } + } + "#, ); } @@ -105,14 +105,14 @@ mod tests { expect_passes_rule::<_, _, DefaultScalarValue>( factory, r#" - query WithDefaultValues( - $a: Int = 1, - $b: String = "ok", - $c: ComplexInput = { requiredField: true, intField: 3 } - ) { - dog { name } - } - "#, + query WithDefaultValues( + $a: Int = 1, + $b: String = "ok", + $c: ComplexInput = { requiredField: true, intField: 3 } + ) { + dog { name } + } + "#, ); } @@ -121,18 +121,18 @@ mod tests { expect_fails_rule::<_, _, DefaultScalarValue>( factory, r#" - query UnreachableDefaultValues($a: Int! = 3, $b: String! = "default") { - dog { name } - } - "#, + query UnreachableDefaultValues($a: Int! = 3, $b: String! = "default") { + dog { name } + } + "#, &[ RuleError::new( &non_null_error_message("a", "Int!"), - &[SourcePosition::new(53, 1, 52)], + &[SourcePosition::new(55, 1, 54)], ), RuleError::new( &non_null_error_message("b", "String!"), - &[SourcePosition::new(70, 1, 69)], + &[SourcePosition::new(72, 1, 71)], ), ], ); @@ -143,38 +143,30 @@ mod tests { expect_fails_rule::<_, _, DefaultScalarValue>( factory, r#" - query InvalidDefaultValues( - $a: Int = "one", - $b: String = 4, - $c: ComplexInput = "notverycomplex" - ) { - dog { name } - } - "#, + query InvalidDefaultValues( + $a: Int = "one", + $b: String = 4, + $c: ComplexInput = "notverycomplex" + ) { + dog { name } + } + "#, &[ RuleError::new( - &type_error_message( - "a", - "Int", - &utilities::type_error_message("\"one\"", "Int"), - ), - &[SourcePosition::new(61, 2, 22)], + &type_error_message("a", "Int", error::type_value("\"one\"", "Int")), + &[SourcePosition::new(67, 2, 26)], ), RuleError::new( - &type_error_message( - "b", - "String", - &utilities::type_error_message("4", "String"), - ), - &[SourcePosition::new(93, 3, 25)], + &type_error_message("b", "String", error::type_value("4", "String")), + &[SourcePosition::new(103, 3, 29)], ), RuleError::new( &type_error_message( "c", "ComplexInput", - &utilities::type_error_message("\"notverycomplex\"", "ComplexInput"), + error::type_value("\"notverycomplex\"", "ComplexInput"), ), - &[SourcePosition::new(127, 4, 31)], + &[SourcePosition::new(141, 4, 35)], ), ], ); @@ -185,17 +177,17 @@ mod tests { expect_fails_rule::<_, _, DefaultScalarValue>( factory, r#" - query MissingRequiredField($a: ComplexInput = {intField: 3}) { - dog { name } - } - "#, + query MissingRequiredField($a: ComplexInput = {intField: 3}) { + dog { name } + } + "#, &[RuleError::new( &type_error_message( "a", "ComplexInput", - &utilities::missing_field_error_message("ComplexInput", "\"requiredField\""), + error::missing_fields("ComplexInput", "\"requiredField\""), ), - &[SourcePosition::new(57, 1, 56)], + &[SourcePosition::new(59, 1, 58)], )], ); } @@ -205,17 +197,13 @@ mod tests { expect_fails_rule::<_, _, DefaultScalarValue>( factory, r#" - query InvalidItem($a: [String] = ["one", 2]) { - dog { name } - } - "#, + query InvalidItem($a: [String] = ["one", 2]) { + dog { name } + } + "#, &[RuleError::new( - &type_error_message( - "a", - "[String]", - &utilities::type_error_message("2", "String"), - ), - &[SourcePosition::new(44, 1, 43)], + &type_error_message("a", "[String]", error::type_value("2", "String")), + &[SourcePosition::new(46, 1, 45)], )], ); } From b99df67fee9eab24b54a94cd644572f37ac8a8c3 Mon Sep 17 00:00:00 2001 From: tyranron Date: Thu, 30 Nov 2023 19:16:09 +0100 Subject: [PATCH 3/6] Improve --- juniper/src/types/utilities.rs | 20 ++- .../rules/arguments_of_correct_type.rs | 140 +++++++++--------- 2 files changed, 84 insertions(+), 76 deletions(-) diff --git a/juniper/src/types/utilities.rs b/juniper/src/types/utilities.rs index 29ca1b679..19fb44960 100644 --- a/juniper/src/types/utilities.rs +++ b/juniper/src/types/utilities.rs @@ -48,9 +48,18 @@ pub(crate) mod error { pub(crate) fn unknown_field(arg_type: impl Display, field_name: impl Display) -> String { format!("Field \"{field_name}\" does not exist on type \"{arg_type}\"") } + + pub(crate) fn invalid_list_length( + arg_value: impl Display, + actual: usize, + expected: usize, + ) -> String { + format!("Expected list of length {actual}, but \"{arg_value}\" has length \"{expected}\"") + } } -/// Returns an error string if the field is invalid +/// Validates the specified field of a GraphQL object and returns an error message if the field is +/// invalid. fn validate_object_field( schema: &SchemaType, object_type: &TypeType, @@ -76,7 +85,7 @@ where } } -/// Returns an error string if the value is invalid +/// Validates the specified GraphQL literal and returns an error message if the it's invalid. pub fn validate_literal_value( schema: &SchemaType, arg_type: &TypeType, @@ -98,7 +107,7 @@ where InputValue::List(ref items) => { if let Some(expected) = expected_size { if items.len() != expected { - return todo!(); + return Some(error::invalid_list_length(arg_value, items.len(), expected)); } } items @@ -108,7 +117,7 @@ where ref v => { if let Some(expected) = expected_size { if expected != 1 { - return todo!(); + return Some(error::invalid_list_length(arg_value, 1, expected)); } } validate_literal_value(schema, inner, v) @@ -128,13 +137,12 @@ where ref v @ InputValue::Scalar(_) | ref v @ InputValue::Enum(_) => { if let Some(parse_fn) = t.input_value_parse_fn() { if parse_fn(v).is_ok() { - // TODO: reuse error? None } else { Some(error::type_value(arg_value, arg_type)) } } else { - Some(error::parser(arg_type, "Not an input type")) + Some(error::parser(arg_type, "no parser present")) } } InputValue::List(_) => Some("Input lists are not literals".to_owned()), diff --git a/juniper/src/validation/rules/arguments_of_correct_type.rs b/juniper/src/validation/rules/arguments_of_correct_type.rs index 55b71b715..4524cf8d6 100644 --- a/juniper/src/validation/rules/arguments_of_correct_type.rs +++ b/juniper/src/validation/rules/arguments_of_correct_type.rs @@ -90,7 +90,7 @@ mod tests { intArgField(intArg: null) } } - "#, + "#, ); } @@ -104,7 +104,7 @@ mod tests { stringListArgField(stringListArg: null) } } - "#, + "#, ); } @@ -118,7 +118,7 @@ mod tests { nonNullIntArgField(nonNullIntArg: null) } } - "#, + "#, &[RuleError::new( &error_message("nonNullIntArg", error::non_null("Int!")), &[SourcePosition::new(97, 3, 50)], @@ -154,7 +154,7 @@ mod tests { intArgField(intArg: 2) } } - "#, + "#, ); } @@ -168,7 +168,7 @@ mod tests { booleanArgField(booleanArg: true) } } - "#, + "#, ); } @@ -182,7 +182,7 @@ mod tests { stringArgField(stringArg: "foo") } } - "#, + "#, ); } @@ -196,7 +196,7 @@ mod tests { floatArgField(floatArg: 1.1) } } - "#, + "#, ); } @@ -210,7 +210,7 @@ mod tests { floatArgField(floatArg: 1) } } - "#, + "#, ); } @@ -224,7 +224,7 @@ mod tests { idArgField(idArg: 1) } } - "#, + "#, ); } @@ -238,7 +238,7 @@ mod tests { idArgField(idArg: "someIdString") } } - "#, + "#, ); } @@ -252,7 +252,7 @@ mod tests { doesKnowCommand(dogCommand: SIT) } } - "#, + "#, ); } @@ -266,7 +266,7 @@ mod tests { stringArgField(stringArg: 1) } } - "#, + "#, &[RuleError::new( &error_message("stringArg", error::type_value("1", "String")), &[SourcePosition::new(89, 3, 42)], @@ -284,7 +284,7 @@ mod tests { stringArgField(stringArg: 1.0) } } - "#, + "#, &[RuleError::new( &error_message("stringArg", error::type_value("1", "String")), &[SourcePosition::new(89, 3, 42)], @@ -302,7 +302,7 @@ mod tests { stringArgField(stringArg: true) } } - "#, + "#, &[RuleError::new( &error_message("stringArg", error::type_value("true", "String")), &[SourcePosition::new(89, 3, 42)], @@ -320,7 +320,7 @@ mod tests { stringArgField(stringArg: BAR) } } - "#, + "#, &[RuleError::new( &error_message("stringArg", error::type_value("BAR", "String")), &[SourcePosition::new(89, 3, 42)], @@ -338,7 +338,7 @@ mod tests { intArgField(intArg: "3") } } - "#, + "#, &[RuleError::new( &error_message("intArg", error::type_value("\"3\"", "Int")), &[SourcePosition::new(83, 3, 36)], @@ -356,7 +356,7 @@ mod tests { intArgField(intArg: FOO) } } - "#, + "#, &[RuleError::new( &error_message("intArg", error::type_value("FOO", "Int")), &[SourcePosition::new(83, 3, 36)], @@ -374,7 +374,7 @@ mod tests { intArgField(intArg: 3.0) } } - "#, + "#, &[RuleError::new( &error_message("intArg", error::type_value("3", "Int")), &[SourcePosition::new(83, 3, 36)], @@ -392,7 +392,7 @@ mod tests { intArgField(intArg: 3.333) } } - "#, + "#, &[RuleError::new( &error_message("intArg", error::type_value("3.333", "Int")), &[SourcePosition::new(83, 3, 36)], @@ -410,7 +410,7 @@ mod tests { floatArgField(floatArg: "3.333") } } - "#, + "#, &[RuleError::new( &error_message("floatArg", error::type_value("\"3.333\"", "Float")), &[SourcePosition::new(87, 3, 40)], @@ -428,7 +428,7 @@ mod tests { floatArgField(floatArg: true) } } - "#, + "#, &[RuleError::new( &error_message("floatArg", error::type_value("true", "Float")), &[SourcePosition::new(87, 3, 40)], @@ -446,7 +446,7 @@ mod tests { floatArgField(floatArg: FOO) } } - "#, + "#, &[RuleError::new( &error_message("floatArg", error::type_value("FOO", "Float")), &[SourcePosition::new(87, 3, 40)], @@ -464,7 +464,7 @@ mod tests { booleanArgField(booleanArg: 2) } } - "#, + "#, &[RuleError::new( &error_message("booleanArg", error::type_value("2", "Boolean")), &[SourcePosition::new(91, 3, 44)], @@ -482,7 +482,7 @@ mod tests { booleanArgField(booleanArg: 1.0) } } - "#, + "#, &[RuleError::new( &error_message("booleanArg", error::type_value("1", "Boolean")), &[SourcePosition::new(91, 3, 44)], @@ -500,7 +500,7 @@ mod tests { booleanArgField(booleanArg: "true") } } - "#, + "#, &[RuleError::new( &error_message("booleanArg", error::type_value("\"true\"", "Boolean")), &[SourcePosition::new(91, 3, 44)], @@ -518,7 +518,7 @@ mod tests { booleanArgField(booleanArg: TRUE) } } - "#, + "#, &[RuleError::new( &error_message("booleanArg", error::type_value("TRUE", "Boolean")), &[SourcePosition::new(91, 3, 44)], @@ -536,7 +536,7 @@ mod tests { idArgField(idArg: 1.0) } } - "#, + "#, &[RuleError::new( &error_message("idArg", error::type_value("1", "ID")), &[SourcePosition::new(81, 3, 34)], @@ -554,7 +554,7 @@ mod tests { idArgField(idArg: true) } } - "#, + "#, &[RuleError::new( &error_message("idArg", error::type_value("true", "ID")), &[SourcePosition::new(81, 3, 34)], @@ -572,7 +572,7 @@ mod tests { idArgField(idArg: SOMETHING) } } - "#, + "#, &[RuleError::new( &error_message("idArg", error::type_value("SOMETHING", "ID")), &[SourcePosition::new(81, 3, 34)], @@ -590,7 +590,7 @@ mod tests { doesKnowCommand(dogCommand: 2) } } - "#, + "#, &[RuleError::new( &error_message("dogCommand", error::enum_value("2", "DogCommand")), &[SourcePosition::new(79, 3, 44)], @@ -608,7 +608,7 @@ mod tests { doesKnowCommand(dogCommand: 1.0) } } - "#, + "#, &[RuleError::new( &error_message("dogCommand", error::enum_value("1", "DogCommand")), &[SourcePosition::new(79, 3, 44)], @@ -626,7 +626,7 @@ mod tests { doesKnowCommand(dogCommand: "SIT") } } - "#, + "#, &[RuleError::new( &error_message("dogCommand", error::enum_value("\"SIT\"", "DogCommand")), &[SourcePosition::new(79, 3, 44)], @@ -644,7 +644,7 @@ mod tests { doesKnowCommand(dogCommand: true) } } - "#, + "#, &[RuleError::new( &error_message("dogCommand", error::enum_value("true", "DogCommand")), &[SourcePosition::new(79, 3, 44)], @@ -662,7 +662,7 @@ mod tests { doesKnowCommand(dogCommand: JUGGLE) } } - "#, + "#, &[RuleError::new( &error_message("dogCommand", error::type_value("JUGGLE", "DogCommand")), &[SourcePosition::new(79, 3, 44)], @@ -680,7 +680,7 @@ mod tests { doesKnowCommand(dogCommand: sit) } } - "#, + "#, &[RuleError::new( &error_message("dogCommand", error::type_value("sit", "DogCommand")), &[SourcePosition::new(79, 3, 44)], @@ -698,7 +698,7 @@ mod tests { stringListArgField(stringListArg: ["one", "two"]) } } - "#, + "#, ); } @@ -712,7 +712,7 @@ mod tests { stringListArgField(stringListArg: []) } } - "#, + "#, ); } @@ -726,7 +726,7 @@ mod tests { stringListArgField(stringListArg: "one") } } - "#, + "#, ); } @@ -740,7 +740,7 @@ mod tests { stringListArgField(stringListArg: ["one", 2]) } } - "#, + "#, &[RuleError::new( &error_message("stringListArg", error::type_value("2", "String")), &[SourcePosition::new(97, 3, 50)], @@ -758,7 +758,7 @@ mod tests { stringListArgField(stringListArg: 1) } } - "#, + "#, &[RuleError::new( &error_message("stringListArg", error::type_value("1", "String")), &[SourcePosition::new(97, 3, 50)], @@ -776,7 +776,7 @@ mod tests { isHousetrained(atOtherHomes: true) } } - "#, + "#, ); } @@ -790,7 +790,7 @@ mod tests { isHousetrained } } - "#, + "#, ); } @@ -804,7 +804,7 @@ mod tests { multipleReqs(req1: 1, req2: 2) } } - "#, + "#, ); } @@ -818,7 +818,7 @@ mod tests { multipleReqs(req2: 2, req1: 1) } } - "#, + "#, ); } @@ -832,7 +832,7 @@ mod tests { multipleOpts } } - "#, + "#, ); } @@ -846,7 +846,7 @@ mod tests { multipleOpts(opt1: 1) } } - "#, + "#, ); } @@ -860,7 +860,7 @@ mod tests { multipleOpts(opt2: 1) } } - "#, + "#, ); } @@ -874,7 +874,7 @@ mod tests { multipleOptAndReq(req1: 3, req2: 4) } } - "#, + "#, ); } @@ -888,7 +888,7 @@ mod tests { multipleOptAndReq(req1: 3, req2: 4, opt1: 5) } } - "#, + "#, ); } @@ -902,7 +902,7 @@ mod tests { multipleOptAndReq(req1: 3, req2: 4, opt1: 5, opt2: 6) } } - "#, + "#, ); } @@ -916,7 +916,7 @@ mod tests { multipleReqs(req2: "two", req1: "one") } } - "#, + "#, &[ RuleError::new( &error_message("req2", error::type_value("\"two\"", "Int")), @@ -940,7 +940,7 @@ mod tests { multipleReqs(req1: "one") } } - "#, + "#, &[RuleError::new( &error_message("req1", error::type_value("\"one\"", "Int")), &[SourcePosition::new(82, 3, 35)], @@ -958,7 +958,7 @@ mod tests { complexArgField } } - "#, + "#, ); } @@ -972,7 +972,7 @@ mod tests { complexArgField(complexArg: { requiredField: true }) } } - "#, + "#, ); } @@ -986,7 +986,7 @@ mod tests { complexArgField(complexArg: { requiredField: false }) } } - "#, + "#, ); } @@ -1000,7 +1000,7 @@ mod tests { complexArgField(complexArg: { requiredField: true, intField: 4 }) } } - "#, + "#, ); } @@ -1020,7 +1020,7 @@ mod tests { }) } } - "#, + "#, ); } @@ -1040,7 +1040,7 @@ mod tests { }) } } - "#, + "#, ); } @@ -1054,7 +1054,7 @@ mod tests { complexArgField(complexArg: { intField: 4 }) } } - "#, + "#, &[RuleError::new( &error_message( "complexArg", @@ -1078,7 +1078,7 @@ mod tests { }) } } - "#, + "#, &[RuleError::new( &error_message( "complexArg", @@ -1106,7 +1106,7 @@ mod tests { }) } } - "#, + "#, &[RuleError::new( &error_message( "complexArg", @@ -1130,7 +1130,7 @@ mod tests { name } } - "#, + "#, ); } @@ -1139,20 +1139,20 @@ mod tests { expect_fails_rule::<_, _, DefaultScalarValue>( factory, r#" - { - dog @include(if: "yes") { - name @skip(if: ENUM) - } - } - "#, + { + dog @include(if: "yes") { + name @skip(if: ENUM) + } + } + "#, &[ RuleError::new( &error_message("if", error::type_value("\"yes\"", "Boolean")), - &[SourcePosition::new(38, 2, 27)], + &[SourcePosition::new(46, 2, 31)], ), RuleError::new( &error_message("if", error::type_value("ENUM", "Boolean")), - &[SourcePosition::new(74, 3, 27)], + &[SourcePosition::new(86, 3, 31)], ), ], ); From 761361322ef48cec1f05e0f88480ada03d3e8561 Mon Sep 17 00:00:00 2001 From: tyranron Date: Thu, 30 Nov 2023 19:26:52 +0100 Subject: [PATCH 4/6] Make Clippy happy [run ci] --- juniper/src/types/utilities.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/juniper/src/types/utilities.rs b/juniper/src/types/utilities.rs index 19fb44960..86b92ea7b 100644 --- a/juniper/src/types/utilities.rs +++ b/juniper/src/types/utilities.rs @@ -63,7 +63,7 @@ pub(crate) mod error { fn validate_object_field( schema: &SchemaType, object_type: &TypeType, - object_fields: &Vec>, + object_fields: &[Argument], field_value: &InputValue, field_key: &str, ) -> Option From da5683de3bd43bc6f6214d357fc668300a7e0f74 Mon Sep 17 00:00:00 2001 From: tyranron Date: Thu, 30 Nov 2023 20:06:18 +0100 Subject: [PATCH 5/6] Fix --- juniper/src/types/utilities.rs | 2 +- tests/integration/tests/array.rs | 45 ++++++++++--------- .../tests/codegen_input_object_derive.rs | 3 +- 3 files changed, 28 insertions(+), 22 deletions(-) diff --git a/juniper/src/types/utilities.rs b/juniper/src/types/utilities.rs index 86b92ea7b..a4f5c45b4 100644 --- a/juniper/src/types/utilities.rs +++ b/juniper/src/types/utilities.rs @@ -54,7 +54,7 @@ pub(crate) mod error { actual: usize, expected: usize, ) -> String { - format!("Expected list of length {actual}, but \"{arg_value}\" has length \"{expected}\"") + format!("Expected list of length {expected}, but \"{arg_value}\" has length {actual}") } } diff --git a/tests/integration/tests/array.rs b/tests/integration/tests/array.rs index 1686e5197..4783be1dc 100644 --- a/tests/integration/tests/array.rs +++ b/tests/integration/tests/array.rs @@ -87,11 +87,12 @@ mod as_input_field { let schema = RootNode::new(Query, EmptyMutation::new(), EmptySubscription::new()); let res = juniper::execute(query, None, &schema, &graphql_vars! {}, &()).await; - assert!(res.is_err()); - assert!(res - .unwrap_err() - .to_string() - .contains(r#"Invalid value for argument "input", expected type "Input!""#)); + assert!(res.is_err(), "result succeeded: {res:#?}"); + assert_eq!( + res.unwrap_err().to_string(), + "Invalid value for argument \"input\", reason: Error on \"Input\" field \"two\": \ + Expected list of length 2, but \"[true, true, false]\" has length 3. At 2:30\n", + ); } #[tokio::test] @@ -105,11 +106,12 @@ mod as_input_field { let schema = RootNode::new(Query, EmptyMutation::new(), EmptySubscription::new()); let res = juniper::execute(query, None, &schema, &graphql_vars! {}, &()).await; - assert!(res.is_err()); - assert!(res - .unwrap_err() - .to_string() - .contains(r#"Invalid value for argument "input", expected type "Input!""#)); + assert!(res.is_err(), "result succeeded: {res:#?}"); + assert_eq!( + res.unwrap_err().to_string(), + "Invalid value for argument \"input\", reason: Error on \"Input\" field \"two\": \ + Expected list of length 2, but \"true\" has length 1. At 2:30\n", + ); } #[tokio::test] @@ -178,11 +180,12 @@ mod as_input_argument { let schema = RootNode::new(Query, EmptyMutation::new(), EmptySubscription::new()); let res = juniper::execute(query, None, &schema, &graphql_vars! {}, &()).await; - assert!(res.is_err()); - assert!(res - .unwrap_err() - .to_string() - .contains(r#"Invalid value for argument "input", expected type "[Boolean!]!""#)); + assert!(res.is_err(), "result succeeded: {res:#?}"); + assert_eq!( + res.unwrap_err().to_string(), + "Invalid value for argument \"input\", reason: Expected list of length 2, \ + but \"[true, true, false]\" has length 3. At 2:30\n", + ); } #[tokio::test] @@ -196,11 +199,13 @@ mod as_input_argument { let schema = RootNode::new(Query, EmptyMutation::new(), EmptySubscription::new()); let res = juniper::execute(query, None, &schema, &graphql_vars! {}, &()).await; - assert!(res.is_err()); - assert!(res - .unwrap_err() - .to_string() - .contains(r#"Invalid value for argument "input", expected type "[Boolean!]!""#)); + assert!(res.is_err(), "result succeeded: {res:#?}"); + assert_eq!( + res.unwrap_err().to_string(), + "Invalid value for argument \"input\", reason: Expected list of length 2, \ + but \"true\" has length 1. At 2:30\n", + "invalid error returned", + ); } #[tokio::test] diff --git a/tests/integration/tests/codegen_input_object_derive.rs b/tests/integration/tests/codegen_input_object_derive.rs index fe9bac700..0cff21a4e 100644 --- a/tests/integration/tests/codegen_input_object_derive.rs +++ b/tests/integration/tests/codegen_input_object_derive.rs @@ -189,7 +189,8 @@ mod default_value { assert_eq!( execute(DOC, None, &schema, &graphql_vars! {}, &()).await, Err(GraphQLError::ValidationError(vec![RuleError::new( - "Invalid value for argument \"point\", expected type \"Point2D!\"", + "Invalid value for argument \"point\", reason: Error on \"Point2D\" field \"y\": \ + \"null\" specified for not nullable type \"Float!\"", &[SourcePosition::new(11, 0, 11)], )])) ); From 53aa659e56457b9511aebf30659b0751d829163e Mon Sep 17 00:00:00 2001 From: tyranron Date: Thu, 30 Nov 2023 21:18:49 +0100 Subject: [PATCH 6/6] Mention in CHANGELOG --- juniper/CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/juniper/CHANGELOG.md b/juniper/CHANGELOG.md index 0962c04b3..da0d0c942 100644 --- a/juniper/CHANGELOG.md +++ b/juniper/CHANGELOG.md @@ -78,6 +78,7 @@ All user visible changes to `juniper` crate will be documented in this file. Thi - Made `GraphQLRequest` fields public. ([#750]) - Relaxed [object safety] requirement for `GraphQLValue` and `GraphQLValueAsync` traits. ([ba1ed85b]) - Updated [GraphQL Playground] to 1.7.28 version. ([#1190]) +- Improve validation errors for input values. ([#811], [#693]) ## Fixed @@ -94,8 +95,10 @@ All user visible changes to `juniper` crate will be documented in this file. Thi [#456]: /../../issues/456 [#503]: /../../issues/503 [#528]: /../../issues/528 +[#693]: /../../issues/693 [#750]: /../../issues/750 [#798]: /../../issues/798 +[#811]: /../../pull/811 [#918]: /../../issues/918 [#965]: /../../pull/965 [#966]: /../../pull/966