Always attach RawResponse to ErrorKind::HttpResponse (Azure#3039)

Author: heaths

sdk/core/azure_core/CHANGELOG.md

@@ -9,6 +9,9 @@
 ### Breaking Changes
 
 - Changed `ClientOptions::retry` from `Option<RetryOptions>` to `RetryOptions`.
+- Changed `RawResponse::json()` from `async` to synchronous function. The body was already buffered.
+- Changed `RawResponse::xml()` from `async` to synchronous function. The body was already buffered.
+- Removed `ErrorKind::http_response()`. Construct an `ErrorResponse::HttpResponse` variant instead.
 - Removed several unreferenced HTTP headers and accessor structures for those headers.
 - Renamed `TransportOptions` to `Transport`.
 - Renamed `TransportOptions::new_custom_policy()` to `Transport::with_policy()`.
@@ -24,6 +27,8 @@
 
 ### Bugs Fixed
 
+- `ErrorKind::HttpResponse { raw_response, .. }` may have been incorrectly `None`.
+
 ### Other Changes
 
 ## 0.28.0 (2025-09-11)

sdk/core/azure_core/src/error/error_response.rs

@@ -148,50 +148,65 @@ pub async fn check_success(
         return Ok(response);
     }
 
-    let response = response.try_into_raw_response().await?;
+    let raw_response = response.try_into_raw_response().await?;
 
     // If there's no body, we can't extract any more information.
-    if response.body().is_empty() {
-        let code = response.headers().get_optional_str(&ERROR_CODE);
-        let error_kind = ErrorKind::http_response(status, code.map(str::to_owned));
+    if raw_response.body().is_empty() {
+        let error_code = raw_response
+            .headers()
+            .get_optional_str(&ERROR_CODE)
+            .map(str::to_owned);
+        let error_kind = ErrorKind::HttpResponse {
+            status,
+            error_code,
+            raw_response: Some(Box::new(raw_response)),
+        };
         return Err(Error::with_message(error_kind, status.to_string()));
     }
     let internal_response =
-        serde_json::de::from_slice::<ErrorResponseInternal>(response.body()).map_err(Error::from);
+        serde_json::de::from_slice::<ErrorResponseInternal>(raw_response.body())
+            .map_err(Error::from);
 
     let internal_response = match internal_response {
         Ok(r) => r,
         Err(_) => {
             // If we can't parse the body, return a generic error with the status code and body
-            let code = response.headers().get_optional_str(&ERROR_CODE);
-            let error_kind = ErrorKind::http_response(
+            let error_code = raw_response
+                .headers()
+                .get_optional_str(&ERROR_CODE)
+                .map_or_else(|| raw_response.status().to_string(), str::to_owned);
+            let message = str::from_utf8(raw_response.body())
+                .unwrap_or("(invalid utf-8 in body)")
+                .to_string();
+            let error_kind = ErrorKind::HttpResponse {
                 status,
-                Some(code.map_or_else(|| response.status().to_string(), str::to_owned)),
-            );
+                error_code: Some(error_code),
+                raw_response: Some(Box::new(raw_response)),
+            };
             return Err(Error::with_message(
                 error_kind,
-                format!(
-                    "{}: {}",
-                    status,
-                    str::from_utf8(response.body()).unwrap_or("(invalid utf-8 in body)")
-                ),
+                format!("{}: {}", status, message),
             ));
         }
     };
 
     // We give priority to the error code in the header, and try the body version if it's not present.
-    let code = response.headers().get_optional_str(&ERROR_CODE);
-    let code = code.or(internal_response.error.code);
-
-    let error_kind = ErrorKind::http_response(status, code.map(str::to_owned));
+    let error_code = raw_response
+        .headers()
+        .get_optional_str(&ERROR_CODE)
+        .or(internal_response.error.code)
+        .map(str::to_owned);
+    let message = internal_response
+        .error
+        .message
+        .map_or_else(|| status.to_string(), str::to_owned);
+    let error_kind = ErrorKind::HttpResponse {
+        status,
+        error_code,
+        raw_response: Some(Box::new(raw_response)),
+    };
 
-    Err(Error::with_message(
-        error_kind,
-        internal_response
-            .error
-            .message
-            .map_or_else(|| status.to_string(), |m| m.to_owned()),
-    ))
+    Err(Error::with_message(error_kind, message))
 }
 
 #[cfg(test)]
@@ -217,7 +232,7 @@ mod tests {
             ErrorKind::HttpResponse {
                 status: StatusCode::ImATeapot,
                 error_code,
-                raw_response: None
+                raw_response: Some(_),
             }
             if error_code.as_deref() == Some("teapot")
         ));
@@ -242,7 +257,7 @@ mod tests {
             ErrorKind::HttpResponse {
                 status: StatusCode::ImATeapot,
                 error_code,
-                raw_response: None
+                raw_response: Some(_),
             }
             if error_code.as_deref() == Some("teapot")
         ));
