From 7b695c406cf98c4a5a5c2c9f2d254de92c409602 Mon Sep 17 00:00:00 2001 From: Magnus Hallin Date: Sat, 18 Nov 2017 18:19:11 +0100 Subject: [PATCH] Propagate errors to closest nullable parent field This also fixes a bug where context switching on optional values did not properly work. --- juniper/src/executor.rs | 12 +- juniper/src/executor_tests/executor.rs | 178 ++++++++++++++++++++++--- juniper/src/lib.rs | 3 +- juniper/src/macros/tests/object.rs | 131 ++++++++++++++---- juniper/src/types/base.rs | 29 +++- juniper/src/value.rs | 3 +- 6 files changed, 295 insertions(+), 61 deletions(-) diff --git a/juniper/src/executor.rs b/juniper/src/executor.rs index 9b95217d..f3d1852c 100644 --- a/juniper/src/executor.rs +++ b/juniper/src/executor.rs @@ -200,9 +200,9 @@ impl<'a, T: GraphQLType, C> IntoResolvable<'a, T, C> for (&'a T::Context, T) { } } -impl<'a, T: GraphQLType, C> IntoResolvable<'a, T, C> for Option<(&'a T::Context, T)> { - fn into(self, _: &'a C) -> FieldResult> { - Ok(self) +impl<'a, T: GraphQLType, C> IntoResolvable<'a, Option, C> for Option<(&'a T::Context, T)> { + fn into(self, _: &'a C) -> FieldResult)>> { + Ok(self.map(|(ctx, v)| (ctx, Some(v)))) } } @@ -212,9 +212,9 @@ impl<'a, T: GraphQLType, C> IntoResolvable<'a, T, C> for FieldResult<(&'a T::Con } } -impl<'a, T: GraphQLType, C> IntoResolvable<'a, T, C> for FieldResult> { - fn into(self, _: &'a C) -> FieldResult> { - self +impl<'a, T: GraphQLType, C> IntoResolvable<'a, Option, C> for FieldResult> { + fn into(self, _: &'a C) -> FieldResult)>> { + self.map(|o| o.map(|(ctx, v)| (ctx, Some(v)))) } } diff --git a/juniper/src/executor_tests/executor.rs b/juniper/src/executor_tests/executor.rs index 69d9bbf4..3aa35278 100644 --- a/juniper/src/executor_tests/executor.rs +++ b/juniper/src/executor_tests/executor.rs @@ -297,11 +297,44 @@ mod dynamic_context_switching { } #[test] - fn test_res() { + fn test_res_success() { let schema = RootNode::new(Schema, EmptyMutation::::new()); let doc = r" { first: itemRes(key: 0) { value } + } + "; + + let vars = vec![].into_iter().collect(); + + let ctx = OuterContext { + items: vec![ + (0, InnerContext { value: "First value".to_owned() }), + (1, InnerContext { value: "Second value".to_owned() }), + ].into_iter() + .collect(), + }; + + let (result, errs) = ::execute(doc, None, &schema, &vars, &ctx).expect("Execution failed"); + + assert_eq!(errs, vec![]); + + println!("Result: {:?}", result); + + assert_eq!( + result, + Value::object(vec![ + ("first", Value::object(vec![ + ("value", Value::string("First value")), + ].into_iter().collect())), + ].into_iter().collect())); + } + + #[test] + fn test_res_fail() { + let schema = RootNode::new(Schema, EmptyMutation::::new()); + let doc = r" + { missing: itemRes(key: 2) { value } } "; @@ -320,7 +353,7 @@ mod dynamic_context_switching { assert_eq!(errs, vec![ ExecutionError::new( - SourcePosition::new(70, 3, 12), + SourcePosition::new(25, 2, 12), &["missing"], FieldError::new("Could not find key 2", Value::null()), ), @@ -328,14 +361,7 @@ mod dynamic_context_switching { println!("Result: {:?}", result); - assert_eq!( - result, - Value::object(vec![ - ("first", Value::object(vec![ - ("value", Value::string("First value")), - ].into_iter().collect())), - ("missing", Value::null()), - ].into_iter().collect())); + assert_eq!(result, Value::null()); } #[test] @@ -413,7 +439,7 @@ mod dynamic_context_switching { } } -mod nulls_out_errors { +mod propagates_errors_to_nullable_fields { use value::Value; use schema::model::RootNode; use executor::{ExecutionError, FieldError, FieldResult}; @@ -421,16 +447,23 @@ mod nulls_out_errors { use types::scalars::EmptyMutation; struct Schema; + struct Inner; graphql_object!(Schema: () |&self| { - field sync() -> FieldResult<&str> { Ok("sync") } - field sync_error() -> FieldResult<&str> { Err("Error for syncError")? } + field inner() -> Inner { Inner } + }); + + graphql_object!(Inner: () |&self| { + field nullable_field() -> Option { Some(Inner) } + field non_nullable_field() -> Inner { Inner } + field nullable_error_field() -> FieldResult> { Err("Error for nullableErrorField")? } + field non_nullable_error_field() -> FieldResult<&str> { Err("Error for nonNullableErrorField")? } }); #[test] - fn test() { + fn nullable_first_level() { let schema = RootNode::new(Schema, EmptyMutation::<()>::new()); - let doc = r"{ sync, syncError }"; + let doc = r"{ inner { nullableErrorField } }"; let vars = vec![].into_iter().collect(); @@ -440,18 +473,119 @@ mod nulls_out_errors { assert_eq!( result, - Value::object(vec![ - ("sync", Value::string("sync")), - ("syncError", Value::null()), - ].into_iter().collect())); + graphql_value!({ "inner": { "nullableErrorField": None } })); assert_eq!( errs, vec![ ExecutionError::new( - SourcePosition::new(8, 0, 8), - &["syncError"], - FieldError::new("Error for syncError", Value::null()), + SourcePosition::new(10, 0, 10), + &["inner", "nullableErrorField"], + FieldError::new("Error for nullableErrorField", Value::null()), + ), + ]); + } + + #[test] + fn non_nullable_first_level() { + let schema = RootNode::new(Schema, EmptyMutation::<()>::new()); + let doc = r"{ inner { nonNullableErrorField } }"; + + let vars = vec![].into_iter().collect(); + + let (result, errs) = ::execute(doc, None, &schema, &vars, &()).expect("Execution failed"); + + println!("Result: {:?}", result); + + assert_eq!( + result, + graphql_value!(None)); + + assert_eq!( + errs, + vec![ + ExecutionError::new( + SourcePosition::new(10, 0, 10), + &["inner", "nonNullableErrorField"], + FieldError::new("Error for nonNullableErrorField", Value::null()), + ), + ]); + } + + #[test] + fn nullable_nested_level() { + let schema = RootNode::new(Schema, EmptyMutation::<()>::new()); + let doc = r"{ inner { nullableField { nonNullableErrorField } } }"; + + let vars = vec![].into_iter().collect(); + + let (result, errs) = ::execute(doc, None, &schema, &vars, &()).expect("Execution failed"); + + println!("Result: {:?}", result); + + assert_eq!( + result, + graphql_value!({ "inner": { "nullableField": None } })); + + assert_eq!( + errs, + vec![ + ExecutionError::new( + SourcePosition::new(26, 0, 26), + &["inner", "nullableField", "nonNullableErrorField"], + FieldError::new("Error for nonNullableErrorField", Value::null()), + ), + ]); + } + + #[test] + fn non_nullable_nested_level() { + let schema = RootNode::new(Schema, EmptyMutation::<()>::new()); + let doc = r"{ inner { nonNullableField { nonNullableErrorField } } }"; + + let vars = vec![].into_iter().collect(); + + let (result, errs) = ::execute(doc, None, &schema, &vars, &()).expect("Execution failed"); + + println!("Result: {:?}", result); + + assert_eq!( + result, + graphql_value!(None)); + + assert_eq!( + errs, + vec![ + ExecutionError::new( + SourcePosition::new(29, 0, 29), + &["inner", "nonNullableField", "nonNullableErrorField"], + FieldError::new("Error for nonNullableErrorField", Value::null()), + ), + ]); + } + + #[test] + fn nullable_innermost() { + let schema = RootNode::new(Schema, EmptyMutation::<()>::new()); + let doc = r"{ inner { nonNullableField { nullableErrorField } } }"; + + let vars = vec![].into_iter().collect(); + + let (result, errs) = ::execute(doc, None, &schema, &vars, &()).expect("Execution failed"); + + println!("Result: {:?}", result); + + assert_eq!( + result, + graphql_value!({ "inner": { "nonNullableField": { "nullableErrorField": None } } })); + + assert_eq!( + errs, + vec![ + ExecutionError::new( + SourcePosition::new(29, 0, 29), + &["inner", "nonNullableField", "nullableErrorField"], + FieldError::new("Error for nullableErrorField", Value::null()), ), ]); } diff --git a/juniper/src/lib.rs b/juniper/src/lib.rs index 2ffb8803..da27c845 100644 --- a/juniper/src/lib.rs +++ b/juniper/src/lib.rs @@ -138,11 +138,12 @@ extern crate uuid; use std::borrow::Cow; +#[macro_use] +mod value; #[macro_use] mod macros; mod ast; pub mod parser; -mod value; mod types; mod schema; mod validation; diff --git a/juniper/src/macros/tests/object.rs b/juniper/src/macros/tests/object.rs index 34ebc8e0..e8f58b08 100644 --- a/juniper/src/macros/tests/object.rs +++ b/juniper/src/macros/tests/object.rs @@ -5,6 +5,7 @@ use ast::InputValue; use value::Value; use schema::model::RootNode; use types::scalars::EmptyMutation; +use executor::{FieldResult, Context}; /* @@ -14,6 +15,7 @@ Syntax to validate: * Optional Generics/lifetimes * Custom name vs. default name * Optional commas between items +* Nullable/fallible context switching */ @@ -102,6 +104,22 @@ graphql_object!(CommasOnMeta: () |&self| { field simple() -> i32 { 0 } }); +struct InnerContext; +const INNER_CONTEXT: InnerContext = InnerContext; +impl Context for InnerContext {} + +struct InnerType; +graphql_object!(InnerType: InnerContext |&self| { +}); + +struct CtxSwitcher; +graphql_object!(CtxSwitcher: () |&self| { + field ctx_switch_always() -> (&InnerContext, InnerType) { (&INNER_CONTEXT, InnerType) } + field ctx_switch_opt() -> Option<(&InnerContext, InnerType)> { Some((&INNER_CONTEXT, InnerType)) } + field ctx_switch_res() -> FieldResult<(&InnerContext, InnerType)> { Ok((&INNER_CONTEXT, InnerType)) } + field ctx_switch_res_opt() -> FieldResult> { Ok(Some((&INNER_CONTEXT, InnerType))) } +}); + graphql_object!(<'a> Root: () as "Root" |&self| { field custom_name() -> CustomName { CustomName {} } @@ -114,6 +132,8 @@ graphql_object!(<'a> Root: () as "Root" |&self| { field commas_with_trailing() -> CommasWithTrailing { CommasWithTrailing {} } field commas_on_meta() -> CommasOnMeta { CommasOnMeta {} } + + field ctx_switcher() -> CtxSwitcher { CtxSwitcher {} } }); @@ -128,6 +148,14 @@ where description fields(includeDeprecated: true) { name + type { + kind + name + ofType { + kind + name + } + } } interfaces { name @@ -172,9 +200,10 @@ fn introspect_custom_name() { assert_eq!(object.get("description"), Some(&Value::null())); assert_eq!(object.get("interfaces"), Some(&Value::list(vec![]))); - assert!(fields.contains(&Value::object(vec![ - ("name", Value::string("simple")), - ].into_iter().collect()))); + assert!(fields.contains(&graphql_value!({ + "name": "simple", + "type": { "kind": "NON_NULL", "name": None, "ofType": { "kind": "SCALAR", "name": "Int" } } + }))); }); } @@ -185,9 +214,10 @@ fn introspect_with_lifetime() { assert_eq!(object.get("description"), Some(&Value::null())); assert_eq!(object.get("interfaces"), Some(&Value::list(vec![]))); - assert!(fields.contains(&Value::object(vec![ - ("name", Value::string("simple")), - ].into_iter().collect()))); + assert!(fields.contains(&graphql_value!({ + "name": "simple", + "type": { "kind": "NON_NULL", "name": None, "ofType": { "kind": "SCALAR", "name": "Int" } } + }))); }); } @@ -198,9 +228,10 @@ fn introspect_with_generics() { assert_eq!(object.get("description"), Some(&Value::null())); assert_eq!(object.get("interfaces"), Some(&Value::list(vec![]))); - assert!(fields.contains(&Value::object(vec![ - ("name", Value::string("simple")), - ].into_iter().collect()))); + assert!(fields.contains(&graphql_value!({ + "name": "simple", + "type": { "kind": "NON_NULL", "name": None, "ofType": { "kind": "SCALAR", "name": "Int" } } + }))); }); } @@ -216,9 +247,10 @@ fn introspect_description_first() { ].into_iter().collect()), ]))); - assert!(fields.contains(&Value::object(vec![ - ("name", Value::string("simple")), - ].into_iter().collect()))); + assert!(fields.contains(&graphql_value!({ + "name": "simple", + "type": { "kind": "NON_NULL", "name": None, "ofType": { "kind": "SCALAR", "name": "Int" } } + }))); }); } @@ -234,9 +266,10 @@ fn introspect_fields_first() { ].into_iter().collect()), ]))); - assert!(fields.contains(&Value::object(vec![ - ("name", Value::string("simple")), - ].into_iter().collect()))); + assert!(fields.contains(&graphql_value!({ + "name": "simple", + "type": { "kind": "NON_NULL", "name": None, "ofType": { "kind": "SCALAR", "name": "Int" } } + }))); }); } @@ -252,9 +285,10 @@ fn introspect_interfaces_first() { ].into_iter().collect()), ]))); - assert!(fields.contains(&Value::object(vec![ - ("name", Value::string("simple")), - ].into_iter().collect()))); + assert!(fields.contains(&graphql_value!({ + "name": "simple", + "type": { "kind": "NON_NULL", "name": None, "ofType": { "kind": "SCALAR", "name": "Int" } } + }))); }); } @@ -270,9 +304,10 @@ fn introspect_commas_with_trailing() { ].into_iter().collect()), ]))); - assert!(fields.contains(&Value::object(vec![ - ("name", Value::string("simple")), - ].into_iter().collect()))); + assert!(fields.contains(&graphql_value!({ + "name": "simple", + "type": { "kind": "NON_NULL", "name": None, "ofType": { "kind": "SCALAR", "name": "Int" } } + }))); }); } @@ -288,8 +323,56 @@ fn introspect_commas_on_meta() { ].into_iter().collect()), ]))); - assert!(fields.contains(&Value::object(vec![ - ("name", Value::string("simple")), - ].into_iter().collect()))); + assert!(fields.contains(&graphql_value!({ + "name": "simple", + "type": { "kind": "NON_NULL", "name": None, "ofType": { "kind": "SCALAR", "name": "Int" } } + }))); + }); +} + +#[test] +fn introspect_ctx_switch() { + run_type_info_query("CtxSwitcher", |_, fields| { + assert!(fields.contains(&graphql_value!({ + "name": "ctxSwitchAlways", + "type": { + "kind": "NON_NULL", + "name": None, + "ofType": { + "kind": "OBJECT", + "name": "InnerType", + } + } + }))); + + assert!(fields.contains(&graphql_value!({ + "name": "ctxSwitchOpt", + "type": { + "kind": "OBJECT", + "name": "InnerType", + "ofType": None + } + }))); + + assert!(fields.contains(&graphql_value!({ + "name": "ctxSwitchRes", + "type": { + "kind": "NON_NULL", + "name": None, + "ofType": { + "kind": "OBJECT", + "name": "InnerType", + } + } + }))); + + assert!(fields.contains(&graphql_value!({ + "name": "ctxSwitchResOpt", + "type": { + "kind": "OBJECT", + "name": "InnerType", + "ofType": None + } + }))); }); } diff --git a/juniper/src/types/base.rs b/juniper/src/types/base.rs index 21374f13..b88031e6 100644 --- a/juniper/src/types/base.rs +++ b/juniper/src/types/base.rs @@ -307,8 +307,11 @@ pub trait GraphQLType: Sized { ) -> Value { if let Some(selection_set) = selection_set { let mut result = OrderMap::new(); - resolve_selection_set_into(self, info, selection_set, executor, &mut result); - Value::object(result) + if resolve_selection_set_into(self, info, selection_set, executor, &mut result) { + Value::object(result) + } else { + Value::null() + } } else { panic!("resolve() must be implemented by non-object output types"); } @@ -321,7 +324,7 @@ fn resolve_selection_set_into( selection_set: &[Selection], executor: &Executor, result: &mut OrderMap, -) where +) -> bool where T: GraphQLType, { let meta_type = executor @@ -388,9 +391,15 @@ fn resolve_selection_set_into( ); match field_result { + Ok(Value::Null) if meta_field.field_type.is_non_null() => return false, Ok(v) => merge_key_into(result, response_name, v), Err(e) => { sub_exec.push_error_at(e, start_pos.clone()); + + if meta_field.field_type.is_non_null() { + return false; + } + result.insert((*response_name).to_owned(), Value::null()); } } @@ -406,13 +415,15 @@ fn resolve_selection_set_into( .fragment_by_name(spread.name.item) .expect("Fragment could not be found"); - resolve_selection_set_into( + if !resolve_selection_set_into( instance, info, &fragment.selection_set[..], executor, result, - ); + ) { + return false; + } } Selection::InlineFragment(Spanning { item: ref fragment, @@ -442,17 +453,21 @@ fn resolve_selection_set_into( sub_exec.push_error_at(e, start_pos.clone()); } } else { - resolve_selection_set_into( + if !resolve_selection_set_into( instance, info, &fragment.selection_set[..], &sub_exec, result, - ); + ) { + return false; + } } } } } + + true } fn is_excluded(directives: &Option>>, vars: &Variables) -> bool { diff --git a/juniper/src/value.rs b/juniper/src/value.rs index a186cd25..7c61efba 100644 --- a/juniper/src/value.rs +++ b/juniper/src/value.rs @@ -205,6 +205,7 @@ macro_rules! graphql_value { $( ($key, graphql_value!($val)), )* ].into_iter().collect()) }; + (None) => ($crate::Value::null()); ($e:expr) => ($crate::Value::from($e)) } @@ -251,7 +252,7 @@ mod tests { Value::string("test") ); assert_eq!( - graphql_value!((None as Option)), + graphql_value!(None), Value::null() ); }