Merge pull request #18 from hasura:improve-error-handling

Improve-error-handling
hasura · Apr 26, 2024 · ece8fb0 · ece8fb0
2 parents 7bafb9f + ba1ed59
commit ece8fb0
Show file tree

Hide file tree

Showing 31 changed files with 11,826 additions and 35 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+## [0.2.7]
+
+- Fix a bug introduced in 0.2.6 that would cause responses including a single quote to be serialized as invalid JSON strings
+
 ## [0.2.6]
 
 - Add additional trace spans for SQL generation and query execution

diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -6,5 +6,5 @@ members = [
 ]
 resolver = "2"
 
-package.version = "0.2.6"
+package.version = "0.2.7"
 package.edition = "2021"
diff --git a/Dockerfile b/Dockerfile
@@ -10,8 +10,9 @@ COPY crates crates
 RUN cargo chef prepare --recipe-path recipe.json
 
 FROM chef AS builder
+ARG RUSTFLAGS
 COPY --from=planner /app/recipe.json recipe.json
-RUN cargo chef cook --release --recipe-path recipe.json
+RUN cargo chef cook --profile release --recipe-path recipe.json
 COPY Cargo.toml ./
 COPY Cargo.lock ./
 COPY crates crates

diff --git a/crates/ndc-clickhouse/src/connector/handler/explain.rs b/crates/ndc-clickhouse/src/connector/handler/explain.rs
@@ -16,9 +16,7 @@ pub async fn explain(
     state: &ServerState,
     request: models::QueryRequest,
 ) -> Result<JsonResponse<models::ExplainResponse>, ExplainError> {
-    let unsafe_statement = QueryBuilder::new(&request, configuration)
-        .build()
-        .map_err(|err| ExplainError::Other(Box::new(err)))?;
+    let unsafe_statement = QueryBuilder::new(&request, configuration).build()?;
 
     let unsafe_statement = unsafe_statement.explain();
 

diff --git a/crates/ndc-clickhouse/src/connector/handler/query.rs b/crates/ndc-clickhouse/src/connector/handler/query.rs
@@ -1,5 +1,9 @@
 use common::{client::execute_query, config::ServerConfig};
-use ndc_sdk::{connector::QueryError, json_response::JsonResponse, models};
+use ndc_sdk::{
+    connector::QueryError,
+    json_response::JsonResponse,
+    models::{self, QueryResponse},
+};
 use tracing::{Instrument, Level};
 
 use crate::{connector::state::ServerState, sql::QueryBuilder};
@@ -9,11 +13,26 @@ pub async fn query(
     state: &ServerState,
     request: models::QueryRequest,
 ) -> Result<JsonResponse<models::QueryResponse>, QueryError> {
+    #[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::Other(err.to_string().into()))?;
+
+        tracing::event!(Level::DEBUG, "Incoming IR" = request_string);
+    }
+
     let (statement_string, parameters) =
         tracing::info_span!("Build SQL Query").in_scope(|| -> Result<_, QueryError> {
-            let statement = QueryBuilder::new(&request, configuration)
-                .build()
-                .map_err(|err| QueryError::Other(Box::new(err)))?;
+            let statement = QueryBuilder::new(&request, configuration).build()?;
+
+            #[cfg(debug_assertions)]
+            {
+                // this block only present in debug builds, to avoid leaking sensitive information
+                let unsafe_statement_string = statement.to_unsafe_sql_string();
+
+                tracing::event!(Level::DEBUG, "Generated SQL" = unsafe_statement_string);
+            }
 
             let (statement, parameters) = statement.into_parameterized_statement();
 
@@ -27,17 +46,31 @@ pub async fn query(
         .await
         .map_err(|err| QueryError::Other(err.to_string().into()))?;
 
-    tracing::event!(Level::DEBUG, "Generated SQL" = statement_string);
+    let execution_span = tracing::info_span!(
+        "Execute SQL query",
+        db.system = "clickhouse",
+        db.user = configuration.connection.username,
+        db.statement = statement_string,
+    );
 
     let rowsets = execute_query(
         &client,
         &configuration.connection,
         &statement_string,
         &parameters,
     )
-    .instrument(tracing::info_span!("Execute SQL query"))
+    .instrument(execution_span)
     .await
-    .map_err(|err| QueryError::Other(err.to_string().into()))?;
+    .map_err(|err| QueryError::UnprocessableContent(err.to_string().into()))?;
+
+    #[cfg(debug_assertions)]
+    {
+        // this block only present in debug builds, to avoid leaking sensitive information
+        let result_string =
+            std::str::from_utf8(&rowsets).map_err(|err| QueryError::Other(err.into()))?;
+
+        tracing::event!(Level::DEBUG, "Response" = result_string);
+    }
 
     // we assume the response is a valid JSON string, and send those bytes back without parsing
     Ok(JsonResponse::Serialized(rowsets))

diff --git a/crates/ndc-clickhouse/src/sql/query_builder.rs b/crates/ndc-clickhouse/src/sql/query_builder.rs
@@ -57,8 +57,7 @@ impl<'r, 'c> QueryBuilder<'r, 'c> {
                                 query,
                                 &self.request.collection_relationships,
                                 &self.configuration,
-                            )
-                            .map_err(|err| QueryBuilderError::Typecasting(err.to_string()))?
+                            )?
                             .to_string(),
                         ))
                         .into_arg(),
@@ -71,6 +70,12 @@ impl<'r, 'c> QueryBuilder<'r, 'c> {
             .into_select(Some("rowsets"))];
 
         let with = if let Some(variables) = &self.request.variables {
+            // we make the following assumptions here:
+            // variables is an array with at least one item
+            // all items in variables are object with keys and primitive values
+            // all items have the same keys
+            // if those assumptions aren't true, we may generate a misshapen object
+            // clickhouse will reject this at runtime
             let mut variable_values: IndexMap<String, Vec<serde_json::Value>> = IndexMap::new();
 
             variable_values.insert(
@@ -165,7 +170,8 @@ impl<'r, 'c> QueryBuilder<'r, 'c> {
             .select(select)
             .from(from)
             .order_by(order_by)
-            .into_statement())
+            .into_statement()
+            .format("TabSeparatedRaw"))
     }
     fn rowset_subquery(
         &self,
@@ -236,7 +242,7 @@ impl<'r, 'c> QueryBuilder<'r, 'c> {
                         }
                         .into_arg())
                     })
