Better error codes

crates/connectors/ndc-postgres/Cargo.toml

@@ -15,7 +15,7 @@ name = "ndc-postgres"
 path = "bin/main.rs"
 
 [dependencies]
-ndc-sdk = { git = "https://github.com/hasura/ndc-hub.git", rev = "ae09995" }
+ndc-sdk = { git = "https://github.com/hasura/ndc-hub.git", rev = "3b6c480" }
 query-engine-execution = { path = "../../query-engine/execution" }
 query-engine-metadata = { path = "../../query-engine/metadata" }
 query-engine-sql = { path = "../../query-engine/sql" }

crates/connectors/ndc-postgres/src/explain.rs

@@ -50,17 +50,22 @@ pub async fn explain<'a>(
                 // log error metric
                 match &err {
                     query_engine_execution::query::QueryError::ReservedVariableName(_) => {
-                        state.metrics.error_metrics.record_invalid_request()
+                        state.metrics.error_metrics.record_invalid_request();
+                        connector::ExplainError::InvalidRequest(err.to_string())
                     }
                     query_engine_execution::query::QueryError::VariableNotFound(_) => {
-                        state.metrics.error_metrics.record_invalid_request()
+                        state.metrics.error_metrics.record_invalid_request();
+                        connector::ExplainError::InvalidRequest(err.to_string())
                     }
                     query_engine_execution::query::QueryError::NotSupported(_) => {
-                        state.metrics.error_metrics.record_unsupported_feature()
+                        state.metrics.error_metrics.record_unsupported_feature();
+                        connector::ExplainError::UnsupportedOperation(err.to_string())
+                    }
+                    query_engine_execution::query::QueryError::DBError(_) => {
+                        state.metrics.error_metrics.record_invalid_request();
+                        connector::ExplainError::UnprocessableContent(err.to_string())
                     }
                 }
