Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add errors for response validation #5787

Open
wants to merge 32 commits into
base: dev
Choose a base branch
from
Open

add errors for response validation #5787

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
b2bddc3
add errors for response validation
Geal Aug 7, 2024
97a748d
Merge branch 'dev' into geal/response-validation-errors
Geal Aug 20, 2024
6d62d38
lint
Geal Aug 20, 2024
ffde309
lint
Geal Aug 20, 2024
6733583
align error messages with the gateway
Geal Aug 20, 2024
4747e4a
error messages for invalid typename values
Geal Aug 20, 2024
24d46b9
lint
Geal Aug 20, 2024
36231f5
update error messages
Geal Aug 20, 2024
9cf401e
Merge branch 'dev' into geal/response-validation-errors
Geal Aug 27, 2024
cecf18a
Update apollo-router/src/json_ext.rs
Geal Aug 27, 2024
965cfd2
revert __typename validation
Geal Aug 27, 2024
6f2fcaf
let null values go through
Geal Aug 27, 2024
99aef34
fix
Geal Aug 27, 2024
a59f15d
changeset
Geal Aug 27, 2024
b201d28
Merge branch 'dev' into geal/response-validation-errors
Geal Aug 30, 2024
cb6eda8
Merge branch 'dev' into geal/response-validation-errors
Geal Sep 16, 2024
d14767a
add a test
Geal Sep 16, 2024
8fe0b68
fix enum validation error message
Geal Sep 16, 2024
9d17497
check lists
Geal Sep 16, 2024
d23b782
Merge branch 'dev' into geal/response-validation-errors
Geal Oct 15, 2024
6881f9b
add an error code
Geal Oct 15, 2024
4006b2e
Add a test for out-of-range values for `Int`
andrewmcgivery Oct 11, 2024
468ae57
Update test from #6143 to match the implementation from #5787
goto-bus-stop Oct 17, 2024
e6020bb
Fix parent type when result-coercing non-null types
goto-bus-stop Oct 17, 2024
45c7e8a
Fix result coercion for ID fields
goto-bus-stop Oct 17, 2024
7c7ba29
Add more tests for response value coercion/validation
goto-bus-stop Oct 22, 2024
c34b7d1
Move the response validation tests together
goto-bus-stop Oct 22, 2024
536d9f6
Adjust test expectations to match current behaviour
goto-bus-stop Oct 22, 2024
877eb2c
Correct parent type used for top-level fields
goto-bus-stop Oct 22, 2024
4fa8353
lint
goto-bus-stop Oct 22, 2024
e206b6d
Fix snapshot
goto-bus-stop Oct 22, 2024
f0f7300
Revert ID validation fix--moving to separate PR
goto-bus-stop Oct 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions apollo-router/src/json_ext.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,9 @@ pub(crate) trait ValueExt {
/// function to handle `PathElement::Fragment`).
#[track_caller]
fn is_object_of_type(&self, schema: &Schema, maybe_type: &str) -> bool;

/// value type
fn json_type_name(&self) -> &'static str;
}

impl ValueExt for Value {
Expand Down Expand Up @@ -468,6 +471,17 @@ impl ValueExt for Value {
typename == maybe_type || schema.is_subtype(maybe_type, typename)
})
}

fn json_type_name(&self) -> &'static str {
match self {
Value::Array(_) => "array",
Value::Null => "null",
Value::Bool(_) => "booleqn",
Geal marked this conversation as resolved.
Show resolved Hide resolved
Value::Number(_) => "number",
Value::String(_) => "string",
Value::Object(_) => "object",
}
}
}

