improve errors

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<reqwest::Client, Box<dyn std::error::Error>> {
+) -> Result<reqwest::Client, reqwest::Error> {
     let mut headers = HeaderMap::new();
     headers.insert(CONTENT_TYPE, HeaderValue::from_static("application/json"));
 
@@ -24,7 +24,7 @@ pub async fn execute_graphql<T: serde::de::DeserializeOwned>(
     headers: &BTreeMap<String, String>,
     client: &reqwest::Client,
     return_headers: &Vec<String>,
-) -> Result<(BTreeMap<String, String>, graphql_client::Response<T>), Box<dyn Error>> {
+) -> Result<(BTreeMap<String, String>, graphql_client::Response<T>), reqwest::Error> {
     let mut request = client.post(endpoint);
 
     for (header_name, header_value) in headers {
@@ -52,11 +52,7 @@ pub async fn execute_graphql<T: serde::de::DeserializeOwned>(
         })
         .collect();
 
-    if response.error_for_status_ref().is_err() {
-        return Err(response.text().await?.into());
-    }
-
-    let response: graphql_client::Response<T> = response.json().await?;
+    let response: graphql_client::Response<T> = response.error_for_status()?.json().await?;
 
     Ok((headers, response))
 }

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"
 

crates/ndc-graphql/src/connector/mutation.rs

@@ -4,22 +4,23 @@ 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};
 
 pub async fn handle_mutation_explain(
     configuration: &ServerConfig,
     _state: &ServerState,
     request: models::MutationRequest,
-) -> Result<models::ExplainResponse, MutationError> {
+) -> Result<models::ExplainResponse, ErrorResponse> {
     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<models::MutationResponse, MutationError> {
+) -> Result<models::MutationResponse, ErrorResponse> {
     #[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::<Result<Vec<_>, 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"
+                }),
             ))
         }
     })

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<models::ExplainResponse, QueryError> {
+) -> Result<models::ExplainResponse, ErrorResponse> {
     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<models::QueryResponse, QueryError> {
+) -> Result<models::QueryResponse, ErrorResponse> {
     #[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"
+                }),
             ))
         }
     })

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<reqwest::Client, Box<dyn Error>> {
+    pub async fn client(&self, config: &ServerConfig) -> Result<reqwest::Client, reqwest::Error> {
         if let Some(client) = &*self.client.read().await {
             Ok(client.clone())
         } else {