From d774227f1ff455021727b453ef7fc842fc50cb12 Mon Sep 17 00:00:00 2001 From: Benoit Ranque Date: Tue, 1 Oct 2024 13:28:05 -0400 Subject: [PATCH] improve errors --- Cargo.lock | 2 + crates/common/src/client.rs | 12 +- crates/ndc-graphql/Cargo.toml | 2 + crates/ndc-graphql/src/connector/mutation.rs | 33 +++-- crates/ndc-graphql/src/connector/query.rs | 32 +++-- crates/ndc-graphql/src/connector/state.rs | 5 +- crates/ndc-graphql/src/query_builder/error.rs | 117 ++++++------------ crates/ndc-graphql/tests/query_builder.rs | 6 +- 8 files changed, 88 insertions(+), 121 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3e4c2d4..3437c9c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1198,6 +1198,7 @@ dependencies = [ "common", "glob-match", "graphql-parser", + "http 0.2.12", "indexmap 2.5.0", "insta", "ndc-sdk", @@ -1206,6 +1207,7 @@ dependencies = [ "schemars", "serde", "serde_json", + "thiserror", "tokio", "tracing", ] diff --git a/crates/common/src/client.rs b/crates/common/src/client.rs index e5835b7..f4494a7 100644 --- a/crates/common/src/client.rs +++ b/crates/common/src/client.rs @@ -2,11 +2,11 @@ use crate::config::ConnectionConfig; use glob_match::glob_match; use reqwest::header::{HeaderMap, HeaderValue, CONTENT_TYPE}; use serde::Serialize; -use std::{collections::BTreeMap, error::Error, fmt::Debug}; +use std::{collections::BTreeMap, fmt::Debug}; pub fn get_http_client( _connection_config: &ConnectionConfig, -) -> Result> { +) -> Result { let mut headers = HeaderMap::new(); headers.insert(CONTENT_TYPE, HeaderValue::from_static("application/json")); @@ -24,7 +24,7 @@ pub async fn execute_graphql( headers: &BTreeMap, client: &reqwest::Client, return_headers: &Vec, -) -> Result<(BTreeMap, graphql_client::Response), Box> { +) -> Result<(BTreeMap, graphql_client::Response), reqwest::Error> { let mut request = client.post(endpoint); for (header_name, header_value) in headers { @@ -52,11 +52,7 @@ pub async fn execute_graphql( }) .collect(); - if response.error_for_status_ref().is_err() { - return Err(response.text().await?.into()); - } - - let response: graphql_client::Response = response.json().await?; + let response: graphql_client::Response = response.error_for_status()?.json().await?; Ok((headers, response)) } diff --git a/crates/ndc-graphql/Cargo.toml b/crates/ndc-graphql/Cargo.toml index 59d2506..c75e5ad 100644 --- a/crates/ndc-graphql/Cargo.toml +++ b/crates/ndc-graphql/Cargo.toml @@ -8,6 +8,7 @@ async-trait = "0.1.78" common = { path = "../common" } glob-match = "0.2.1" graphql-parser = "0.4.0" +http = "0.2" indexmap = "2.1.0" ndc-sdk = { git = "https://github.com/hasura/ndc-sdk-rs", tag = "v0.4.0", package = "ndc-sdk", features = [ "rustls", @@ -19,6 +20,7 @@ reqwest = { version = "0.12.3", features = [ ], default-features = false } serde = { version = "1.0.197", features = ["derive"] } serde_json = "1.0.114" +thiserror = "1.0.64" tokio = "1.36.0" tracing = "0.1.40" diff --git a/crates/ndc-graphql/src/connector/mutation.rs b/crates/ndc-graphql/src/connector/mutation.rs index e0abdff..b3c5627 100644 --- a/crates/ndc-graphql/src/connector/mutation.rs +++ b/crates/ndc-graphql/src/connector/mutation.rs @@ -4,8 +4,9 @@ use common::{ client::{execute_graphql, GraphQLRequest}, config::ServerConfig, }; +use http::StatusCode; use indexmap::IndexMap; -use ndc_sdk::{connector::MutationError, models}; +use ndc_sdk::{connector::ErrorResponse, models}; use std::{collections::BTreeMap, mem}; use tracing::{Instrument, Level}; @@ -13,13 +14,13 @@ pub async fn handle_mutation_explain( configuration: &ServerConfig, _state: &ServerState, request: models::MutationRequest, -) -> Result { +) -> Result { let operation = tracing::info_span!("Build Mutation Document", internal.visibility = "user") .in_scope(|| build_mutation_document(&request, configuration))?; let query = serde_json::to_string_pretty(&GraphQLRequest::new(&operation.query, &operation.variables)) - .map_err(|err| MutationError::new_invalid_request(&err))?; + .map_err(ErrorResponse::from_error)?; let details = BTreeMap::from_iter(vec![ ("SQL Query".to_string(), operation.query), @@ -37,12 +38,11 @@ pub async fn handle_mutation( configuration: &ServerConfig, state: &ServerState, request: models::MutationRequest, -) -> Result { +) -> Result { #[cfg(debug_assertions)] { // this block only present in debug builds, to avoid leaking sensitive information - let request_string = serde_json::to_string(&request) - .map_err(|err| MutationError::new_invalid_request(&err))?; + let request_string = serde_json::to_string(&request).map_err(ErrorResponse::from_error)?; tracing::event!(Level::DEBUG, "Incoming IR" = request_string); } @@ -52,7 +52,7 @@ pub async fn handle_mutation( let client = state .client(configuration) .await - .map_err(|err| MutationError::new_invalid_request(&err))?; + .map_err(ErrorResponse::from_error)?; let execution_span = tracing::info_span!("Execute GraphQL Mutation", internal.visibility = "user"); @@ -67,12 +67,17 @@ pub async fn handle_mutation( ) .instrument(execution_span) .await - .map_err(|err| MutationError::new_unprocessable_content(&err))?; + .map_err(ErrorResponse::from_error)?; tracing::info_span!("Process Response").in_scope(|| { if let Some(errors) = response.errors { - Err(MutationError::new_unprocessable_content(&errors[0].message) - .with_details(serde_json::json!({ "errors": errors }))) + Err(ErrorResponse::new( + StatusCode::UNPROCESSABLE_ENTITY, + "Errors in graphql query response".to_string(), + serde_json::json!({ + "errors": errors + }), + )) } else if let Some(mut data) = response.data { let forward_response_headers = !configuration.response.forward_headers.is_empty(); @@ -103,12 +108,14 @@ pub async fn handle_mutation( }), }) .collect::, serde_json::Error>>() - .map_err(|err| MutationError::new_unprocessable_content(&err))?; + .map_err(ErrorResponse::from_error)?; Ok(models::MutationResponse { operation_results }) } else { - Err(MutationError::new_unprocessable_content( - &"No data or errors in response", + Err(ErrorResponse::new_internal_with_details( + serde_json::json!({ + "message": "No data or errors in response" + }), )) } }) diff --git a/crates/ndc-graphql/src/connector/query.rs b/crates/ndc-graphql/src/connector/query.rs index 2c9acaf..4d95281 100644 --- a/crates/ndc-graphql/src/connector/query.rs +++ b/crates/ndc-graphql/src/connector/query.rs @@ -4,9 +4,10 @@ use common::{ client::{execute_graphql, GraphQLRequest}, config::ServerConfig, }; +use http::StatusCode; use indexmap::IndexMap; use ndc_sdk::{ - connector::QueryError, + connector::{ErrorResponse, QueryError}, models::{self, FieldName}, }; use std::collections::BTreeMap; @@ -16,10 +17,9 @@ pub async fn handle_query_explain( configuration: &ServerConfig, _state: &ServerState, request: models::QueryRequest, -) -> Result { +) -> Result { let operation = tracing::info_span!("Build Query Document", internal.visibility = "user") - .in_scope(|| build_query_document(&request, configuration)) - .map_err(|err| QueryError::new_invalid_request(&err))?; + .in_scope(|| build_query_document(&request, configuration))?; let query = serde_json::to_string_pretty(&GraphQLRequest::new(&operation.query, &operation.variables)) @@ -41,12 +41,11 @@ pub async fn handle_query( configuration: &ServerConfig, state: &ServerState, request: models::QueryRequest, -) -> Result { +) -> Result { #[cfg(debug_assertions)] { // this block only present in debug builds, to avoid leaking sensitive information - let request_string = - serde_json::to_string(&request).map_err(|err| QueryError::new_invalid_request(&err))?; + let request_string = serde_json::to_string(&request).map_err(ErrorResponse::from_error)?; tracing::event!(Level::DEBUG, "Incoming IR" = request_string); } @@ -56,7 +55,7 @@ pub async fn handle_query( let client = state .client(configuration) .await - .map_err(|err| QueryError::new_invalid_request(&err))?; + .map_err(ErrorResponse::from_error)?; let execution_span = tracing::info_span!("Execute GraphQL Query", internal.visibility = "user"); @@ -70,12 +69,17 @@ pub async fn handle_query( ) .instrument(execution_span) .await - .map_err(|err| QueryError::new_invalid_request(&err))?; + .map_err(ErrorResponse::from_error)?; tracing::info_span!("Process Response").in_scope(|| { if let Some(errors) = response.errors { - Err(QueryError::new_unprocessable_content(&errors[0].message) - .with_details(serde_json::json!({ "errors": errors }))) + Err(ErrorResponse::new( + StatusCode::UNPROCESSABLE_ENTITY, + "Errors in graphql query response".to_string(), + serde_json::json!({ + "errors": errors + }), + )) } else if let Some(data) = response.data { let forward_response_headers = !configuration.response.forward_headers.is_empty(); @@ -104,8 +108,10 @@ pub async fn handle_query( rows: Some(vec![row]), }])) } else { - Err(QueryError::new_unprocessable_content( - &"No data or errors in response", + Err(ErrorResponse::new_internal_with_details( + serde_json::json!({ + "message": "No data or errors in response" + }), )) } }) diff --git a/crates/ndc-graphql/src/connector/state.rs b/crates/ndc-graphql/src/connector/state.rs index 9d114b4..41d341e 100644 --- a/crates/ndc-graphql/src/connector/state.rs +++ b/crates/ndc-graphql/src/connector/state.rs @@ -1,6 +1,5 @@ -use std::{error::Error, sync::Arc}; - use common::{client::get_http_client, config::ServerConfig}; +use std::sync::Arc; use tokio::sync::RwLock; #[derive(Debug, Clone)] @@ -17,7 +16,7 @@ impl ServerState { client: Arc::new(RwLock::new(client)), } } - pub async fn client(&self, config: &ServerConfig) -> Result> { + pub async fn client(&self, config: &ServerConfig) -> Result { if let Some(client) = &*self.client.read().await { Ok(client.clone()) } else { diff --git a/crates/ndc-graphql/src/query_builder/error.rs b/crates/ndc-graphql/src/query_builder/error.rs index 2bef84e..0400d42 100644 --- a/crates/ndc-graphql/src/query_builder/error.rs +++ b/crates/ndc-graphql/src/query_builder/error.rs @@ -1,107 +1,66 @@ -use std::fmt::Display; - +use http::StatusCode; use ndc_sdk::{ - connector::{MutationError, QueryError}, + connector::ErrorResponse, models::{ArgumentName, CollectionName, FieldName, ProcedureName, TypeName, VariableName}, }; -#[derive(Debug)] +#[derive(Debug, thiserror::Error)] pub enum QueryBuilderError { - SchemaDefinitionNotFound, + #[error("Object Type {0} not found in configuration")] ObjectTypeNotFound(TypeName), + #[error("Input Object Type {0} not found in configuration")] InputObjectTypeNotFound(TypeName), + #[error("No fields for query")] NoRequesQueryFields, + #[error("No query type in schema definition")] NoQueryType, + #[error("No mutation type in schema definition")] NoMutationType, + #[error("Feature not supported: {0}")] NotSupported(String), - QueryFieldNotFound { - field: CollectionName, - }, - MutationFieldNotFound { - field: ProcedureName, - }, - ObjectFieldNotFound { - object: TypeName, - field: FieldName, - }, + #[error("Field {field} not found in Query type")] + QueryFieldNotFound { field: CollectionName }, + #[error("Field {field} not found in Mutation type")] + MutationFieldNotFound { field: ProcedureName }, + #[error("Field {field} not found in Object Type {object}")] + ObjectFieldNotFound { object: TypeName, field: FieldName }, + #[error("Field {field} not found in Input Object Type {input_object}")] InputObjectFieldNotFound { input_object: TypeName, field: FieldName, }, + #[error("Argument {argument} for field {field} not found in Object Type {object}")] ArgumentNotFound { object: TypeName, field: FieldName, argument: ArgumentName, }, + #[error("Misshapen headers argument: {0}")] MisshapenHeadersArgument(serde_json::Value), + #[error("Unexpected: {0}")] Unexpected(String), + #[error("Missing variable {0}")] MissingVariable(VariableName), } -impl std::error::Error for QueryBuilderError {} - -impl From for QueryError { +impl From for ErrorResponse { fn from(value: QueryBuilderError) -> Self { - QueryError::new_invalid_request(&value) - } -} -impl From for MutationError { - fn from(value: QueryBuilderError) -> Self { - MutationError::new_invalid_request(&value) - } -} - -impl Display for QueryBuilderError { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - QueryBuilderError::SchemaDefinitionNotFound => { - write!(f, "Schema definition not found in configuration") - } - QueryBuilderError::ObjectTypeNotFound(obj) => { - write!(f, "Object Type {obj} not found in configuration") - } - QueryBuilderError::InputObjectTypeNotFound(input) => { - write!(f, "Input Object Type {input} not found in configuration") - } - QueryBuilderError::NoRequesQueryFields => { - write!(f, "Misshapen request: no fields for query") - } - QueryBuilderError::NoQueryType => write!(f, "No query type in schema definition"), - QueryBuilderError::NoMutationType => write!(f, "No mutation type in schema definition"), - QueryBuilderError::NotSupported(feature) => { - write!(f, "Feature not supported: {feature}") - } - QueryBuilderError::ObjectFieldNotFound { object, field } => { - write!(f, "Field {field} not found in Object Type {object}") - } - QueryBuilderError::InputObjectFieldNotFound { - input_object, - field, - } => { - write!( - f, - "Field {field} not found in Input Object Type {input_object}" - ) - } - QueryBuilderError::ArgumentNotFound { - object, - field, - argument, - } => write!( - f, - "Argument {argument} for field {field} not found in Object Type {object}" - ), - QueryBuilderError::Unexpected(s) => write!(f, "Unexpected: {s}"), - QueryBuilderError::QueryFieldNotFound { field } => { - write!(f, "Field {field} not found in Query type") - } - QueryBuilderError::MutationFieldNotFound { field } => { - write!(f, "Field {field} not found in Mutation type") - } - QueryBuilderError::MisshapenHeadersArgument(headers) => { - write!(f, "Misshapen headers argument: {}", headers) - } - QueryBuilderError::MissingVariable(name) => write!(f, "Missing variable {name}"), - } + let status = match value { + QueryBuilderError::ObjectTypeNotFound(_) + | QueryBuilderError::InputObjectTypeNotFound(_) => StatusCode::INTERNAL_SERVER_ERROR, + QueryBuilderError::NoRequesQueryFields + | QueryBuilderError::NoQueryType + | QueryBuilderError::NoMutationType + | QueryBuilderError::NotSupported(_) + | QueryBuilderError::QueryFieldNotFound { .. } + | QueryBuilderError::MutationFieldNotFound { .. } + | QueryBuilderError::ObjectFieldNotFound { .. } + | QueryBuilderError::InputObjectFieldNotFound { .. } + | QueryBuilderError::ArgumentNotFound { .. } + | QueryBuilderError::MisshapenHeadersArgument(_) + | QueryBuilderError::Unexpected(_) + | QueryBuilderError::MissingVariable(_) => StatusCode::BAD_REQUEST, + }; + ErrorResponse::new(status, value.to_string(), serde_json::Value::Null) } } diff --git a/crates/ndc-graphql/tests/query_builder.rs b/crates/ndc-graphql/tests/query_builder.rs index 54a3acb..5567183 100644 --- a/crates/ndc-graphql/tests/query_builder.rs +++ b/crates/ndc-graphql/tests/query_builder.rs @@ -113,9 +113,5 @@ fn test_capabilities() { #[test] fn configuration_schema() { - assert_snapshot!( - "Configuration JSON Schema", - serde_json::to_string_pretty(&schema_for!(ServerConfigFile)) - .expect("Should serialize schema to json") - ) + assert_json_snapshot!("Configuration JSON Schema", schema_for!(ServerConfigFile)) }