From 56e7ba649cc79e4b883a715aebea984ccd56aa9e Mon Sep 17 00:00:00 2001 From: TylerBloom Date: Thu, 16 Oct 2025 13:57:47 -0400 Subject: [PATCH 01/21] Split out `format_value` into sub-methods --- apollo-router/src/spec/query.rs | 453 ++++++++++++++++++-------------- 1 file changed, 263 insertions(+), 190 deletions(-) diff --git a/apollo-router/src/spec/query.rs b/apollo-router/src/spec/query.rs index 8c55f01658..d810fc69a7 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; @@ -340,227 +341,299 @@ impl Query { // 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(input, output), + "Float" => self.format_float(input, output), + "Boolean" => self.format_boolean(input, output), + "String" => self.format_string(input, output), + "Id" => self.format_id(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, + field_type, + 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, + parent_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] + #[allow(clippy::too_many_arguments)] + 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>, + parent_type: &executable::Type, + 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()), + _ => unreachable!(), + }; + match self.format_value( + parameters, + &inner_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(), + ); + + Err(InvalidValue) + } else { + Ok(()) } } + } + } - // 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)); + #[inline] + #[allow(clippy::too_many_arguments)] + fn format_list<'a: 'b, 'b>( + &'a self, + parameters: &mut FormatParameters, + field_type: &executable::Type, + input: &mut Value, + inner_type: &executable::Type, + output: &mut Value, + path: &mut Vec>, + selection_set: &'a [Selection], + ) -> Result<(), InvalidValue> { + if let Value::Array(input_array) = input { + 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)); + let res = self.format_value( + parameters, + inner_type, + element, + &mut output_array[i], + path, + field_type, + selection_set, + ); + path.pop(); + res + }) + { + parameters.nullified.push(Path::from_response_slice(path)); + *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(()); + } + 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(()) } - 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 + None => { + *output = Value::Null; + 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(()) } - executable::Type::Named(name) if name == "Float" => { - if input.as_f64().is_some() { - *output = input.clone(); - } else { - *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(()) - } - executable::Type::Named(name) if name == "Boolean" => { - if input.as_bool().is_some() { - *output = input.clone(); - } else { - *output = Value::Null; + + if output.is_null() { + *output = Value::Object(Object::with_capacity(selection_set.len())); } - Ok(()) - } - executable::Type::Named(name) if name == "String" => { - if input.as_str().is_some() { - *output = input.clone(); - } else { + 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, + }; + + 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; } + Ok(()) } - 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; - } + _ => { + parameters.nullified.push(Path::from_response_slice(path)); + *output = Value::Null; Ok(()) } - 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(()) - } - }; - } - _ => {} - } + } + } - 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(()); - }; - } + #[inline] + fn format_integer(&self, input: &mut Value, output: &mut Value) { + 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 + }; - 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, - }; + // 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; + } + } - 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, input: &mut Value, output: &mut Value) { + if input.as_f64().is_some() { + *output = input.clone(); + } else { + *output = Value::Null; + } + } - Ok(()) - } - _ => { - parameters.nullified.push(Path::from_response_slice(path)); - *output = Value::Null; - Ok(()) - } - } - } + #[inline] + fn format_boolean(&self, input: &mut Value, output: &mut Value) { + if input.as_bool().is_some() { + *output = input.clone(); + } else { + *output = Value::Null; + } + } + + #[inline] + fn format_string(&self, input: &mut Value, output: &mut Value) { + if input.as_str().is_some() { + *output = input.clone(); + } else { + *output = Value::Null; + } + } + + #[inline] + fn format_id(&self, input: &mut Value, output: &mut Value) { + if input.is_string() || input.is_i64() || input.is_u64() || input.is_f64() { + *output = input.clone(); + } else { + *output = Value::Null; } } From ad4809e967384739f30bd0f59cb687f85f650ff5 Mon Sep 17 00:00:00 2001 From: TylerBloom Date: Thu, 16 Oct 2025 14:20:50 -0400 Subject: [PATCH 02/21] General cleanup of format_* methods --- apollo-router/src/spec/query.rs | 179 +++++++++++++++----------------- 1 file changed, 81 insertions(+), 98 deletions(-) diff --git a/apollo-router/src/spec/query.rs b/apollo-router/src/spec/query.rs index d810fc69a7..e0ced858b8 100644 --- a/apollo-router/src/spec/query.rs +++ b/apollo-router/src/spec/query.rs @@ -402,9 +402,11 @@ impl Query { 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 _ => unreachable!(), }; - match self.format_value( + + self.format_value( parameters, &inner_type, input, @@ -412,31 +414,28 @@ impl Query { 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(), - ); + )?; - Err(InvalidValue) - } else { - 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(), + ); + + Err(InvalidValue) + } else { + Ok(()) } } @@ -503,98 +502,82 @@ impl Query { 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(()) - } - }; + *output = input + .as_str() + .filter(|s| enum_type.values.contains_key(*s)) + .map(|_| input.clone()) + .unwrap_or_default(); + return Ok(()); } _ => {} } - 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(()); - }; - } - - 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, - }; - - if self - .apply_selection_set( - selection_set, - parameters, - input_object, - output_object, - path, - current_type, - ) - .is_err() - { + 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; - } + 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, + }; + + 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; - Ok(()) } + } else { + parameters.nullified.push(Path::from_response_slice(path)); + *output = Value::Null; } + + Ok(()) } #[inline] fn format_integer(&self, input: &mut Value, output: &mut Value) { - 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 - }; - // if the value is invalid, we do not insert it in the output object // which is equivalent to inserting null - if opt.is_some() { + 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 { *output = Value::Null; From 2a8929e0905a97f2747888f8110b5c527291e8cd Mon Sep 17 00:00:00 2001 From: TylerBloom Date: Thu, 16 Oct 2025 16:21:42 -0400 Subject: [PATCH 03/21] Changed format_list to reformat all elements --- apollo-router/src/spec/query.rs | 50 +++++++++++++-------------- apollo-router/src/spec/query/tests.rs | 8 +++++ 2 files changed, 33 insertions(+), 25 deletions(-) diff --git a/apollo-router/src/spec/query.rs b/apollo-router/src/spec/query.rs index e0ced858b8..bdb589e7d0 100644 --- a/apollo-router/src/spec/query.rs +++ b/apollo-router/src/spec/query.rs @@ -451,33 +451,33 @@ impl Query { path: &mut Vec>, selection_set: &'a [Selection], ) -> Result<(), InvalidValue> { - if let Value::Array(input_array) = input { - 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)); - let res = self.format_value( - parameters, - inner_type, - element, - &mut output_array[i], - path, - field_type, - selection_set, - ); - path.pop(); - res - }) - { + 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)?; + let mut nullify = false; + for (i, element) in input_array.iter_mut().enumerate() { + path.push(ResponsePathElement::Index(i)); + if let Err(InvalidValue) = self.format_value( + parameters, + inner_type, + element, + &mut output_array[i], + path, + field_type, + selection_set, + ) { + // TODO: Insert error parameters.nullified.push(Path::from_response_slice(path)); - *output = Value::Null; + nullify = true; } + path.pop(); + } + if nullify { + *output = Value::Null; } Ok(()) } diff --git a/apollo-router/src/spec/query/tests.rs b/apollo-router/src/spec/query/tests.rs index 8ed158d677..cee82ef03c 100644 --- a/apollo-router/src/spec/query/tests.rs +++ b/apollo-router/src/spec/query/tests.rs @@ -3423,6 +3423,14 @@ fn filter_list_errors() { { "message": "Cannot return null for non-nullable array element of type String at index 1", "path": ["list", "l2", 1] + }, + { + "message": "Cannot return null for non-nullable array element of type String at index 2", + "path": ["list", "l2", 2] + }, + { + "message": "Cannot return null for non-nullable array element of type String at index 3", + "path": ["list", "l2", 3] } ] }},) From ba86aa070cc89bed212b737ce6073ed139440c12 Mon Sep 17 00:00:00 2001 From: TylerBloom Date: Thu, 16 Oct 2025 19:14:10 -0400 Subject: [PATCH 04/21] Updated coersion list tests --- apollo-router/src/spec/query.rs | 45 +++++++++++++++++++++-- apollo-router/src/spec/query/tests.rs | 53 ++++++++++++++++----------- 2 files changed, 73 insertions(+), 25 deletions(-) diff --git a/apollo-router/src/spec/query.rs b/apollo-router/src/spec/query.rs index bdb589e7d0..dd1bee9cde 100644 --- a/apollo-router/src/spec/query.rs +++ b/apollo-router/src/spec/query.rs @@ -146,6 +146,7 @@ impl Query { variables: &variables, schema, errors: Vec::new(), + coersion_errors: Vec::new(), nullified: Vec::new(), }; @@ -203,6 +204,7 @@ impl Query { variables: &all_variables, schema, errors: Vec::new(), + coersion_errors: Vec::new(), nullified: Vec::new(), }; @@ -227,6 +229,10 @@ impl Query { .insert(EXTENSIONS_VALUE_COMPLETION_KEY, value); } + if !parameters.coersion_errors.is_empty() { + response.errors.append(&mut parameters.coersion_errors); + } + return parameters.nullified; } } @@ -342,7 +348,7 @@ impl Query { // and return Ok(()), because values are optional by default match field_type { executable::Type::Named(name) => match name.as_str() { - "Int" => self.format_integer(input, output), + "Int" => self.format_integer(parameters, path, input, output), "Float" => self.format_float(input, output), "Boolean" => self.format_boolean(input, output), "String" => self.format_string(input, output), @@ -421,15 +427,22 @@ impl Query { 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}" + Some(ResponsePathElement::Index(_)) => format!( + "Cannot return null for non-nullable array element of type {inner_type}" ), _ => todo!(), }; parameters.errors.push( + Error::builder() + .message(&message) + .path(Path::from_response_slice(path)) + .build(), + ); + parameters.coersion_errors.push( Error::builder() .message(message) .path(Path::from_response_slice(path)) + .extension("code", ERROR_CODE_RESPONSE_VALIDATION) .build(), ); @@ -477,6 +490,14 @@ impl Query { path.pop(); } if nullify { + parameters.coersion_errors.push( + Error::builder() + // FIXME: This needs to be a real error message + .message("Invalid value found for field Query.thing.a") + .path(Path::from_response_slice(path)) + .extension("code", ERROR_CODE_RESPONSE_VALIDATION) + .build(), + ); *output = Value::Null; } Ok(()) @@ -572,7 +593,13 @@ impl Query { } #[inline] - fn format_integer(&self, input: &mut Value, output: &mut Value) { + 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()) @@ -580,6 +607,15 @@ impl Query { { *output = input.clone(); } else { + if !input.is_null() { + parameters.coersion_errors.push( + 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; } } @@ -1081,6 +1117,7 @@ impl Query { struct FormatParameters<'a> { variables: &'a Object, errors: Vec, + coersion_errors: Vec, nullified: Vec, schema: &'a ApiSchema, } diff --git a/apollo-router/src/spec/query/tests.rs b/apollo-router/src/spec/query/tests.rs index cee82ef03c..41ef3fa08e 100644 --- a/apollo-router/src/spec/query/tests.rs +++ b/apollo-router/src/spec/query/tests.rs @@ -1676,19 +1676,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(); @@ -1716,24 +1713,31 @@ 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", + "message": "Cannot return null for non-nullable array element of type Int", "path": ["thing", "a", 1], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, + { + "message": "Invalid value found for the type Int", + "path": ["thing", "a", 2], + "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } + }, + { + "message": "Cannot return null for non-nullable array element of type Int", + "path": ["thing", "a", 2], + "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } + }, { "message": "Invalid value found for field Query.thing.a", "path": ["thing", "a"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } } - */ ])) .test(); @@ -1759,29 +1763,36 @@ fn reformat_response_coersion_propagation_into_list() { "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.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": "Cannot return null for non-nullable array element of type Int", "path": ["thing", "a", 1], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, + { + "message": "Invalid value found for the type Int", + "path": ["thing", "a", 2], + "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } + }, + { + "message": "Cannot return null for non-nullable array element of type Int", + "path": ["thing", "a", 2], + "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } + }, { "message": "Invalid value found for field Query.thing.a", "path": ["thing", "a"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Invalid value found for field Query.thing", - "path": ["thing"], + "message": "Cannot return null for non-nullable field Thing.a", + "path": ["thing", "a"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } } - */ ])) .test(); } From e06f983b74ef277b5c695ff73b5db475988cfe6c Mon Sep 17 00:00:00 2001 From: TylerBloom Date: Thu, 16 Oct 2025 20:20:28 -0400 Subject: [PATCH 05/21] Working on object coersion test --- apollo-router/src/spec/query/tests.rs | 84 +++++++++++++-------------- 1 file changed, 42 insertions(+), 42 deletions(-) diff --git a/apollo-router/src/spec/query/tests.rs b/apollo-router/src/spec/query/tests.rs index 41ef3fa08e..1b5beea2d6 100644 --- a/apollo-router/src/spec/query/tests.rs +++ b/apollo-router/src/spec/query/tests.rs @@ -1829,19 +1829,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(); @@ -1871,21 +1868,26 @@ fn reformat_response_coersion_propagation_into_object() { "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", + "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": "Cannot return null for non-nullable field Thing.b", + "path": ["thing", "b"], + "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } + }, + /* FIXME: The first improperly nullified field causes an early exit, so the other fields + * are not validated. Do we want to keep with this behavior or report all error? + { + "message": "Invalid value found for the type Int", "path": ["thing", "c"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Invalid value found for field Query.thing", - "path": ["thing"], + "message": "Cannot return null for non-nullable field Thing.c", + "path": ["thing", "c"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } } */ @@ -1908,30 +1910,46 @@ fn reformat_response_coersion_propagation_into_object() { ) .query(r#"{ thing { a, b, c } }"#) .response(json!({})) + // FIXME: All of the errors are discarded .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", "a"], + "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } + }, + { + "message": "Cannot return null for non-nullable field Thing.b", + "path": ["thing", "a"], + "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } + }, + /* FIXME: The first improperly nullified field causes an early exit, so the other fields + * are not validated. Do we want to keep with this behavior or report all error? + { + "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": "Cannot return null for non-nullable field Thing.b", + "path": ["thing", "b"], + "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } + }, + { + "message": "Invalid value found for the type Int", "path": ["thing", "c"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Invalid value found for field Query.thing", - "path": ["thing"], + "message": "Cannot return null for non-nullable field Thing.c", + "path": ["thing", "c"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, + */ { - "message": "Invalid value found for field Query", - "path": [], + "message": "Cannot return null for non-nullable field thing", + "path": ["thing"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } } - */ ])) .test(); } @@ -2011,8 +2029,6 @@ fn reformat_response_coersion_propagation_into_union() { .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"], @@ -2028,7 +2044,6 @@ fn reformat_response_coersion_propagation_into_union() { "path": ["thing"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } } - */ ])) .test(); @@ -2044,6 +2059,7 @@ fn reformat_response_coersion_propagation_into_union() { * 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"], @@ -2059,7 +2075,6 @@ fn reformat_response_coersion_propagation_into_union() { "path": ["thing"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } } - */ ])) .test(); @@ -2078,8 +2093,6 @@ 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", "path": ["thing", "b"], @@ -2090,7 +2103,6 @@ fn reformat_response_coersion_propagation_into_union() { "path": ["thing", "c"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, - */ ])) .test(); @@ -2100,8 +2112,6 @@ fn reformat_response_coersion_propagation_into_union() { .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. { "message": "Invalid value found for field Query.thing.b", "path": ["thing", "b"], @@ -2112,7 +2122,6 @@ fn reformat_response_coersion_propagation_into_union() { "path": ["thing", "c"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } } - */ ])) .test(); @@ -2125,8 +2134,6 @@ fn reformat_response_coersion_propagation_into_union() { // `__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"], @@ -2138,16 +2145,15 @@ fn reformat_response_coersion_propagation_into_union() { "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Invalid values found for field of an abstract type without `__typename` + "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", + "message": "__typename was queried but not part of the response", "path": ["thing"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } } - */ ])) .test(); @@ -2163,6 +2169,7 @@ fn reformat_response_coersion_propagation_into_union() { * 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"], @@ -2183,7 +2190,6 @@ fn reformat_response_coersion_propagation_into_union() { "path": ["thing"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } } - */ ])) .test(); @@ -2201,8 +2207,6 @@ 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", "path": ["thing", "b"], @@ -2213,7 +2217,6 @@ fn reformat_response_coersion_propagation_into_union() { "path": ["thing", "c"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } } - */ ])) .test(); @@ -2223,8 +2226,6 @@ fn reformat_response_coersion_propagation_into_union() { .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. { "message": "Invalid value found for field Query.thing.b", "path": ["thing", "b"], @@ -2240,7 +2241,6 @@ fn reformat_response_coersion_propagation_into_union() { "path": ["thing"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } } - */ ])) .test(); } From a5a710d2802ff696444964e7cb20665bc8376e3c Mon Sep 17 00:00:00 2001 From: TylerBloom Date: Thu, 16 Oct 2025 20:24:24 -0400 Subject: [PATCH 06/21] Added coersion errors for other leaf types --- apollo-router/src/spec/query.rs | 76 +++++++++++++++++++++++++++++---- 1 file changed, 68 insertions(+), 8 deletions(-) diff --git a/apollo-router/src/spec/query.rs b/apollo-router/src/spec/query.rs index dd1bee9cde..5e64315527 100644 --- a/apollo-router/src/spec/query.rs +++ b/apollo-router/src/spec/query.rs @@ -349,10 +349,10 @@ impl Query { match field_type { executable::Type::Named(name) => match name.as_str() { "Int" => self.format_integer(parameters, path, input, output), - "Float" => self.format_float(input, output), - "Boolean" => self.format_boolean(input, output), - "String" => self.format_string(input, output), - "Id" => self.format_id(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, @@ -621,37 +621,97 @@ impl Query { } #[inline] - fn format_float(&self, input: &mut Value, output: &mut Value) { + 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.coersion_errors.push( + 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; } } #[inline] - fn format_boolean(&self, input: &mut Value, output: &mut Value) { + 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.coersion_errors.push( + 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, input: &mut Value, output: &mut Value) { + 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.coersion_errors.push( + 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, input: &mut Value, output: &mut Value) { + 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.coersion_errors.push( + 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; } } From b893731d63cb3d19e370bdf7b0be470f23985062 Mon Sep 17 00:00:00 2001 From: TylerBloom Date: Thu, 16 Oct 2025 21:35:32 -0400 Subject: [PATCH 07/21] Updated coersion union tests --- apollo-router/src/spec/query.rs | 4 +- apollo-router/src/spec/query/tests.rs | 64 ++++++++++++++++++--------- 2 files changed, 46 insertions(+), 22 deletions(-) diff --git a/apollo-router/src/spec/query.rs b/apollo-router/src/spec/query.rs index 5e64315527..6e56ce5ddc 100644 --- a/apollo-router/src/spec/query.rs +++ b/apollo-router/src/spec/query.rs @@ -730,6 +730,7 @@ impl Query { // validate_variables should have already checked that // the variable is present and it is of the correct type for selection in selection_set { + println!("Applying selection set: {selection:?}"); match selection { Selection::Field { name, @@ -822,7 +823,8 @@ impl Query { == type_condition.as_str() || parameters .schema - .is_subtype(type_condition, current_type.inner_named_type().as_str()); + // NOTE(@TylerBloom): Were these in backwards order? + .is_subtype(current_type.inner_named_type().as_str(), type_condition); if is_apply { // if this is the filtered query, we must keep the __typename field because the original query must know the type diff --git a/apollo-router/src/spec/query/tests.rs b/apollo-router/src/spec/query/tests.rs index 1b5beea2d6..3f51150362 100644 --- a/apollo-router/src/spec/query/tests.rs +++ b/apollo-router/src/spec/query/tests.rs @@ -2027,23 +2027,31 @@ fn reformat_response_coersion_propagation_into_union() { .schema(nullable_schema) .query(query_wo_type_info) .response(resp_wo_type_info.clone()) - .expected(json!({ "thing": { } })) + .expected(json!({ + "thing": { + "a": 1, + "b": null, + "c": null, + } + })) .expected_errors(json!([ { - "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" } }, + /* NOTE: This is reliant on the `is_subtype` reordering being true { "message": "Invalid values found for field of an abstract type without `__typename`, entire object must be nullified", "path": ["thing"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } } + */ ])) .test(); @@ -2053,7 +2061,7 @@ fn reformat_response_coersion_propagation_into_union() { .response(resp_wo_type_info.clone()) // 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(json!({ "thing": null })) .expected_errors(json!([ /* FIXME(@TylerBloom): This, per the spec, *is* expected. However, persently, the router * does not produce these errors. @@ -2061,12 +2069,18 @@ fn reformat_response_coersion_propagation_into_union() { * 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", + "message": "Cannot return null for non-nullable field Thing.b", + "path": ["thing", "b"], + "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } + } + /* + { + "message": "Invalid value found for the type Int", "path": ["thing", "c"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, @@ -2075,6 +2089,7 @@ fn reformat_response_coersion_propagation_into_union() { "path": ["thing"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } } + */ ])) .test(); @@ -2094,12 +2109,12 @@ fn reformat_response_coersion_propagation_into_union() { })) .expected_errors(json!([ { - "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" } }, @@ -2113,15 +2128,22 @@ fn reformat_response_coersion_propagation_into_union() { .expected(json!({ "thing": null })) .expected_errors(json!([ { - "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": "Cannot return null for non-nullable field Foo.b", + "path": ["thing", "b"], + "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } + } + /* + { + "message": "Invalid value found for the type Int", "path": ["thing", "c"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } } + */ ])) .test(); @@ -2134,6 +2156,9 @@ fn reformat_response_coersion_propagation_into_union() { // `__typename` being queried. .expected(json!({ "thing": null })) .expected_errors(json!([ + /* FIXME: Because __typename isn't here but maditory, so the rest is discarded. This is + * related to the `object` problem. + * At a minimun, an error for the lack of typename is needed { "message": "Invalid value found for field Query.thing.b", "path": ["thing", "b"], @@ -2154,6 +2179,7 @@ fn reformat_response_coersion_propagation_into_union() { "path": ["thing"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } } + */ ])) .test(); @@ -2169,7 +2195,7 @@ fn reformat_response_coersion_propagation_into_union() { * does not produce these errors. * FIXME(@TylerBloom): Should the `__typename` error be returned over the more general * result coersion error? - */ + * NOTE: Same problem as above { "message": "Invalid value found for field Query.thing.b", "path": ["thing", "b"], @@ -2190,6 +2216,7 @@ fn reformat_response_coersion_propagation_into_union() { "path": ["thing"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } } + */ ])) .test(); @@ -2208,12 +2235,12 @@ fn reformat_response_coersion_propagation_into_union() { })) .expected_errors(json!([ { - "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" } } @@ -2227,18 +2254,13 @@ fn reformat_response_coersion_propagation_into_union() { .expected(json!({ "thing": null })) .expected_errors(json!([ { - "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": "Cannot return null for non-nullable field Foo.b", + "path": ["thing", "b"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } } ])) From d15b0580f816f8683b3e1f751e70c1f1846cf199 Mon Sep 17 00:00:00 2001 From: TylerBloom Date: Thu, 16 Oct 2025 21:50:05 -0400 Subject: [PATCH 08/21] Updated coersion interface tests --- apollo-router/src/spec/query.rs | 1 - apollo-router/src/spec/query/tests.rs | 108 ++++++++++---------------- 2 files changed, 39 insertions(+), 70 deletions(-) diff --git a/apollo-router/src/spec/query.rs b/apollo-router/src/spec/query.rs index 6e56ce5ddc..afb71b8a6d 100644 --- a/apollo-router/src/spec/query.rs +++ b/apollo-router/src/spec/query.rs @@ -730,7 +730,6 @@ impl Query { // validate_variables should have already checked that // the variable is present and it is of the correct type for selection in selection_set { - println!("Applying selection set: {selection:?}"); match selection { Selection::Field { name, diff --git a/apollo-router/src/spec/query/tests.rs b/apollo-router/src/spec/query/tests.rs index 3f51150362..236c0d5348 100644 --- a/apollo-router/src/spec/query/tests.rs +++ b/apollo-router/src/spec/query/tests.rs @@ -2343,26 +2343,24 @@ 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(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(); @@ -2374,28 +2372,23 @@ 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(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": "Cannot return null for non-nullable field Thing.b", + "path": ["thing", "b"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } } - */ ])) .test(); @@ -2404,26 +2397,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(); @@ -2433,26 +2424,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": "Cannot return null for non-nullable field Foo.b", + "path": ["thing", "b"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } } - */ ])) .test(); @@ -2520,26 +2501,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(); @@ -2549,26 +2529,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": "Cannot return null for non-nullable field Foo.b", + "path": ["thing", "b"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } } - */ ])) .test(); } From 79d8ee75982b9ab544fa8d79cdea3f12bc9bd247 Mon Sep 17 00:00:00 2001 From: TylerBloom Date: Thu, 16 Oct 2025 22:26:57 -0400 Subject: [PATCH 09/21] Updated remaining tests --- apollo-router/src/spec/query/tests.rs | 79 ++++++++++++--------------- 1 file changed, 36 insertions(+), 43 deletions(-) diff --git a/apollo-router/src/spec/query/tests.rs b/apollo-router/src/spec/query/tests.rs index 236c0d5348..5d8e6bbfb2 100644 --- a/apollo-router/src/spec/query/tests.rs +++ b/apollo-router/src/spec/query/tests.rs @@ -1270,28 +1270,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 +1350,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 +1418,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 +1442,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": "Cannot return null for non-nullable field User.someOtherNumber", + "path": ["me", "someOtherNumber"], + "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } + }, ])) .test(); } @@ -1495,29 +1491,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 +1548,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(); @@ -1912,6 +1905,8 @@ fn reformat_response_coersion_propagation_into_object() { .response(json!({})) // FIXME: All of the errors are discarded .expected_errors(json!([ + /* FIXME: The first improperly nullified field causes an early exit, so the other fields + * are not validated. Do we want to keep with this behavior or report all error? { "message": "Invalid value found for the type Int", "path": ["thing", "a"], @@ -1922,8 +1917,6 @@ fn reformat_response_coersion_propagation_into_object() { "path": ["thing", "a"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, - /* FIXME: The first improperly nullified field causes an early exit, so the other fields - * are not validated. Do we want to keep with this behavior or report all error? { "message": "Invalid value found for the type Int", "path": ["thing", "b"], @@ -1944,12 +1937,12 @@ fn reformat_response_coersion_propagation_into_object() { "path": ["thing", "c"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, - */ { "message": "Cannot return null for non-nullable field thing", "path": ["thing"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } } + */ ])) .test(); } @@ -3424,15 +3417,15 @@ fn filter_list_errors() { .expected_extensions(json! {{ "valueCompletion": [ { - "message": "Cannot return null for non-nullable array element of type String at index 1", + "message": "Cannot return null for non-nullable array element of type String", "path": ["list", "l2", 1] }, { - "message": "Cannot return null for non-nullable array element of type String at index 2", + "message": "Cannot return null for non-nullable array element of type String", "path": ["list", "l2", 2] }, { - "message": "Cannot return null for non-nullable array element of type String at index 3", + "message": "Cannot return null for non-nullable array element of type String", "path": ["list", "l2", 3] } ] From c8e9d41c6e5e415b4bd80e241a4b731b73e8458e Mon Sep 17 00:00:00 2001 From: TylerBloom Date: Fri, 17 Oct 2025 16:46:23 -0400 Subject: [PATCH 10/21] General cleanup from comments and discussions --- apollo-router/src/spec/query.rs | 104 ++++++++++++-------------- apollo-router/src/spec/query/tests.rs | 10 ++- 2 files changed, 54 insertions(+), 60 deletions(-) diff --git a/apollo-router/src/spec/query.rs b/apollo-router/src/spec/query.rs index afb71b8a6d..46726a0903 100644 --- a/apollo-router/src/spec/query.rs +++ b/apollo-router/src/spec/query.rs @@ -341,7 +341,6 @@ 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 @@ -367,15 +366,9 @@ impl Query { // 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, - field_type, - input, - inner_type, - output, - path, - selection_set, - )?, + 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 @@ -386,7 +379,6 @@ impl Query { input, output, path, - parent_type, selection_set, )?, } @@ -402,7 +394,6 @@ impl Query { input: &mut Value, output: &mut Value, path: &mut Vec>, - parent_type: &executable::Type, selection_set: &'a [Selection], ) -> Result<(), InvalidValue> { let inner_type = match field_type { @@ -412,26 +403,10 @@ impl Query { _ => unreachable!(), }; - self.format_value( - parameters, - &inner_type, - input, - output, - path, - field_type, - selection_set, - )?; + self.format_value(parameters, &inner_type, input, output, path, selection_set)?; 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(_)) => format!( - "Cannot return null for non-nullable array element of type {inner_type}" - ), - _ => todo!(), - }; + let message = format!("Null value found for non-nullable type {inner_type}"); parameters.errors.push( Error::builder() .message(&message) @@ -457,7 +432,6 @@ impl Query { fn format_list<'a: 'b, 'b>( &'a self, parameters: &mut FormatParameters, - field_type: &executable::Type, input: &mut Value, inner_type: &executable::Type, output: &mut Value, @@ -471,33 +445,37 @@ impl Query { *output = Value::Array(vec![Value::Null; input_array.len()]); } let output_array = output.as_array_mut().ok_or(InvalidValue)?; - let mut nullify = false; - for (i, element) in input_array.iter_mut().enumerate() { - path.push(ResponsePathElement::Index(i)); - if let Err(InvalidValue) = self.format_value( - parameters, - inner_type, - element, - &mut output_array[i], - path, - field_type, - selection_set, - ) { - // TODO: Insert error - parameters.nullified.push(Path::from_response_slice(path)); - nullify = true; - } - path.pop(); - } - if nullify { + 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(()) + }) + { + parameters.nullified.push(Path::from_response_slice(path)); parameters.coersion_errors.push( Error::builder() - // FIXME: This needs to be a real error message - .message("Invalid value found for field Query.thing.a") + .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(), ); + // We pop here because, if an error is found, the path still contains the index of the + // invalid value and that needs to be removed. + path.pop(); *output = Value::Null; } Ok(()) @@ -782,7 +760,6 @@ impl Query { input_value, output_value, path, - current_type, selection_set, ); path.pop(); @@ -818,12 +795,26 @@ 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 .schema - // NOTE(@TylerBloom): Were these in backwards order? - .is_subtype(current_type.inner_named_type().as_str(), type_condition); + .is_subtype(type_condition, current_type.inner_named_type().as_str()); if is_apply { // if this is the filtered query, we must keep the __typename field because the original query must know the type @@ -859,6 +850,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( @@ -944,7 +937,6 @@ impl Query { input_value, output_value, path, - &field_type.0, selection_set, ); path.pop(); diff --git a/apollo-router/src/spec/query/tests.rs b/apollo-router/src/spec/query/tests.rs index 5d8e6bbfb2..ea6c7481f0 100644 --- a/apollo-router/src/spec/query/tests.rs +++ b/apollo-router/src/spec/query/tests.rs @@ -1996,8 +1996,8 @@ fn reformat_response_coersion_propagation_into_union() { bar: String }"#; - let query_wo_type_info = r#"{ thing { ... on Foo { a, b, c } } }"#; - let query_with_type_info = r#"{ thing { __typename ... on Foo { a, b, c } } }"#; + let query_wo_type_info = r#"{ thing { ... on Foo { a, b } ... on Bar { bar } } }"#; + let query_with_type_info = r#"{ thing { __typename ... on Foo { a, b, c } ... on Bar { bar} } }"#; let resp_wo_type_info = json!({ "thing": { @@ -2018,13 +2018,14 @@ fn reformat_response_coersion_propagation_into_union() { // Case 1: __typename isn't queried and isn't returned FormatTest::builder() .schema(nullable_schema) - .query(query_wo_type_info) - .response(resp_wo_type_info.clone()) + .query(query_with_type_info) + .response(resp_with_type_info.clone()) .expected(json!({ "thing": { "a": 1, "b": null, "c": null, + "bar": null, } })) .expected_errors(json!([ @@ -2047,6 +2048,7 @@ fn reformat_response_coersion_propagation_into_union() { */ ])) .test(); + panic!(); FormatTest::builder() .schema(non_nullable_schema) From df21d81b000f1e0cf38f9bb016bf3a30d8445193 Mon Sep 17 00:00:00 2001 From: TylerBloom Date: Mon, 20 Oct 2025 10:48:41 -0400 Subject: [PATCH 11/21] Addressed review comments and corrected tests accordingly --- apollo-router/src/spec/query.rs | 15 +- apollo-router/src/spec/query/tests.rs | 323 +++++--------------------- 2 files changed, 63 insertions(+), 275 deletions(-) diff --git a/apollo-router/src/spec/query.rs b/apollo-router/src/spec/query.rs index 46726a0903..622431a4bf 100644 --- a/apollo-router/src/spec/query.rs +++ b/apollo-router/src/spec/query.rs @@ -333,7 +333,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, @@ -386,7 +385,6 @@ impl Query { } #[inline] - #[allow(clippy::too_many_arguments)] fn format_non_nullable_value<'a: 'b, 'b>( &'a self, parameters: &mut FormatParameters, @@ -428,7 +426,6 @@ impl Query { } #[inline] - #[allow(clippy::too_many_arguments)] fn format_list<'a: 'b, 'b>( &'a self, parameters: &mut FormatParameters, @@ -463,6 +460,9 @@ impl Query { 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.coersion_errors.push( Error::builder() @@ -473,9 +473,6 @@ impl Query { .extension("code", ERROR_CODE_RESPONSE_VALIDATION) .build(), ); - // We pop here because, if an error is found, the path still contains the index of the - // invalid value and that needs to be removed. - path.pop(); *output = Value::Null; } Ok(()) @@ -772,11 +769,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); diff --git a/apollo-router/src/spec/query/tests.rs b/apollo-router/src/spec/query/tests.rs index ea6c7481f0..156f362cf6 100644 --- a/apollo-router/src/spec/query/tests.rs +++ b/apollo-router/src/spec/query/tests.rs @@ -1448,7 +1448,7 @@ fn reformat_response_expected_int_range() { "extensions": { "code": "RESPONSE_VALIDATION_FAILED" }, }, { - "message": "Cannot return null for non-nullable field User.someOtherNumber", + "message": "Null value found for non-nullable type Int", "path": ["me", "someOtherNumber"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, @@ -1705,6 +1705,8 @@ 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!([ { "message": "Invalid value found for the type Int", @@ -1712,22 +1714,12 @@ fn reformat_response_coersion_propagation_into_list() { "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Cannot return null for non-nullable array element of type Int", + "message": "Null value found for non-nullable type Int", "path": ["thing", "a", 1], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Invalid value found for the type Int", - "path": ["thing", "a", 2], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - }, - { - "message": "Cannot return null for non-nullable array element of type Int", - "path": ["thing", "a", 2], - "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" } } @@ -1755,6 +1747,8 @@ 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!([ { "message": "Invalid value found for the type Int", @@ -1762,27 +1756,17 @@ fn reformat_response_coersion_propagation_into_list() { "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Cannot return null for non-nullable array element of type Int", + "message": "Null value found for non-nullable type Int", "path": ["thing", "a", 1], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Invalid value found for the type Int", - "path": ["thing", "a", 2], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - }, - { - "message": "Cannot return null for non-nullable array element of type Int", - "path": ["thing", "a", 2], - "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": "Cannot return null for non-nullable field Thing.a", + "message": "Null value found for non-nullable type [Int!]", "path": ["thing", "a"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } } @@ -1860,6 +1844,8 @@ 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!([ { "message": "Invalid value found for the type Int", @@ -1867,23 +1853,10 @@ fn reformat_response_coersion_propagation_into_object() { "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Cannot return null for non-nullable field Thing.b", + "message": "Null value found for non-nullable type Int", "path": ["thing", "b"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, - /* FIXME: The first improperly nullified field causes an early exit, so the other fields - * are not validated. Do we want to keep with this behavior or report all error? - { - "message": "Invalid value found for the type Int", - "path": ["thing", "c"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - }, - { - "message": "Cannot return null for non-nullable field Thing.c", - "path": ["thing", "c"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - } - */ ])) .test(); @@ -1902,47 +1875,29 @@ fn reformat_response_coersion_propagation_into_object() { "#, ) .query(r#"{ thing { a, b, c } }"#) - .response(json!({})) - // FIXME: All of the errors are discarded - .expected_errors(json!([ - /* FIXME: The first improperly nullified field causes an early exit, so the other fields - * are not validated. Do we want to keep with this behavior or report all error? - { - "message": "Invalid value found for the type Int", - "path": ["thing", "a"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - }, - { - "message": "Cannot return null for non-nullable field Thing.b", - "path": ["thing", "a"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - }, - { + .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": "Cannot return null for non-nullable field Thing.b", + "message": "Null value found for non-nullable type Int", "path": ["thing", "b"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - }, - { - "message": "Invalid value found for the type Int", - "path": ["thing", "c"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - }, - { - "message": "Cannot return null for non-nullable field Thing.c", - "path": ["thing", "c"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - }, - { - "message": "Cannot return null for non-nullable field thing", + }, { + "message": "Null value found for non-nullable type Thing", "path": ["thing"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } } - */ ])) .test(); } @@ -1996,8 +1951,8 @@ fn reformat_response_coersion_propagation_into_union() { bar: String }"#; - let query_wo_type_info = r#"{ thing { ... on Foo { a, b } ... on Bar { bar } } }"#; - let query_with_type_info = r#"{ thing { __typename ... on Foo { a, b, c } ... on Bar { bar} } }"#; + let query_wo_type_info = r#"{ thing { ... on Foo { a, b, c } } }"#; + let query_with_type_info = r#"{ thing { __typename ... on Foo { a, b, c } } }"#; let resp_wo_type_info = json!({ "thing": { @@ -2018,14 +1973,13 @@ fn reformat_response_coersion_propagation_into_union() { // Case 1: __typename isn't queried and isn't returned FormatTest::builder() .schema(nullable_schema) - .query(query_with_type_info) + .query(query_wo_type_info) .response(resp_with_type_info.clone()) .expected(json!({ "thing": { "a": 1, "b": null, "c": null, - "bar": null, } })) .expected_errors(json!([ @@ -2039,16 +1993,8 @@ fn reformat_response_coersion_propagation_into_union() { "path": ["thing", "c"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, - /* NOTE: This is reliant on the `is_subtype` reordering being true - { - "message": "Invalid values found for field of an abstract type without `__typename`, entire object must be nullified", - "path": ["thing"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - } - */ ])) .test(); - panic!(); FormatTest::builder() .schema(non_nullable_schema) @@ -2056,36 +2002,9 @@ fn reformat_response_coersion_propagation_into_union() { .response(resp_wo_type_info.clone()) // 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": 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 the type Int", - "path": ["thing", "b"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - }, - { - "message": "Cannot return null for non-nullable field Thing.b", - "path": ["thing", "b"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - } - /* - { - "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`, entire object must be nullified", - "path": ["thing"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - } - */ - ])) + .expected(json!({ "thing": { } })) + // 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 @@ -2121,6 +2040,8 @@ 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!([ { "message": "Invalid value found for the type Int", @@ -2128,17 +2049,10 @@ fn reformat_response_coersion_propagation_into_union() { "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Cannot return null for non-nullable field Foo.b", + "message": "Null value found for non-nullable type Int", "path": ["thing", "b"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } } - /* - { - "message": "Invalid value found for the type Int", - "path": ["thing", "c"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - } - */ ])) .test(); @@ -2147,72 +2061,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: Because __typename isn't here but maditory, so the rest is discarded. This is - * related to the `object` problem. - * At a minimun, an error for the lack of typename is needed - { - "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? - * NOTE: Same problem as above - { - "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 @@ -2247,6 +2105,8 @@ 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!([ { "message": "Invalid value found for the type Int", @@ -2254,7 +2114,7 @@ fn reformat_response_coersion_propagation_into_union() { "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Cannot return null for non-nullable field Foo.b", + "message": "Null value found for non-nullable type Int", "path": ["thing", "b"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } } @@ -2341,22 +2201,9 @@ fn reformat_response_coersion_propagation_into_interfaces() { .expected(json!({ "thing": { "a": 1, - "b": null, - "c": null, } })) - .expected_errors(json!([ - { - "message": "Invalid value found for the type Int", - "path": ["thing", "b"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - }, - { - "message": "Invalid value found for the type Int", - "path": ["thing", "c"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - }, - ])) + .expected_errors(json!([])) .test(); FormatTest::builder() @@ -2367,24 +2214,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": 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 the type Int", - "path": ["thing", "b"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - }, - { - "message": "Cannot return null for non-nullable field Thing.b", - "path": ["thing", "b"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } + .expected(json!({ + "thing": { + "a": 1 } - ])) + })) + .expected_errors(json!([])) .test(); // Case 2: __typename isn't queried but is returned @@ -2418,6 +2253,8 @@ 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!([ { "message": "Invalid value found for the type Int", @@ -2425,7 +2262,7 @@ fn reformat_response_coersion_propagation_into_interfaces() { "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Cannot return null for non-nullable field Foo.b", + "message": "Null value found for non-nullable type Int", "path": ["thing", "b"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } } @@ -2440,26 +2277,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() @@ -2467,28 +2285,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 @@ -2523,6 +2320,8 @@ fn reformat_response_coersion_propagation_into_interfaces() { .query(query_with_type_info) .response(resp_with_type_info.clone()) .expected(json!({ "thing": null })) + // FIXME(@TylerBloom): This is not expected. This should behave the same with(out) + // `__typename` being queried. .expected_errors(json!([ { "message": "Invalid value found for the type Int", @@ -2530,7 +2329,7 @@ fn reformat_response_coersion_propagation_into_interfaces() { "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } }, { - "message": "Cannot return null for non-nullable field Foo.b", + "message": "Null value found for non-nullable type Int", "path": ["thing", "b"], "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } } @@ -3419,19 +3218,11 @@ fn filter_list_errors() { .expected_extensions(json! {{ "valueCompletion": [ { - "message": "Cannot return null for non-nullable array element of type String", + "message": "Null value found for non-nullable type String", "path": ["list", "l2", 1] }, - { - "message": "Cannot return null for non-nullable array element of type String", - "path": ["list", "l2", 2] - }, - { - "message": "Cannot return null for non-nullable array element of type String", - "path": ["list", "l2", 3] - } ] - }},) + }}) .test(); FormatTest::builder() @@ -3630,7 +3421,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] } ] From ba2e2a01c0a85c208fc71824bd62d0ac301d26aa Mon Sep 17 00:00:00 2001 From: TylerBloom Date: Mon, 20 Oct 2025 12:10:04 -0400 Subject: [PATCH 12/21] Addressed more review comments --- apollo-router/src/spec/query.rs | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/apollo-router/src/spec/query.rs b/apollo-router/src/spec/query.rs index 622431a4bf..8a7ea4026d 100644 --- a/apollo-router/src/spec/query.rs +++ b/apollo-router/src/spec/query.rs @@ -146,7 +146,7 @@ impl Query { variables: &variables, schema, errors: Vec::new(), - coersion_errors: Vec::new(), + coercion_errors: Vec::new(), nullified: Vec::new(), }; @@ -204,7 +204,7 @@ impl Query { variables: &all_variables, schema, errors: Vec::new(), - coersion_errors: Vec::new(), + coercion_errors: Vec::new(), nullified: Vec::new(), }; @@ -229,8 +229,8 @@ impl Query { .insert(EXTENSIONS_VALUE_COMPLETION_KEY, value); } - if !parameters.coersion_errors.is_empty() { - response.errors.append(&mut parameters.coersion_errors); + if !parameters.coercion_errors.is_empty() { + response.errors.append(&mut parameters.coercion_errors); } return parameters.nullified; @@ -398,7 +398,11 @@ impl Query { 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 - _ => unreachable!(), + _ => { + tracing::error!("`format_non_nullable_value` was called with a nullable type!!"); + debug_assert!(field_type.is_non_null()); + return Err(InvalidValue); + } }; self.format_value(parameters, &inner_type, input, output, path, selection_set)?; @@ -411,7 +415,7 @@ impl Query { .path(Path::from_response_slice(path)) .build(), ); - parameters.coersion_errors.push( + parameters.coercion_errors.push( Error::builder() .message(message) .path(Path::from_response_slice(path)) @@ -464,7 +468,7 @@ impl Query { // invalid value. path.pop(); parameters.nullified.push(Path::from_response_slice(path)); - parameters.coersion_errors.push( + parameters.coercion_errors.push( Error::builder() .message(format!( "Invalid value found inside the array of type [{inner_type}]" @@ -583,7 +587,7 @@ impl Query { *output = input.clone(); } else { if !input.is_null() { - parameters.coersion_errors.push( + parameters.coercion_errors.push( Error::builder() .message("Invalid value found for the type Int") .path(Path::from_response_slice(path)) @@ -607,7 +611,7 @@ impl Query { *output = input.clone(); } else { if !input.is_null() { - parameters.coersion_errors.push( + parameters.coercion_errors.push( Error::builder() .message("Invalid value found for the type Float") .path(Path::from_response_slice(path)) @@ -631,7 +635,7 @@ impl Query { *output = input.clone(); } else { if !input.is_null() { - parameters.coersion_errors.push( + parameters.coercion_errors.push( Error::builder() .message("Invalid value found for the type Boolean") .path(Path::from_response_slice(path)) @@ -655,7 +659,7 @@ impl Query { *output = input.clone(); } else { if !input.is_null() { - parameters.coersion_errors.push( + parameters.coercion_errors.push( Error::builder() .message("Invalid value found for the type String") .path(Path::from_response_slice(path)) @@ -679,7 +683,7 @@ impl Query { *output = input.clone(); } else { if !input.is_null() { - parameters.coersion_errors.push( + parameters.coercion_errors.push( Error::builder() .message("Invalid value found for the type ID") .path(Path::from_response_slice(path)) @@ -1167,7 +1171,7 @@ impl Query { struct FormatParameters<'a> { variables: &'a Object, errors: Vec, - coersion_errors: Vec, + coercion_errors: Vec, nullified: Vec, schema: &'a ApiSchema, } From 339979580f3c652f1fd24ea9c3752934c36749f4 Mon Sep 17 00:00:00 2001 From: TylerBloom Date: Mon, 20 Oct 2025 15:52:30 -0400 Subject: [PATCH 13/21] Minor cleanup in tests --- apollo-router/src/spec/query/tests.rs | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/apollo-router/src/spec/query/tests.rs b/apollo-router/src/spec/query/tests.rs index 156f362cf6..9e3922203b 100644 --- a/apollo-router/src/spec/query/tests.rs +++ b/apollo-router/src/spec/query/tests.rs @@ -1644,7 +1644,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#" @@ -1775,7 +1775,7 @@ fn reformat_response_coersion_propagation_into_list() { } #[test] -fn reformat_response_coersion_propagation_into_object() { +fn reformat_response_coercion_propagation_into_object() { FormatTest::builder() .schema( r#" @@ -1916,7 +1916,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 @@ -1974,14 +1974,8 @@ fn reformat_response_coersion_propagation_into_union() { FormatTest::builder() .schema(nullable_schema) .query(query_wo_type_info) - .response(resp_with_type_info.clone()) - .expected(json!({ - "thing": { - "a": 1, - "b": null, - "c": null, - } - })) + .response(resp_wo_type_info.clone()) + .expected(json!({ "thing": { } })) .expected_errors(json!([ { "message": "Invalid value found for the type Int", @@ -2133,7 +2127,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 @@ -2320,8 +2314,6 @@ fn reformat_response_coersion_propagation_into_interfaces() { .query(query_with_type_info) .response(resp_with_type_info.clone()) .expected(json!({ "thing": null })) - // FIXME(@TylerBloom): This is not expected. This should behave the same with(out) - // `__typename` being queried. .expected_errors(json!([ { "message": "Invalid value found for the type Int", From 567862fae0a1a8c0efc60154207d176cc46aab09 Mon Sep 17 00:00:00 2001 From: TylerBloom Date: Mon, 20 Oct 2025 18:36:51 -0400 Subject: [PATCH 14/21] Updated snapshots --- ...eferred_fragment_bounds_nullability-2.snap | 7 ++--- ...filter_nullified_deferred_responses-2.snap | 3 ++- ...pergraph__tests__nullability_bubbling.snap | 3 ++- ...ntext_unrelated_fetch_failure_rust_qp.snap | 26 ++++++++++++++++--- 4 files changed, 31 insertions(+), 8 deletions(-) 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/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" ] From 5fdd31bbcbba61c1e63cdbf5f7da5d817c477e32 Mon Sep 17 00:00:00 2001 From: TylerBloom Date: Mon, 20 Oct 2025 18:55:25 -0400 Subject: [PATCH 15/21] Corrected union test --- apollo-router/src/spec/query/tests.rs | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/apollo-router/src/spec/query/tests.rs b/apollo-router/src/spec/query/tests.rs index 9e3922203b..58ee8f59e5 100644 --- a/apollo-router/src/spec/query/tests.rs +++ b/apollo-router/src/spec/query/tests.rs @@ -1976,18 +1976,7 @@ fn reformat_response_coercion_propagation_into_union() { .query(query_wo_type_info) .response(resp_wo_type_info.clone()) .expected(json!({ "thing": { } })) - .expected_errors(json!([ - { - "message": "Invalid value found for the type Int", - "path": ["thing", "b"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - }, - { - "message": "Invalid value found for the type Int", - "path": ["thing", "c"], - "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } - }, - ])) + .expected_errors(json!([])) .test(); FormatTest::builder() From e0dfa52c13cd8711c209b7861807645df6340b6f Mon Sep 17 00:00:00 2001 From: TylerBloom Date: Tue, 21 Oct 2025 13:56:12 -0400 Subject: [PATCH 16/21] Made coercion errors opt-in --- .../feat_tylerb_add_result_coercion_errors.md | 8 +++++ apollo-router/src/configuration/mod.rs | 12 +++++++ .../src/services/execution/service.rs | 10 ++++++ .../src/services/supergraph/service.rs | 1 + apollo-router/src/spec/query.rs | 35 ++++++++++++------- apollo-router/src/spec/query/tests.rs | 4 +++ 6 files changed, 58 insertions(+), 12 deletions(-) create mode 100644 .changesets/feat_tylerb_add_result_coercion_errors.md 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..cb56d069c3 --- /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 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..bb8954650a 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 return 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 the + /// response. + pub(crate) insert_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(), + insert_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(), + insert_result_coercion_errors: insert_result_coercion_errors.unwrap_or_default(), } } } diff --git a/apollo-router/src/services/execution/service.rs b/apollo-router/src/services/execution/service.rs index 912e949eb7..8c628f7030 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.insert_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: self.configuration.clone(), } .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..a557d97be4 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: configuration.clone(), }; let execution_service: execution::BoxCloneService = ServiceBuilder::new() diff --git a/apollo-router/src/spec/query.rs b/apollo-router/src/spec/query.rs index 8a7ea4026d..a1c8f16bb7 100644 --- a/apollo-router/src/spec/query.rs +++ b/apollo-router/src/spec/query.rs @@ -128,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); @@ -146,7 +147,7 @@ impl Query { variables: &variables, schema, errors: Vec::new(), - coercion_errors: Vec::new(), + coercion_errors: include_coercion_errors.then(Vec::new), nullified: Vec::new(), }; @@ -204,7 +205,7 @@ impl Query { variables: &all_variables, schema, errors: Vec::new(), - coercion_errors: Vec::new(), + coercion_errors: include_coercion_errors.then(Vec::new), nullified: Vec::new(), }; @@ -229,8 +230,10 @@ impl Query { .insert(EXTENSIONS_VALUE_COMPLETION_KEY, value); } - if !parameters.coercion_errors.is_empty() { - response.errors.append(&mut parameters.coercion_errors); + if let Some(errors) = parameters.coercion_errors.as_mut() + && !errors.is_empty() + { + response.errors.append(errors); } return parameters.nullified; @@ -415,7 +418,7 @@ impl Query { .path(Path::from_response_slice(path)) .build(), ); - parameters.coercion_errors.push( + parameters.insert_coercion_error( Error::builder() .message(message) .path(Path::from_response_slice(path)) @@ -468,7 +471,7 @@ impl Query { // invalid value. path.pop(); parameters.nullified.push(Path::from_response_slice(path)); - parameters.coercion_errors.push( + parameters.insert_coercion_error( Error::builder() .message(format!( "Invalid value found inside the array of type [{inner_type}]" @@ -587,7 +590,7 @@ impl Query { *output = input.clone(); } else { if !input.is_null() { - parameters.coercion_errors.push( + parameters.insert_coercion_error( Error::builder() .message("Invalid value found for the type Int") .path(Path::from_response_slice(path)) @@ -611,7 +614,7 @@ impl Query { *output = input.clone(); } else { if !input.is_null() { - parameters.coercion_errors.push( + parameters.insert_coercion_error( Error::builder() .message("Invalid value found for the type Float") .path(Path::from_response_slice(path)) @@ -635,7 +638,7 @@ impl Query { *output = input.clone(); } else { if !input.is_null() { - parameters.coercion_errors.push( + parameters.insert_coercion_error( Error::builder() .message("Invalid value found for the type Boolean") .path(Path::from_response_slice(path)) @@ -659,7 +662,7 @@ impl Query { *output = input.clone(); } else { if !input.is_null() { - parameters.coercion_errors.push( + parameters.insert_coercion_error( Error::builder() .message("Invalid value found for the type String") .path(Path::from_response_slice(path)) @@ -683,7 +686,7 @@ impl Query { *output = input.clone(); } else { if !input.is_null() { - parameters.coercion_errors.push( + parameters.insert_coercion_error( Error::builder() .message("Invalid value found for the type ID") .path(Path::from_response_slice(path)) @@ -1171,11 +1174,19 @@ impl Query { struct FormatParameters<'a> { variables: &'a Object, errors: Vec, - coercion_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 58ee8f59e5..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 { @@ -6254,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(), @@ -6965,6 +6967,7 @@ fn filtered_defer_fragment() { Object::new(), schema.api_schema(), BooleanValues { bits: 0 }, + true, ); assert_json_snapshot!(response); @@ -6974,6 +6977,7 @@ fn filtered_defer_fragment() { Object::new(), schema.api_schema(), BooleanValues { bits: 0 }, + true, ); assert_json_snapshot!(response); From 1d93b1c49e55c3d29573bd32b7a1408d4599a8e4 Mon Sep 17 00:00:00 2001 From: TylerBloom Date: Tue, 21 Oct 2025 14:28:02 -0400 Subject: [PATCH 17/21] Addressed more review comments --- apollo-router/src/configuration/mod.rs | 6 +++--- apollo-router/src/services/execution/service.rs | 4 ++-- apollo-router/src/services/supergraph/service.rs | 2 +- apollo-router/tests/set_context.rs | 1 + 4 files changed, 7 insertions(+), 6 deletions(-) diff --git a/apollo-router/src/configuration/mod.rs b/apollo-router/src/configuration/mod.rs index bb8954650a..7d3f267bde 100644 --- a/apollo-router/src/configuration/mod.rs +++ b/apollo-router/src/configuration/mod.rs @@ -738,7 +738,7 @@ pub(crate) struct Supergraph { /// 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 the /// response. - pub(crate) insert_result_coercion_errors: bool, + pub(crate) enable_result_coercion_errors: bool, /// Log a message if the client closes the connection before the response is sent. /// Default: false. @@ -780,7 +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(), - insert_result_coercion_errors: insert_result_coercion_errors.unwrap_or_default(), + enable_result_coercion_errors: insert_result_coercion_errors.unwrap_or_default(), } } } @@ -813,7 +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(), - insert_result_coercion_errors: insert_result_coercion_errors.unwrap_or_default(), + enable_result_coercion_errors: insert_result_coercion_errors.unwrap_or_default(), } } } diff --git a/apollo-router/src/services/execution/service.rs b/apollo-router/src/services/execution/service.rs index 8c628f7030..bfbf720885 100644 --- a/apollo-router/src/services/execution/service.rs +++ b/apollo-router/src/services/execution/service.rs @@ -195,7 +195,7 @@ impl ExecutionService { let execution_span = Span::current(); let insert_result_coercion_errors = - self.configuration.supergraph.insert_result_coercion_errors; + self.configuration.supergraph.enable_result_coercion_errors; let stream = stream .map(move |mut response: Response| { @@ -672,7 +672,7 @@ impl ServiceFactory for ExecutionServiceFactory { subscription_config: subscription_plugin_conf, subgraph_schemas: self.subgraph_schemas.clone(), apollo_telemetry_config: apollo_telemetry_conf, - configuration: self.configuration.clone(), + 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 a557d97be4..8116bf5f78 100644 --- a/apollo-router/src/services/supergraph/service.rs +++ b/apollo-router/src/services/supergraph/service.rs @@ -561,7 +561,7 @@ impl PluggableSupergraphServiceBuilder { subgraph_schemas: query_planner_service.subgraph_schemas(), plugins: self.plugins.clone(), fetch_service_factory, - configuration: configuration.clone(), + configuration: Arc::clone(&configuration), }; let execution_service: execution::BoxCloneService = ServiceBuilder::new() 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, } From d37d4abea7637c0f9bea4da8811a88a91bb3d1c2 Mon Sep 17 00:00:00 2001 From: Tyler Bloom Date: Tue, 21 Oct 2025 14:28:31 -0400 Subject: [PATCH 18/21] Spell check Co-authored-by: Aaron --- apollo-router/src/configuration/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apollo-router/src/configuration/mod.rs b/apollo-router/src/configuration/mod.rs index 7d3f267bde..720944d24c 100644 --- a/apollo-router/src/configuration/mod.rs +++ b/apollo-router/src/configuration/mod.rs @@ -732,7 +732,7 @@ 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 return in + /// 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 From 452067dd10e4a4730f77bf26e5daa87be216006b Mon Sep 17 00:00:00 2001 From: TylerBloom Date: Tue, 21 Oct 2025 14:29:25 -0400 Subject: [PATCH 19/21] Spelling checking --- .changesets/feat_tylerb_add_result_coercion_errors.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changesets/feat_tylerb_add_result_coercion_errors.md b/.changesets/feat_tylerb_add_result_coercion_errors.md index cb56d069c3..a7c0259a9a 100644 --- a/.changesets/feat_tylerb_add_result_coercion_errors.md +++ b/.changesets/feat_tylerb_add_result_coercion_errors.md @@ -3,6 +3,6 @@ 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 response. +the errors array in the response. By [@TylerBloom](https://github.com/TylerBloom) in https://github.com/apollographql/router/pull/8441 From 032ebf59f8d1cc2d19ac610875488a0288523cf0 Mon Sep 17 00:00:00 2001 From: TylerBloom Date: Tue, 21 Oct 2025 14:30:37 -0400 Subject: [PATCH 20/21] Spelling checking --- apollo-router/src/configuration/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apollo-router/src/configuration/mod.rs b/apollo-router/src/configuration/mod.rs index 720944d24c..3e7a547a85 100644 --- a/apollo-router/src/configuration/mod.rs +++ b/apollo-router/src/configuration/mod.rs @@ -736,8 +736,8 @@ pub(crate) struct Supergraph { /// 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 the - /// response. + /// 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. From d9d97d0ba78f87df8374cd46ced9bc6f6f060a95 Mon Sep 17 00:00:00 2001 From: TylerBloom Date: Tue, 21 Oct 2025 14:49:43 -0400 Subject: [PATCH 21/21] Updated config snapshot test --- ...llo_router__configuration__tests__schema_generation.snap | 6 ++++++ 1 file changed, 6 insertions(+) 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,