-
-                connector::ExplainError::Other(err.to_string().into())
             }
             query_engine_execution::query::Error::DB(err) => {
                 tracing::error!("{}", err);

crates/connectors/ndc-postgres/src/mutation.rs

@@ -109,24 +109,37 @@ async fn execute_mutation(
     .map(JsonResponse::Serialized)
     .map_err(|err| {
         tracing::error!("{}", err);
-        log_err_metrics(state, &err);
-        connector::MutationError::Other(err.to_string().into())
+        log_err_metrics_and_convert_error(state, &err)
     })
 }
 
-fn log_err_metrics(state: &state::State, err: &query_engine_execution::mutation::Error) {
+fn log_err_metrics_and_convert_error(
+    state: &state::State,
+    err: &query_engine_execution::mutation::Error,
+) -> connector::MutationError {
     match err {
         query_engine_execution::mutation::Error::Query(err) => match &err {
             query_engine_execution::mutation::QueryError::NotSupported(_) => {
-                state.metrics.error_metrics.record_unsupported_feature()
+                state.metrics.error_metrics.record_unsupported_feature();
+                connector::MutationError::UnsupportedOperation(err.to_string())
+            }
+            query_engine_execution::mutation::QueryError::DBError(_) => {
+                state.metrics.error_metrics.record_invalid_request();
+                connector::MutationError::UnprocessableContent(err.to_string())
+            }
+            query_engine_execution::mutation::QueryError::DBConstraintError(_) => {
+                state.metrics.error_metrics.record_invalid_request();
+                connector::MutationError::ConstraintNotMet(err.to_string())
             }
         },
         query_engine_execution::mutation::Error::DB(_) => {
             state.metrics.error_metrics.record_database_error();
+            connector::MutationError::Other(err.to_string().into())
         }
         query_engine_execution::mutation::Error::Multiple(err1, err2) => {
-            log_err_metrics(state, err1);
-            log_err_metrics(state, err2);
+            log_err_metrics_and_convert_error(state, err1);
+            log_err_metrics_and_convert_error(state, err2);
+            connector::MutationError::Other(err.to_string().into())
         }
     }
 }

crates/connectors/ndc-postgres/src/query.rs

@@ -94,16 +94,22 @@ async fn execute_query(
                 // log error metric
                 match &err {
                     query_engine_execution::query::QueryError::ReservedVariableName(_) => {
-                        state.metrics.error_metrics.record_invalid_request()
+                        state.metrics.error_metrics.record_invalid_request();
+                        connector::QueryError::InvalidRequest(err.to_string())
                     }
                     query_engine_execution::query::QueryError::VariableNotFound(_) => {
-                        state.metrics.error_metrics.record_invalid_request()
+                        state.metrics.error_metrics.record_invalid_request();
+                        connector::QueryError::InvalidRequest(err.to_string())
                     }
                     query_engine_execution::query::QueryError::NotSupported(_) => {
-                        state.metrics.error_metrics.record_unsupported_feature()
+                        state.metrics.error_metrics.record_unsupported_feature();
+                        connector::QueryError::UnsupportedOperation(err.to_string())
+                    }
+                    query_engine_execution::query::QueryError::DBError(_) => {
+                        state.metrics.error_metrics.record_invalid_request();
+                        connector::QueryError::UnprocessableContent(err.to_string())
                     }
                 }
-                connector::QueryError::Other(err.to_string().into())
             }
             query_engine_execution::query::Error::DB(err) => {
                 tracing::error!("{}", err);

crates/query-engine/execution/src/mutation.rs

@@ -192,6 +192,8 @@ pub enum Error {
 
 pub enum QueryError {
     NotSupported(String),
+    DBError(sqlx::Error),
+    DBConstraintError(sqlx::Error),
 }
 
 impl std::fmt::Display for QueryError {
@@ -200,6 +202,12 @@ impl std::fmt::Display for QueryError {
             QueryError::NotSupported(thing) => {
                 write!(f, "{} are not supported.", thing)
             }
+            QueryError::DBError(thing) => {
+                write!(f, "{}", thing)
+            }
+            QueryError::DBConstraintError(thing) => {
+                write!(f, "{}", thing)
+            }
         }
     }
 }
@@ -222,6 +230,23 @@ impl std::fmt::Display for Error {
 
 impl From<sqlx::Error> for Error {
     fn from(err: sqlx::Error) -> Error {
-        Error::DB(err)
+        match err
+            .as_database_error()
+            .and_then(|e| e.try_downcast_ref())
+            .map(|e: &sqlx::postgres::PgDatabaseError| e.code())
+        {
+            None => Error::DB(err),
+            Some(code) => {
+                // We want to map data and constraint exceptions to query errors
+                // https://www.postgresql.org/docs/current/errcodes-appendix.html
+                if code.starts_with("22") {
+                    Error::Query(QueryError::DBError(err))
+                } else if code.starts_with("23") {
+                    Error::Query(QueryError::DBConstraintError(err))
+                } else {
+                    Error::DB(err)
+                }
+            }
+        }
     }
 }

crates/query-engine/execution/src/query.rs

@@ -261,6 +261,7 @@ pub enum QueryError {
     ReservedVariableName(String),
     VariableNotFound(String),
     NotSupported(String),
+    DBError(sqlx::Error),
 }
 
 impl std::fmt::Display for QueryError {
@@ -279,12 +280,29 @@ impl std::fmt::Display for QueryError {
             QueryError::NotSupported(thing) => {
                 write!(f, "{} are not supported.", thing)
             }
+            QueryError::DBError(thing) => {
+                write!(f, "{}", thing)
+            }
         }
     }
 }
 
 impl From<sqlx::Error> for Error {
     fn from(err: sqlx::Error) -> Error {
-        Error::DB(err)
+        match err
+            .as_database_error()
+            .and_then(|e| e.try_downcast_ref())
+            .map(|e: &sqlx::postgres::PgDatabaseError| e.code())
+        {
+            None => Error::DB(err),
+            Some(code) => {
+                // We want to map data exceptions to query errors https://www.postgresql.org/docs/current/errcodes-appendix.html
+                if code.starts_with("22") {
+                    Error::Query(QueryError::DBError(err))
+                } else {
+                    Error::DB(err)
+                }
+            }
+        }
     }
 }

crates/query-engine/translation/Cargo.toml

@@ -5,7 +5,7 @@ edition = "2021"
 license = "Apache-2.0"
 
 [dependencies]
-ndc-sdk = { git = "https://github.com/hasura/ndc-hub.git", rev = "ae09995" }
+ndc-sdk = { git = "https://github.com/hasura/ndc-hub.git", rev = "3b6c480" }
 query-engine-metadata = { path = "../metadata" }
 query-engine-sql = { path = "../sql" }
 

crates/tests/databases-tests/src/postgres/mutation_tests.rs

@@ -71,7 +71,7 @@ mod basic {
 mod negative {
     use super::super::common;
     use tests_common::deployment::{clean_up_deployment, create_fresh_deployment};
-    use tests_common::request::{run_mutation500, run_query};
+    use tests_common::request::{run_mutation403, run_query};
 
     #[tokio::test]
     /// Check that the second statement fails on duplicate key constraint,
@@ -87,7 +87,7 @@ mod negative {
         let router =
             tests_common::router::create_router_from_deployment(&deployment.deployment_path).await;
 
-        let mutation_result = run_mutation500(router.clone(), "insert_artist_album_bad").await;
+        let mutation_result = run_mutation403(router.clone(), "insert_artist_album_bad").await;
 
         // expect no rows returned because first operation was rolled back.
         let selection_result = run_query(router, "mutations/select_specific_artist").await;

crates/tests/databases-tests/src/postgres/query_tests.rs

@@ -467,3 +467,15 @@ mod types {
         insta::assert_json_snapshot!(result);
     }
 }
+
+#[cfg(test)]
+mod negative {
+    use super::super::common::create_router;
+    use tests_common::request::run_query422;
+
+    #[tokio::test]
+    async fn select_by_pk() {
+        let result = run_query422(create_router().await, "select_by_pk_bad").await;
+        insta::assert_json_snapshot!(result);
+    }
+}

crates/tests/databases-tests/src/postgres/snapshots/databases_tests__postgres__mutation_tests__negative__insert_artist_album_bad.snap

@@ -4,9 +4,9 @@ expression: result
 ---
 [
   {
-    "message": "Internal error",
+    "message": "Constraint not met",
     "details": {
-      "cause": "error returned from database: duplicate key value violates unique constraint \"PK_Album\""
+      "detail": "error returned from database: duplicate key value violates unique constraint \"PK_Album\""
     }
   },
   [

crates/tests/databases-tests/src/postgres/snapshots/databases_tests__postgres__query_tests__negative__select_by_pk.snap

@@ -0,0 +1,10 @@
+---
+source: crates/tests/databases-tests/src/postgres/query_tests.rs
+expression: result
+---
+{
+  "message": "Unprocessable content",
+  "details": {
+    "detail": "error returned from database: invalid input syntax for type integer: \"\""
+  }
+}

crates/tests/tests-common/Cargo.toml

@@ -11,7 +11,7 @@ path = "src/lib.rs"
 [dependencies]
 ndc-client = { git = "https://github.com/hasura/ndc-spec.git", tag = "v0.1.0-rc.13" }
 ndc-postgres = { path = "../../connectors/ndc-postgres" }
-ndc-sdk = { git = "https://github.com/hasura/ndc-hub.git", rev = "ae09995" }
+ndc-sdk = { git = "https://github.com/hasura/ndc-hub.git", rev = "3b6c480" }
 ndc-test = { git = "https://github.com/hasura/ndc-spec.git", tag = "v0.1.0-rc.13" }
 
 axum = "0.6.20"