@@ -292,7 +307,7 @@ mod tests {
             ErrorKind::HttpResponse {
                 status: StatusCode::Ok,
                 error_code,
-                raw_response: None
+                raw_response: Some(_),
             }
             if error_code.as_deref() == Some("teapot")
         ));
@@ -306,14 +321,15 @@ mod tests {
 
         let err = check_success(response, None).await.unwrap_err();
         let kind = err.kind();
-        assert_eq!(
-            *kind,
+        assert!(matches!(
+            kind,
             ErrorKind::HttpResponse {
                 status: StatusCode::ImATeapot,
-                error_code: Some("testError".to_string()),
-                raw_response: None
+                error_code,
+                raw_response: Some(_),
             }
-        );
+            if error_code.as_deref() == Some("testError")
+        ));
     }
 
     #[tokio::test]
@@ -327,16 +343,23 @@ mod tests {
         );
 
         let err = check_success(response, None).await.unwrap_err();
-        let kind = err.kind();
-        assert_eq!(
-            *kind,
-            ErrorKind::HttpResponse {
-                status: StatusCode::ImATeapot,
-                error_code: Some("testError".to_string()),
-                raw_response: None
-            }
-        );
+        let ErrorKind::HttpResponse {
+            status,
+            error_code: Some(error_code),
+            raw_response: Some(raw_response),
+        } = err.kind()
+        else {
+            panic!("expected ErrorKind::HttpResponse");
+        };
+
         assert!(err.to_string().contains(r#"{"json": "error"}"#));
+        assert_eq!(status, &StatusCode::ImATeapot);
+        assert_eq!(error_code, "testError");
+        assert_eq!(raw_response.status(), StatusCode::ImATeapot);
+        assert_eq!(raw_response.headers().iter().count(), 1);
+        assert!(
+            matches!(str::from_utf8(raw_response.body()), Ok(body) if body == r#"{"json": "error"}"#)
+        );
     }
 
     #[test]

sdk/core/azure_core/tests/core_error_http_response.rs

@@ -0,0 +1,119 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+use azure_core::{
+    error::{ErrorKind, ErrorResponse},
+    http::{headers::Headers, BufResponse, ClientOptions, HttpClient, StatusCode, Transport},
+    Bytes,
+};
+use azure_core_test::{credentials::MockCredential, http::MockHttpClient};
+use azure_security_keyvault_secrets::{SecretClient, SecretClientOptions};
+use futures::FutureExt as _;
+use std::{
+    str,
+    sync::{Arc, LazyLock},
+};
+
+#[tokio::test]
+async fn deconstruct_raw_response() -> Result<(), Box<dyn std::error::Error>> {
+    let options = SecretClientOptions {
+        client_options: ClientOptions {
+            transport: Some(Transport::new(TRANSPORT.clone())),
+            ..Default::default()
+        },
+        ..Default::default()
+    };
+    let client = SecretClient::new(
+        "https://my-vault.vault.azure.net",
+        MockCredential::new()?,
+        Some(options),
+    )?;
+
+    let err = client.get_secret("secret-name", None).await.unwrap_err();
+
+    // Deconstruct the HttpResponse.
+    let ErrorKind::HttpResponse {
+        status,
+        error_code,
+        raw_response,
+    } = err.kind()
+    else {
+        panic!("expected ErrorKind::HttpResponse");
+    };
+
+    assert_eq!(status, &StatusCode::BadRequest);
+    assert_eq!(error_code.as_deref(), Some("BadParameter"));
+    assert!(
+        matches!(raw_response, Some(r) if str::from_utf8(r.body())?.contains("Unknown parameter"))
+    );
+
+    Ok(())
+}
+
+#[tokio::test]
+async fn deserialize_error_response() -> Result<(), Box<dyn std::error::Error>> {
+    let options = SecretClientOptions {
+        client_options: ClientOptions {
+            transport: Some(Transport::new(TRANSPORT.clone())),
+            ..Default::default()
+        },
+        ..Default::default()
+    };
+    let client = SecretClient::new(
+        "https://my-vault.vault.azure.net",
+        MockCredential::new()?,
+        Some(options),
+    )?;
+
+    let err = client.get_secret("secret-name", None).await.unwrap_err();
+
+    // Deconstruct the HttpResponse
+    let ErrorKind::HttpResponse {
+        status,
+        error_code,
+        raw_response: Some(raw_response),
+    } = err.kind()
+    else {
+        panic!("expected ErrorKind::HttpResponse");
+    };
+
+    // Deserialize the RawResponse
+    let error_response: ErrorResponse = raw_response.clone().json()?;
+
+    assert_eq!(status, &StatusCode::BadRequest);
+    assert_eq!(error_code.as_deref(), Some("BadParameter"));
+    let Some(error) = error_response.error else {
+        panic!("expected error");
+    };
+    assert_eq!(error.details.len(), 1);
+    assert_eq!(error.details[0].code.as_deref(), Some("BadParameter"));
+    assert_eq!(error.details[0].target.as_deref(), Some("foo"));
+
+    Ok(())
+}
+
+static TRANSPORT: LazyLock<Arc<dyn HttpClient>> = LazyLock::new(|| {
+    Arc::new(MockHttpClient::new(|_| {
+        async {
+            Ok(BufResponse::from_bytes(
+                StatusCode::BadRequest,
+                Headers::new(),
+                Bytes::from_static(
+                    br#"{
+                    "error": {
+                        "code": "BadParameter",
+                        "message": "Unknown parameter",
+                        "details": [
+                            {
+                                "code": "BadParameter",
+                                "target": "foo"
+                            }
+                        ]
+                    }
+                }"#,
+                ),
+            ))
+        }
+        .boxed()
+    }))
+});

sdk/identity/azure_identity/src/azure_pipelines_credential.rs

@@ -157,7 +157,7 @@ impl ClientAssertion for Client {
 
             return Err(
                 azure_core::Error::with_message(
-                    ErrorKind::http_response(status_code, Some(status_code.canonical_reason().to_string())),
+                    ErrorKind::HttpResponse { status: status_code, error_code: Some(status_code.canonical_reason().to_string()), raw_response: None },
                      format!("{status_code} response from the OIDC endpoint. Check service connection ID and pipeline configuration. {err_headers}"),
                 )
             );

sdk/typespec/CHANGELOG.md

@@ -8,6 +8,9 @@
 
 ### Breaking Changes
 
+- Changed `RawResponse::json()` from `async` to synchronous function. The body was already buffered.
+- Changed `RawResponse::xml()` from `async` to synchronous function. The body was already buffered.
+- Removed `ErrorKind::http_response()`. Construct an `ErrorResponse::HttpResponse` variant instead.
 - Renamed a number of construction functions for `Error` to align with [guidelines](https://azure.github.io/azure-sdk/rust_introduction.html)"
   - Renamed `Error::full()` to `Error::with_error()`.
   - Renamed `Error::with_message()` to `Error::with_message_fn()`.

sdk/typespec/src/error/mod.rs

@@ -48,16 +48,6 @@ impl ErrorKind {
             context: Repr::Simple(self),
         }
     }
-
-    /// Create an `ErrorKind` from an HTTP response.
-    #[cfg(feature = "http")]
-    pub fn http_response(status: StatusCode, error_code: Option<String>) -> Self {
-        Self::HttpResponse {
-            status,
-            error_code,
-            raw_response: None,
-        }
-    }
 }
 
 impl Display for ErrorKind {

sdk/typespec/src/http/response.rs

@@ -50,7 +50,7 @@ impl RawResponse {
 
     /// Deserialize the JSON stream into type `T`.
     #[cfg(feature = "json")]
-    pub async fn json<T>(self) -> crate::Result<T>
+    pub fn json<T>(self) -> crate::Result<T>
     where
         T: DeserializeOwned,
     {
@@ -59,7 +59,7 @@ impl RawResponse {
 
     /// Deserialize the XML stream into type `T`.
     #[cfg(feature = "xml")]
-    pub async fn xml<T>(self) -> crate::Result<T>
+    pub fn xml<T>(self) -> crate::Result<T>
     where
         T: DeserializeOwned,
     {

sdk/typespec/typespec_client_core/CHANGELOG.md

@@ -7,6 +7,9 @@
 ### Breaking Changes
 
 - Changed `ClientOptions::retry` from `Option<RetryOptions>` to `RetryOptions`.
+- Changed `RawResponse::json()` from `async` to synchronous function. The body was already buffered.
+- Changed `RawResponse::xml()` from `async` to synchronous function. The body was already buffered.
+- Removed `ErrorKind::http_response()`. Construct an `ErrorResponse::HttpResponse` variant instead.
 - Renamed `TransportOptions` to `Transport`.
 - Renamed `TransportOptions::new_custom_policy()` to `Transport::with_policy()`.
 

sdk/typespec/typespec_client_core/src/http/response.rs

@@ -411,7 +411,7 @@ mod tests {
                 .iter()
                 .any(|(k, v)| k.as_str() == "x-ms-error" && v.as_str() == "BadParameter"));
 
-            let err: ErrorResponse = err.json().await.expect("convert to ErrorResponse");
+            let err: ErrorResponse = err.json().expect("convert to ErrorResponse");
             assert_eq!(err.code, Some("BadParameter".into()));
             assert_eq!(err.message, Some("Bad parameter".into()));
         }