From 11b9571d42c3abf757900e22cd30feffc049c1e4 Mon Sep 17 00:00:00 2001 From: Arda TANRIKULU Date: Thu, 12 Mar 2026 13:30:01 +0300 Subject: [PATCH] Cleanup error handling --- .changeset/error-handling.md | 5 +++ bin/router/src/lib.rs | 2 +- bin/router/src/pipeline/error.rs | 43 ++++++++-------------- bin/router/src/pipeline/normalize.rs | 8 ++--- bin/router/src/pipeline/parser.rs | 44 ++++------------------- bin/router/src/pipeline/query_plan.rs | 16 ++++----- lib/graphql-tools/src/parser/query/mod.rs | 2 +- 7 files changed, 37 insertions(+), 83 deletions(-) create mode 100644 .changeset/error-handling.md diff --git a/.changeset/error-handling.md b/.changeset/error-handling.md new file mode 100644 index 000000000..41450e58c --- /dev/null +++ b/.changeset/error-handling.md @@ -0,0 +1,5 @@ +--- +hive-router: patch +--- + +Cleanup Error Handling implementation \ No newline at end of file diff --git a/bin/router/src/lib.rs b/bin/router/src/lib.rs index 945cfaad4..83a1ef103 100644 --- a/bin/router/src/lib.rs +++ b/bin/router/src/lib.rs @@ -144,7 +144,7 @@ async fn graphql_endpoint_dispatch( // If the request handler returns an error, convert it to an HTTP response. Err(err) => { write_graphql_response_metric_status(request, GraphQLResponseStatus::Error); - handle_pipeline_error(err, &app_state, &response_mode) + handle_pipeline_error(&err, &app_state, &response_mode) } }; diff --git a/bin/router/src/pipeline/error.rs b/bin/router/src/pipeline/error.rs index b9c2ae65e..c1dbefe4b 100644 --- a/bin/router/src/pipeline/error.rs +++ b/bin/router/src/pipeline/error.rs @@ -1,6 +1,6 @@ use std::{sync::Arc, vec}; -use graphql_tools::validation::utils::ValidationError; +use graphql_tools::{parser::query::MinifyError, validation::utils::ValidationError}; use hive_router_plan_executor::{ execution::{error::PlanExecutionError, jwt_forward::JwtForwardingError}, headers::errors::HeaderRuleRuntimeError, @@ -69,13 +69,13 @@ pub enum PipelineError { FailedToParseExtensions(sonic_rs::Error), #[error("Failed to parse GraphQL operation: {0}")] #[strum(serialize = "GRAPHQL_PARSE_FAILED")] - FailedToParseOperation(#[from] Arc), + FailedToParseOperation(#[from] graphql_tools::parser::query::ParseError), #[error("Failed to minify parsed GraphQL operation: {0}")] #[strum(serialize = "GRAPHQL_PARSE_MINIFY_FAILED")] - FailedToMinifyParsedOperation(String), + FailedToMinifyParsedOperation(#[from] MinifyError), #[error("Failed to normalize GraphQL operation")] #[strum(serialize = "OPERATION_RESOLUTION_FAILURE")] - NormalizationError(#[from] Arc), + NormalizationError(#[from] NormalizationError), #[error("Failed to collect GraphQL variables: {0}")] #[strum(serialize = "BAD_USER_INPUT")] VariablesCoercionError(String), @@ -90,7 +90,7 @@ pub enum PipelineError { PlanExecutionError(#[from] PlanExecutionError), #[error("Failed to produce a plan: {0}")] #[strum(serialize = "QUERY_PLAN_BUILD_FAILED")] - PlannerError(#[from] Arc), + PlannerError(#[from] PlannerError), #[error(transparent)] #[strum(serialize = "OVERRIDE_LABEL_EVALUATION_FAILED")] LabelEvaluationError(#[from] LabelEvaluationError), @@ -143,30 +143,9 @@ pub enum PipelineError { #[error("No supergraph available yet, unable to process request")] #[strum(serialize = "NO_SUPERGRAPH_AVAILABLE")] NoSupergraphAvailable, -} - -#[derive(Clone, Debug, thiserror::Error)] -pub enum ParserCacheError { - #[error("Failed to parse GraphQL operation: {0}")] - ParseError(Arc), - #[error("Failed to minify parsed GraphQL operation: {0}")] - MinifyError(String), - #[error("Validation errors")] - ValidationErrors(Arc>), -} -impl From> for PipelineError { - fn from(value: Arc) -> Self { - match value.as_ref() { - ParserCacheError::ParseError(err) => PipelineError::FailedToParseOperation(err.clone()), - ParserCacheError::MinifyError(err) => { - PipelineError::FailedToMinifyParsedOperation(err.clone()) - } - ParserCacheError::ValidationErrors(errs) => { - PipelineError::ValidationErrors(errs.clone()) - } - } - } + #[error(transparent)] + PipelineErrorArc(#[from] Arc), } impl PipelineError { @@ -175,6 +154,7 @@ impl PipelineError { Self::JwtError(err) => err.error_code(), Self::PlanExecutionError(err) => err.error_code(), Self::ReadBodyStreamError(err) => err.error_code(), + Self::PipelineErrorArc(err) => err.graphql_error_code(), _ => self.into(), } } @@ -182,6 +162,7 @@ impl PipelineError { pub fn graphql_error_message(&self) -> String { match self { Self::PlannerError(_) => "Unexpected error".to_string(), + Self::PipelineErrorArc(err) => err.graphql_error_message(), _ => self.to_string(), } } @@ -226,6 +207,7 @@ impl PipelineError { (Self::HeaderPropagation(_), _) => StatusCode::INTERNAL_SERVER_ERROR, (Self::QueryPlanSerializationFailed(_), _) => StatusCode::INTERNAL_SERVER_ERROR, (Self::NoSupergraphAvailable, _) => StatusCode::SERVICE_UNAVAILABLE, + (Self::PipelineErrorArc(err), _) => err.default_status_code(prefer_ok), } } } @@ -237,10 +219,13 @@ struct FailedExecutionResult { #[inline] pub fn handle_pipeline_error( - err: PipelineError, + err: &PipelineError, shared_state: &RouterSharedState, response_mode: &ResponseMode, ) -> web::HttpResponse { + if let PipelineError::PipelineErrorArc(inner) = &err { + return handle_pipeline_error(inner, shared_state, response_mode); + } let single_content_type = response_mode.single_content_type(); let prefer_ok = response_mode.prefer_status_ok_for_errors(); diff --git a/bin/router/src/pipeline/normalize.rs b/bin/router/src/pipeline/normalize.rs index 3a9a1233c..487d1a2dc 100644 --- a/bin/router/src/pipeline/normalize.rs +++ b/bin/router/src/pipeline/normalize.rs @@ -8,7 +8,6 @@ use hive_router_plan_executor::hooks::on_graphql_params::GraphQLParams; use hive_router_plan_executor::hooks::on_supergraph_load::SupergraphData; use hive_router_plan_executor::introspection::partition::partition_operation; use hive_router_plan_executor::projection::plan::FieldProjectionPlan; -use hive_router_query_planner::ast::normalization::error::NormalizationError; use hive_router_query_planner::ast::normalization::normalize_operation; use hive_router_query_planner::ast::operation::OperationDefinition; use xxhash_rust::xxh3::Xxh3; @@ -68,10 +67,10 @@ pub async fn normalize_request_with_cache( None => parser_payload.cache_key, }; - schema_state + Ok(schema_state .normalize_cache .entry(cache_key) - .or_try_insert_with::<_, NormalizationError>(async { + .or_try_insert_with(async { let doc = normalize_operation( &supergraph.planner.supergraph, &parser_payload.parsed_operation, @@ -106,7 +105,6 @@ pub async fn normalize_request_with_cache( Ok(Arc::new(payload)) }) .await - .map_err(PipelineError::from) .into_result_with_hit_miss(|hit_miss| match hit_miss { CacheHitMiss::Hit => { normalize_span.record_cache_hit(true); @@ -116,7 +114,7 @@ pub async fn normalize_request_with_cache( normalize_span.record_cache_hit(false); normalize_cache_capture.finish_miss(); } - }) + })?) } .instrument(normalize_span.clone()) .await diff --git a/bin/router/src/pipeline/parser.rs b/bin/router/src/pipeline/parser.rs index 599dc92f8..028b5de22 100644 --- a/bin/router/src/pipeline/parser.rs +++ b/bin/router/src/pipeline/parser.rs @@ -21,7 +21,7 @@ use hive_router_query_planner::utils::parsing::{ use xxhash_rust::xxh3::Xxh3; use crate::cache_state::{CacheHitMiss, EntryResultHitMissExt}; -use crate::pipeline::error::{ParserCacheError, PipelineError}; +use crate::pipeline::error::PipelineError; use crate::pipeline::execution_request::GetQueryStr; use crate::shared_state::RouterSharedState; use tracing::{error, trace, Instrument}; @@ -33,35 +33,6 @@ pub struct ParseCacheEntry { hive_operation_hash: Arc, } -impl ParseCacheEntry { - #[inline] - pub fn try_new( - parsed_arc: Arc>, - query_str: &str, - ) -> Result { - let minified_arc = { - Arc::new(minify_query(query_str).map_err(|err| { - error!("Failed to minify parsed GraphQL operation: {}", err); - PipelineError::FailedToMinifyParsedOperation(err.to_string()) - })?) - }; - let hive_normalized_operation = hive_sdk_normalize_operation(&parsed_arc); - let hive_minified = - minify_query(hive_normalized_operation.to_string().as_ref()).map_err(|err| { - error!( - "Failed to minify GraphQL operation normalized for Hive SDK: {}", - err - ); - PipelineError::FailedToMinifyParsedOperation(err.to_string()) - })?; - Ok(ParseCacheEntry { - document: parsed_arc, - document_minified_string: minified_arc, - hive_operation_hash: Arc::new(format!("{:x}", md5::compute(hive_minified))), - }) - } -} - #[derive(Debug)] pub struct GraphQLParserPayload { pub parsed_operation: Arc>, @@ -136,7 +107,7 @@ pub async fn parse_operation_with_cache( let parse_cache_item = app_state .parse_cache .entry(cache_key) - .or_try_insert_with::<_, ParserCacheError>(async { + .or_try_insert_with(async { let parsed = match app_state.router_config.limits.max_tokens.as_ref() { Some(cfg) => safe_parse_operation_with_token_limit(query_str, cfg.n), _ => safe_parse_operation(query_str), @@ -146,7 +117,7 @@ pub async fn parse_operation_with_cache( err.0.errors.first() { if *msg == "Token limit exceeded" { - return ParserCacheError::ValidationErrors( + return PipelineError::ValidationErrors( vec![ValidationError { locations: vec![err.0.position], message: "Token limit exceeded.".to_string(), @@ -157,22 +128,20 @@ pub async fn parse_operation_with_cache( } } error!("Failed to parse GraphQL operation: {}", err); - ParserCacheError::ParseError(Arc::new(err)) + err.into() })?; trace!("successfully parsed GraphQL operation"); let parsed_arc = Arc::new(parsed); - let minified_arc = Arc::new(minify_query(query_str).map_err(|err| { + let minified_arc = Arc::new(minify_query(query_str).inspect_err(|err| { error!("Failed to minify parsed GraphQL operation: {}", err); - ParserCacheError::MinifyError(err.to_string()) })?); let hive_normalized_operation = hive_sdk_normalize_operation(&parsed_arc); let hive_minified = minify_query(hive_normalized_operation.to_string().as_ref()) - .map_err(|err| { + .inspect_err(|err| { error!( "Failed to minify GraphQL operation normalized for Hive SDK: {}", err ); - ParserCacheError::MinifyError(err.to_string()) })?; Ok(ParseCacheEntry { @@ -182,7 +151,6 @@ pub async fn parse_operation_with_cache( }) }) .await - .map_err(PipelineError::from) .into_result_with_hit_miss(|hit_miss| match hit_miss { CacheHitMiss::Hit => { parse_span.record_cache_hit(true); diff --git a/bin/router/src/pipeline/query_plan.rs b/bin/router/src/pipeline/query_plan.rs index 92ad37a3e..c2685db11 100644 --- a/bin/router/src/pipeline/query_plan.rs +++ b/bin/router/src/pipeline/query_plan.rs @@ -109,17 +109,15 @@ pub async fn plan_operation_with_cache( return Ok(EMPTY_QUERY_PLAN.clone()); } - supergraph - .planner - .plan_from_normalized_operation( - filtered_operation_for_plan, - (&request_override_context.clone()).into(), - cancellation_token, - ) - .map(Arc::new) + let query_plan = supergraph.planner.plan_from_normalized_operation( + filtered_operation_for_plan, + (&request_override_context.clone()).into(), + cancellation_token, + )?; + + Ok(query_plan.into()) }) .await - .map_err(PipelineError::from) .into_result_with_hit_miss(|hit_miss| match hit_miss { CacheHitMiss::Hit => { cache_hint = CacheHint::Hit; diff --git a/lib/graphql-tools/src/parser/query/mod.rs b/lib/graphql-tools/src/parser/query/mod.rs index a76edb47f..c4cc1d20b 100644 --- a/lib/graphql-tools/src/parser/query/mod.rs +++ b/lib/graphql-tools/src/parser/query/mod.rs @@ -9,4 +9,4 @@ mod minify; pub use self::ast::*; pub use self::error::ParseError; pub use self::grammar::{consume_definition, parse_query, parse_query_with_token_limit}; -pub use self::minify::{minify_query, minify_query_document}; +pub use self::minify::{minify_query, minify_query_document, MinifyError};