refactor: adjust outermost error message (#859)

* refactor: adjust outermost error message Signed-off-by: Ruihang Xia <[email protected]> * fix clippy Signed-off-by: Ruihang Xia <[email protected]> * preserve tonic status code Signed-off-by: Ruihang Xia <[email protected]> Signed-off-by: Ruihang Xia <[email protected]>
GreptimeTeam · Jan 10, 2023 · 32d5194 · 32d5194
1 parent 5fb417e
commit 32d5194
Show file tree

Hide file tree

Showing 9 changed files with 49 additions and 24 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/src/client/src/database.rs b/src/client/src/database.rs
@@ -12,6 +12,8 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+use std::str::FromStr;
+
 use api::v1::ddl_request::Expr as DdlExpr;
 use api::v1::greptime_request::Request;
 use api::v1::query_request::Query;
@@ -113,15 +115,17 @@ impl Database {
             .await
             .map_err(|e| {
                 let code = get_metadata_value(&e, INNER_ERROR_CODE)
-                    .and_then(|s| s.parse::<u32>().ok())
-                    .unwrap_or(e.code() as u32);
-                let err_msg = get_metadata_value(&e, INNER_ERROR_MSG).unwrap_or(e.to_string());
-                error::FlightGetSnafu {
-                    addr: client.addr(),
-                    code,
-                    err_msg,
-                }
-                .build()
+                    .and_then(|s| StatusCode::from_str(&s).ok())
+                    .unwrap_or(StatusCode::Unknown);
+                let msg = get_metadata_value(&e, INNER_ERROR_MSG).unwrap_or(e.to_string());
+                error::ExternalSnafu { code, msg }
+                    .fail::<()>()
+                    .map_err(BoxedError::new)
+                    .context(error::FlightGetSnafu {
+                        tonic_code: e.code(),
+                        addr: client.addr(),
+                    })
+                    .unwrap_err()
             })?;
 
         let decoder = &mut FlightDecoder::default();

diff --git a/src/client/src/error.rs b/src/client/src/error.rs
@@ -15,6 +15,7 @@
 use std::any::Any;
 
 use common_error::prelude::*;
+use tonic::Code;
 
 #[derive(Debug, Snafu)]
 #[snafu(visibility(pub))]
@@ -26,16 +27,15 @@ pub enum Error {
     },
 
     #[snafu(display(
-        "Failed to do Flight get, addr: {}, code: {}, err_msg: {}",
+        "Failed to do Flight get, addr: {}, code: {}, source: {}",
         addr,
-        code,
-        err_msg
+        tonic_code,
+        source
     ))]
     FlightGet {
         addr: String,
-        code: u32,
-        err_msg: String,
-        backtrace: Backtrace,
+        tonic_code: Code,
+        source: BoxedError,
     },
 
     #[snafu(display("Failed to convert FlightData, source: {}", source))]
@@ -69,6 +69,10 @@ pub enum Error {
         #[snafu(backtrace)]
         source: common_grpc::error::Error,
     },
+
+    /// Error deserialized from gRPC metadata
+    #[snafu(display("{}", msg))]
+    ExternalError { code: StatusCode, msg: String },
 }
 
 pub type Result<T> = std::result::Result<T, Error>;
