diff --git a/apollo-router/src/axum_factory/tests.rs b/apollo-router/src/axum_factory/tests.rs index 25f04fb6ad..7e709bd105 100644 --- a/apollo-router/src/axum_factory/tests.rs +++ b/apollo-router/src/axum_factory/tests.rs @@ -60,6 +60,7 @@ use crate::ApolloRouterError; use crate::Configuration; use crate::ListenAddr; use crate::TestHarness; +use crate::assert_response_eq_ignoring_error_id; use crate::axum_factory::connection_handle::connection_counts; use crate::configuration::Homepage; use crate::configuration::Sandbox; @@ -1066,7 +1067,7 @@ async fn response_failure() -> Result<(), ApolloRouterError> { .await .unwrap(); - assert_eq!( + assert_response_eq_ignoring_error_id!( response, crate::error::FetchError::SubrequestHttpError { status_code: Some(200), diff --git a/apollo-router/src/error.rs b/apollo-router/src/error.rs index a036b3b47b..1146b034e2 100644 --- a/apollo-router/src/error.rs +++ b/apollo-router/src/error.rs @@ -157,12 +157,12 @@ impl FetchError { } } - Error { - message: self.to_string(), - locations: Default::default(), - path, - extensions: value.as_object().unwrap().to_owned(), - } + Error::builder() + .message(self.to_string()) + .locations(Vec::default()) + .and_path(path) + .extensions(value.as_object().unwrap().to_owned()) + .build() } /// Convert the error to an appropriate response. @@ -648,6 +648,7 @@ pub(crate) enum SubgraphBatchingError { #[cfg(test)] mod tests { use super::*; + use crate::assert_error_eq_ignoring_id; use crate::graphql; #[test] @@ -668,6 +669,6 @@ mod tests { ) .build(); - assert_eq!(expected_gql_error, error.to_graphql_error(None)); + assert_error_eq_ignoring_id!(expected_gql_error, error.to_graphql_error(None)); } } diff --git a/apollo-router/src/graphql/mod.rs b/apollo-router/src/graphql/mod.rs index 28ff608d92..ac6c114dac 100644 --- a/apollo-router/src/graphql/mod.rs +++ b/apollo-router/src/graphql/mod.rs @@ -6,6 +6,7 @@ mod visitor; use std::fmt; use std::pin::Pin; +use std::str::FromStr; use apollo_compiler::response::GraphQLError as CompilerExecutionError; use apollo_compiler::response::ResponseDataPathSegment; @@ -20,6 +21,7 @@ use serde::Serialize; use serde_json_bytes::ByteString; use serde_json_bytes::Map as JsonMap; use serde_json_bytes::Value; +use uuid::Uuid; pub(crate) use visitor::ResponseVisitor; use crate::json_ext::Object; @@ -52,15 +54,15 @@ pub struct Location { /// as may be found in the `errors` field of a GraphQL [`Response`]. /// /// Converted to (or from) JSON with serde. -#[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize, Default)] -#[serde(rename_all = "camelCase")] +#[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase", default)] #[non_exhaustive] pub struct Error { /// The error message. pub message: String, /// The locations of the error in the GraphQL document of the originating request. - #[serde(skip_serializing_if = "Vec::is_empty", default)] + #[serde(skip_serializing_if = "Vec::is_empty")] pub locations: Vec, /// If this is a field error, the JSON path to that field in [`Response::data`] @@ -68,9 +70,26 @@ pub struct Error { pub path: Option, /// The optional GraphQL extensions for this error. - #[serde(default, skip_serializing_if = "Object::is_empty")] + #[serde(skip_serializing_if = "Object::is_empty")] pub extensions: Object, + + /// A unique identifier for this error + #[serde(skip_serializing)] + apollo_id: Uuid, } + +impl Default for Error { + fn default() -> Self { + Self { + message: String::new(), + locations: Vec::new(), + path: None, + extensions: Object::new(), + apollo_id: generate_uuid(), + } + } +} + // Implement getter and getter_mut to not use pub field directly #[buildstructor::buildstructor] @@ -103,25 +122,39 @@ impl Error { /// Optional, may be called multiple times. /// Adds one item to the [`Error::extensions`] map. /// + /// * `.extension_code(impl Into<`[`String`]`>)` + /// Optional. + /// Sets the "code" in the extension map. Will be ignored if extension already has this key + /// set. + /// + /// * `.apollo_id(impl Into<`[`Uuid`]`>)` + /// Optional. + /// Sets the unique identifier for this Error. This should only be used in cases of + /// deserialization or testing. If not given, the ID will be auto-generated. + /// /// * `.build()` /// Finishes the builder and returns a GraphQL [`Error`]. #[builder(visibility = "pub")] - fn new>( + fn new( message: String, locations: Vec, path: Option, - extension_code: T, - // Skip the `Object` type alias in order to use buildstructor’s map special-casing + extension_code: Option, + // Skip the `Object` type alias in order to use buildstructor's map special-casing mut extensions: JsonMap, + apollo_id: Option, ) -> Self { - extensions - .entry("code") - .or_insert_with(|| extension_code.into().into()); + if let Some(code) = extension_code { + extensions + .entry("code") + .or_insert(Value::String(ByteString::from(code))); + } Self { message, locations, path, extensions, + apollo_id: apollo_id.unwrap_or_else(Uuid::new_v4), } } @@ -160,13 +193,24 @@ impl Error { .map_err(|err| MalformedResponseError { reason: format!("invalid `path` within error: {}", err), })?; - - Ok(Error { - message, - locations, - path, - extensions, + let apollo_id: Option = extract_key_value_from_object!( + object, + "apolloId", + Value::String(s) => s + ) + .map_err(|err| MalformedResponseError { + reason: format!("invalid `apolloId` within error: {}", err), + })? + .map(|s| { + Uuid::from_str(s.as_str()).map_err(|err| MalformedResponseError { + reason: format!("invalid `apolloId` within error: {}", err), + }) }) + .transpose()?; + + Ok(Self::new( + message, locations, path, None, extensions, apollo_id, + )) } pub(crate) fn from_value_completion_value(value: &Value) -> Option { @@ -196,13 +240,41 @@ impl Error { .and_then(|p: &serde_json_bytes::Value| -> Option { serde_json_bytes::from_value(p.clone()).ok() }); - Some(Error { - message, - locations, - path, - extensions, + + Some(Self::new( + message, locations, path, None, extensions, + None, // apollo_id is not serialized, so it will never exist in a serialized vc error + )) + } + + /// Extract the error code from [`Error::extensions`] as a String if it is set. + pub fn extension_code(&self) -> Option { + self.extensions.get("code").and_then(|c| match c { + Value::String(s) => Some(s.as_str().to_owned()), + Value::Number(n) => Some(n.to_string()), + Value::Null | Value::Array(_) | Value::Object(_) | Value::Bool(_) => None, }) } + + /// Retrieve the internal Apollo unique ID for this error + pub fn apollo_id(&self) -> Uuid { + self.apollo_id + } + + #[cfg(test)] + /// Returns a duplicate of the error where [`self.apollo_id`] is `Uuid::nil()`. Used for + /// comparing errors in tests where you cannot control the randomly generated Uuid + pub fn with_null_id(&self) -> Self { + let mut new_err = self.clone(); + new_err.apollo_id = Uuid::nil(); + new_err + } +} + +/// Generate a random Uuid. For use in generating a default [`Error::apollo_id`] when not supplied +/// during deserialization. +fn generate_uuid() -> Uuid { + Uuid::new_v4() } /// GraphQL spec require that both "line" and "column" are positive numbers. @@ -282,6 +354,44 @@ impl From for Error { locations, path, extensions, + apollo_id: Uuid::new_v4(), } } } + +/// Assert that the expected and actual [`Error`] are equal when ignoring their +/// [`Error::apollo_id`]. +#[macro_export] +macro_rules! assert_error_eq_ignoring_id { + ($expected:expr, $actual:expr) => { + assert_eq!($expected.with_null_id(), $actual.with_null_id()); + }; +} + +/// Assert that the expected and actual lists of [`Error`] are equal when ignoring their +/// [`Error::apollo_id`]. +#[macro_export] +macro_rules! assert_errors_eq_ignoring_id { + ($expected:expr, $actual:expr) => {{ + let normalize = + |v: &[graphql::Error]| v.iter().map(|e| e.with_null_id()).collect::>(); + + assert_eq!(normalize(&$expected), normalize(&$actual)); + }}; +} + +/// Assert that the expected and actual [`Response`] are equal when ignoring the +/// [`Error::apollo_id`] on any [`Error`] in their [`Response::errors`]. +#[macro_export] +macro_rules! assert_response_eq_ignoring_error_id { + ($expected:expr, $actual:expr) => {{ + let normalize = + |v: &[graphql::Error]| v.iter().map(|e| e.with_null_id()).collect::>(); + let mut expected_response: graphql::Response = $expected.clone(); + let mut actual_response: graphql::Response = $actual.clone(); + expected_response.errors = normalize(&expected_response.errors); + actual_response.errors = normalize(&actual_response.errors); + + assert_eq!(expected_response, actual_response); + }}; +} diff --git a/apollo-router/src/graphql/response.rs b/apollo-router/src/graphql/response.rs index f14c2762bf..b4bcdd7868 100644 --- a/apollo-router/src/graphql/response.rs +++ b/apollo-router/src/graphql/response.rs @@ -258,34 +258,41 @@ impl From for Response { mod tests { use serde_json::json; use serde_json_bytes::json as bjson; + use uuid::Uuid; use super::*; + use crate::assert_response_eq_ignoring_error_id; + use crate::graphql; + use crate::graphql::Error; use crate::graphql::Location; + use crate::graphql::Response; #[test] fn test_append_errors_path_fallback_and_override() { + let uuid1 = Uuid::new_v4(); + let uuid2 = Uuid::new_v4(); let expected_errors = vec![ - Error { - message: "Something terrible happened!".to_string(), - path: Some(Path::from("here")), - ..Default::default() - }, - Error { - message: "I mean for real".to_string(), - ..Default::default() - }, + Error::builder() + .message("Something terrible happened!") + .path(Path::from("here")) + .apollo_id(uuid1) + .build(), + Error::builder() + .message("I mean for real") + .apollo_id(uuid2) + .build(), ]; let mut errors_to_append = vec![ - Error { - message: "Something terrible happened!".to_string(), - path: Some(Path::from("here")), - ..Default::default() - }, - Error { - message: "I mean for real".to_string(), - ..Default::default() - }, + Error::builder() + .message("Something terrible happened!") + .path(Path::from("here")) + .apollo_id(uuid1) + .build(), + Error::builder() + .message("I mean for real") + .apollo_id(uuid2) + .build(), ]; let mut response = Response::builder().build(); @@ -334,8 +341,9 @@ mod tests { .to_string() .as_str(), ); - assert_eq!( - result.unwrap(), + let response = result.unwrap(); + assert_response_eq_ignoring_error_id!( + response, Response::builder() .data(json!({ "hero": { @@ -356,17 +364,19 @@ mod tests { ] } })) - .errors(vec![Error { - message: "Name for character with ID 1002 could not be fetched.".into(), - locations: vec!(Location { line: 6, column: 7 }), - path: Some(Path::from("hero/heroFriends/1/name")), - extensions: bjson!({ - "error-extension": 5, - }) - .as_object() - .cloned() - .unwrap() - }]) + .errors(vec![ + Error::builder() + .message("Name for character with ID 1002 could not be fetched.") + .locations(vec!(Location { line: 6, column: 7 })) + .path(Path::from("hero/heroFriends/1/name")) + .extensions( + bjson!({ "error-extension": 5, }) + .as_object() + .cloned() + .unwrap() + ) + .build() + ]) .extensions( bjson!({ "response-extension": 3, @@ -423,8 +433,9 @@ mod tests { .to_string() .as_str(), ); - assert_eq!( - result.unwrap(), + let response = result.unwrap(); + assert_response_eq_ignoring_error_id!( + response, Response::builder() .label("part".to_owned()) .data(json!({ @@ -447,17 +458,19 @@ mod tests { } })) .path(Path::from("hero/heroFriends/1/name")) - .errors(vec![Error { - message: "Name for character with ID 1002 could not be fetched.".into(), - locations: vec!(Location { line: 6, column: 7 }), - path: Some(Path::from("hero/heroFriends/1/name")), - extensions: bjson!({ - "error-extension": 5, - }) - .as_object() - .cloned() - .unwrap() - }]) + .errors(vec![ + Error::builder() + .message("Name for character with ID 1002 could not be fetched.") + .locations(vec!(Location { line: 6, column: 7 })) + .path(Path::from("hero/heroFriends/1/name")) + .extensions( + bjson!({ "error-extension": 5, }) + .as_object() + .cloned() + .unwrap() + ) + .build() + ]) .extensions( bjson!({ "response-extension": 3, diff --git a/apollo-router/src/plugins/authentication/tests.rs b/apollo-router/src/plugins/authentication/tests.rs index 3e865a7563..423fa4325d 100644 --- a/apollo-router/src/plugins/authentication/tests.rs +++ b/apollo-router/src/plugins/authentication/tests.rs @@ -49,6 +49,8 @@ use super::JWTConf; use super::JwtStatus; use super::Source; use super::authenticate; +use crate::assert_errors_eq_ignoring_id; +use crate::assert_response_eq_ignoring_error_id; use crate::assert_snapshot_subscriber; use crate::graphql; use crate::plugin::test; @@ -235,7 +237,7 @@ async fn it_rejects_when_there_is_no_auth_header() { .extension_code("AUTH_ERROR") .build(); - assert_eq!(response.errors, vec![expected_error]); + assert_errors_eq_ignoring_id!(response.errors, [expected_error]); assert_eq!(StatusCode::UNAUTHORIZED, service_response.response.status()); } @@ -274,7 +276,7 @@ async fn it_rejects_when_auth_prefix_is_missing() { .extension_code("AUTH_ERROR") .build(); - assert_eq!(response.errors, vec![expected_error]); + assert_errors_eq_ignoring_id!(response.errors, [expected_error]); assert_eq!(StatusCode::BAD_REQUEST, service_response.response.status()); } @@ -313,7 +315,7 @@ async fn it_rejects_when_auth_prefix_has_no_jwt_token() { .extension_code("AUTH_ERROR") .build(); - assert_eq!(response.errors, vec![expected_error]); + assert_errors_eq_ignoring_id!(response.errors, [expected_error]); assert_eq!(StatusCode::BAD_REQUEST, service_response.response.status()); } @@ -351,7 +353,7 @@ async fn it_rejects_when_auth_prefix_has_invalid_format_jwt() { .extension_code("AUTH_ERROR") .build(); - assert_eq!(response.errors, vec![expected_error]); + assert_errors_eq_ignoring_id!(response.errors, [expected_error]); assert_eq!(StatusCode::BAD_REQUEST, service_response.response.status()); } @@ -386,11 +388,11 @@ async fn it_rejects_when_auth_prefix_has_correct_format_but_invalid_jwt() { .unwrap(); let expected_error = graphql::Error::builder() - .message(format!("'{HEADER_TOKEN_TRUNCATED}' is not a valid JWT header: Base64 error: Invalid last symbol 114, offset 5.")) - .extension_code("AUTH_ERROR") - .build(); + .message(format!("'{HEADER_TOKEN_TRUNCATED}' is not a valid JWT header: Base64 error: Invalid last symbol 114, offset 5.")) + .extension_code("AUTH_ERROR") + .build(); - assert_eq!(response.errors, vec![expected_error]); + assert_errors_eq_ignoring_id!(response.errors, [expected_error]); assert_eq!(StatusCode::BAD_REQUEST, service_response.response.status()); } @@ -429,7 +431,7 @@ async fn it_rejects_when_auth_prefix_has_correct_format_and_invalid_jwt() { .extension_code("AUTH_ERROR") .build(); - assert_eq!(response.errors, vec![expected_error]); + assert_errors_eq_ignoring_id!(response.errors, [expected_error]); assert_eq!(StatusCode::UNAUTHORIZED, service_response.response.status()); } @@ -777,7 +779,7 @@ async fn it_inserts_failure_jwt_status_into_context() { .extension_code("AUTH_ERROR") .build(); - assert_eq!(response.errors, vec![expected_error]); + assert_errors_eq_ignoring_id!(response.errors, [expected_error]); assert_eq!(StatusCode::UNAUTHORIZED, service_response.response.status()); @@ -1277,8 +1279,12 @@ async fn issuer_check() { .unwrap(), ) .unwrap(); - assert_eq!(response, graphql::Response::builder() - .errors(vec![graphql::Error::builder().extension_code("AUTH_ERROR").message("Invalid issuer: the token's `iss` was 'hallo', but signed with a key from JWKS configured to only accept from 'hello'").build()]).build()); + assert_response_eq_ignoring_error_id!(response, graphql::Response::builder() + .errors(vec![graphql::Error::builder() + .extension_code("AUTH_ERROR") + .message("Invalid issuer: the token's `iss` was 'hallo', but signed with a key from JWKS configured to only accept from 'hello'") + .build() + ]).build()); } ControlFlow::Continue(req) => { println!("got req with issuer check"); @@ -1317,8 +1323,11 @@ async fn issuer_check() { .unwrap(), ) .unwrap(); - assert_eq!(response, graphql::Response::builder() - .errors(vec![graphql::Error::builder().extension_code("AUTH_ERROR").message("Invalid issuer: the token's `iss` was 'AAAA', but signed with a key from JWKS configured to only accept from 'goodbye, hello'").build()]).build()); + assert_response_eq_ignoring_error_id!(response, graphql::Response::builder() + .errors(vec![graphql::Error::builder() + .extension_code("AUTH_ERROR") + .message("Invalid issuer: the token's `iss` was 'AAAA', but signed with a key from JWKS configured to only accept from 'goodbye, hello'") + .build()]).build()); } ControlFlow::Continue(_) => { panic!("issuer check should have failed") @@ -1501,7 +1510,7 @@ async fn audience_check() { .unwrap(), ) .unwrap(); - assert_eq!(response, graphql::Response::builder() + assert_response_eq_ignoring_error_id!(response, graphql::Response::builder() .errors(vec![ graphql::Error::builder() .extension_code("AUTH_ERROR") diff --git a/apollo-router/src/plugins/connectors/handle_responses.rs b/apollo-router/src/plugins/connectors/handle_responses.rs index 6e31aef931..32b6ce45d1 100644 --- a/apollo-router/src/plugins/connectors/handle_responses.rs +++ b/apollo-router/src/plugins/connectors/handle_responses.rs @@ -980,7 +980,7 @@ mod tests { .unwrap(), ); - let res = super::aggregate_responses(vec![ + let mut res = super::aggregate_responses(vec![ process_response( Ok(response_plaintext), response_key_plaintext, @@ -1028,7 +1028,13 @@ mod tests { ]) .unwrap(); - assert_debug_snapshot!(res, @r###" + // Overwrite error IDs to avoid random Uuid mismatch. + // Since assert_debug_snapshot does not support redactions (which would be useful for error IDs), + // we have to do it manually. + let body = res.response.body_mut(); + body.errors = body.errors.iter_mut().map(|e| e.with_null_id()).collect(); + + assert_debug_snapshot!(res, @r#" Response { response: Response { status: 200, @@ -1087,6 +1093,7 @@ mod tests { "subgraph_name", ), }, + apollo_id: 00000000-0000-0000-0000-000000000000, }, Error { message: "Request failed", @@ -1123,6 +1130,7 @@ mod tests { "subgraph_name", ), }, + apollo_id: 00000000-0000-0000-0000-000000000000, }, Error { message: "Request failed", @@ -1159,6 +1167,7 @@ mod tests { "subgraph_name", ), }, + apollo_id: 00000000-0000-0000-0000-000000000000, }, ], extensions: {}, @@ -1169,7 +1178,7 @@ mod tests { }, }, } - "###); + "#); } #[tokio::test] diff --git a/apollo-router/src/plugins/coprocessor/test.rs b/apollo-router/src/plugins/coprocessor/test.rs index 1b92093d6c..d60192344d 100644 --- a/apollo-router/src/plugins/coprocessor/test.rs +++ b/apollo-router/src/plugins/coprocessor/test.rs @@ -19,6 +19,8 @@ mod tests { use tower::ServiceExt; use super::super::*; + use crate::assert_response_eq_ignoring_error_id; + use crate::graphql::Response; use crate::json_ext::Object; use crate::json_ext::Value; use crate::plugin::test::MockInternalHttpClientService; @@ -969,9 +971,9 @@ mod tests { let actual_response = response.into_body(); - assert_eq!( + assert_response_eq_ignoring_error_id!( actual_response, - serde_json_bytes::from_value(json!({ + serde_json_bytes::from_value::(json!({ "errors": [{ "message": "my error message", "extensions": { @@ -979,7 +981,7 @@ mod tests { } }] })) - .unwrap(), + .unwrap() ); } diff --git a/apollo-router/src/plugins/forbid_mutations.rs b/apollo-router/src/plugins/forbid_mutations.rs index aad5750b9d..730c87dcc6 100644 --- a/apollo-router/src/plugins/forbid_mutations.rs +++ b/apollo-router/src/plugins/forbid_mutations.rs @@ -75,8 +75,8 @@ mod forbid_http_get_mutations_tests { use tower::ServiceExt; use super::*; + use crate::assert_error_eq_ignoring_id; use crate::graphql; - use crate::graphql::Response; use crate::http_ext::Request; use crate::plugin::PluginInit; use crate::plugin::test::MockExecutionService; @@ -129,10 +129,11 @@ mod forbid_http_get_mutations_tests { .execution_service(MockExecutionService::new().boxed()); let request = create_request(Method::GET, OperationKind::Mutation); - let mut actual_error = service_stack.oneshot(request).await.unwrap(); + let mut response = service_stack.oneshot(request).await.unwrap(); + let actual_error = &response.next_response().await.unwrap().errors[0]; - assert_eq!(expected_status, actual_error.response.status()); - assert_error_matches(&expected_error, actual_error.next_response().await.unwrap()); + assert_eq!(expected_status, response.response.status()); + assert_error_eq_ignoring_id!(actual_error, expected_error); } #[tokio::test] @@ -163,10 +164,6 @@ mod forbid_http_get_mutations_tests { .unwrap(); } - fn assert_error_matches(expected_error: &Error, response: Response) { - assert_eq!(&response.errors[0], expected_error); - } - fn create_request(method: Method, operation_kind: OperationKind) -> ExecutionRequest { let root: PlanNode = if operation_kind == OperationKind::Mutation { serde_json::from_value(json!({ diff --git a/apollo-router/src/plugins/rhai/execution.rs b/apollo-router/src/plugins/rhai/execution.rs index bdcd646577..5af1fabc93 100644 --- a/apollo-router/src/plugins/rhai/execution.rs +++ b/apollo-router/src/plugins/rhai/execution.rs @@ -28,10 +28,11 @@ pub(super) fn request_failure( .build()? } else { Response::error_builder() - .errors(vec![Error { - message: error_details.message.unwrap_or_default(), - ..Default::default() - }]) + .errors(vec![ + Error::builder() + .message(error_details.message.unwrap_or_default()) + .build(), + ]) .context(context) .status_code(error_details.status) .build()? @@ -53,10 +54,11 @@ pub(super) fn response_failure(context: Context, error_details: ErrorDetails) -> .build() } else { Response::error_builder() - .errors(vec![Error { - message: error_details.message.unwrap_or_default(), - ..Default::default() - }]) + .errors(vec![ + Error::builder() + .message(error_details.message.unwrap_or_default()) + .build(), + ]) .status_code(error_details.status) .context(context) .build() diff --git a/apollo-router/src/plugins/rhai/mod.rs b/apollo-router/src/plugins/rhai/mod.rs index 3cf5f21fbc..9ab1478d77 100644 --- a/apollo-router/src/plugins/rhai/mod.rs +++ b/apollo-router/src/plugins/rhai/mod.rs @@ -558,10 +558,9 @@ macro_rules! gen_map_deferred_response { let mut guard = shared_response.lock(); let response_opt = guard.take(); let $base::DeferredResponse { mut response, .. } = response_opt.unwrap(); - let error = Error { - message: error_details.message.unwrap_or_default(), - ..Default::default() - }; + let error = Error::builder() + .message(error_details.message.unwrap_or_default()) + .build(); response.errors = vec![error]; return Some(response); } diff --git a/apollo-router/src/plugins/rhai/router.rs b/apollo-router/src/plugins/rhai/router.rs index b968b28270..06d9faa911 100644 --- a/apollo-router/src/plugins/rhai/router.rs +++ b/apollo-router/src/plugins/rhai/router.rs @@ -30,10 +30,11 @@ pub(super) fn request_failure( .build()? } else { crate::services::router::Response::error_builder() - .errors(vec![Error { - message: error_details.message.unwrap_or_default(), - ..Default::default() - }]) + .errors(vec![ + Error::builder() + .message(error_details.message.unwrap_or_default()) + .build(), + ]) .context(context) .status_code(error_details.status) .build()? @@ -58,10 +59,11 @@ pub(super) fn response_failure( .build() } else { crate::services::router::Response::error_builder() - .errors(vec![Error { - message: error_details.message.unwrap_or_default(), - ..Default::default() - }]) + .errors(vec![ + Error::builder() + .message(error_details.message.unwrap_or_default()) + .build(), + ]) .status_code(error_details.status) .context(context) .build() diff --git a/apollo-router/src/plugins/rhai/subgraph.rs b/apollo-router/src/plugins/rhai/subgraph.rs index 4e80b97ffe..17ecc41405 100644 --- a/apollo-router/src/plugins/rhai/subgraph.rs +++ b/apollo-router/src/plugins/rhai/subgraph.rs @@ -26,10 +26,11 @@ pub(super) fn request_failure( .build() } else { Response::error_builder() - .errors(vec![Error { - message: error_details.message.unwrap_or_default(), - ..Default::default() - }]) + .errors(vec![ + Error::builder() + .message(error_details.message.unwrap_or_default()) + .build(), + ]) .context(context) .status_code(error_details.status) .subgraph_name(String::default()) // XXX: We don't know the subgraph name @@ -53,10 +54,11 @@ pub(super) fn response_failure(context: Context, error_details: ErrorDetails) -> .build() } else { Response::error_builder() - .errors(vec![Error { - message: error_details.message.unwrap_or_default(), - ..Default::default() - }]) + .errors(vec![ + Error::builder() + .message(error_details.message.unwrap_or_default()) + .build(), + ]) .status_code(error_details.status) .context(context) .subgraph_name(String::default()) // XXX: We don't know the subgraph name diff --git a/apollo-router/src/plugins/rhai/supergraph.rs b/apollo-router/src/plugins/rhai/supergraph.rs index 2b5b7bc804..4b7ac475dc 100644 --- a/apollo-router/src/plugins/rhai/supergraph.rs +++ b/apollo-router/src/plugins/rhai/supergraph.rs @@ -28,10 +28,11 @@ pub(super) fn request_failure( .build()? } else { Response::error_builder() - .errors(vec![Error { - message: error_details.message.unwrap_or_default(), - ..Default::default() - }]) + .errors(vec![ + Error::builder() + .message(error_details.message.unwrap_or_default()) + .build(), + ]) .context(context) .status_code(error_details.status) .build()? @@ -53,10 +54,11 @@ pub(super) fn response_failure(context: Context, error_details: ErrorDetails) -> .build() } else { Response::error_builder() - .errors(vec![Error { - message: error_details.message.unwrap_or_default(), - ..Default::default() - }]) + .errors(vec![ + Error::builder() + .message(error_details.message.unwrap_or_default()) + .build(), + ]) .status_code(error_details.status) .context(context) .build() diff --git a/apollo-router/src/plugins/rhai/tests.rs b/apollo-router/src/plugins/rhai/tests.rs index 7a711df497..0776be7a63 100644 --- a/apollo-router/src/plugins/rhai/tests.rs +++ b/apollo-router/src/plugins/rhai/tests.rs @@ -25,6 +25,7 @@ use super::Rhai; use super::process_error; use super::subgraph; use crate::Context; +use crate::assert_response_eq_ignoring_error_id; use crate::assert_snapshot_subscriber; use crate::graphql; use crate::graphql::Error; @@ -607,18 +608,16 @@ async fn it_can_process_om_subgraph_forbidden_with_graphql_payload() { let processed_error = process_error(error); assert_eq!(processed_error.status, StatusCode::FORBIDDEN); - assert_eq!( - processed_error.body, - Some( - graphql::Response::builder() - .errors(vec![{ - Error::builder() - .message("I have raised a 403") - .extension_code("ACCESS_DENIED") - .build() - }]) - .build() - ) + assert_response_eq_ignoring_error_id!( + processed_error.body.unwrap(), + graphql::Response::builder() + .errors(vec![{ + Error::builder() + .message("I have raised a 403") + .extension_code("ACCESS_DENIED") + .build() + }]) + .build() ); } diff --git a/apollo-router/src/plugins/subscription.rs b/apollo-router/src/plugins/subscription.rs index 6d008b81b5..e507b172eb 100644 --- a/apollo-router/src/plugins/subscription.rs +++ b/apollo-router/src/plugins/subscription.rs @@ -773,6 +773,7 @@ mod tests { use super::*; use crate::Notify; + use crate::assert_response_eq_ignoring_error_id; use crate::graphql::Request; use crate::http_ext; use crate::plugin::DynPlugin; @@ -1142,8 +1143,7 @@ mod tests { let resp = web_endpoint.clone().oneshot(http_req).await.unwrap(); assert_eq!(resp.status(), http::StatusCode::ACCEPTED); let msg = handler.next().await.unwrap(); - - assert_eq!( + assert_response_eq_ignoring_error_id!( msg, graphql::Response::builder() .errors(vec![ @@ -1225,7 +1225,7 @@ mod tests { .await .unwrap(); - assert_eq!( + assert_response_eq_ignoring_error_id!( subgraph_response.response.body(), &graphql::Response::builder() .data(serde_json_bytes::Value::Null) diff --git a/apollo-router/src/protocols/websocket.rs b/apollo-router/src/protocols/websocket.rs index 199b57cdb0..6d09548b78 100644 --- a/apollo-router/src/protocols/websocket.rs +++ b/apollo-router/src/protocols/websocket.rs @@ -697,6 +697,7 @@ mod tests { use uuid::Uuid; use super::*; + use crate::assert_response_eq_ignoring_error_id; use crate::graphql::Request; async fn emulate_correct_websocket_server_new_protocol( @@ -982,7 +983,7 @@ mod tests { .unwrap(); let next_payload = gql_read_stream.next().await.unwrap(); - assert_eq!(next_payload, graphql::Response::builder() + assert_response_eq_ignoring_error_id!(next_payload, graphql::Response::builder() .error( graphql::Error::builder() .message( @@ -1044,7 +1045,8 @@ mod tests { .unwrap(); let next_payload = gql_read_stream.next().await.unwrap(); - assert_eq!(next_payload, graphql::Response::builder() + + assert_response_eq_ignoring_error_id!(next_payload, graphql::Response::builder() .error( graphql::Error::builder() .message( diff --git a/apollo-router/src/query_planner/fetch.rs b/apollo-router/src/query_planner/fetch.rs index 566c61d2ed..3401ac9e4b 100644 --- a/apollo-router/src/query_planner/fetch.rs +++ b/apollo-router/src/query_planner/fetch.rs @@ -362,16 +362,21 @@ impl FetchNode { for values_path in inverted_paths.get(*i).iter().flat_map(|v| v.iter()) { - errors.push(Error { - locations: error.locations.clone(), - // append to the entitiy's path the error's path without - //`_entities` and the index - path: Some(Path::from_iter( - values_path.0.iter().chain(&path.0[2..]).cloned(), - )), - message: error.message.clone(), - extensions: error.extensions.clone(), - }) + errors.push( + Error::builder() + .locations(error.locations.clone()) + // append to the entity's path the error's path without + //`_entities` and the index + .path(Path::from_iter( + values_path.0.iter().chain(&path.0[2..]).cloned(), + )) + .message(error.message.clone()) + .and_extension_code(error.extension_code()) + .extensions(error.extensions.clone()) + // re-use the original ID so we don't double count this error + .apollo_id(error.apollo_id()) + .build(), + ) } } _ => { @@ -450,12 +455,14 @@ impl FetchNode { }) .unwrap_or_else(|| current_dir.clone()); - Error { - locations: error.locations, - path: Some(path), - message: error.message, - extensions: error.extensions, - } + Error::builder() + .locations(error.locations.clone()) + .path(path) + .message(error.message.clone()) + .and_extension_code(error.extension_code()) + .extensions(error.extensions.clone()) + .apollo_id(error.apollo_id()) + .build() }) .collect(); let mut data = response.data.unwrap_or_default(); diff --git a/apollo-router/src/services/layers/allow_only_http_post_mutations.rs b/apollo-router/src/services/layers/allow_only_http_post_mutations.rs index f16bf23ddc..dd3ca1370d 100644 --- a/apollo-router/src/services/layers/allow_only_http_post_mutations.rs +++ b/apollo-router/src/services/layers/allow_only_http_post_mutations.rs @@ -150,8 +150,8 @@ mod forbid_http_get_mutations_tests { use super::*; use crate::Context; + use crate::assert_error_eq_ignoring_id; use crate::error::Error; - use crate::graphql::Response; use crate::plugin::test::MockSupergraphService; use crate::query_planner::fetch::OperationKind; use crate::services::layers::query_analysis::ParsedDocumentInner; @@ -239,17 +239,10 @@ mod forbid_http_get_mutations_tests { #[tokio::test] async fn it_doesnt_let_non_http_post_mutations_pass_through() { - let expected_error = Error { - message: "Mutations can only be sent over HTTP POST".to_string(), - locations: Default::default(), - path: Default::default(), - extensions: serde_json_bytes::json!({ - "code": "MUTATION_FORBIDDEN" - }) - .as_object() - .unwrap() - .to_owned(), - }; + let expected_error = Error::builder() + .message("Mutations can only be sent over HTTP POST".to_string()) + .extension_code("MUTATION_FORBIDDEN") + .build(); let expected_status = StatusCode::METHOD_NOT_ALLOWED; let expected_allow_header = "POST"; @@ -276,21 +269,18 @@ mod forbid_http_get_mutations_tests { let mut service_stack = AllowOnlyHttpPostMutationsLayer::default().layer(mock_service); let services = service_stack.ready().await.unwrap(); - let mut actual_error = services.call(request).await.unwrap(); + let mut error_response = services.call(request).await.unwrap(); + let response = error_response.next_response().await.unwrap(); - assert_eq!(expected_status, actual_error.response.status()); + assert_eq!(expected_status, error_response.response.status()); assert_eq!( expected_allow_header, - actual_error.response.headers().get("Allow").unwrap() + error_response.response.headers().get("Allow").unwrap() ); - assert_error_matches(&expected_error, actual_error.next_response().await.unwrap()); + assert_error_eq_ignoring_id!(expected_error, response.errors[0]); } } - fn assert_error_matches(expected_error: &Error, response: Response) { - assert_eq!(&response.errors[0], expected_error); - } - fn create_request(method: Method, operation_kind: OperationKind) -> SupergraphRequest { let query = match operation_kind { OperationKind::Query => { diff --git a/apollo-router/src/services/layers/apq.rs b/apollo-router/src/services/layers/apq.rs index b8c91a96a5..088709aad3 100644 --- a/apollo-router/src/services/layers/apq.rs +++ b/apollo-router/src/services/layers/apq.rs @@ -8,7 +8,6 @@ use http::StatusCode; use http::header::CACHE_CONTROL; use serde::Deserialize; use serde_json_bytes::Value; -use serde_json_bytes::json; use sha2::Digest; use sha2::Sha256; @@ -129,15 +128,13 @@ async fn apq_request( Ok(request) } else { tracing::debug!("apq: graphql request doesn't match provided sha256Hash"); - let errors = vec![crate::error::Error { - message: "provided sha does not match query".to_string(), - locations: Default::default(), - path: Default::default(), - extensions: serde_json_bytes::from_value(json!({ - "code": "PERSISTED_QUERY_HASH_MISMATCH", - })) - .unwrap(), - }]; + let errors = vec![ + crate::error::Error::builder() + .message("provided sha does not match query".to_string()) + .locations(Default::default()) + .extension_code("PERSISTED_QUERY_HASH_MISMATCH") + .build(), + ]; let res = SupergraphResponse::builder() .status_code(StatusCode::BAD_REQUEST) .data(Value::default()) @@ -162,15 +159,13 @@ async fn apq_request( } else { let _ = request.context.insert(PERSISTED_QUERY_CACHE_HIT, false); tracing::trace!("apq: cache miss"); - let errors = vec![crate::error::Error { - message: "PersistedQueryNotFound".to_string(), - locations: Default::default(), - path: Default::default(), - extensions: serde_json_bytes::from_value(json!({ - "code": "PERSISTED_QUERY_NOT_FOUND", - })) - .unwrap(), - }]; + let errors = vec![ + crate::error::Error::builder() + .message("PersistedQueryNotFound".to_string()) + .locations(Default::default()) + .extension_code("PERSISTED_QUERY_NOT_FOUND") + .build(), + ]; let res = SupergraphResponse::builder() .data(Value::default()) .errors(errors) @@ -215,15 +210,13 @@ async fn disabled_apq_request( .extensions .contains_key("persistedQuery") { - let errors = vec![crate::error::Error { - message: "PersistedQueryNotSupported".to_string(), - locations: Default::default(), - path: Default::default(), - extensions: serde_json_bytes::from_value(json!({ - "code": "PERSISTED_QUERY_NOT_SUPPORTED", - })) - .unwrap(), - }]; + let errors = vec![ + crate::error::Error::builder() + .message("PersistedQueryNotSupported".to_string()) + .locations(Default::default()) + .extension_code("PERSISTED_QUERY_NOT_SUPPORTED") + .build(), + ]; let res = SupergraphResponse::builder() .data(Value::default()) .errors(errors) @@ -250,8 +243,8 @@ mod apq_tests { use super::*; use crate::Configuration; use crate::Context; + use crate::assert_error_eq_ignoring_id; use crate::error::Error; - use crate::graphql::Response; use crate::services::router::ClientRequestAccepts; use crate::services::router::service::from_supergraph_mock_callback; use crate::services::router::service::from_supergraph_mock_callback_and_configuration; @@ -261,15 +254,11 @@ mod apq_tests { let hash = Cow::from("ecf4edb46db40b5132295c0291d62fb65d6759a9eedfa4d5d612dd5ec54a6b38"); let hash2 = hash.clone(); - let expected_apq_miss_error = Error { - message: "PersistedQueryNotFound".to_string(), - locations: Default::default(), - path: Default::default(), - extensions: serde_json_bytes::from_value(json!({ - "code": "PERSISTED_QUERY_NOT_FOUND", - })) - .unwrap(), - }; + let expected_apq_miss_error = Error::builder() + .message("PersistedQueryNotFound".to_string()) + .locations(Default::default()) + .extension_code("PERSISTED_QUERY_NOT_FOUND") + .build(); let mut router_service = from_supergraph_mock_callback(move |req| { let body = req.supergraph_request.body(); @@ -330,7 +319,7 @@ mod apq_tests { .unwrap() .unwrap(); - assert_error_matches(&expected_apq_miss_error, apq_error); + assert_error_eq_ignoring_id!(expected_apq_miss_error, apq_error.errors[0]); let with_query = SupergraphRequest::fake_builder() .extension("persistedQuery", persisted.clone()) @@ -387,15 +376,11 @@ mod apq_tests { let hash = Cow::from("ecf4edb46db40b5132295c0291d62fb65d6759a9eedfa4d5d612dd5ec54a6b36"); let hash2 = hash.clone(); - let expected_apq_miss_error = Error { - message: "PersistedQueryNotFound".to_string(), - locations: Default::default(), - path: Default::default(), - extensions: serde_json_bytes::from_value(json!({ - "code": "PERSISTED_QUERY_NOT_FOUND", - })) - .unwrap(), - }; + let expected_apq_miss_error = Error::builder() + .message("PersistedQueryNotFound".to_string()) + .locations(Default::default()) + .extension_code("PERSISTED_QUERY_NOT_FOUND") + .build(); let mut router_service = from_supergraph_mock_callback(move |req| { let body = req.supergraph_request.body(); @@ -466,7 +451,7 @@ mod apq_tests { .unwrap() .unwrap(); - assert_error_matches(&expected_apq_miss_error, apq_error); + assert_error_eq_ignoring_id!(expected_apq_miss_error, apq_error.errors[0]); // sha256 is wrong, apq insert won't happen let insert_failed_response = router_service @@ -489,16 +474,12 @@ mod apq_tests { .await .unwrap() .unwrap(); - let expected_apq_insert_failed_error = Error { - message: "provided sha does not match query".to_string(), - locations: Default::default(), - path: Default::default(), - extensions: serde_json_bytes::from_value(json!({ - "code": "PERSISTED_QUERY_HASH_MISMATCH", - })) - .unwrap(), - }; - assert_eq!(graphql_response.errors[0], expected_apq_insert_failed_error); + let expected_apq_insert_failed_error = Error::builder() + .message("provided sha does not match query".to_string()) + .locations(Default::default()) + .extension_code("PERSISTED_QUERY_HASH_MISMATCH") + .build(); + assert_error_eq_ignoring_id!(expected_apq_insert_failed_error, graphql_response.errors[0]); // apq insert failed, this call will miss let second_apq_error = router_service @@ -515,20 +496,16 @@ mod apq_tests { .unwrap() .unwrap(); - assert_error_matches(&expected_apq_miss_error, second_apq_error); + assert_error_eq_ignoring_id!(expected_apq_miss_error, second_apq_error.errors[0]); } #[tokio::test] async fn return_not_supported_when_disabled() { - let expected_apq_miss_error = Error { - message: "PersistedQueryNotSupported".to_string(), - locations: Default::default(), - path: Default::default(), - extensions: serde_json_bytes::from_value(json!({ - "code": "PERSISTED_QUERY_NOT_SUPPORTED", - })) - .unwrap(), - }; + let expected_apq_miss_error = Error::builder() + .message("PersistedQueryNotSupported".to_string()) + .locations(Default::default()) + .extension_code("PERSISTED_QUERY_NOT_SUPPORTED") + .build(); let mut config = Configuration::default(); config.apq.enabled = false; @@ -572,7 +549,7 @@ mod apq_tests { .unwrap() .unwrap(); - assert_error_matches(&expected_apq_miss_error, apq_error); + assert_error_eq_ignoring_id!(expected_apq_miss_error, apq_error.errors[0]); let with_query = SupergraphRequest::fake_builder() .extension("persistedQuery", persisted.clone()) @@ -599,7 +576,7 @@ mod apq_tests { .unwrap() .unwrap(); - assert_error_matches(&expected_apq_miss_error, apq_error); + assert_error_eq_ignoring_id!(expected_apq_miss_error, apq_error.errors[0]); let without_apq = SupergraphRequest::fake_builder() .query("{__typename}".to_string()) @@ -628,10 +605,6 @@ mod apq_tests { assert!(without_apq_graphql_response.errors.is_empty()); } - fn assert_error_matches(expected_error: &Error, res: Response) { - assert_eq!(&res.errors[0], expected_error); - } - fn new_context() -> Context { let context = Context::new(); context.extensions().with_lock(|lock| { diff --git a/apollo-router/src/services/layers/persisted_queries/mod.rs b/apollo-router/src/services/layers/persisted_queries/mod.rs index 1e2ac67c01..386c3e6e5b 100644 --- a/apollo-router/src/services/layers/persisted_queries/mod.rs +++ b/apollo-router/src/services/layers/persisted_queries/mod.rs @@ -477,11 +477,13 @@ mod tests { use super::manifest::ManifestOperation; use super::*; use crate::Context; + use crate::assert_errors_eq_ignoring_id; use crate::assert_snapshot_subscriber; use crate::configuration::Apq; use crate::configuration::PersistedQueries; use crate::configuration::PersistedQueriesSafelist; use crate::configuration::Supergraph; + use crate::graphql; use crate::metrics::FutureMetricsExt; use crate::services::layers::persisted_queries::freeform_graphql_behavior::FreeformGraphQLBehavior; use crate::services::layers::query_analysis::QueryAnalysisLayer; @@ -714,9 +716,9 @@ mod tests { .next_response() .await .expect("could not get response from pq layer"); - assert_eq!( + assert_errors_eq_ignoring_id!( response.errors, - vec![graphql_err_operation_not_found(invalid_id, None)] + [graphql_err_operation_not_found(invalid_id, None)] ); } @@ -848,10 +850,7 @@ mod tests { .next_response() .await .expect("could not get response from pq layer"); - assert_eq!( - response.errors, - vec![graphql_err_operation_not_in_safelist()] - ); + assert_errors_eq_ignoring_id!(response.errors, [graphql_err_operation_not_in_safelist()]); let mut metric_attributes = vec![opentelemetry::KeyValue::new( "persisted_queries.safelist.rejected.unknown".to_string(), true, @@ -1090,9 +1089,9 @@ mod tests { .next_response() .await .expect("could not get response from pq layer"); - assert_eq!( + assert_errors_eq_ignoring_id!( response.errors, - vec![graphql_err_operation_not_found( + [graphql_err_operation_not_found( invalid_id, Some("SomeOperation".to_string()), )] @@ -1217,7 +1216,7 @@ mod tests { .next_response() .await .expect("could not get response from pq layer"); - assert_eq!(response.errors, vec![graphql_err_pq_id_required()]); + assert_errors_eq_ignoring_id!(response.errors, [graphql_err_pq_id_required()]); // Try again skipping enforcement. let context = Context::new(); @@ -1345,7 +1344,7 @@ mod tests { .next_response() .await .expect("could not get response from pq layer"); - assert_eq!(response.errors, vec![graphql_err_cannot_send_id_and_body()]); + assert_errors_eq_ignoring_id!(response.errors, [graphql_err_cannot_send_id_and_body()]); } #[tokio::test(flavor = "multi_thread")] @@ -1378,6 +1377,6 @@ mod tests { .next_response() .await .expect("could not get response from pq layer"); - assert_eq!(response.errors, vec![graphql_err_cannot_send_id_and_body()]); + assert_errors_eq_ignoring_id!(response.errors, [graphql_err_cannot_send_id_and_body()]); } } diff --git a/apollo-router/src/services/subgraph_service.rs b/apollo-router/src/services/subgraph_service.rs index c3e1a909d7..cdf153dec4 100644 --- a/apollo-router/src/services/subgraph_service.rs +++ b/apollo-router/src/services/subgraph_service.rs @@ -1685,6 +1685,7 @@ mod tests { use super::*; use crate::Context; + use crate::assert_response_eq_ignoring_error_id; use crate::graphql::Error; use crate::graphql::Request; use crate::graphql::Response; @@ -3247,7 +3248,7 @@ mod tests { .to_graphql_error(None), ) .build(); - assert_eq!(actual, expected); + assert_response_eq_ignoring_error_id!(actual, expected); } #[test] @@ -3308,7 +3309,7 @@ mod tests { .data(json["data"].take()) .error(error) .build(); - assert_eq!(actual, expected); + assert_response_eq_ignoring_error_id!(actual, expected); } #[test] @@ -3350,6 +3351,6 @@ mod tests { ) .error(error) .build(); - assert_eq!(actual, expected); + assert_response_eq_ignoring_error_id!(expected, actual); } } diff --git a/apollo-router/src/services/supergraph/service.rs b/apollo-router/src/services/supergraph/service.rs index da4cb3a317..b86d2dc982 100644 --- a/apollo-router/src/services/supergraph/service.rs +++ b/apollo-router/src/services/supergraph/service.rs @@ -167,16 +167,12 @@ impl Service for SupergraphService { self.license, ) .or_else(|error: BoxError| async move { - let errors = vec![crate::error::Error { - message: error.to_string(), - extensions: serde_json_bytes::json!({ - "code": "INTERNAL_SERVER_ERROR", - }) - .as_object() - .unwrap() - .to_owned(), - ..Default::default() - }]; + let errors = vec![ + crate::error::Error::builder() + .message(error.to_string()) + .extension_code("INTERNAL_SERVER_ERROR") + .build(), + ]; Ok(SupergraphResponse::infallible_builder() .errors(errors) diff --git a/apollo-router/src/spec/query.rs b/apollo-router/src/spec/query.rs index 75760eab24..3c9ccd419a 100644 --- a/apollo-router/src/spec/query.rs +++ b/apollo-router/src/spec/query.rs @@ -370,11 +370,12 @@ impl Query { ), _ => todo!(), }; - parameters.errors.push(Error { - message, - path: Some(Path::from_response_slice(path)), - ..Error::default() - }); + parameters.errors.push( + Error::builder() + .message(message) + .path(Path::from_response_slice(path)) + .build(), + ); Err(InvalidValue) } else { @@ -639,14 +640,15 @@ impl Query { output.insert((*field_name).clone(), Value::Null); } if field_type.is_non_null() { - parameters.errors.push(Error { - message: format!( - "Cannot return null for non-nullable field {current_type}.{}", - field_name.as_str() - ), - path: Some(Path::from_response_slice(path)), - ..Error::default() - }); + parameters.errors.push( + Error::builder() + .message(format!( + "Cannot return null for non-nullable field {current_type}.{}", + field_name.as_str() + )) + .path(Path::from_response_slice(path)) + .build() + ); return Err(InvalidValue); } @@ -795,14 +797,15 @@ impl Query { path.pop(); res? } else if field_type.is_non_null() { - parameters.errors.push(Error { - message: format!( - "Cannot return null for non-nullable field {}.{field_name_str}", - root_type_name - ), - path: Some(Path::from_response_slice(path)), - ..Error::default() - }); + parameters.errors.push( + Error::builder() + .message(format!( + "Cannot return null for non-nullable field {}.{field_name_str}", + root_type_name + )) + .path(Path::from_response_slice(path)) + .build(), + ); return Err(InvalidValue); } else { output.insert(field_name.clone(), Value::Null); diff --git a/apollo-router/tests/integration_tests.rs b/apollo-router/tests/integration_tests.rs index 08a4537402..65e746864b 100644 --- a/apollo-router/tests/integration_tests.rs +++ b/apollo-router/tests/integration_tests.rs @@ -11,6 +11,7 @@ use apollo_router::_private::create_test_service_factory_from_yaml; use apollo_router::Configuration; use apollo_router::Context; use apollo_router::graphql; +use apollo_router::graphql::Error; use apollo_router::plugin::Plugin; use apollo_router::plugin::PluginInit; use apollo_router::services::router; @@ -30,6 +31,7 @@ use parking_lot::Mutex; use serde_json_bytes::json; use tower::BoxError; use tower::ServiceExt; +use uuid::Uuid; use walkdir::DirEntry; use walkdir::WalkDir; @@ -118,10 +120,6 @@ async fn simple_queries_should_not_work() { Please either specify a 'content-type' header \ (with a mime-type that is not one of application/x-www-form-urlencoded, multipart/form-data, text/plain) \ or provide one of the following headers: x-apollo-operation-name, apollo-require-preflight"; - let expected_error = graphql::Error::builder() - .message(message) - .extension_code("CSRF_ERROR") - .build(); let mut get_request: router::Request = supergraph::Request::builder() .query("{ topProducts { upc name reviews {id product { name } author { id name } } } }") @@ -145,6 +143,13 @@ async fn simple_queries_should_not_work() { let actual = query_with_router(router, get_request).await; + let expected_error = graphql::Error::builder() + .message(message) + .extension_code("CSRF_ERROR") + // Overwrite error ID to avoid comparing random Uuids + .apollo_id(actual.errors[0].apollo_id()) + .build(); + assert_eq!( 1, actual.errors.len(), @@ -179,6 +184,7 @@ async fn empty_posts_should_not_work() { .message(message) .extension_code("INVALID_GRAPHQL_REQUEST") .extensions(extensions_map) + .apollo_id(actual.errors[0].apollo_id()) .build(); assert_eq!(expected_error, actual.errors[0]); assert_eq!(registry.totals(), hashmap! {}); @@ -235,11 +241,6 @@ async fn service_errors_should_be_propagated() { let message = "Unknown operation named \"invalidOperationName\""; let mut extensions_map = serde_json_bytes::map::Map::new(); extensions_map.insert("code", "GRAPHQL_UNKNOWN_OPERATION_NAME".into()); - let expected_error = apollo_router::graphql::Error::builder() - .message(message) - .extensions(extensions_map) - .extension_code("VALIDATION_ERROR") - .build(); let request = supergraph::Request::fake_builder() .query(r#"{ topProducts { name } }"#) @@ -251,6 +252,14 @@ async fn service_errors_should_be_propagated() { let (actual, registry) = query_rust(request).await; + let expected_error = apollo_router::graphql::Error::builder() + .message(message) + .extensions(extensions_map) + .extension_code("VALIDATION_ERROR") + // Overwrite error ID to avoid comparing random Uuids + .apollo_id(actual.errors[0].apollo_id()) + .build(); + assert_eq!(expected_error, actual.errors[0]); assert_eq!(registry.totals(), expected_service_hits); } @@ -339,11 +348,6 @@ async fn mutation_should_work_over_post() { async fn automated_persisted_queries() { let (router, registry) = setup_router_and_registry(serde_json::json!({})).await; - let expected_apq_miss_error = apollo_router::graphql::Error::builder() - .message("PersistedQueryNotFound") - .extension_code("PERSISTED_QUERY_NOT_FOUND") - .build(); - let persisted = json!({ "version" : 1u8, "sha256Hash" : "9d1474aa069127ff795d3412b11dfc1f1be0853aed7a54c4a619ee0b1725382e" @@ -361,6 +365,12 @@ async fn automated_persisted_queries() { let actual = query_with_router(router.clone(), apq_only_request.try_into().unwrap()).await; + let expected_apq_miss_error = apollo_router::graphql::Error::builder() + .message("PersistedQueryNotFound") + .extension_code("PERSISTED_QUERY_NOT_FOUND") + .apollo_id(actual.errors[0].apollo_id()) + .build(); + assert_eq!(expected_apq_miss_error, actual.errors[0]); assert_eq!(1, actual.errors.len()); assert_eq!(registry.totals(), expected_service_hits); @@ -473,6 +483,8 @@ async fn persisted_queries() { "Persisted query '{UNKNOWN_QUERY_ID}' not found in the persisted query list" )) .extension_code("PERSISTED_QUERY_NOT_IN_LIST") + // Overwrite error ID to avoid comparing random Uuids + .apollo_id(actual.errors[0].apollo_id()) .build() ] ); @@ -561,7 +573,7 @@ async fn missing_variables() { assert_eq!(StatusCode::BAD_REQUEST, http_response.response.status()); - let mut response = serde_json::from_slice::( + let response = serde_json::from_slice::( http_response .next_response() .await @@ -572,11 +584,15 @@ async fn missing_variables() { ) .unwrap(); - let mut expected = vec![ + let mut normalized_actual_errors = normalize_errors(response.errors); + normalized_actual_errors.sort_by_key(|e| e.message.clone()); + + let mut expected_errors = vec![ graphql::Error::builder() .message("missing variable `$missingVariable`: for required GraphQL type `Int!`") .extension_code("VALIDATION_INVALID_TYPE_VARIABLE") .extension("name", "missingVariable") + .apollo_id(Uuid::nil()) .build(), graphql::Error::builder() .message( @@ -584,11 +600,12 @@ async fn missing_variables() { ) .extension_code("VALIDATION_INVALID_TYPE_VARIABLE") .extension("name", "yetAnotherMissingVariable") + .apollo_id(Uuid::nil()) .build(), ]; - response.errors.sort_by_key(|e| e.message.clone()); - expected.sort_by_key(|e| e.message.clone()); - assert_eq!(response.errors, expected); + + expected_errors.sort_by_key(|e| e.message.clone()); + assert_eq!(normalized_actual_errors, expected_errors); } /// @@ -674,7 +691,8 @@ async fn input_object_variable_validation() { .next_response() .await .unwrap(); - insta::assert_debug_snapshot!(&response.errors, @r###" + let normalized_errors = normalize_errors(response.errors); + insta::assert_debug_snapshot!(normalized_errors, @r###" [ Error { message: "missing input value at `$x.coordinates[0].longitude`: for required GraphQL type `Float!`", @@ -688,6 +706,7 @@ async fn input_object_variable_validation() { "VALIDATION_INVALID_TYPE_VARIABLE", ), }, + apollo_id: 00000000-0000-0000-0000-000000000000, }, ] "###); @@ -697,7 +716,6 @@ const PARSER_LIMITS_TEST_QUERY: &str = r#"{ me { reviews { author { reviews { author { name } } } } } }"#; const PARSER_LIMITS_TEST_QUERY_TOKEN_COUNT: usize = 36; const PARSER_LIMITS_TEST_QUERY_RECURSION: usize = 6; - #[tokio::test(flavor = "multi_thread")] async fn query_just_under_recursion_limit() { let config = serde_json::json!({ @@ -1519,3 +1537,19 @@ fn it_will_not_start_with_loose_file_permissions() { "Apollo key file permissions (0o777) are too permissive\n" ) } + +fn normalize_errors(errors: Vec) -> Vec { + errors + .into_iter() + .map(|e| { + Error::builder() + // Overwrite error ID to avoid comparing random Uuids + .apollo_id(Uuid::nil()) + .message(e.message) + .locations(e.locations) + .and_path(e.path) + .extensions(e.extensions) + .build() + }) + .collect() +}