-                    .collect::<Result<Vec<_>, _>>()?;
+                    .collect::<Result<Vec<_>, QueryBuilderError>>()?;
                 Function::new_unquoted("tuple").args(args).into_expr()
             })
         } else {
@@ -400,7 +406,7 @@ impl<'r, 'c> QueryBuilder<'r, 'c> {
                                 .into_box(),
                             })
                         })
-                        .collect::<Result<Vec<_>, _>>()?;
+                        .collect::<Result<Vec<_>, QueryBuilderError>>()?;
 
                     if self.request.variables.is_some() {
                         join_expr.push(Expr::BinaryOp {
@@ -612,7 +618,7 @@ impl<'r, 'c> QueryBuilder<'r, 'c> {
                                             .into_box(),
                                         })
                                     })
-                                    .collect::<Result<Vec<_>, _>>()?;
+                                    .collect::<Result<Vec<_>, QueryBuilderError>>()?;
 
                                 let join_operator = join_exprs
                                     .into_iter()
@@ -723,7 +729,7 @@ impl<'r, 'c> QueryBuilder<'r, 'c> {
                                         .into_box(),
                                     })
                                 })
-                                .collect::<Result<Vec<_>, _>>()?;
+                                .collect::<Result<Vec<_>, QueryBuilderError>>()?;
 
                             if self.request.variables.is_some() {
                                 join_exprs.push(Expr::BinaryOp {
@@ -790,7 +796,7 @@ impl<'r, 'c> QueryBuilder<'r, 'c> {
                     self.column_ident(relkey, current_collection)?,
                 ]))
             })
-            .collect::<Result<Vec<_>, _>>()?;
+            .collect::<Result<Vec<_>, QueryBuilderError>>()?;
 
         if self.request.variables.is_some() {
             limit_by_cols.push(Expr::CompoundIdentifier(vec![
@@ -1160,7 +1166,7 @@ impl<'r, 'c> QueryBuilder<'r, 'c> {
                             right,
                         })
                     })
-                    .collect::<Result<_, _>>()?
+                    .collect::<Result<_, QueryBuilderError>>()?
             }
             models::ExistsInCollection::Unrelated {
                 collection: _,
@@ -1353,7 +1359,7 @@ impl<'r, 'c> QueryBuilder<'r, 'c> {
                                                 .into_box(),
                                             })
                                         })
-                                        .collect::<Result<Vec<_>, _>>()?;
+                                        .collect::<Result<Vec<_>, QueryBuilderError>>()?;
 
                                     let join_operator = join_exprs
                                         .into_iter()
@@ -1443,7 +1449,7 @@ impl<'r, 'c> QueryBuilder<'r, 'c> {
                                         .into_box(),
                                     })
                                 })
