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

Revert "add errors for response validation (#5787)" #6314

Merged
merged 2 commits into from
Nov 21, 2024
Merged

Revert "add errors for response validation (#5787)" #6314

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
72373f5
Revert "add errors for response validation (#5787)"
goto-bus-stop Nov 21, 2024
bd054dc
Break snapshot
goto-bus-stop Nov 21, 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
5 changes: 0 additions & 5 deletions .changesets/fix_geal_response_validation_errors.md
View file
Open in desktop

This file was deleted.

192 changes: 15 additions & 177 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,7 +30,6 @@ 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 All @@ -52,7 +51,6 @@ pub(crate) mod transform;
pub(crate) mod traverse;

pub(crate) const TYPENAME: &str = "__typename";
pub(crate) const RESPONSE_VALIDATION: &str = "RESPONSE_VALIDATION_FAILED";

/// A GraphQL query.
#[derive(Derivative, Serialize, Deserialize)]
Expand Down Expand Up @@ -145,9 +143,8 @@ impl Query {
let mut parameters = FormatParameters {
variables: &variables,
schema,
nullification_errors: Vec::new(),
errors: Vec::new(),
nullified: Vec::new(),
validation_errors: Vec::new(),
};

response.data = Some(
Expand All @@ -164,18 +161,12 @@ impl Query {
},
);

if !parameters.nullification_errors.is_empty() {
if let Ok(value) =
serde_json_bytes::to_value(&parameters.nullification_errors)
{
if !parameters.errors.is_empty() {
if let Ok(value) = serde_json_bytes::to_value(&parameters.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 @@ -207,9 +198,8 @@ impl Query {
let mut parameters = FormatParameters {
variables: &all_variables,
schema,
nullification_errors: Vec::new(),
errors: Vec::new(),
nullified: Vec::new(),
validation_errors: Vec::new(),
};

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

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

return parameters.nullified;
}
}
Expand Down Expand Up @@ -358,7 +342,6 @@ 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 @@ -379,8 +362,7 @@ impl Query {
input,
output,
path,
parent_type,
field_or_index,
field_type,
selection_set,
) {
Err(_) => Err(InvalidValue),
Expand All @@ -395,7 +377,7 @@ impl Query {
),
_ => todo!(),
};
parameters.nullification_errors.push(Error {
parameters.errors.push(Error {
message,
path: Some(Path::from_response_slice(path)),
..Error::default()
Expand Down Expand Up @@ -435,7 +417,6 @@ impl Query {
&mut output_array[i],
path,
field_type,
FieldOrIndex::Index(i),
selection_set,
);
path.pop();
Expand All @@ -449,22 +430,7 @@ impl Query {
Ok(()) => Ok(()),
}
}
Value::Null => Ok(()),
v => {
parameters.validation_errors.push(
Error::builder()
.message(format!(
"Invalid non-list value of type {} for list type {field_type}",
v.json_type_name()
))
.path(Path::from_response_slice(path))
.extension_code(RESPONSE_VALIDATION)
.build(),
);

*output = Value::Null;
Ok(())
}
_ => Ok(()),
},
executable::Type::Named(name) if name == "Int" => {
let opt = if input.is_i64() {
Expand All @@ -480,19 +446,6 @@ impl Query {
if opt.is_some() {
*output = input.clone();
} else {
if !input.is_null() {
parameters.validation_errors.push(
Error::builder()
.message(invalid_value_message(
parent_type,
field_type,
field_or_index,
))
.path(Path::from_response_slice(path))
.extension_code(RESPONSE_VALIDATION)
.build(),
);
}
*output = Value::Null;
}
Ok(())
Expand All @@ -501,19 +454,6 @@ impl Query {
if input.as_f64().is_some() {
*output = input.clone();
} else {
if !input.is_null() {
parameters.validation_errors.push(
Error::builder()
.message(invalid_value_message(
parent_type,
field_type,
field_or_index,
))
.path(Path::from_response_slice(path))
.extension_code(RESPONSE_VALIDATION)
.build(),
);
}
*output = Value::Null;
}
Ok(())
Expand All @@ -522,19 +462,6 @@ impl Query {
if input.as_bool().is_some() {
*output = input.clone();
} else {
if !input.is_null() {
parameters.validation_errors.push(
Error::builder()
.message(invalid_value_message(
parent_type,
field_type,
field_or_index,
))
.path(Path::from_response_slice(path))
.extension_code(RESPONSE_VALIDATION)
.build(),
);
}
*output = Value::Null;
}
Ok(())
Expand All @@ -543,19 +470,6 @@ impl Query {
if input.as_str().is_some() {
*output = input.clone();
} else {
if !input.is_null() {
parameters.validation_errors.push(
Error::builder()
.message(invalid_value_message(
parent_type,
field_type,
field_or_index,
))
.path(Path::from_response_slice(path))
.extension_code(RESPONSE_VALIDATION)
.build(),
);
}
*output = Value::Null;
}
Ok(())
Expand All @@ -564,19 +478,6 @@ impl Query {
if input.is_string() || input.is_i64() || input.is_u64() || input.is_f64() {
*output = input.clone();
} else {
if !input.is_null() {
parameters.validation_errors.push(
Error::builder()
.message(invalid_value_message(
parent_type,
field_type,
field_or_index,
))
.path(Path::from_response_slice(path))
.extension_code(RESPONSE_VALIDATION)
.build(),
);
}
*output = Value::Null;
}
Ok(())
Expand All @@ -596,31 +497,11 @@ impl Query {
*output = input.clone();
Ok(())
} else {
parameters.validation_errors.push(
Error::builder()
.message(format!(
"Expected a valid enum value for type {}",
enum_type.name
))
.path(Path::from_response_slice(path))
.extension_code(RESPONSE_VALIDATION)
.build(),
);
*output = Value::Null;
Ok(())
}
}
None => {
parameters.validation_errors.push(
Error::builder()
.message(format!(
"Expected a valid enum value for type {}",
enum_type.name
))
.path(Path::from_response_slice(path))
.extension_code(RESPONSE_VALIDATION)
.build(),
);
*output = Value::Null;
Ok(())
}
Expand All @@ -631,9 +512,6 @@ impl Query {

match input {
Value::Object(ref mut input_object) => {
// FIXME: we should return an error if __typename is not a string
// but this might cause issues for some production deployments where
// __typename might be missing or invalid (cf https://github.com/apollographql/router/commit/4a592f4933b7b9e46f14c7a98404b9e067687f09 )
if let Some(input_type) =
input_object.get(TYPENAME).and_then(|val| val.as_str())
{
Expand Down Expand Up @@ -686,20 +564,8 @@ impl Query {

Ok(())
}
Value::Null => {
*output = Value::Null;
Ok(())
}
v => {
parameters.validation_errors.push(
Error::builder()
.message(format!(
"Invalid non-object value of type {} for composite type {type_name}", v.json_type_name()
))
.path(Path::from_response_slice(path))
.extension_code(RESPONSE_VALIDATION)
.build(),
);
_ => {
parameters.nullified.push(Path::from_response_slice(path));
*output = Value::Null;
Ok(())
}
Expand Down Expand Up @@ -775,7 +641,6 @@ impl Query {
output_value,
path,
current_type,
FieldOrIndex::Field(field_name.as_str()),
selection_set,
);
path.pop();
Expand All @@ -785,7 +650,7 @@ impl Query {
output.insert((*field_name).clone(), Value::Null);
}
if field_type.is_non_null() {
parameters.nullification_errors.push(Error {
parameters.errors.push(Error {
message: format!(
"Cannot return null for non-nullable field {current_type}.{}",
field_name.as_str()
Expand Down Expand Up @@ -907,11 +772,6 @@ impl Query {
continue;
}

let root_type = apollo_compiler::ast::Type::Named(
// Unchecked name instantiation is always safe, and we know the name is
// valid here
apollo_compiler::Name::new_unchecked(root_type_name),
);
let field_name = alias.as_ref().unwrap_or(name);
let field_name_str = field_name.as_str();

Expand Down Expand Up @@ -940,14 +800,13 @@ impl Query {
input_value,
output_value,
path,
&root_type,
FieldOrIndex::Field(field_name_str),
&field_type.0,
selection_set,
);
path.pop();
res?
} else if field_type.is_non_null() {
parameters.nullification_errors.push(Error {
parameters.errors.push(Error {
message: format!(
"Cannot return null for non-nullable field {}.{field_name_str}",
root_type_name
Expand Down Expand Up @@ -1155,8 +1014,7 @@ impl Query {
/// Intermediate structure for arguments passed through the entire formatting
struct FormatParameters<'a> {
variables: &'a Object,
nullification_errors: Vec<Error>,
validation_errors: Vec<Error>,
errors: Vec<Error>,
nullified: Vec<Path>,
schema: &'a ApiSchema,
}
Expand All @@ -1176,26 +1034,6 @@ 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 {
fn empty() -> Self {
Self {
Expand Down
Loading