Merge pull request #547 from input-output-hk/djo/better-error-log-message-4-http-server

Better error logs for /register-signer bad request

Author: Alenar

mithril-aggregator/src/http_server/routes/reply.rs

@@ -1,4 +1,4 @@
-use mithril_common::entities::InternalServerError;
+use mithril_common::entities::{ClientError, InternalServerError};
 use serde::Serialize;
 use warp::http::StatusCode;
 
@@ -16,6 +16,10 @@ pub fn empty(status_code: StatusCode) -> Box<dyn warp::Reply> {
     Box::new(warp::reply::with_status(warp::reply::reply(), status_code))
 }
 
+pub fn bad_request(label: String, message: String) -> Box<dyn warp::Reply> {
+    json(&ClientError::new(label, message), StatusCode::BAD_REQUEST)
+}
+
 pub fn internal_server_error(message: String) -> Box<dyn warp::Reply> {
     json(
         &InternalServerError::new(message),

mithril-aggregator/src/http_server/routes/signer_routes.rs

@@ -45,11 +45,17 @@ mod handlers {
             }
             Err(ProtocolError::Codec(err)) => {
                 warn!("register_signer::failed_signer_decoding"; "error" => ?err);
-                Ok(reply::empty(StatusCode::BAD_REQUEST))
+                Ok(reply::bad_request(
+                    "failed_signer_decoding".to_string(),
+                    err,
+                ))
             }
             Err(ProtocolError::FailedSignerRegistration(err)) => {
                 warn!("register_signer::failed_signer_registration"; "error" => ?err);
-                Ok(reply::empty(StatusCode::BAD_REQUEST))
+                Ok(reply::bad_request(
+                    "failed_signer_registration".to_string(),
+                    err.to_string(),
+                ))
             }
             Err(err) => {
                 warn!("register_signer::error"; "error" => ?err);

mithril-common/src/apispec.rs

@@ -118,7 +118,7 @@ impl<'a> APISpec<'a> {
         match schema {
             Null => match value {
                 Null => Ok(self),
-                _ => Err("null schema provided".to_string()),
+                _ => Err(format!("Expected nothing but got: {:?}", value)),
             },
             _ => {
                 let schema = &mut schema.as_object_mut().unwrap().clone();

mithril-common/src/entities/internal_server_error.rs renamed to mithril-common/src/entities/http_server_error.rs

@@ -13,3 +13,20 @@ impl InternalServerError {
         InternalServerError { message }
     }
 }
+
+/// Representation of a Client Error raised by an http server
+#[derive(Clone, Debug, PartialEq, Eq, Default, Serialize, Deserialize)]
+pub struct ClientError {
+    /// error label
+    pub label: String,
+
+    /// error message
+    pub message: String,
+}
+
+impl ClientError {
+    /// ClientError factory
+    pub fn new(label: String, message: String) -> ClientError {
+        ClientError { label, message }
+    }
+}

mithril-common/src/entities/mod.rs

@@ -7,7 +7,7 @@ mod certificate_metadata;
 mod certificate_pending;
 mod epoch;
 mod epoch_settings;
-mod internal_server_error;
+mod http_server_error;
 mod protocol_message;
 mod protocol_parameters;
 mod signer;
@@ -22,7 +22,7 @@ pub use certificate_metadata::CertificateMetadata;
 pub use certificate_pending::CertificatePending;
 pub use epoch::{Epoch, EpochError};
 pub use epoch_settings::EpochSettings;
-pub use internal_server_error::InternalServerError;
+pub use http_server_error::{ClientError, InternalServerError};
 pub use protocol_message::{ProtocolMessage, ProtocolMessagePartKey, ProtocolMessagePartValue};
 pub use protocol_parameters::ProtocolParameters;
 pub use signer::{Signer, SignerWithStake};

mithril-signer/src/certificate_handler.rs

@@ -132,7 +132,7 @@ impl CertificateHandler for CertificateHandlerHTTPClient {
             Ok(response) => match response.status() {
                 StatusCode::CREATED => Ok(()),
                 StatusCode::BAD_REQUEST => Err(CertificateHandlerError::RemoteServerLogical(
-                    "bad request".to_string(),
+                    format!("bad request: {}", response.text().await.unwrap_or_default()),
                 )),
                 _ => Err(CertificateHandlerError::RemoteServerTechnical(
                     response.text().await.unwrap_or_default(),
@@ -156,7 +156,7 @@ impl CertificateHandler for CertificateHandlerHTTPClient {
             Ok(response) => match response.status() {
                 StatusCode::CREATED => Ok(()),
                 StatusCode::BAD_REQUEST => Err(CertificateHandlerError::RemoteServerLogical(
-                    "bad request".to_string(),
+                    format!("bad request: {}", response.text().await.unwrap_or_default()),
                 )),
                 StatusCode::CONFLICT => Err(CertificateHandlerError::RemoteServerLogical(
                     "already registered single signatures".to_string(),
@@ -262,6 +262,7 @@ impl CertificateHandler for DumbCertificateHandler {
 mod tests {
     use super::*;
     use httpmock::prelude::*;
+    use mithril_common::entities::ClientError;
     use serde_json::json;
     use std::path::{Path, PathBuf};
 
@@ -386,12 +387,21 @@ mod tests {
         let (server, config) = setup_test();
         let _snapshots_mock = server.mock(|when, then| {
             when.method(POST).path("/register-signer");
-            then.status(400);
+            then.status(400).body(
+                serde_json::to_vec(&ClientError::new(
+                    "error".to_string(),
+                    "an error".to_string(),
+                ))
+                .unwrap(),
+            );
         });
         let certificate_handler = CertificateHandlerHTTPClient::new(config.aggregator_endpoint);
         let register_signer = certificate_handler.register_signer(single_signer).await;
         assert_eq!(
-            CertificateHandlerError::RemoteServerLogical("bad request".to_string()).to_string(),
+            CertificateHandlerError::RemoteServerLogical(
+                "bad request: {\"label\":\"error\",\"message\":\"an error\"}".to_string()
+            )
+            .to_string(),
             register_signer.unwrap_err().to_string()
         );
     }
@@ -435,14 +445,23 @@ mod tests {
         let (server, config) = setup_test();
         let _snapshots_mock = server.mock(|when, then| {
             when.method(POST).path("/register-signatures");
-            then.status(400);
+            then.status(400).body(
+                serde_json::to_vec(&ClientError::new(
+                    "error".to_string(),
+                    "an error".to_string(),
+                ))
+                .unwrap(),
+            );
         });
         let certificate_handler = CertificateHandlerHTTPClient::new(config.aggregator_endpoint);
         let register_signatures = certificate_handler
             .register_signatures(&single_signatures)
             .await;
         assert_eq!(
-            CertificateHandlerError::RemoteServerLogical("bad request".to_string()).to_string(),
+            CertificateHandlerError::RemoteServerLogical(
+                "bad request: {\"label\":\"error\",\"message\":\"an error\"}".to_string()
+            )
+            .to_string(),
             register_signatures.unwrap_err().to_string()
         );
     }

openapi.yaml

@@ -184,6 +184,10 @@ paths:
           description: signer registration succeeded
         "400":
           description: signer registration bad request
+          content:
+            application/json:
+              schema:
+                $ref: "#/components/schemas/Error"
         default:
           description: signer registration error
           content:
@@ -207,6 +211,10 @@ paths:
           description: signatures registration succeeded
         "400":
           description: signer registration bad request
+          content:
+            application/json:
+              schema:
+                $ref: "#/components/schemas/Error"
         "409":
           description: signatures registration already done
         default:
@@ -636,6 +644,9 @@ components:
       required:
         - message
       properties:
+        label:
+          description: optional label
+          type: string
         message:
           description: error message
           type: string