-                                .collect::<Result<Vec<_>, _>>()?;
+                                .collect::<Result<Vec<_>, QueryBuilderError>>()?;
 
                             if self.request.variables.is_some() {
                                 join_exprs.push(Expr::BinaryOp {
@@ -1537,7 +1543,7 @@ impl<'r, 'c> QueryBuilder<'r, 'c> {
                                         .into_box(),
                                     })
                                 })
-                                .collect::<Result<Vec<_>, _>>()?;
+                                .collect::<Result<Vec<_>, QueryBuilderError>>()?;
 
                             let join_operator = join_exprs
                                 .into_iter()

diff --git a/crates/ndc-clickhouse/src/sql/query_builder/error.rs b/crates/ndc-clickhouse/src/sql/query_builder/error.rs
@@ -1,5 +1,9 @@
 use std::fmt;
 
+use ndc_sdk::connector::{ExplainError, QueryError};
+
+use super::typecasting::TypeStringError;
+
 #[derive(Debug)]
 pub enum QueryBuilderError {
     /// A relationship referenced in the query is missing from the collection_relationships map
@@ -27,7 +31,7 @@ pub enum QueryBuilderError {
     /// An error that should never happen, and indicates a bug if triggered
     Unexpected(String),
     /// There was an issue creating typecasting strings
-    Typecasting(String),
+    Typecasting(TypeStringError),
 }
 
 impl fmt::Display for QueryBuilderError {
@@ -69,3 +73,51 @@ impl fmt::Display for QueryBuilderError {
 }
 
 impl std::error::Error for QueryBuilderError {}
+
+impl From<QueryBuilderError> for QueryError {
+    fn from(value: QueryBuilderError) -> Self {
+        match value {
+            QueryBuilderError::MissingRelationship(_)
+            | QueryBuilderError::MissingNativeQueryArgument { .. }
+            | QueryBuilderError::UnknownTable(_)
+            | QueryBuilderError::UnknownTableArgument { .. }
+            | QueryBuilderError::UnknownQueryArgument { .. }
+            | QueryBuilderError::UnknownTableType(_)
+            | QueryBuilderError::UnknownColumn(_, _)
+            | QueryBuilderError::CannotSerializeVariables(_)
+            | QueryBuilderError::UnknownSingleColumnAggregateFunction(_)
+            | QueryBuilderError::UnknownBinaryComparisonOperator(_)
+            | QueryBuilderError::Typecasting(_) => {
+                QueryError::UnprocessableContent(value.to_string())
+            }
+            QueryBuilderError::NotSupported(_) => {
+                QueryError::UnsupportedOperation(value.to_string())
+            }
+            QueryBuilderError::Unexpected(_) => QueryError::Other(Box::new(value)),
+        }
+    }
+}
+
+impl From<QueryBuilderError> for ExplainError {
+    fn from(value: QueryBuilderError) -> Self {
+        match value {
+            QueryBuilderError::MissingRelationship(_)
+            | QueryBuilderError::MissingNativeQueryArgument { .. }
+            | QueryBuilderError::UnknownTable(_)
+            | QueryBuilderError::UnknownTableArgument { .. }
+            | QueryBuilderError::UnknownQueryArgument { .. }
+            | QueryBuilderError::UnknownTableType(_)
+            | QueryBuilderError::UnknownColumn(_, _)
+            | QueryBuilderError::CannotSerializeVariables(_)
+            | QueryBuilderError::UnknownSingleColumnAggregateFunction(_)
+            | QueryBuilderError::UnknownBinaryComparisonOperator(_)
+            | QueryBuilderError::Typecasting(_) => {
+                ExplainError::UnprocessableContent(value.to_string())
+            }
+            QueryBuilderError::NotSupported(_) => {
+                ExplainError::UnsupportedOperation(value.to_string())
+            }
+            QueryBuilderError::Unexpected(_) => ExplainError::Other(Box::new(value)),
+        }
+    }
+}
diff --git a/crates/ndc-clickhouse/src/sql/query_builder/typecasting.rs b/crates/ndc-clickhouse/src/sql/query_builder/typecasting.rs
@@ -6,6 +6,8 @@ use ndc_sdk::models;
 
 use crate::schema::{ClickHouseSingleColumnAggregateFunction, ClickHouseTypeDefinition};
 
+use super::QueryBuilderError;
+
 /// Tuple(rows <RowsCastString>, aggregates <RowsCastString>)
 pub struct RowsetTypeString {
     rows: Option<RowsTypeString>,
@@ -314,3 +316,9 @@ impl Display for TypeStringError {
 }
 
 impl std::error::Error for TypeStringError {}
+
+impl From<TypeStringError> for QueryBuilderError {
+    fn from(value: TypeStringError) -> Self {
+        QueryBuilderError::Typecasting(value)
+    }
+}