Merge pull request #92 from mkuratczyk/parse-error-reason

Parse error/reason from failed API calls

Author: michaelklishin

src/api/client.rs

@@ -14,7 +14,10 @@
 #![allow(clippy::result_large_err)]
 
 use crate::commons::RetrySettings;
-use crate::error::Error::{ClientErrorResponse, NotFound, ServerErrorResponse};
+use crate::error::{
+    Error::{ClientErrorResponse, NotFound, ServerErrorResponse},
+    ErrorDetails,
+};
 use backtrace::Backtrace;
 use log::trace;
 use reqwest::{Client as HttpClient, StatusCode, header::HeaderMap};
@@ -636,9 +639,11 @@ where
                     // so we copy the key parts into the error first
                     let body = response.text().await?;
                     trace!("HTTP API response: {} from {}: {}", status, url, body);
+                    let error_details = ErrorDetails::from_json(&body);
                     return Err(ClientErrorResponse {
                         url: Some(url),
                         body: Some(body),
+                        error_details,
                         headers: Some(headers),
                         status_code: status,
                         backtrace: Backtrace::new(),
@@ -657,9 +662,11 @@ where
                     // so we copy the key parts into the error first
                     let body = response.text().await?;
                     trace!("HTTP API response: {} from {}: {}", status, url, body);
+                    let error_details = ErrorDetails::from_json(&body);
                     return Err(ServerErrorResponse {
                         url: Some(url),
                         body: Some(body),
+                        error_details,
                         headers: Some(headers),
                         status_code: status,
                         backtrace: Backtrace::new(),

src/blocking_api/client.rs

@@ -14,7 +14,10 @@
 #![allow(clippy::result_large_err)]
 
 use crate::commons::RetrySettings;
-use crate::error::Error::{ClientErrorResponse, NotFound, ServerErrorResponse};
+use crate::error::{
+    Error::{ClientErrorResponse, NotFound, ServerErrorResponse},
+    ErrorDetails,
+};
 use backtrace::Backtrace;
 use log::trace;
 use reqwest::{StatusCode, blocking::Client as HttpClient, header::HeaderMap};
@@ -620,9 +623,11 @@ where
                     // so we copy the key parts into the error first
                     let body = response.text()?;
                     trace!("Client error response: {} from {}: {}", status, url, body);
+                    let error_details = ErrorDetails::from_json(&body);
                     return Err(ClientErrorResponse {
                         url: Some(url),
                         body: Some(body),
+                        error_details,
                         headers: Some(headers),
                         status_code: status,
                         backtrace: Backtrace::new(),
@@ -641,9 +646,11 @@ where
                     // so we copy the key parts into the error first
                     let body = response.text()?;
                     trace!("Server error response: {} from {}: {}", status, url, body);
+                    let error_details = ErrorDetails::from_json(&body);
                     return Err(ServerErrorResponse {
                         url: Some(url),
                         body: Some(body),
+                        error_details,
                         headers: Some(headers),
                         status_code: status,
                         backtrace: Backtrace::new(),

src/error.rs

@@ -20,6 +20,7 @@ use reqwest::{
     StatusCode, Url,
     header::{HeaderMap, InvalidHeaderValue},
 };
+use serde::Deserialize;
 
 #[derive(Error, Debug)]
 pub enum ConversionError {
@@ -31,13 +32,34 @@ pub enum ConversionError {
     ParsingError { message: String },
 }
 
+/// The API returns JSON with "error" and "reason" fields in error responses.
+#[derive(Debug, Clone, PartialEq, Eq, Deserialize)]
+pub struct ErrorDetails {
+    /// Generic error type, e.g., "bad_request"
+    pub error: Option<String>,
+    /// Detailed reason for the error
+    pub reason: Option<String>,
+}
+
+impl ErrorDetails {
+    pub fn from_json(body: &str) -> Option<Self> {
+        serde_json::from_str(body).ok()
+    }
+
+    /// `reason` (typically more detailed) over `error` (generic).
+    pub fn reason(&self) -> Option<&str> {
+        self.reason.as_deref().or(self.error.as_deref())
+    }
+}
+
 #[derive(Error, Debug)]
 pub enum Error<U, S, E, BT> {
     #[error("API responded with a client error: status code of {status_code}")]
     ClientErrorResponse {
         url: Option<U>,
         status_code: S,
         body: Option<String>,
+        error_details: Option<ErrorDetails>,
         headers: Option<HeaderMap>,
         backtrace: BT,
     },
@@ -46,6 +68,7 @@ pub enum Error<U, S, E, BT> {
         url: Option<U>,
         status_code: S,
         body: Option<String>,
+        error_details: Option<ErrorDetails>,
         headers: Option<HeaderMap>,
         backtrace: BT,
     },
@@ -96,6 +119,7 @@ impl From<reqwest::Error> for HttpClientError {
                         url: req_err.url().cloned(),
                         status_code,
                         body: None,
+                        error_details: None,
                         headers: None,
                         backtrace: Backtrace::new(),
                     };
@@ -106,6 +130,7 @@ impl From<reqwest::Error> for HttpClientError {
                         url: req_err.url().cloned(),
                         status_code,
                         body: None,
+                        error_details: None,
                         headers: None,
                         backtrace: Backtrace::new(),
                     };

tests/blocking_policy_tests.rs

@@ -276,3 +276,94 @@ fn test_an_operator_policy(rc: &Client<&str, &str, &str>, policy: &PolicyParams)
     let policies = rc.list_operator_policies().unwrap();
     assert!(!policies.iter().any(|p| p.name == policy.name));
 }
+
+#[test]
+fn test_blocking_policy_validation_error() {
+    let endpoint = endpoint();
+    let rc = Client::new(endpoint.as_str(), USERNAME, PASSWORD);
+
+    let vh_params = VirtualHostParams::named("test_policy_validation_error");
+    let _ = rc.delete_vhost(vh_params.name, false);
+    let result1 = rc.create_vhost(&vh_params);
+    assert!(result1.is_ok());
+
+    // Create a policy with an invalid/unknown property in the definition
+    let mut map = Map::<String, Value>::new();
+    map.insert("foo".to_owned(), json!("bar"));
+    map.insert("invalid-setting".to_owned(), json!(12345));
+    let invalid_definition = map.clone();
+
+    let invalid_policy = PolicyParams {
+        vhost: vh_params.name,
+        name: "invalid_policy",
+        pattern: "^qq$",
+        apply_to: PolicyTarget::Queues,
+        priority: 1,
+        definition: invalid_definition,
+    };
+
+    // Attempting to declare this policy should fail with a validation error
+    let result = rc.declare_policy(&invalid_policy);
+    assert!(
+        result.is_err(),
+        "Expected policy declaration to fail with validation error"
+    );
+
+    // Extract the error and verify it contains structured error details
+    if let Err(err) = result {
+        match err {
+            rabbitmq_http_client::error::Error::ClientErrorResponse {
+                status_code,
+                error_details,
+                ..
+            } => {
+                // Should be a 400 Bad Request
+                assert_eq!(status_code, reqwest::StatusCode::BAD_REQUEST);
+
+                // Should have parsed error details
+                assert!(
+                    error_details.is_some(),
+                    "Expected error_details to be parsed"
+                );
+
+                let details = error_details.unwrap();
+
+                // Should have an error type
+                assert!(
+                    details.error.is_some(),
+                    "Expected error field to be present"
+                );
+                assert_eq!(details.error.as_deref(), Some("bad_request"));
+
+                // Should have a detailed reason
+                assert!(
+                    details.reason.is_some(),
+                    "Expected reason field to be present"
+                );
+                let reason = details.reason.as_ref().unwrap();
+
+                // The reason should mention validation failure
+                assert!(
+                    reason.contains("Validation failed"),
+                    "Expected reason to contain 'Validation failed', got: {}",
+                    reason
+                );
+
+                // The reason should mention unrecognized properties
+                assert!(
+                    reason.contains("not recognised") || reason.contains("not recognized"),
+                    "Expected reason to mention unrecognized properties, got: {}",
+                    reason
+                );
+
+                // Verify reason() returns the reason (more detailed message)
+                let detailed_msg = details.reason();
+                assert!(detailed_msg.is_some());
+                assert_eq!(detailed_msg, details.reason.as_deref());
+            }
+            _ => panic!("Expected ClientErrorResponse, got: {:?}", err),
+        }
+    }
+
+    let _ = rc.delete_vhost(vh_params.name, false);
+}