Support xml error respsonse (Azure#1631)

merlinio2000 · maggimer · heaths · web-flow · commit d5b8a4c2a1c3 · 2024-04-19T07:58:27.000+02:00
* core: make HttpError use content-type header to deserialize body * core: respect Content-Type in ErrorKind::http_response_from_body * core: correctly de-serialize XML errors with different case-ing * cleanup if statements * core: only parse xml error bodies if 'xml' feature is present * core: refactor ErrorKind::HttpResponse to make use of headers * Update sdk/core/src/error/mod.rs Co-authored-by: Heath Stewart <heaths@outlook.com> * Update sdk/core/src/error/http_error.rs Co-authored-by: Heath Stewart <heaths@outlook.com> * Update sdk/core/src/error/mod.rs Co-authored-by: Heath Stewart <heaths@outlook.com> * Revert "Update sdk/core/src/error/http_error.rs" This reverts commit a2f3b4b. --------- Co-authored-by: Merlin Maggi <merlin.maggi@jls.ch> Co-authored-by: Heath Stewart <heaths@outlook.com>
diff --git a/sdk/core/src/error/http_error.rs b/sdk/core/src/error/http_error.rs
@@ -1,14 +1,17 @@
-use crate::{from_json, headers, Response, StatusCode};
+use crate::{
+    content_type, from_json,
+    headers::{self, Headers},
+    Response, StatusCode,
+};
 use bytes::Bytes;
 use serde::Deserialize;
-use std::collections::HashMap;
 
 /// An unsuccessful HTTP response
 #[derive(Debug)]
 pub struct HttpError {
     status: StatusCode,
     details: ErrorDetails,
-    headers: std::collections::HashMap<String, String>,
+    headers: Headers,
     body: Bytes,
 }
 
@@ -17,14 +20,8 @@ impl HttpError {
     ///
     /// This does not check whether the response was a success and should only be used with unsuccessful responses.
     pub async fn new(response: Response) -> Self {
-        let status = response.status();
-        let headers: HashMap<String, String> = response
-            .headers()
-            .iter()
-            .map(|(name, value)| (name.as_str().to_owned(), value.as_str().to_owned()))
-            .collect();
-        let body = response
-            .into_body()
+        let (status, headers, body) = response.deconstruct();
+        let body = body
             .collect()
             .await
             .unwrap_or_else(|_| Bytes::from_static(b"<ERROR COLLECTING BODY>"));
@@ -71,11 +68,15 @@ impl std::fmt::Display for HttpError {
         write!(f, "{tab}Body: \"{:?}\",{newline}", self.body)?;
         write!(f, "{tab}Headers: [{newline}")?;
         // TODO: sanitize headers
-        for (k, v) in &self.headers {
-            write!(f, "{tab}{tab}{k}:{v}{newline}")?;
+        for (k, v) in self.headers.iter() {
+            write!(
+                f,
+                "{tab}{tab}{k}:{v}{newline}",
+                k = k.as_str(),
+                v = v.as_str()
+            )?;
         }
         write!(f, "{tab}],{newline}}}{newline}")?;
-
         Ok(())
     }
 }
@@ -89,46 +90,87 @@ struct ErrorDetails {
 }
 
 impl ErrorDetails {
-    fn new(headers: &HashMap<String, String>, body: &[u8]) -> Self {
-        let mut code = get_error_code_from_header(headers);
-        code = code.or_else(|| get_error_code_from_body(body));
-        let message = get_error_message_from_body(body);
-        Self { code, message }
+    fn new(headers: &Headers, body: &[u8]) -> Self {
+        let header_err_code = get_error_code_from_header(headers);
+        let content_type = headers.get_optional_str(&headers::CONTENT_TYPE);
+        let (body_err_code, body_err_message) =
+            get_error_code_message_from_body(body, content_type);
+
+        let code = header_err_code.or(body_err_code);
+        Self {
+            code,
+            message: body_err_message,
+        }
     }
 }
 
 /// Gets the error code if it's present in the headers
 ///
 /// For more info, see [here](https://github.com/microsoft/api-guidelines/blob/vNext/azure/Guidelines.md#handling-errors)
-fn get_error_code_from_header(headers: &HashMap<String, String>) -> Option<String> {
-    headers.get(headers::ERROR_CODE.as_str()).cloned()
+pub(crate) fn get_error_code_from_header(headers: &Headers) -> Option<String> {
+    headers.get_optional_string(&headers::ERROR_CODE)
 }
 
 #[derive(Deserialize)]
 struct NestedError {
+    #[serde(alias = "Message")]
     message: Option<String>,
+    #[serde(alias = "Code")]
     code: Option<String>,
 }
 
+/// Error from a response body, aliases are set because XML responses follow different case-ing
 #[derive(Deserialize)]
 struct ErrorBody {
+    #[serde(alias = "Error")]
     error: Option<NestedError>,
+    #[serde(alias = "Message")]
     message: Option<String>,
+    #[serde(alias = "Code")]
     code: Option<String>,
 }
 
-/// Gets the error code if it's present in the body
-///
-/// For more info, see [here](https://github.com/microsoft/api-guidelines/blob/vNext/azure/Guidelines.md#handling-errors)
-pub(crate) fn get_error_code_from_body(body: &[u8]) -> Option<String> {
-    let decoded: ErrorBody = from_json(body).ok()?;
-    decoded.error.and_then(|e| e.code).or(decoded.code)
+impl ErrorBody {
+    /// Deconstructs self into error (code, message)
+    ///
+    /// The nested errors fields take precedence over those in the root of the structure
+    fn into_code_message(self) -> (Option<String>, Option<String>) {
+        let (nested_code, nested_message) = self
+            .error
+            .map(|nested_error| (nested_error.code, nested_error.message))
+            .unwrap_or((None, None));
+        (nested_code.or(self.code), nested_message.or(self.message))
+    }
 }
 
-/// Gets the error message if it's present in the body
+/// Gets the error code and message from the body based on the specified content_type
+/// Support for xml decoding is dependent on the 'xml' feature flag
 ///
+/// Assumes JSON if unspecified/inconclusive to maintain old behaviour
+/// [#1275](https://github.com/Azure/azure-sdk-for-rust/issues/1275)
 /// For more info, see [here](https://github.com/microsoft/api-guidelines/blob/vNext/azure/Guidelines.md#handling-errors)
-pub(crate) fn get_error_message_from_body(body: &[u8]) -> Option<String> {
-    let decoded: ErrorBody = from_json(body).ok()?;
-    decoded.error.and_then(|e| e.message).or(decoded.message)
+pub(crate) fn get_error_code_message_from_body(
+    body: &[u8],
+    content_type: Option<&str>,
+) -> (Option<String>, Option<String>) {
+    let err_body: Option<ErrorBody> = if content_type
+        .is_some_and(|ctype| ctype == content_type::APPLICATION_XML.as_str())
+    {
+        #[cfg(feature = "xml")]
+        {
+            crate::xml::read_xml(body).ok()
+        }
+        #[cfg(not(feature = "xml"))]
+        {
+            tracing::warn!("encountered XML response but the 'xml' feature flag was not specified");
+            None
+        }
+    } else {
+        // keep old default of assuming JSON
+        from_json(body).ok()
+    };
+
+    err_body
+        .map(ErrorBody::into_code_message)
+        .unwrap_or((None, None))
 }
diff --git a/sdk/core/src/error/mod.rs b/sdk/core/src/error/mod.rs
@@ -3,8 +3,11 @@ use std::borrow::Cow;
 use std::fmt::{Debug, Display};
 mod http_error;
 mod macros;
+use crate::headers::{self, Headers};
 pub use http_error::HttpError;
 
+use self::http_error::get_error_code_from_header;
+
 /// A convenience alias for `Result` where the error type is hard coded to `Error`
 pub type Result<T> = std::result::Result<T, Error>;
 
@@ -41,9 +44,21 @@ impl ErrorKind {
         Self::HttpResponse { status, error_code }
     }
 
-    pub fn http_response_from_body(status: StatusCode, body: &[u8]) -> Self {
-        let error_code = http_error::get_error_code_from_body(body);
-        Self::HttpResponse { status, error_code }
+    /// Constructs an [`ErrorKind::HttpResponse`] with given status code. The error code is taken from
+    /// the header [`headers::ERROR_CODE`] if present; otherwise, it is taken from the body.
+    pub fn http_response_from_parts(status: StatusCode, headers: &Headers, body: &[u8]) -> Self {
+        if let Some(header_err_code) = get_error_code_from_header(headers) {
+            Self::HttpResponse {
+                status,
+                error_code: Some(header_err_code),
+            }
+        } else {
+            let (error_code, _) = http_error::get_error_code_message_from_body(
+                body,
+                headers.get_optional_str(&headers::CONTENT_TYPE),
+            );
+            Self::HttpResponse { status, error_code }
+        }
     }
 }
 
@@ -469,7 +484,8 @@ mod tests {
 
     #[test]
     fn matching_against_http_error() {
-        let kind = ErrorKind::http_response_from_body(StatusCode::ImATeapot, b"{}");
+        let kind =
+            ErrorKind::http_response_from_parts(StatusCode::ImATeapot, &Headers::new(), b"{}");
 
         assert!(matches!(
             kind,
@@ -479,8 +495,9 @@ mod tests {
             }
         ));
 
-        let kind = ErrorKind::http_response_from_body(
+        let kind = ErrorKind::http_response_from_parts(
             StatusCode::ImATeapot,
+            &Headers::new(),
             br#"{"error": {"code":"teepot"}}"#,
         );
 
@@ -492,6 +509,19 @@ mod tests {
             }
             if error_code.as_deref() == Some("teepot")
         ));
+
+        let mut headers = Headers::new();
+        headers.insert(headers::ERROR_CODE, "teapot");
+        let kind = ErrorKind::http_response_from_parts(StatusCode::ImATeapot, &headers, br#"{}"#);
+
+        assert!(matches!(
+            kind,
+            ErrorKind::HttpResponse {
+                status: StatusCode::ImATeapot,
+                error_code
+            }
+            if error_code.as_deref() == Some("teapot")
+        ));
     }
 
     #[test]
diff --git a/sdk/core/src/http_client/mod.rs b/sdk/core/src/http_client/mod.rs
@@ -49,7 +49,7 @@ pub trait HttpClient: Send + Sync + std::fmt::Debug {
         if status.is_success() {
             Ok(crate::CollectedResponse::new(status, headers, body))
         } else {
-            Err(ErrorKind::http_response_from_body(status, &body).into_error())
+            Err(ErrorKind::http_response_from_parts(status, &headers, &body).into_error())
         }
     }
 }
diff --git a/sdk/identity/src/client_credentials_flow/mod.rs b/sdk/identity/src/client_credentials_flow/mod.rs
@@ -76,10 +76,12 @@ pub async fn perform(
     );
     req.set_body(encoded);
     let rsp = http_client.execute_request(&req).await?;
-    let rsp_status = rsp.status();
-    let rsp_body = rsp.into_body().collect().await?;
+    let (rsp_status, rsp_headers, rsp_body) = rsp.deconstruct();
+    let rsp_body = rsp_body.collect().await?;
     if !rsp_status.is_success() {
-        return Err(ErrorKind::http_response_from_body(rsp_status, &rsp_body).into_error());
+        return Err(
+            ErrorKind::http_response_from_parts(rsp_status, &rsp_headers, &rsp_body).into_error(),
+        );
     }
     from_json(&rsp_body)
 }
diff --git a/sdk/identity/src/device_code_flow/mod.rs b/sdk/identity/src/device_code_flow/mod.rs
@@ -36,10 +36,12 @@ where
         .finish();
 
     let rsp = post_form(http_client.clone(), url, encoded).await?;
-    let rsp_status = rsp.status();
-    let rsp_body = rsp.into_body().collect().await?;
+    let (rsp_status, rsp_headers, rsp_body) = rsp.deconstruct();
+    let rsp_body = rsp_body.collect().await?;
     if !rsp_status.is_success() {
-        return Err(ErrorKind::http_response_from_body(rsp_status, &rsp_body).into_error());
+        return Err(
+            ErrorKind::http_response_from_parts(rsp_status, &rsp_headers, &rsp_body).into_error(),
+        );
     }
     let device_code_response: DeviceCodePhaseOneResponse = from_json(&rsp_body)?;
 
diff --git a/sdk/identity/src/federated_credentials_flow/mod.rs b/sdk/identity/src/federated_credentials_flow/mod.rs
@@ -83,9 +83,10 @@ pub async fn perform(
     if rsp_status.is_success() {
         rsp.json().await
     } else {
-        let rsp_body = rsp.into_body().collect().await?;
+        let (rsp_status, rsp_headers, rsp_body) = rsp.deconstruct();
+        let rsp_body = rsp_body.collect().await?;
         let text = std::str::from_utf8(&rsp_body)?;
         error!("rsp_body == {:?}", text);
-        Err(ErrorKind::http_response_from_body(rsp_status, &rsp_body).into_error())
+        Err(ErrorKind::http_response_from_parts(rsp_status, &rsp_headers, &rsp_body).into_error())
     }
 }
diff --git a/sdk/identity/src/refresh_token.rs b/sdk/identity/src/refresh_token.rs
@@ -50,9 +50,11 @@ pub async fn exchange(
     if rsp_status.is_success() {
         rsp.json().await.map_kind(ErrorKind::Credential)
     } else {
-        let rsp_body = rsp.into_body().collect().await?;
-        let token_error: RefreshTokenError = from_json(&rsp_body)
-            .map_err(|_| ErrorKind::http_response_from_body(rsp_status, &rsp_body))?;
+        let (rsp_status, rsp_headers, rsp_body) = rsp.deconstruct();
+        let rsp_body = rsp_body.collect().await?;
+        let token_error: RefreshTokenError = from_json(&rsp_body).map_err(|_| {
+            ErrorKind::http_response_from_parts(rsp_status, &rsp_headers, &rsp_body)
+        })?;
         Err(Error::new(ErrorKind::Credential, token_error))
     }
 }
diff --git a/sdk/identity/src/token_credentials/imds_managed_identity_credentials.rs b/sdk/identity/src/token_credentials/imds_managed_identity_credentials.rs
@@ -89,8 +89,9 @@ impl ImdsManagedIdentityCredential {
         };
 
         let rsp = self.http_client.execute_request(&req).await?;
-        let rsp_status = rsp.status();
-        let rsp_body = rsp.into_body().collect().await?;
+
+        let (rsp_status, rsp_headers, rsp_body) = rsp.deconstruct();
+        let rsp_body = rsp_body.collect().await?;
 
         if !rsp_status.is_success() {
             match rsp_status {
@@ -107,9 +108,12 @@ impl ImdsManagedIdentityCredential {
                     ))
                 }
                 rsp_status => {
-                    return Err(
-                        ErrorKind::http_response_from_body(rsp_status, &rsp_body).into_error()
+                    return Err(ErrorKind::http_response_from_parts(
+                        rsp_status,
+                        &rsp_headers,
+                        &rsp_body,
                     )
+                    .into_error())
                 }
             }
         }

Original file line number	Diff line number	Diff line change
`@@ -49,7 +49,7 @@ pub trait HttpClient: Send + Sync + std::fmt::Debug {`
`49`	`49`	`if status.is_success() {`
`50`	`50`	`Ok(crate::CollectedResponse::new(status, headers, body))`
`51`	`51`	`} else {`
`52`		`- Err(ErrorKind::http_response_from_body(status, &body).into_error())`
	`52`	`+ Err(ErrorKind::http_response_from_parts(status, &headers, &body).into_error())`
`53`	`53`	`}`
`54`	`54`	`}`
`55`	`55`	`}`
Original file line number	Diff line number	Diff line change
`@@ -83,9 +83,10 @@ pub async fn perform(`
`83`	`83`	`if rsp_status.is_success() {`
`84`	`84`	`rsp.json().await`
`85`	`85`	`} else {`
`86`		`- let rsp_body = rsp.into_body().collect().await?;`
	`86`	`+ let (rsp_status, rsp_headers, rsp_body) = rsp.deconstruct();`
	`87`	`+ let rsp_body = rsp_body.collect().await?;`
`87`	`88`	`let text = std::str::from_utf8(&rsp_body)?;`
`88`	`89`	`error!("rsp_body == {:?}", text);`
`89`		`- Err(ErrorKind::http_response_from_body(rsp_status, &rsp_body).into_error())`
	`90`	`+ Err(ErrorKind::http_response_from_parts(rsp_status, &rsp_headers, &rsp_body).into_error())`
`90`	`91`	`}`
`91`	`92`	`}`
Original file line number	Diff line number	Diff line change
`@@ -89,8 +89,9 @@ impl ImdsManagedIdentityCredential {`
`89`	`89`	`};`
`90`	`90`
`91`	`91`	`let rsp = self.http_client.execute_request(&req).await?;`
`92`		`- let rsp_status = rsp.status();`
`93`		`- let rsp_body = rsp.into_body().collect().await?;`
	`92`	`+`
	`93`	`+ let (rsp_status, rsp_headers, rsp_body) = rsp.deconstruct();`
	`94`	`+ let rsp_body = rsp_body.collect().await?;`
`94`	`95`
`95`	`96`	`if !rsp_status.is_success() {`
`96`	`97`	`match rsp_status {`
`@@ -107,9 +108,12 @@ impl ImdsManagedIdentityCredential {`
`107`	`108`	`))`
`108`	`109`	`}`
`109`	`110`	`rsp_status => {`
`110`		`- return Err(`
`111`		`- ErrorKind::http_response_from_body(rsp_status, &rsp_body).into_error()`
	`111`	`+ return Err(ErrorKind::http_response_from_parts(`
	`112`	`+ rsp_status,`
	`113`	`+ &rsp_headers,`
	`114`	`+ &rsp_body,`
`112`	`115`	`)`
	`116`	`+ .into_error())`
`113`	`117`	`}`
`114`	`118`	`}`
`115`	`119`	`}`