Make mithril-client bubble up aggregator error text

Before it would replace the whole text with "Unhandled error
{status_code}", loosing potential details on the error sent by the
aggregator.

Author: Alenar

mithril-client/src/aggregator_client.rs

@@ -7,19 +7,18 @@
 //!
 //! An implementation using HTTP is available: [AggregatorHTTPClient].
 
+use std::sync::Arc;
+
 use anyhow::{anyhow, Context};
 use async_recursion::async_recursion;
 use async_trait::async_trait;
 use reqwest::{Response, StatusCode, Url};
 use semver::Version;
 use slog::{debug, Logger};
-use std::sync::Arc;
 use thiserror::Error;
 use tokio::sync::RwLock;
 
-#[cfg(test)]
-use mockall::automock;
-
+use mithril_common::entities::{ClientError, ServerError};
 use mithril_common::MITHRIL_API_VERSION_HEADER;
 
 use crate::{MithrilError, MithrilResult};
@@ -28,11 +27,11 @@ use crate::{MithrilError, MithrilResult};
 #[derive(Error, Debug)]
 pub enum AggregatorClientError {
     /// Error raised when querying the aggregator returned a 5XX error.
-    #[error("remote server technical error")]
+    #[error("Internal error of the Aggregator")]
     RemoteServerTechnical(#[source] MithrilError),
 
     /// Error raised when querying the aggregator returned a 4XX error.
-    #[error("remote server logical error")]
+    #[error("Invalid request to the Aggregator")]
     RemoteServerLogical(#[source] MithrilError),
 
     /// Error raised when the server API version mismatch the client API version.
@@ -258,11 +257,12 @@ impl AggregatorHTTPClient {
                 Err(self.handle_api_error(&response).await)
             }
             StatusCode::NOT_FOUND => Err(AggregatorClientError::RemoteServerLogical(anyhow!(
-                "Url='{url} not found"
-            ))),
-            status_code => Err(AggregatorClientError::RemoteServerTechnical(anyhow!(
-                "Unhandled error {status_code}"
+                "Url='{url}' not found"
             ))),
+            status_code if status_code.is_client_error() => {
+                Err(Self::remote_logical_error(response).await)
+            }
+            _ => Err(Self::remote_technical_error(response).await),
         }
     }
 
@@ -303,9 +303,10 @@ impl AggregatorHTTPClient {
             StatusCode::NOT_FOUND => Err(AggregatorClientError::RemoteServerLogical(anyhow!(
                 "Url='{url} not found"
             ))),
-            status_code => Err(AggregatorClientError::RemoteServerTechnical(anyhow!(
-                "Unhandled error {status_code}"
-            ))),
+            status_code if status_code.is_client_error() => {
+                Err(Self::remote_logical_error(response).await)
+            }
+            _ => Err(Self::remote_technical_error(response).await),
         }
     }
 
@@ -336,9 +337,32 @@ impl AggregatorHTTPClient {
             })
             .map_err(AggregatorClientError::SubsystemError)
     }
+
+    async fn remote_logical_error(response: Response) -> AggregatorClientError {
+        let status_code = response.status();
+        let client_error = response
+            .json::<ClientError>()
+            .await
+            .unwrap_or(ClientError::new(
+                format!("Unhandled error {status_code}"),
+                "",
+            ));
+
+        AggregatorClientError::RemoteServerLogical(anyhow!("{client_error}"))
+    }
+
+    async fn remote_technical_error(response: Response) -> AggregatorClientError {
+        let status_code = response.status();
+        let server_error = response
+            .json::<ServerError>()
+            .await
+            .unwrap_or(ServerError::new(format!("Unhandled error {status_code}")));
+
+        AggregatorClientError::RemoteServerTechnical(anyhow!("{server_error}"))
+    }
 }
 