@@ -77,13 +81,14 @@ impl ErrorExt for Error {
     fn status_code(&self) -> StatusCode {
         match self {
             Error::IllegalFlightMessages { .. }
-            | Error::FlightGet { .. }
             | Error::ColumnDataType { .. }
             | Error::MissingField { .. } => StatusCode::Internal,
+            Error::FlightGet { source, .. } => source.status_code(),
             Error::CreateChannel { source, .. } | Error::ConvertFlightData { source } => {
                 source.status_code()
             }
             Error::IllegalGrpcClientState { .. } => StatusCode::Unexpected,
+            Error::ExternalError { code, .. } => *code,
         }
     }
 

diff --git a/src/common/error/Cargo.toml b/src/common/error/Cargo.toml
@@ -5,4 +5,5 @@ edition.workspace = true
 license.workspace = true
 
 [dependencies]
+strum = "0.24.1"
 snafu = { version = "0.7", features = ["backtraces"] }
diff --git a/src/common/error/src/status_code.rs b/src/common/error/src/status_code.rs
@@ -14,8 +14,10 @@
 
 use std::fmt;
 
+use strum::EnumString;
+
 /// Common status code for public API.
-#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+#[derive(Debug, Clone, Copy, PartialEq, Eq, EnumString)]
 pub enum StatusCode {
     // ====== Begin of common status code ==============
     /// Success.

diff --git a/src/servers/src/error.rs b/src/servers/src/error.rs
@@ -347,11 +347,11 @@ impl From<Error> for tonic::Status {
 
         // If either of the status_code or error msg cannot convert to valid HTTP header value
         // (which is a very rare case), just ignore. Client will use Tonic status code and message.
-        if let Ok(code) = HeaderValue::from_bytes((err.status_code() as u32).to_string().as_bytes())
-        {
+        if let Ok(code) = HeaderValue::from_bytes(err.status_code().to_string().as_bytes()) {
             headers.insert(INNER_ERROR_CODE, code);
         }
-        if let Ok(err_msg) = HeaderValue::from_bytes(err.to_string().as_bytes()) {
+        let root_error = err.iter_chain().last().unwrap();
+        if let Ok(err_msg) = HeaderValue::from_bytes(root_error.to_string().as_bytes()) {
             headers.insert(INNER_ERROR_MSG, err_msg);
         }
 

diff --git a/tests/cases/standalone/select/dummy.result b/tests/cases/standalone/select/dummy.result
@@ -24,13 +24,13 @@ select 4 + 0.5;
 
 select "a";
 
-Failed to do Flight get, addr: 127.0.0.1:4001, code: 3000, err_msg: Failed to execute sql statement, source: Failed to execute sql, source: Failure during query planning, source: Cannot plan SQL: SELECT "a", source: Schema error: No field named 'a'. Valid fields are .
+Error: 3000(PlanQuery), Schema error: No field named 'a'. Valid fields are .
 
 select "A";
 
-Failed to do Flight get, addr: 127.0.0.1:4001, code: 3000, err_msg: Failed to execute sql statement, source: Failed to execute sql, source: Failure during query planning, source: Cannot plan SQL: SELECT "A", source: Schema error: No field named 'A'. Valid fields are .
+Error: 3000(PlanQuery), Schema error: No field named 'A'. Valid fields are .
 
 select * where "a" = "A";
 
-Failed to do Flight get, addr: 127.0.0.1:4001, code: 3000, err_msg: Failed to execute sql statement, source: Failed to execute sql, source: Failure during query planning, source: Cannot plan SQL: SELECT * WHERE "a" = "A", source: Schema error: No field named 'a'. Valid fields are .
+Error: 3000(PlanQuery), Schema error: No field named 'a'. Valid fields are .
 
diff --git a/tests/runner/Cargo.toml b/tests/runner/Cargo.toml
@@ -8,6 +8,7 @@ license.workspace = true
 async-trait = "0.1"
 client = { path = "../../src/client" }
 common-base = { path = "../../src/common/base" }
+common-error = { path = "../../src/common/error" }
 common-grpc = { path = "../../src/common/grpc" }
 common-query = { path = "../../src/common/query" }
 sqlness = "0.1"

diff --git a/tests/runner/src/env.rs b/tests/runner/src/env.rs
@@ -20,6 +20,8 @@ use std::time::Duration;
 
 use async_trait::async_trait;
 use client::{Client, Database as DB, Error as ClientError};
+use common_error::ext::ErrorExt;
+use common_error::snafu::ErrorCompat;
 use common_query::Output;
 use sqlness::{Database, EnvController};
 use tokio::process::{Child, Command};
@@ -140,7 +142,15 @@ impl Display for ResultDisplayer {
                 }
                 Output::Stream(_) => unreachable!(),
             },
-            Err(e) => write!(f, "{e}"),
+            Err(e) => {
+                let status_code = e.status_code();
+                let root_cause = e.iter_chain().last().unwrap();
+                write!(
+                    f,
+                    "Error: {}({status_code}), {root_cause}",
+                    status_code as u32
+                )
+            }
         }
     }
 }