fn filter_type_conditions(value: Value, type_conditions: &Option<TypeConditions>) -> Value {
Expand Down
156 changes: 131 additions & 25 deletions apollo-router/src/spec/query.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ use crate::json_ext::Object;
use crate::json_ext::Path;
use crate::json_ext::ResponsePathElement;
use crate::json_ext::Value;
use crate::json_ext::ValueExt;
use crate::plugins::authorization::UnauthorizedPaths;
use crate::query_planner::fetch::OperationKind;
use crate::query_planner::fetch::QueryHash;
Expand Down Expand Up @@ -144,8 +145,9 @@ impl Query {
let mut parameters = FormatParameters {
variables: &variables,
schema,
errors: Vec::new(),
nullification_errors: Vec::new(),
nullified: Vec::new(),
validation_errors: Vec::new(),
};
// Detect if root __typename is asked in the original query (the qp doesn't put root __typename in subselections)
// cf https://github.com/apollographql/router/issues/1677
Expand Down Expand Up @@ -174,12 +176,18 @@ impl Query {
},
);

if !parameters.errors.is_empty() {
if let Ok(value) = serde_json_bytes::to_value(&parameters.errors) {
if !parameters.nullification_errors.is_empty() {
if let Ok(value) =
serde_json_bytes::to_value(&parameters.nullification_errors)
{
response.extensions.insert("valueCompletion", value);
}
}

if !parameters.validation_errors.is_empty() {
response.errors.append(&mut parameters.validation_errors);
}

return parameters.nullified;
}
None => {
Expand Down Expand Up @@ -211,8 +219,9 @@ impl Query {
let mut parameters = FormatParameters {
variables: &all_variables,
schema,
errors: Vec::new(),
nullification_errors: Vec::new(),
nullified: Vec::new(),
validation_errors: Vec::new(),
};

response.data = Some(
Expand All @@ -228,12 +237,18 @@ impl Query {
Err(InvalidValue) => Value::Null,
},
);
if !parameters.errors.is_empty() {
if let Ok(value) = serde_json_bytes::to_value(&parameters.errors) {
if !parameters.nullification_errors.is_empty() {
if let Ok(value) =
serde_json_bytes::to_value(&parameters.nullification_errors)
{
response.extensions.insert("valueCompletion", value);
}
}

if !parameters.validation_errors.is_empty() {
response.errors.append(&mut parameters.validation_errors);
}

return parameters.nullified;
} else {
failfast_debug!("can't find operation for {:?}", operation_name);
Expand Down Expand Up @@ -374,6 +389,7 @@ impl Query {
output: &mut Value,
path: &mut Vec<ResponsePathElement<'b>>,
parent_type: &executable::Type,
field_or_index: FieldOrIndex<'a>,
selection_set: &'a [Selection],
) -> Result<(), InvalidValue> {
// for every type, if we have an invalid value, we will replace it with null
Expand All @@ -395,6 +411,7 @@ impl Query {
output,
path,
field_type,
field_or_index,
selection_set,
) {
Err(_) => Err(InvalidValue),
Expand All @@ -409,7 +426,7 @@ impl Query {
),
_ => todo!(),
};
parameters.errors.push(Error {
parameters.nullification_errors.push(Error {
message,
path: Some(Path::from_response_slice(path)),
..Error::default()
Expand Down Expand Up @@ -449,6 +466,7 @@ impl Query {
&mut output_array[i],
path,
field_type,
FieldOrIndex::Index(i),
selection_set,
);
path.pop();
Expand All @@ -462,7 +480,19 @@ impl Query {
Ok(()) => Ok(()),
}
}
_ => Ok(()),
Value::Null => Ok(()),
v => {
parameters.validation_errors.push(Error {
message: format!(
"Invalid non-list value of type {} for list type {field_type}",
v.json_type_name()
),
path: Some(Path::from_response_slice(path)),
..Error::default()
});
*output = Value::Null;
Ok(())
}
},
executable::Type::Named(name) if name == "Int" => {
let opt = if input.is_i64() {
Expand All @@ -478,6 +508,11 @@ impl Query {
if opt.is_some() {
*output = input.clone();
} else {
parameters.validation_errors.push(Error {
message: invalid_value_message(parent_type, field_type, field_or_index),
path: Some(Path::from_response_slice(path)),
..Error::default()
});
*output = Value::Null;
}
Ok(())
Expand All @@ -486,6 +521,11 @@ impl Query {
if input.as_f64().is_some() {
*output = input.clone();
} else {
parameters.validation_errors.push(Error {
message: invalid_value_message(parent_type, field_type, field_or_index),
path: Some(Path::from_response_slice(path)),
..Error::default()
});
*output = Value::Null;
}
Ok(())
Expand All @@ -494,6 +534,11 @@ impl Query {
if input.as_bool().is_some() {
*output = input.clone();
} else {
parameters.validation_errors.push(Error {
message: invalid_value_message(parent_type, field_type, field_or_index),
path: Some(Path::from_response_slice(path)),
..Error::default()
});
*output = Value::Null;
}
Ok(())
Expand All @@ -502,6 +547,11 @@ impl Query {
if input.as_str().is_some() {
*output = input.clone();
} else {
parameters.validation_errors.push(Error {
message: invalid_value_message(parent_type, field_type, field_or_index),
path: Some(Path::from_response_slice(path)),
..Error::default()
});
*output = Value::Null;
}
Ok(())
Expand All @@ -510,6 +560,11 @@ impl Query {
if input.is_string() || input.is_i64() || input.is_u64() || input.is_f64() {
*output = input.clone();
} else {
parameters.validation_errors.push(Error {
message: invalid_value_message(parent_type, field_type, field_or_index),
path: Some(Path::from_response_slice(path)),
..Error::default()
});
*output = Value::Null;
}
Ok(())
Expand All @@ -529,11 +584,25 @@ impl Query {
*output = input.clone();
Ok(())
} else {
parameters.validation_errors.push(Error {
message: format!(
"Expected a valid enum value for type {enum_type}"
),
path: Some(Path::from_response_slice(path)),
..Error::default()
});
*output = Value::Null;
Ok(())
}
}
None => {
parameters.validation_errors.push(Error {
message: format!(
"Expected a valid enum value for type {enum_type}"
),
path: Some(Path::from_response_slice(path)),
..Error::default()
});
*output = Value::Null;
Ok(())
}
Expand All @@ -544,20 +613,28 @@ impl Query {

match input {
Value::Object(ref mut 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));
if let Some(typename_val) = input_object.get(TYPENAME) {
if let Some(input_type) = typename_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(());
};
} else {
parameters.validation_errors.push(Error {
message: format!("Invalid non-string value of type {} for __typename at {path:?}", typename_val.json_type_name()),
path: Some(Path::from_response_slice(path)),
..Error::default()
});
*output = Value::Null;
return Ok(());
};
}
}

if output.is_null() {
Expand Down Expand Up @@ -602,8 +679,14 @@ impl Query {

Ok(())
}
_ => {
parameters.nullified.push(Path::from_response_slice(path));
v => {
parameters.validation_errors.push(Error {
message: format!(
"Invalid non-object value of type {} for composite type {type_name}", v.json_type_name()
),
path: Some(Path::from_response_slice(path)),
..Error::default()
});
*output = Value::Null;
Ok(())
}
Expand Down Expand Up @@ -682,6 +765,7 @@ impl Query {
output_value,
path,
current_type,
FieldOrIndex::Field(field_name.as_str()),
selection_set,
);
path.pop();
Expand All @@ -691,7 +775,7 @@ impl Query {
output.insert((*field_name).clone(), Value::Null);
}
if field_type.is_non_null() {
parameters.errors.push(Error {
parameters.nullification_errors.push(Error {
message: format!(
"Cannot return null for non-nullable field {current_type}.{}",
field_name.as_str()
Expand Down Expand Up @@ -833,6 +917,7 @@ impl Query {
output_value,
path,
&field_type.0,
FieldOrIndex::Field(field_name_str),
selection_set,
);
path.pop();
Expand All @@ -842,7 +927,7 @@ impl Query {
output.insert(field_name.clone(), Value::String(root_type_name.into()));
}
} else if field_type.is_non_null() {
parameters.errors.push(Error {
parameters.nullification_errors.push(Error {
message: format!(
"Cannot return null for non-nullable field {}.{field_name_str}",
root_type_name
Expand Down Expand Up @@ -1093,7 +1178,8 @@ impl Query {
/// Intermediate structure for arguments passed through the entire formatting
struct FormatParameters<'a> {
variables: &'a Object,
errors: Vec<Error>,
nullification_errors: Vec<Error>,
validation_errors: Vec<Error>,
nullified: Vec<Path>,
schema: &'a ApiSchema,
}
Expand All @@ -1113,6 +1199,26 @@ pub(crate) struct Variable {
default_value: Option<Value>,
}

enum FieldOrIndex<'a> {
Field(&'a str),
Index(usize),
}

fn invalid_value_message(
parent_type: &executable::Type,
field_type: &executable::Type,
field_or_index: FieldOrIndex,
) -> String {
match field_or_index {
FieldOrIndex::Field(field_name) => {
format!("Invalid value found for field {parent_type}.{field_name}")
}
FieldOrIndex::Index(i) => {
format!("Invalid value found for array element of type {field_type} at index {i}")
}
}
}

impl Operation {
pub(crate) fn from_hir(
operation: &executable::Operation,
Expand Down
Loading