-#[cfg_attr(test, automock)]
+#[cfg_attr(test, mockall::automock)]
 #[cfg_attr(target_family = "wasm", async_trait(?Send))]
 #[cfg_attr(not(target_family = "wasm"), async_trait)]
 impl AggregatorClient for AggregatorHTTPClient {
@@ -377,8 +401,30 @@ impl AggregatorClient for AggregatorHTTPClient {
 
 #[cfg(test)]
 mod tests {
+    use httpmock::MockServer;
+
+    use mithril_common::api_version::APIVersionProvider;
+    use mithril_common::entities::{ClientError, ServerError};
+
     use super::*;
 
+    macro_rules! assert_error_eq {
+        ($left:expr, $right:expr) => {
+            assert_eq!(format!("{:?}", &$left), format!("{:?}", &$right),);
+        };
+    }
+
+    fn setup_server_and_client() -> (MockServer, AggregatorHTTPClient) {
+        let server = MockServer::start();
+        let client = AggregatorHTTPClient::new(
+            Url::parse(&server.url("")).unwrap(),
+            APIVersionProvider::compute_all_versions_sorted().unwrap(),
+            crate::test_utils::test_logger(),
+        )
+        .expect("building aggregator http client should not fail");
+        (server, client)
+    }
+
     #[test]
     fn always_append_trailing_slash_at_build() {
         for (expected, url) in [
@@ -482,4 +528,55 @@ mod tests {
             );
         }
     }
+
+    #[tokio::test]
+    async fn test_client_handle_4xx_errors() {
+        let client_error = ClientError::new("label", "message");
+
+        let (aggregator, client) = setup_server_and_client();
+        aggregator.mock(|_when, then| {
+            then.status(StatusCode::IM_A_TEAPOT.as_u16())
+                .json_body_obj(&client_error);
+        });
+
+        let expected_error = AggregatorClientError::RemoteServerLogical(anyhow!("{client_error}"));
+
+        let get_content_error = client
+            .get_content(AggregatorRequest::ListCertificates)
+            .await
+            .unwrap_err();
+        assert_error_eq!(get_content_error, expected_error);
+
+        let post_content_error = client
+            .post_content(AggregatorRequest::ListCertificates)
+            .await
+            .unwrap_err();
+        assert_error_eq!(post_content_error, expected_error);
+    }
+
+    #[tokio::test]
+    async fn test_client_handle_5xx_errors() {
+        let server_error = ServerError::new("message");
+
+        let (aggregator, client) = setup_server_and_client();
+        aggregator.mock(|_when, then| {
+            then.status(StatusCode::INTERNAL_SERVER_ERROR.as_u16())
+                .json_body_obj(&server_error);
+        });
+
+        let expected_error =
+            AggregatorClientError::RemoteServerTechnical(anyhow!("{server_error}"));
+
+        let get_content_error = client
+            .get_content(AggregatorRequest::ListCertificates)
+            .await
+            .unwrap_err();
+        assert_error_eq!(get_content_error, expected_error);
+
+        let post_content_error = client
+            .post_content(AggregatorRequest::ListCertificates)
+            .await
+            .unwrap_err();
+        assert_error_eq!(post_content_error, expected_error);
+    }
 }

mithril-common/src/entities/http_server_error.rs

@@ -1,6 +1,9 @@
-use crate::StdError;
+use std::fmt::Display;
+
 use serde::{Deserialize, Serialize};
 
+use crate::StdError;
+
 /// Representation of a Server Error raised by a http server
 #[derive(Clone, Debug, PartialEq, Eq, Default, Serialize, Deserialize)]
 pub struct ServerError {
@@ -10,8 +13,16 @@ pub struct ServerError {
 
 impl ServerError {
     /// InternalServerError factory
-    pub fn new(message: String) -> ServerError {
-        ServerError { message }
+    pub fn new<M: Into<String>>(message: M) -> ServerError {
+        ServerError {
+            message: message.into(),
+        }
+    }
+}
+
+impl Display for ServerError {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "{}", self.message)
     }
 }
 
@@ -52,3 +63,9 @@ impl ClientError {
         }
     }
 }
+
+impl Display for ClientError {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "{}: {}", self.label, self.message)
+    }
+}