diff --git a/.changesets/feat_tylerb_add_result_coercion_errors.md b/.changesets/feat_tylerb_add_result_coercion_errors.md new file mode 100644 index 0000000000..a7c0259a9a --- /dev/null +++ b/.changesets/feat_tylerb_add_result_coercion_errors.md @@ -0,0 +1,8 @@ +### Response reformatting and result coercion errors ([PR #8441](https://github.com/apollographql/router/pull/8441)) + +All subgraph responses are checked and corrected to ensure alignment with the +schema and query. When a misaligned value is returned, it is nullified. Now, +when the feature is enabled, an errors for this nullification are included in +the errors array in the response. + +By [@TylerBloom](https://github.com/TylerBloom) in https://github.com/apollographql/router/pull/8441 diff --git a/apollo-router/src/configuration/mod.rs b/apollo-router/src/configuration/mod.rs index 3e4a4018fa..3e7a547a85 100644 --- a/apollo-router/src/configuration/mod.rs +++ b/apollo-router/src/configuration/mod.rs @@ -732,6 +732,14 @@ pub(crate) struct Supergraph { /// but request handling will stop immediately when the client connection is closed. pub(crate) early_cancel: bool, + /// Enable errors generated during response reformatting and result coercion to be returned in + /// responses. + /// Default: false + /// All subgraph responses are checked and corrected to ensure alignment with the schema and + /// query. When enabled, misaligned values will generate errors which are included in errors + /// array in the response. + pub(crate) enable_result_coercion_errors: bool, + /// Log a message if the client closes the connection before the response is sent. /// Default: false. pub(crate) experimental_log_on_broken_pipe: bool, @@ -758,6 +766,7 @@ impl Supergraph { generate_query_fragments: Option, early_cancel: Option, experimental_log_on_broken_pipe: Option, + insert_result_coercion_errors: Option, ) -> Self { Self { listen: listen.unwrap_or_else(default_graphql_listen), @@ -771,6 +780,7 @@ impl Supergraph { .unwrap_or_else(default_generate_query_fragments), early_cancel: early_cancel.unwrap_or_default(), experimental_log_on_broken_pipe: experimental_log_on_broken_pipe.unwrap_or_default(), + enable_result_coercion_errors: insert_result_coercion_errors.unwrap_or_default(), } } } @@ -789,6 +799,7 @@ impl Supergraph { generate_query_fragments: Option, early_cancel: Option, experimental_log_on_broken_pipe: Option, + insert_result_coercion_errors: Option, ) -> Self { Self { listen: listen.unwrap_or_else(test_listen), @@ -802,6 +813,7 @@ impl Supergraph { .unwrap_or_else(default_generate_query_fragments), early_cancel: early_cancel.unwrap_or_default(), experimental_log_on_broken_pipe: experimental_log_on_broken_pipe.unwrap_or_default(), + enable_result_coercion_errors: insert_result_coercion_errors.unwrap_or_default(), } } } diff --git a/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap b/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap index a500e3c050..d51b927227 100644 --- a/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap +++ b/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap @@ -10496,6 +10496,11 @@ expression: "&schema" "description": "abort request handling when the client drops the connection.\nDefault: false.\nWhen set to true, some parts of the request pipeline like telemetry will not work properly,\nbut request handling will stop immediately when the client connection is closed.", "type": "boolean" }, + "enable_result_coercion_errors": { + "default": false, + "description": "Enable errors generated during response reformatting and result coercion to be returned in\nresponses.\nDefault: false\nAll subgraph responses are checked and corrected to ensure alignment with the schema and\nquery. When enabled, misaligned values will generate errors which are included in errors\narray in the response.", + "type": "boolean" + }, "experimental_log_on_broken_pipe": { "default": false, "description": "Log a message if the client closes the connection before the response is sent.\nDefault: false.", @@ -12076,6 +12081,7 @@ expression: "&schema" }, "defer_support": true, "early_cancel": false, + "enable_result_coercion_errors": false, "experimental_log_on_broken_pipe": false, "generate_query_fragments": true, "introspection": false, diff --git a/apollo-router/src/services/execution/service.rs b/apollo-router/src/services/execution/service.rs index 912e949eb7..bfbf720885 100644 --- a/apollo-router/src/services/execution/service.rs +++ b/apollo-router/src/services/execution/service.rs @@ -29,6 +29,7 @@ use tracing::Span; use tracing::event; use tracing_core::Level; +use crate::Configuration; use crate::apollo_studio_interop::ReferencedEnums; use crate::apollo_studio_interop::extract_enums_from_response; use crate::graphql::Error; @@ -64,6 +65,7 @@ pub(crate) struct ExecutionService { pub(crate) schema: Arc, pub(crate) subgraph_schemas: Arc, pub(crate) fetch_service_factory: Arc, + pub(crate) configuration: Arc, /// Subscription config if enabled subscription_config: Option, apollo_telemetry_config: Option, @@ -192,6 +194,8 @@ impl ExecutionService { }; let execution_span = Span::current(); + let insert_result_coercion_errors = + self.configuration.supergraph.enable_result_coercion_errors; let stream = stream .map(move |mut response: Response| { @@ -243,6 +247,7 @@ impl ExecutionService { &mut nullified_paths, metrics_ref_mode, &context, + insert_result_coercion_errors, response, ) })) @@ -261,6 +266,7 @@ impl ExecutionService { nullified_paths: &mut Vec, metrics_ref_mode: ApolloMetricsReferenceMode, context: &crate::Context, + insert_result_coercion_errors: bool, mut response: Response, ) -> Option { // responses that would fall under a path that was previously nullified are not sent @@ -330,6 +336,7 @@ impl ExecutionService { variables.clone(), schema.api_schema(), variables_set, + insert_result_coercion_errors ); } @@ -340,6 +347,7 @@ impl ExecutionService { variables.clone(), schema.api_schema(), variables_set, + insert_result_coercion_errors ) , ); @@ -635,6 +643,7 @@ pub(crate) struct ExecutionServiceFactory { pub(crate) subgraph_schemas: Arc, pub(crate) plugins: Arc, pub(crate) fetch_service_factory: Arc, + pub(crate) configuration: Arc, } impl ServiceFactory for ExecutionServiceFactory { @@ -663,6 +672,7 @@ impl ServiceFactory for ExecutionServiceFactory { subscription_config: subscription_plugin_conf, subgraph_schemas: self.subgraph_schemas.clone(), apollo_telemetry_config: apollo_telemetry_conf, + configuration: Arc::clone(&self.configuration), } .boxed(), |acc, (_, e)| e.execution_service(acc), diff --git a/apollo-router/src/services/supergraph/service.rs b/apollo-router/src/services/supergraph/service.rs index e90bb44958..8116bf5f78 100644 --- a/apollo-router/src/services/supergraph/service.rs +++ b/apollo-router/src/services/supergraph/service.rs @@ -561,6 +561,7 @@ impl PluggableSupergraphServiceBuilder { subgraph_schemas: query_planner_service.subgraph_schemas(), plugins: self.plugins.clone(), fetch_service_factory, + configuration: Arc::clone(&configuration), }; let execution_service: execution::BoxCloneService = ServiceBuilder::new() diff --git a/apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__deferred_fragment_bounds_nullability-2.snap b/apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__deferred_fragment_bounds_nullability-2.snap index cf940ba62d..6a8459af72 100644 --- a/apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__deferred_fragment_bounds_nullability-2.snap +++ b/apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__deferred_fragment_bounds_nullability-2.snap @@ -1,5 +1,6 @@ --- source: apollo-router/src/services/supergraph/tests.rs +assertion_line: 800 expression: stream.next_response().await.unwrap() --- { @@ -31,7 +32,7 @@ expression: stream.next_response().await.unwrap() "extensions": { "valueCompletion": [ { - "message": "Cannot return null for non-nullable field Organization.nonNullId", + "message": "Null value found for non-nullable type ID", "path": [ "currentUser", "activeOrganization", @@ -68,7 +69,7 @@ expression: stream.next_response().await.unwrap() "extensions": { "valueCompletion": [ { - "message": "Cannot return null for non-nullable field Organization.nonNullId", + "message": "Null value found for non-nullable type ID", "path": [ "currentUser", "activeOrganization", @@ -105,7 +106,7 @@ expression: stream.next_response().await.unwrap() "extensions": { "valueCompletion": [ { - "message": "Cannot return null for non-nullable field Organization.nonNullId", + "message": "Null value found for non-nullable type ID", "path": [ "currentUser", "activeOrganization", diff --git a/apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__filter_nullified_deferred_responses-2.snap b/apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__filter_nullified_deferred_responses-2.snap index 9460daa1b0..bfb079ae3e 100644 --- a/apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__filter_nullified_deferred_responses-2.snap +++ b/apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__filter_nullified_deferred_responses-2.snap @@ -1,5 +1,6 @@ --- source: apollo-router/src/services/supergraph/tests.rs +assertion_line: 1679 expression: deferred --- { @@ -15,7 +16,7 @@ expression: deferred "extensions": { "valueCompletion": [ { - "message": "Cannot return null for non-nullable field Organization.nonNullId", + "message": "Null value found for non-nullable type ID", "path": [ "currentUser", "org", diff --git a/apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__nullability_bubbling.snap b/apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__nullability_bubbling.snap index 348a89b2c4..b429d1b28b 100644 --- a/apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__nullability_bubbling.snap +++ b/apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__nullability_bubbling.snap @@ -1,5 +1,6 @@ --- source: apollo-router/src/services/supergraph/tests.rs +assertion_line: 435 expression: response --- { @@ -11,7 +12,7 @@ expression: response "extensions": { "valueCompletion": [ { - "message": "Cannot return null for non-nullable field Organization.nonNullId", + "message": "Null value found for non-nullable type ID", "path": [ "currentUser", "activeOrganization" diff --git a/apollo-router/src/spec/query.rs b/apollo-router/src/spec/query.rs index 8c55f01658..a1c8f16bb7 100644 --- a/apollo-router/src/spec/query.rs +++ b/apollo-router/src/spec/query.rs @@ -8,6 +8,7 @@ use std::collections::HashSet; use std::sync::Arc; use apollo_compiler::ExecutableDocument; +use apollo_compiler::Name; use apollo_compiler::executable; use apollo_compiler::schema::ExtendedType; use derivative::Derivative; @@ -127,6 +128,7 @@ impl Query { variables: Object, schema: &ApiSchema, defer_conditions: BooleanValues, + include_coercion_errors: bool, ) -> Vec { let data = std::mem::take(&mut response.data); @@ -145,6 +147,7 @@ impl Query { variables: &variables, schema, errors: Vec::new(), + coercion_errors: include_coercion_errors.then(Vec::new), nullified: Vec::new(), }; @@ -202,6 +205,7 @@ impl Query { variables: &all_variables, schema, errors: Vec::new(), + coercion_errors: include_coercion_errors.then(Vec::new), nullified: Vec::new(), }; @@ -226,6 +230,12 @@ impl Query { .insert(EXTENSIONS_VALUE_COMPLETION_KEY, value); } + if let Some(errors) = parameters.coercion_errors.as_mut() + && !errors.is_empty() + { + response.errors.append(errors); + } + return parameters.nullified; } } @@ -326,7 +336,6 @@ impl Query { Ok((fragments, operation, defer_stats, hash)) } - #[allow(clippy::too_many_arguments)] fn format_value<'a: 'b, 'b>( &'a self, parameters: &mut FormatParameters, @@ -334,233 +343,358 @@ impl Query { input: &mut Value, output: &mut Value, path: &mut Vec>, - parent_type: &executable::Type, selection_set: &'a [Selection], ) -> Result<(), InvalidValue> { // for every type, if we have an invalid value, we will replace it with null // and return Ok(()), because values are optional by default match field_type { + executable::Type::Named(name) => match name.as_str() { + "Int" => self.format_integer(parameters, path, input, output), + "Float" => self.format_float(parameters, path, input, output), + "Boolean" => self.format_boolean(parameters, path, input, output), + "String" => self.format_string(parameters, path, input, output), + "Id" => self.format_id(parameters, path, input, output), + _ => self.format_named_type( + parameters, + field_type, + input, + name, + output, + path, + selection_set, + )?, + }, + // if the list contains nonnullable types, we will receive a Err(InvalidValue) + // and should replace the entire list with null + // if the types are nullable, the inner call to filter_errors will take care + // of setting the current entry to null + executable::Type::List(inner_type) => { + self.format_list(parameters, input, inner_type, output, path, selection_set)? + } // for non null types, we validate with the inner type, then if we get an InvalidValue // we set it to null and immediately return an error instead of Ok(()), because we // want the error to go up until the next nullable parent - executable::Type::NonNullNamed(_) | executable::Type::NonNullList(_) => { - let inner_type = match field_type { - executable::Type::NonNullList(ty) => ty.clone().list(), - executable::Type::NonNullNamed(name) => executable::Type::Named(name.clone()), - _ => unreachable!(), - }; - match self.format_value( + executable::Type::NonNullNamed(_) | executable::Type::NonNullList(_) => self + .format_non_nullable_value( parameters, - &inner_type, + field_type, input, output, path, - field_type, selection_set, - ) { - Err(_) => Err(InvalidValue), - Ok(_) => { - if output.is_null() { - let message = match path.last() { - Some(ResponsePathElement::Key(k)) => format!( - "Cannot return null for non-nullable field {parent_type}.{k}" - ), - Some(ResponsePathElement::Index(i)) => format!( - "Cannot return null for non-nullable array element of type {inner_type} at index {i}" - ), - _ => todo!(), - }; - parameters.errors.push( - Error::builder() - .message(message) - .path(Path::from_response_slice(path)) - .build(), - ); + )?, + } + Ok(()) + } - Err(InvalidValue) - } else { - Ok(()) - } - } - } + #[inline] + fn format_non_nullable_value<'a: 'b, 'b>( + &'a self, + parameters: &mut FormatParameters, + field_type: &executable::Type, + input: &mut Value, + output: &mut Value, + path: &mut Vec>, + selection_set: &'a [Selection], + ) -> Result<(), InvalidValue> { + let inner_type = match field_type { + executable::Type::NonNullList(ty) => ty.clone().list(), + executable::Type::NonNullNamed(name) => executable::Type::Named(name.clone()), + // This function should never be called for non-nullable types + _ => { + tracing::error!("`format_non_nullable_value` was called with a nullable type!!"); + debug_assert!(field_type.is_non_null()); + return Err(InvalidValue); } + }; - // if the list contains nonnullable types, we will receive a Err(InvalidValue) - // and should replace the entire list with null - // if the types are nullable, the inner call to filter_errors will take care - // of setting the current entry to null - executable::Type::List(inner_type) => match input { - Value::Array(input_array) => { - if output.is_null() { - *output = Value::Array(vec![Value::Null; input_array.len()]); - } - let output_array = output.as_array_mut().ok_or(InvalidValue)?; - match input_array - .iter_mut() - .enumerate() - .try_for_each(|(i, element)| { - path.push(ResponsePathElement::Index(i)); - let res = self.format_value( - parameters, - inner_type, - element, - &mut output_array[i], - path, - field_type, - selection_set, - ); - path.pop(); - res - }) { - Err(InvalidValue) => { - parameters.nullified.push(Path::from_response_slice(path)); - *output = Value::Null; - Ok(()) - } - Ok(()) => Ok(()), - } - } - _ => Ok(()), - }, - executable::Type::Named(name) if name == "Int" => { - let opt = if input.is_i64() { - input.as_i64().and_then(|i| i32::try_from(i).ok()) - } else if input.is_u64() { - input.as_i64().and_then(|i| i32::try_from(i).ok()) - } else { - None - }; + self.format_value(parameters, &inner_type, input, output, path, selection_set)?; + + if output.is_null() { + let message = format!("Null value found for non-nullable type {inner_type}"); + parameters.errors.push( + Error::builder() + .message(&message) + .path(Path::from_response_slice(path)) + .build(), + ); + parameters.insert_coercion_error( + Error::builder() + .message(message) + .path(Path::from_response_slice(path)) + .extension("code", ERROR_CODE_RESPONSE_VALIDATION) + .build(), + ); + + Err(InvalidValue) + } else { + Ok(()) + } + } - // if the value is invalid, we do not insert it in the output object - // which is equivalent to inserting null - if opt.is_some() { - *output = input.clone(); - } else { - *output = Value::Null; - } - Ok(()) + #[inline] + fn format_list<'a: 'b, 'b>( + &'a self, + parameters: &mut FormatParameters, + input: &mut Value, + inner_type: &executable::Type, + output: &mut Value, + path: &mut Vec>, + selection_set: &'a [Selection], + ) -> Result<(), InvalidValue> { + let Value::Array(input_array) = input else { + return Ok(()); + }; + if output.is_null() { + *output = Value::Array(vec![Value::Null; input_array.len()]); + } + let output_array = output.as_array_mut().ok_or(InvalidValue)?; + if let Err(InvalidValue) = + input_array + .iter_mut() + .enumerate() + .try_for_each(|(i, element)| { + path.push(ResponsePathElement::Index(i)); + self.format_value( + parameters, + inner_type, + element, + &mut output_array[i], + path, + selection_set, + )?; + path.pop(); + Ok(()) + }) + { + // We pop here because, if an error is found, the path still contains the index of the + // invalid value. + path.pop(); + parameters.nullified.push(Path::from_response_slice(path)); + parameters.insert_coercion_error( + Error::builder() + .message(format!( + "Invalid value found inside the array of type [{inner_type}]" + )) + .path(Path::from_response_slice(path)) + .extension("code", ERROR_CODE_RESPONSE_VALIDATION) + .build(), + ); + *output = Value::Null; + } + Ok(()) + } + + #[inline] + #[allow(clippy::too_many_arguments)] + fn format_named_type<'a: 'b, 'b>( + &'a self, + parameters: &mut FormatParameters, + field_type: &executable::Type, + input: &mut Value, + type_name: &Name, + output: &mut Value, + path: &mut Vec>, + selection_set: &'a [Selection], + ) -> Result<(), InvalidValue> { + // we cannot know about the expected format of custom scalars + // so we must pass them directly to the client + match parameters.schema.types.get(type_name) { + Some(ExtendedType::Scalar(_)) => { + *output = input.clone(); + return Ok(()); } - executable::Type::Named(name) if name == "Float" => { - if input.as_f64().is_some() { - *output = input.clone(); - } else { - *output = Value::Null; - } - Ok(()) + Some(ExtendedType::Enum(enum_type)) => { + *output = input + .as_str() + .filter(|s| enum_type.values.contains_key(*s)) + .map(|_| input.clone()) + .unwrap_or_default(); + return Ok(()); } - executable::Type::Named(name) if name == "Boolean" => { - if input.as_bool().is_some() { - *output = input.clone(); - } else { + _ => {} + } + + if let Value::Object(input_object) = input { + if let Some(input_type) = input_object.get(TYPENAME).and_then(|val| val.as_str()) { + // If there is a __typename, make sure the pointed type is a valid type of the + // schema. Otherwise, something is wrong, and in case we might be inadvertently + // leaking some data for an @inacessible type or something, nullify the whole + // object. However, do note that due to `@interfaceObject`, some subgraph can have + // returned a __typename that is the name of an interface in the supergraph, and + // this is fine (that is, we should not return such a __typename to the user, but + // as long as it's not returned, having it in the internal data is ok and sometimes + // expected). + let Some(ExtendedType::Object(_) | ExtendedType::Interface(_)) = + parameters.schema.types.get(input_type) + else { + parameters.nullified.push(Path::from_response_slice(path)); *output = Value::Null; - } - Ok(()) + return Ok(()); + }; } - executable::Type::Named(name) if name == "String" => { - if input.as_str().is_some() { - *output = input.clone(); - } else { - *output = Value::Null; - } - Ok(()) + + if output.is_null() { + *output = Value::Object(Object::with_capacity(selection_set.len())); } - executable::Type::Named(name) if name == "Id" => { - if input.is_string() || input.is_i64() || input.is_u64() || input.is_f64() { - *output = input.clone(); - } else { - *output = Value::Null; + let output_object = output.as_object_mut().ok_or(InvalidValue)?; + + let typename = input_object + .get(TYPENAME) + .and_then(|val| val.as_str()) + .and_then(|s| apollo_compiler::ast::NamedType::new(s).ok()) + .map(apollo_compiler::ast::Type::Named); + + let current_type = match parameters.schema.types.get(field_type.inner_named_type()) { + Some(ExtendedType::Interface(..) | ExtendedType::Union(..)) => { + typename.as_ref().unwrap_or(field_type) } - Ok(()) + _ => field_type, + }; + + if self + .apply_selection_set( + selection_set, + parameters, + input_object, + output_object, + path, + current_type, + ) + .is_err() + { + parameters.nullified.push(Path::from_response_slice(path)); + *output = Value::Null; } - executable::Type::Named(type_name) => { - // we cannot know about the expected format of custom scalars - // so we must pass them directly to the client - match parameters.schema.types.get(type_name) { - Some(ExtendedType::Scalar(_)) => { - *output = input.clone(); - return Ok(()); - } - Some(ExtendedType::Enum(enum_type)) => { - return match input.as_str() { - Some(s) => { - if enum_type.values.contains_key(s) { - *output = input.clone(); - Ok(()) - } else { - *output = Value::Null; - Ok(()) - } - } - None => { - *output = Value::Null; - Ok(()) - } - }; - } - _ => {} - } + } else { + parameters.nullified.push(Path::from_response_slice(path)); + *output = Value::Null; + } - match input { - Value::Object(input_object) => { - if let Some(input_type) = - input_object.get(TYPENAME).and_then(|val| val.as_str()) - { - // If there is a __typename, make sure the pointed type is a valid type of the schema. Otherwise, something is wrong, and in case we might - // be inadvertently leaking some data for an @inacessible type or something, nullify the whole object. However, do note that due to `@interfaceObject`, - // some subgraph can have returned a __typename that is the name of an interface in the supergraph, and this is fine (that is, we should not - // return such a __typename to the user, but as long as it's not returned, having it in the internal data is ok and sometimes expected). - let Some(ExtendedType::Object(_) | ExtendedType::Interface(_)) = - parameters.schema.types.get(input_type) - else { - parameters.nullified.push(Path::from_response_slice(path)); - *output = Value::Null; - return Ok(()); - }; - } + Ok(()) + } - if output.is_null() { - *output = Value::Object(Object::with_capacity(selection_set.len())); - } - let output_object = output.as_object_mut().ok_or(InvalidValue)?; - - let typename = input_object - .get(TYPENAME) - .and_then(|val| val.as_str()) - .and_then(|s| apollo_compiler::ast::NamedType::new(s).ok()) - .map(apollo_compiler::ast::Type::Named); - - let current_type = - match parameters.schema.types.get(field_type.inner_named_type()) { - Some(ExtendedType::Interface(..) | ExtendedType::Union(..)) => { - typename.as_ref().unwrap_or(field_type) - } - _ => field_type, - }; + #[inline] + fn format_integer( + &self, + parameters: &mut FormatParameters, + path: &[ResponsePathElement<'_>], + input: &mut Value, + output: &mut Value, + ) { + // if the value is invalid, we do not insert it in the output object + // which is equivalent to inserting null + if input.as_i64().is_some_and(|i| i32::try_from(i).is_ok()) + || input.as_i64().is_some_and(|i| i32::try_from(i).is_ok()) + { + *output = input.clone(); + } else { + if !input.is_null() { + parameters.insert_coercion_error( + Error::builder() + .message("Invalid value found for the type Int") + .path(Path::from_response_slice(path)) + .extension("code", ERROR_CODE_RESPONSE_VALIDATION) + .build(), + ); + } + *output = Value::Null; + } + } - if self - .apply_selection_set( - selection_set, - parameters, - input_object, - output_object, - path, - current_type, - ) - .is_err() - { - parameters.nullified.push(Path::from_response_slice(path)); - *output = Value::Null; - } + #[inline] + fn format_float( + &self, + parameters: &mut FormatParameters, + path: &[ResponsePathElement<'_>], + input: &mut Value, + output: &mut Value, + ) { + if input.as_f64().is_some() { + *output = input.clone(); + } else { + if !input.is_null() { + parameters.insert_coercion_error( + Error::builder() + .message("Invalid value found for the type Float") + .path(Path::from_response_slice(path)) + .extension("code", ERROR_CODE_RESPONSE_VALIDATION) + .build(), + ); + } + *output = Value::Null; + } + } - Ok(()) - } - _ => { - parameters.nullified.push(Path::from_response_slice(path)); - *output = Value::Null; - Ok(()) - } - } + #[inline] + fn format_boolean( + &self, + parameters: &mut FormatParameters, + path: &[ResponsePathElement<'_>], + input: &mut Value, + output: &mut Value, + ) { + if input.as_bool().is_some() { + *output = input.clone(); + } else { + if !input.is_null() { + parameters.insert_coercion_error( + Error::builder() + .message("Invalid value found for the type Boolean") + .path(Path::from_response_slice(path)) + .extension("code", ERROR_CODE_RESPONSE_VALIDATION) + .build(), + ); + } + *output = Value::Null; + } + } + + #[inline] + fn format_string( + &self, + parameters: &mut FormatParameters, + path: &[ResponsePathElement<'_>], + input: &mut Value, + output: &mut Value, + ) { + if input.as_str().is_some() { + *output = input.clone(); + } else { + if !input.is_null() { + parameters.insert_coercion_error( + Error::builder() + .message("Invalid value found for the type String") + .path(Path::from_response_slice(path)) + .extension("code", ERROR_CODE_RESPONSE_VALIDATION) + .build(), + ); } + *output = Value::Null; + } + } + + #[inline] + fn format_id( + &self, + parameters: &mut FormatParameters, + path: &[ResponsePathElement<'_>], + input: &mut Value, + output: &mut Value, + ) { + if input.is_string() || input.is_i64() || input.is_u64() || input.is_f64() { + *output = input.clone(); + } else { + if !input.is_null() { + parameters.insert_coercion_error( + Error::builder() + .message("Invalid value found for the type ID") + .path(Path::from_response_slice(path)) + .extension("code", ERROR_CODE_RESPONSE_VALIDATION) + .build(), + ); + } + *output = Value::Null; } } @@ -630,7 +764,6 @@ impl Query { input_value, output_value, path, - current_type, selection_set, ); path.pop(); @@ -643,11 +776,11 @@ impl Query { parameters.errors.push( Error::builder() .message(format!( - "Cannot return null for non-nullable field {current_type}.{}", - field_name.as_str() + "Null value found for non-nullable type {}", + field_type.0.inner_named_type() )) .path(Path::from_response_slice(path)) - .build() + .build(), ); return Err(InvalidValue); @@ -666,6 +799,21 @@ impl Query { continue; } + // NOTE: The subtype logic is strange. We are trying to determine if a fragment + // should be applied, but we don't have the __typename of the selection set + // (otherwise, we would be on a different branch). Consider the following query + // for a union Thing = Foo | Bar: + // { thing { ... on Foo { foo }, ... on Bar { bar } } } + // + // As we process the `... on Foo` fragment, `Foo` is `type_condition` and + // `Thing` is `current_type`, we *could* reverse the order in calling + // `is_subtype` and apply the fragment; however, the same is true for the `Bar` + // fragment. Without the type info of the data we have in our response, we + // can't know which to apply (or if both should apply in the case of + // interfaces). + // + // Without that information, this is the best we can do without construction a + // much more complicated reformatting heuristic. let is_apply = current_type.inner_named_type().as_str() == type_condition.as_str() || parameters @@ -706,6 +854,8 @@ impl Query { selection_set, }) = self.fragments.get(name) { + // NOTE: This subtype logic is a bit strange. See the InlineFragment + // branch for why its done this way. let is_apply = current_type.inner_named_type().as_str() == type_condition.as_str() || parameters.schema.is_subtype( @@ -791,7 +941,6 @@ impl Query { input_value, output_value, path, - &field_type.0, selection_set, ); path.pop(); @@ -1025,10 +1174,19 @@ impl Query { struct FormatParameters<'a> { variables: &'a Object, errors: Vec, + coercion_errors: Option>, nullified: Vec, schema: &'a ApiSchema, } +impl FormatParameters<'_> { + fn insert_coercion_error(&mut self, error: Error) { + if let Some(errors) = self.coercion_errors.as_mut() { + errors.push(error) + } + } +} + #[derive(Debug, Serialize, Deserialize)] pub(crate) struct Operation { pub(crate) name: Option, diff --git a/apollo-router/src/spec/query/tests.rs b/apollo-router/src/spec/query/tests.rs index 8ed158d677..dd873d5da7 100644 --- a/apollo-router/src/spec/query/tests.rs +++ b/apollo-router/src/spec/query/tests.rs @@ -144,6 +144,7 @@ impl FormatTest { .clone(), api_schema, BooleanValues { bits: 0 }, + true, ); if let Some(e) = self.expected { @@ -1270,28 +1271,28 @@ fn reformat_response_expected_types() { }, }}) .expected_errors(json!([ - /* FIXME(@TylerBloom): This, per the spec, *is* expected. However, persently, the router - * does not produce these errors. { - "message": "Invalid value found for field Thing.i", + "message": "Invalid value found for the type Int", "path": ["get", "i"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Invalid value found for field Thing.s", + "message": "Invalid value found for the type String", "path": ["get", "s"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Invalid value found for field Thing.f", + "message": "Invalid value found for the type Float", "path": ["get", "f"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Invalid value found for field Thing.b", + "message": "Invalid value found for the type Boolean", "path": ["get", "b"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, + /* FIXME(@TylerBloom): This, per the spec, *is* expected. However, persently, the router + * does not produce these errors. { "message": "Expected a valid enum value for type E", "path": ["get", "e"], @@ -1350,39 +1351,36 @@ fn reformat_response_expected_int() { "g": null, })) .expected_errors(json!([ - /* FIXME(@TylerBloom): This, per the spec, *is* expected. However, persently, the router - * does not produce these errors. { - "message": "Invalid value found for field Query.b", + "message": "Invalid value found for the type Int", "path": ["b"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Invalid value found for field Query.c", + "message": "Invalid value found for the type Int", "path": ["c"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Invalid value found for field Query.d", + "message": "Invalid value found for the type Int", "path": ["d"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Invalid value found for field Query.e", + "message": "Invalid value found for the type Int", "path": ["e"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Invalid value found for field Query.f", + "message": "Invalid value found for the type Int", "path": ["f"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Invalid value found for field Query.g", + "message": "Invalid value found for the type Int", "path": ["g"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, - */ ])) .test(); } @@ -1421,14 +1419,11 @@ fn reformat_response_expected_int_range() { }, })) .expected_errors(json!([ - /* FIXME(@TylerBloom): This, per the spec, *is* expected. However, persently, the router - * does not produce these errors. { - "message": "Invalid value found for field User.someNumber", + "message": "Invalid value found for the type Int", "path": ["me", "someNumber"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" }, } - */ ])) .test(); @@ -1448,14 +1443,16 @@ fn reformat_response_expected_int_range() { "me": null, })) .expected_errors(json!([ - /* FIXME(@TylerBloom): This, per the spec, *is* expected. However, persently, the router - * does not produce these errors. { - "message": "Invalid value found for field User.someOtherNumber", + "message": "Invalid value found for the type Int", "path": ["me", "someOtherNumber"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" }, }, - */ + { + "message": "Null value found for non-nullable type Int", + "path": ["me", "someOtherNumber"], + "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } + }, ])) .test(); } @@ -1495,29 +1492,26 @@ fn reformat_response_expected_float() { "f": null, })) .expected_errors(json!([ - /* FIXME(@TylerBloom): This, per the spec, *is* expected. However, persently, the router - * does not produce these errors. { - "message": "Invalid value found for field Query.c", + "message": "Invalid value found for the type Float", "path": ["c"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Invalid value found for field Query.d", + "message": "Invalid value found for the type Float", "path": ["d"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Invalid value found for field Query.e", + "message": "Invalid value found for the type Float", "path": ["e"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Invalid value found for field Query.f", + "message": "Invalid value found for the type Float", "path": ["f"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, - */ ])) .test(); } @@ -1555,33 +1549,33 @@ fn reformat_response_expected_string() { "f": null, })) .expected_errors(json!([ - /* FIXME(@TylerBloom): This, per the spec, *is* expected. However, persently, the router - * does not produce these errors. { - "message": "Invalid value found for field Query.b", + "message": "Invalid value found for the type String", "path": ["b"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Invalid value found for field Query.c", + "message": "Invalid value found for the type String", "path": ["c"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Invalid value found for field Query.d", + "message": "Invalid value found for the type String", "path": ["d"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Invalid value found for field Query.e", + "message": "Invalid value found for the type String", "path": ["e"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Invalid value found for field Query.f", + "message": "Invalid value found for the type String", "path": ["f"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, + /* FIXME(@TylerBloom): This, per the spec, *is* expected. However, persently, the router + * does not produce these errors. */ ])) .test(); @@ -1651,7 +1645,7 @@ fn reformat_response_expected_id() { } #[test] -fn reformat_response_coersion_propagation_into_list() { +fn reformat_response_coercion_propagation_into_list() { FormatTest::builder() .schema( r#" @@ -1676,19 +1670,16 @@ fn reformat_response_coersion_propagation_into_list() { } })) .expected_errors(json!([ - /* FIXME(@TylerBloom): This, per the spec, *is* expected. However, persently, the router - * does not produce these errors. { - "message": "Invalid value found for field Query.thing.a", - "path": ["thing", "a", 0], + "message": "Invalid value found for the type Int", + "path": ["thing", "a", 1], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Invalid value found for field Query.thing.a", - "path": ["thing", "a", 1], + "message": "Invalid value found for the type Int", + "path": ["thing", "a", 2], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, - */ ])) .test(); @@ -1715,25 +1706,24 @@ fn reformat_response_coersion_propagation_into_list() { "a": null } })) + // NOTE: The array validation stops at its first invalid value and "bubbles" up the + // nullification from there .expected_errors(json!([ - /* FIXME(@TylerBloom): This, per the spec, *is* expected. However, persently, the router - * does not produce these errors. { - "message": "Invalid value found for field Query.thing.a", - "path": ["thing", "a", 0], + "message": "Invalid value found for the type Int", + "path": ["thing", "a", 1], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Invalid value found for field Query.thing.a", + "message": "Null value found for non-nullable type Int", "path": ["thing", "a", 1], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Invalid value found for field Query.thing.a", + "message": "Invalid value found inside the array of type [Int!]", "path": ["thing", "a"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } } - */ ])) .test(); @@ -1758,36 +1748,35 @@ fn reformat_response_coersion_propagation_into_list() { .expected(json!({ "thing": null })) + // NOTE: The array validation stops at its first invalid value and "bubbles" up the + // nullification from there .expected_errors(json!([ - /* FIXME(@TylerBloom): This, per the spec, *is* expected. However, persently, the router - * does not produce these errors. { - "message": "Invalid value found for field Query.thing.a", - "path": ["thing", "a", 0], + "message": "Invalid value found for the type Int", + "path": ["thing", "a", 1], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Invalid value found for field Query.thing.a", + "message": "Null value found for non-nullable type Int", "path": ["thing", "a", 1], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Invalid value found for field Query.thing.a", + "message": "Invalid value found inside the array of type [Int!]", "path": ["thing", "a"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Invalid value found for field Query.thing", - "path": ["thing"], + "message": "Null value found for non-nullable type [Int!]", + "path": ["thing", "a"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } } - */ ])) .test(); } #[test] -fn reformat_response_coersion_propagation_into_object() { +fn reformat_response_coercion_propagation_into_object() { FormatTest::builder() .schema( r#" @@ -1818,19 +1807,16 @@ fn reformat_response_coersion_propagation_into_object() { } })) .expected_errors(json!([ - /* FIXME(@TylerBloom): This, per the spec, *is* expected. However, persently, the router - * does not produce these errors. { - "message": "Invalid value found for field Query.thing.b", + "message": "Invalid value found for the type Int", "path": ["thing", "b"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Invalid value found for field Query.thing.c", + "message": "Invalid value found for the type Int", "path": ["thing", "c"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, - */ ])) .test(); @@ -1859,25 +1845,19 @@ fn reformat_response_coersion_propagation_into_object() { .expected(json!({ "thing": null })) + // NOTE: The array validation stops at its first invalid value and "bubbles" up the + // nullification from there .expected_errors(json!([ - /* FIXME(@TylerBloom): This, per the spec, *is* expected. However, persently, the router - * does not produce these errors. { - "message": "Invalid value found for field Query.thing.b", + "message": "Invalid value found for the type Int", "path": ["thing", "b"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Invalid value found for field Query.thing.c", - "path": ["thing", "c"], + "message": "Null value found for non-nullable type Int", + "path": ["thing", "b"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, - { - "message": "Invalid value found for field Query.thing", - "path": ["thing"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - } - */ ])) .test(); @@ -1896,31 +1876,29 @@ fn reformat_response_coersion_propagation_into_object() { "#, ) .query(r#"{ thing { a, b, c } }"#) - .response(json!({})) - .expected_errors(json!([ - /* FIXME(@TylerBloom): This, per the spec, *is* expected. However, persently, the router - * does not produce these errors. - { - "message": "Invalid value found for field Query.thing.b", + .response(json!({ + "thing": { + "a": 1, + "b": 1.1, + "c": 1.2 + } + })) + // NOTE: The array validation stops at its first invalid value and "bubbles" up the + // nullification from there + .expected_errors(json!([ { + "message": "Invalid value found for the type Int", "path": ["thing", "b"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Invalid value found for field Query.thing.c", - "path": ["thing", "c"], + "message": "Null value found for non-nullable type Int", + "path": ["thing", "b"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - }, - { - "message": "Invalid value found for field Query.thing", + }, { + "message": "Null value found for non-nullable type Thing", "path": ["thing"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - }, - { - "message": "Invalid value found for field Query", - "path": [], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } } - */ ])) .test(); } @@ -1939,7 +1917,7 @@ fn reformat_response_coersion_propagation_into_object() { // router-QP boundary and needs to be tested elsewhere. This test is just for ensuring we can // follow the spec as closely as possible. #[test] -fn reformat_response_coersion_propagation_into_union() { +fn reformat_response_coercion_propagation_into_union() { let nullable_schema = r#" type Query { thing: Thing @@ -1999,26 +1977,7 @@ fn reformat_response_coersion_propagation_into_union() { .query(query_wo_type_info) .response(resp_wo_type_info.clone()) .expected(json!({ "thing": { } })) - .expected_errors(json!([ - /* FIXME(@TylerBloom): This, per the spec, *is* expected. However, persently, the router - * does not produce these errors. - { - "message": "Invalid value found for field Query.thing.b", - "path": ["thing", "b"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - }, - { - "message": "Invalid value found for field Query.thing.c", - "path": ["thing", "c"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - }, - { - "message": "Invalid values found for field of an abstract type without `__typename`, entire object must be nullified", - "path": ["thing"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - } - */ - ])) + .expected_errors(json!([])) .test(); FormatTest::builder() @@ -2028,28 +1987,8 @@ fn reformat_response_coersion_propagation_into_union() { // NOTE: This is seemingly strange behavior but is consistent. Because we can't *always* // resolve the type of `thing`, we can't determine if the result is valid or not. .expected(json!({ "thing": { } })) - .expected_errors(json!([ - /* FIXME(@TylerBloom): This, per the spec, *is* expected. However, persently, the router - * does not produce these errors. - * FIXME(@TylerBloom): Should the `__typename` error be returned over the more general - * result coersion error? - { - "message": "Invalid value found for field Query.thing.b", - "path": ["thing", "b"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - }, - { - "message": "Invalid value found for field Query.thing.c", - "path": ["thing", "c"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - }, - { - "message": "Invalid values found for field of an abstract type without `__typename`, entire object must be nullified", - "path": ["thing"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - } - */ - ])) + // NOTE: Same issue. We can not *always* know what type was intented, so we drop the data. + .expected_errors(json!([])) .test(); // Case 2: __typename isn't queried but is returned @@ -2067,19 +2006,16 @@ fn reformat_response_coersion_propagation_into_union() { } })) .expected_errors(json!([ - /* FIXME(@TylerBloom): This, per the spec, *is* expected. However, persently, the router - * does not produce these errors. { - "message": "Invalid value found for field Query.thing.b", + "message": "Invalid value found for the type Int", "path": ["thing", "b"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Invalid value found for field Query.thing.c", + "message": "Invalid value found for the type Int", "path": ["thing", "c"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, - */ ])) .test(); @@ -2088,20 +2024,19 @@ fn reformat_response_coersion_propagation_into_union() { .query(query_wo_type_info) .response(resp_with_type_info.clone()) .expected(json!({ "thing": null })) + // NOTE: The array validation stops at its first invalid value and "bubbles" up the + // nullification from there .expected_errors(json!([ - /* FIXME(@TylerBloom): This, per the spec, *is* expected. However, persently, the router - * does not produce these errors. { - "message": "Invalid value found for field Query.thing.b", + "message": "Invalid value found for the type Int", "path": ["thing", "b"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Invalid value found for field Query.thing.c", - "path": ["thing", "c"], + "message": "Null value found for non-nullable type Int", + "path": ["thing", "b"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } } - */ ])) .test(); @@ -2110,70 +2045,16 @@ fn reformat_response_coersion_propagation_into_union() { .schema(nullable_schema) .query(query_with_type_info) .response(resp_wo_type_info.clone()) - // FIXME(@TylerBloom): This is not expected. This should behave the same with(out) - // `__typename` being queried. .expected(json!({ "thing": null })) - .expected_errors(json!([ - /* FIXME(@TylerBloom): This, per the spec, *is* expected. However, persently, the router - * does not produce these errors. - { - "message": "Invalid value found for field Query.thing.b", - "path": ["thing", "b"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - }, - { - "message": "Invalid value found for field Query.thing.c", - "path": ["thing", "c"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - }, - { - "message": "Invalid values found for field of an abstract type without `__typename` - "path": ["thing"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - }, - { - "message": "`__typename` was queried but not part of the response", - "path": ["thing"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - } - */ - ])) + .expected_errors(json!([])) .test(); FormatTest::builder() .schema(non_nullable_schema) .query(query_with_type_info) .response(resp_wo_type_info.clone()) - // FIXME(@TylerBloom): This is not expected. This should behave the same with(out) - // `__typename` being queried. .expected(json!({ "thing": null })) - .expected_errors(json!([ - /* FIXME(@TylerBloom): This, per the spec, *is* expected. However, persently, the router - * does not produce these errors. - * FIXME(@TylerBloom): Should the `__typename` error be returned over the more general - * result coersion error? - { - "message": "Invalid value found for field Query.thing.b", - "path": ["thing", "b"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - }, - { - "message": "Invalid value found for field Query.thing.c", - "path": ["thing", "c"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - }, - { - "message": "Invalid values found for field of an abstract type without `__typename`, entire object must be nullified", - "path": ["thing"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - }, - { - "message": "`__typename` was queried but not part of the response", - "path": ["thing"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - } - */ - ])) + .expected_errors(json!([])) .test(); // Case 4: __typename is queried and is returned @@ -2190,19 +2071,16 @@ fn reformat_response_coersion_propagation_into_union() { } })) .expected_errors(json!([ - /* FIXME(@TylerBloom): This, per the spec, *is* expected. However, persently, the router - * does not produce these errors. { - "message": "Invalid value found for field Query.thing.b", + "message": "Invalid value found for the type Int", "path": ["thing", "b"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Invalid value found for field Query.thing.c", + "message": "Invalid value found for the type Int", "path": ["thing", "c"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } } - */ ])) .test(); @@ -2211,25 +2089,19 @@ fn reformat_response_coersion_propagation_into_union() { .query(query_with_type_info) .response(resp_with_type_info.clone()) .expected(json!({ "thing": null })) + // NOTE: The array validation stops at its first invalid value and "bubbles" up the + // nullification from there .expected_errors(json!([ - /* FIXME(@TylerBloom): This, per the spec, *is* expected. However, persently, the router - * does not produce these errors. { - "message": "Invalid value found for field Query.thing.b", + "message": "Invalid value found for the type Int", "path": ["thing", "b"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Invalid value found for field Query.thing.c", - "path": ["thing", "c"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - }, - { - "message": "Invalid values found for field of an abstract type without `__typename`, entire object must be nullified", - "path": ["thing"], + "message": "Null value found for non-nullable type Int", + "path": ["thing", "b"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } } - */ ])) .test(); } @@ -2245,7 +2117,7 @@ fn reformat_response_coersion_propagation_into_union() { // Unlike the union tests, if a fragment is part of the query but type info is not // returned, the return fragment data is ignored, regardless of validity. #[test] -fn reformat_response_coersion_propagation_into_interfaces() { +fn reformat_response_coercion_propagation_into_interfaces() { let nullable_schema = r#" type Query { thing: Thing @@ -2310,27 +2182,12 @@ fn reformat_response_coersion_propagation_into_interfaces() { .schema(nullable_schema) .query(query_wo_type_info) .response(resp_wo_type_info.clone()) - .expected(json!({ "thing": { "a": 1 } })) - .expected_errors(json!([ - /* FIXME(@TylerBloom): This, per the spec, *is* expected. However, persently, the router - * does not produce these errors. - { - "message": "Invalid value found for field Query.thing.b", - "path": ["thing", "b"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - }, - { - "message": "Invalid value found for field Query.thing.c", - "path": ["thing", "c"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - }, - { - "message": "Invalid values found for field of an abstract type without `__typename` - "path": ["thing"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } + .expected(json!({ + "thing": { + "a": 1, } - */ - ])) + })) + .expected_errors(json!([])) .test(); FormatTest::builder() @@ -2341,29 +2198,12 @@ fn reformat_response_coersion_propagation_into_interfaces() { // resolve the type of `thing` with __typename, we can't determine if the result is valid // or not. Thus, the nullification of the fragment doesn't "bubble up" to nullifying the // either "thing". - .expected(json!({ "thing": { "a": 1 } })) - .expected_errors(json!([ - /* FIXME(@TylerBloom): This, per the spec, *is* expected. However, persently, the router - * does not produce these errors. - * FIXME(@TylerBloom): Should the `__typename` error be returned over the more general - * result coersion error? - { - "message": "Invalid value found for field Query.thing.b", - "path": ["thing", "b"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - }, - { - "message": "Invalid value found for field Query.thing.c", - "path": ["thing", "c"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - }, - { - "message": "Invalid values found for field of an abstract type without `__typename` - "path": ["thing"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } + .expected(json!({ + "thing": { + "a": 1 } - */ - ])) + })) + .expected_errors(json!([])) .test(); // Case 2: __typename isn't queried but is returned @@ -2371,26 +2211,24 @@ fn reformat_response_coersion_propagation_into_interfaces() { .schema(nullable_schema) .query(query_wo_type_info) .response(resp_with_type_info.clone()) - .expected(json!({ "thing": { "a": 1, "b": null, "c": null } })) + .expected(json!({ + "thing": { + "a": 1, + "b": null, + "c": null, + } + })) .expected_errors(json!([ - /* FIXME(@TylerBloom): This, per the spec, *is* expected. However, persently, the router - * does not produce these errors. { - "message": "Invalid value found for field Query.thing.b", + "message": "Invalid value found for the type Int", "path": ["thing", "b"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Invalid value found for field Query.thing.c", + "message": "Invalid value found for the type Int", "path": ["thing", "c"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, - { - "message": "Invalid values found for field of an abstract type without `__typename` - "path": ["thing"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - } - */ ])) .test(); @@ -2399,27 +2237,19 @@ fn reformat_response_coersion_propagation_into_interfaces() { .query(query_wo_type_info) .response(resp_with_type_info.clone()) .expected(json!({ "thing": null })) + // NOTE: The array validation stops at its first invalid value and "bubbles" up the + // nullification from there .expected_errors(json!([ - /* FIXME(@TylerBloom): This, per the spec, *is* expected. However, persently, the router - * does not produce these errors. - * FIXME(@TylerBloom): Should the `__typename` error be returned over the more general - * result coersion error? { - "message": "Invalid value found for field Query.thing.b", + "message": "Invalid value found for the type Int", "path": ["thing", "b"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Invalid value found for field Query.thing.c", - "path": ["thing", "c"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - }, - { - "message": "Invalid values found for field of an abstract type without `__typename` - "path": ["thing"], + "message": "Null value found for non-nullable type Int", + "path": ["thing", "b"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } } - */ ])) .test(); @@ -2431,26 +2261,7 @@ fn reformat_response_coersion_propagation_into_interfaces() { // FIXME(@TylerBloom): This is not expected. This should behave the same with(out) // `__typename` being queried. .expected(json!({ "thing": null })) - .expected_errors(json!([ - /* FIXME(@TylerBloom): This, per the spec, *is* expected. However, persently, the router - * does not produce these errors. - { - "message": "Invalid value found for field Query.thing.b", - "path": ["thing", "b"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - }, - { - "message": "Invalid value found for field Query.thing.c", - "path": ["thing", "c"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - }, - { - "message": "Invalid values found for field of an abstract type without `__typename` - "path": ["thing"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - } - */ - ])) + .expected_errors(json!([])) .test(); FormatTest::builder() @@ -2458,28 +2269,7 @@ fn reformat_response_coersion_propagation_into_interfaces() { .query(query_with_type_info) .response(resp_wo_type_info.clone()) .expected(json!({ "thing": null })) - .expected_errors(json!([ - /* FIXME(@TylerBloom): This, per the spec, *is* expected. However, persently, the router - * does not produce these errors. - * FIXME(@TylerBloom): Should the `__typename` error be returned over the more general - * result coersion error? - { - "message": "Invalid value found for field Query.thing.b", - "path": ["thing", "b"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - }, - { - "message": "Invalid value found for field Query.thing.c", - "path": ["thing", "c"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - }, - { - "message": "Invalid values found for field of an abstract type without `__typename` - "path": ["thing"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - } - */ - ])) + .expected_errors(json!([])) .test(); // Case 4: __typename is queried and is returned @@ -2487,26 +2277,25 @@ fn reformat_response_coersion_propagation_into_interfaces() { .schema(nullable_schema) .query(query_with_type_info) .response(resp_with_type_info.clone()) - .expected(json!({ "thing": { "__typename": "Foo", "a": 1, "b": null, "c": null } })) + .expected(json!({ + "thing": { + "__typename": "Foo", + "a": 1, + "b": null, + "c": null + } + })) .expected_errors(json!([ - /* FIXME(@TylerBloom): This, per the spec, *is* expected. However, persently, the router - * does not produce these errors. { - "message": "Invalid value found for field Query.thing.b", + "message": "Invalid value found for the type Int", "path": ["thing", "b"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Invalid value found for field Query.thing.c", + "message": "Invalid value found for the type Int", "path": ["thing", "c"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - }, - { - "message": "Invalid values found for field of an abstract type without `__typename` - "path": ["thing"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } } - */ ])) .test(); @@ -2516,26 +2305,16 @@ fn reformat_response_coersion_propagation_into_interfaces() { .response(resp_with_type_info.clone()) .expected(json!({ "thing": null })) .expected_errors(json!([ - /* FIXME(@TylerBloom): This, per the spec, *is* expected. However, persently, the router - * does not produce these errors. - * FIXME(@TylerBloom): Should the `__typename` error be returned over the more general - * result coersion error? { - "message": "Invalid value found for field Query.thing.b", + "message": "Invalid value found for the type Int", "path": ["thing", "b"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Invalid value found for field Query.thing.c", - "path": ["thing", "c"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - }, - { - "message": "Invalid values found for field of an abstract type without `__typename` - "path": ["thing"], + "message": "Null value found for non-nullable type Int", + "path": ["thing", "b"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } } - */ ])) .test(); } @@ -3421,11 +3200,11 @@ fn filter_list_errors() { .expected_extensions(json! {{ "valueCompletion": [ { - "message": "Cannot return null for non-nullable array element of type String at index 1", + "message": "Null value found for non-nullable type String", "path": ["list", "l2", 1] - } + }, ] - }},) + }}) .test(); FormatTest::builder() @@ -3624,7 +3403,7 @@ fn filter_nested_object_errors() { .expected_extensions(json! {{ "valueCompletion": [ { - "message": "Cannot return null for non-nullable field Review.text2", + "message": "Null value found for non-nullable type String", "path": ["me", "reviews1", 0] } ] @@ -6476,6 +6255,7 @@ fn fragment_on_interface_on_query() { Default::default(), api_schema, BooleanValues { bits: 0 }, + true, ); assert_eq_and_ordered!( response.data.as_ref().unwrap(), @@ -7187,6 +6967,7 @@ fn filtered_defer_fragment() { Object::new(), schema.api_schema(), BooleanValues { bits: 0 }, + true, ); assert_json_snapshot!(response); @@ -7196,6 +6977,7 @@ fn filtered_defer_fragment() { Object::new(), schema.api_schema(), BooleanValues { bits: 0 }, + true, ); assert_json_snapshot!(response); diff --git a/apollo-router/tests/set_context.rs b/apollo-router/tests/set_context.rs index a8c17daf5a..472633dcfc 100644 --- a/apollo-router/tests/set_context.rs +++ b/apollo-router/tests/set_context.rs @@ -43,6 +43,7 @@ fn get_configuration() -> serde_json::Value { "all": true }, "supergraph": { + "enable_result_coercion_errors": true, // TODO(@goto-bus-stop): need to update the mocks and remove this, #6013 "generate_query_fragments": false, } diff --git a/apollo-router/tests/snapshots/set_context__set_context_unrelated_fetch_failure_rust_qp.snap b/apollo-router/tests/snapshots/set_context__set_context_unrelated_fetch_failure_rust_qp.snap index bbfe4ad84f..c9740f9687 100644 --- a/apollo-router/tests/snapshots/set_context__set_context_unrelated_fetch_failure_rust_qp.snap +++ b/apollo-router/tests/snapshots/set_context__set_context_unrelated_fetch_failure_rust_qp.snap @@ -1,5 +1,6 @@ --- source: apollo-router/tests/set_context.rs +assertion_line: 280 expression: response --- { @@ -14,6 +15,25 @@ expression: response "extensions": { "service": "Subgraph2" } + }, + { + "message": "Null value found for non-nullable type U", + "path": [ + "t", + "u" + ], + "extensions": { + "code": "RESPONSE_VALIDATION_FAILED" + } + }, + { + "message": "Null value found for non-nullable type T", + "path": [ + "t" + ], + "extensions": { + "code": "RESPONSE_VALIDATION_FAILED" + } } ], "extensions": { @@ -147,21 +167,21 @@ expression: response }, "valueCompletion": [ { - "message": "Cannot return null for non-nullable field U.field", + "message": "Null value found for non-nullable type Int", "path": [ "t", "u" ] }, { - "message": "Cannot return null for non-nullable field T.u", + "message": "Null value found for non-nullable type U", "path": [ "t", "u" ] }, { - "message": "Cannot return null for non-nullable field T!.t", + "message": "Null value found for non-nullable type T", "path": [ "t" ]