Reworked error handling to use new check_status function (#2982)

Fixed layering violation because `HttpError` was defined in
typespec_client_core but was an azure_core construct.
Simplified service client error management.

---------

Co-authored-by: Copilot <[email protected]>
Co-authored-by: Heath Stewart <[email protected]>

Author: 3 people

sdk/core/azure_core/CHANGELOG.md

@@ -9,6 +9,8 @@
 - Added `TryFrom<T> for RequestContent<T, JsonFormat>` for JSON primitives.
 - Added support for WASM to the `async_runtime` module.
 - Added logging policy to log HTTP requests and responses in the pipeline. As a part of this change, sanitization support was added to places which log HTTP headers and URLs. The `azure_core::http::ClientOptions` has been enhanced with a `LoggingOptions` which allows a user/service client to specify headers or URL query parameters which should be allowed. Note that the sanitization feature is disabled if you build with the `debug` feature enabled.
+- Added support for a new `azure_core::error::http::ErrorResponse` structure which describes an error according to the Azure REST API guidelines.
+- Added a new `azure_core::http::check_success(BufResponse)` function to convert a buffered response to an `ErrorKind::HttpResponse`.
 
 ### Breaking Changes
 

sdk/core/azure_core/README.md

@@ -184,7 +184,7 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
 When a service call fails, the returned `Result` will contain an `Error`. The `Error` type provides a status property with an HTTP status code and an error_code property with a service-specific error code.
 
 ```rust no_run
-use azure_core::{error::{ErrorKind, HttpError}, http::{Response, StatusCode}};
+use azure_core::{error::ErrorKind, http::{Response, StatusCode}};
 use azure_identity::DeveloperToolsCredential;
 use azure_security_keyvault_secrets::SecretClient;
 

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

@@ -0,0 +1,345 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+// cspell:ignore innererror
+
+use crate::{
+    error::{Error, ErrorKind},
+    http::{headers::ERROR_CODE, BufResponse},
+};
+use serde::Deserialize;
+use std::{collections::HashMap, str};
+
+/// An HTTP error response.
+///
+/// Implements a standard "ErrorResponse" as described in the [API guidelines](https://github.com/microsoft/api-guidelines/blob/vNext/azure/Guidelines.md#handling-errors).
+///
+/// Can be converted from an `[Error]` if it is of kind `[ErrorKind::HttpResponse]` and has a raw response.
+///
+/// # Example
+///
+/// Converting an `Error` to an `ErrorResponse`:
+///
+///``` no_run
+/// use azure_core::error::ErrorResponse;
+/// # let err = azure_core::Error::from(azure_core::error::ErrorKind::DataConversion);
+/// let error_response = ErrorResponse::try_from(err).expect("expected an ErrorResponse");
+///```
+///
+///
+#[derive(Debug, Deserialize)]
+#[serde(rename_all = "camelCase")]
+pub struct ErrorResponse {
+    /// The error details.
+    pub error: Option<ErrorDetail>,
+}
+
+impl TryFrom<Error> for ErrorResponse {
+    type Error = Error;
+
+    fn try_from(value: Error) -> Result<Self, Self::Error> {
+        match value.kind() {
+            ErrorKind::HttpResponse { raw_response, .. } => {
+                let error_response: Option<crate::Result<ErrorResponse>> = raw_response
+                    .as_ref()
+                    .map(|raw| serde_json::from_slice(raw.body()).map_err(Error::from));
+                match error_response {
+                    Some(result) => Ok(result?),
+                    None => Err(value),
+                }
+            }
+            _ => Err(value),
+        }
+    }
+}
+
+/// Details about an error returned from a service.
+///
+/// Implements a standard "ErrorDetails" as described in the [API guidelines](https://github.com/microsoft/api-guidelines/blob/vNext/azure/Guidelines.md#handling-errors).
+#[derive(Debug, Deserialize)]
+#[serde(rename_all = "camelCase")]
+pub struct ErrorDetail {
+    /// The error code. A machine readable error code defined by the service.
+    pub code: Option<String>,
+
+    /// A human-readable error message describing the error.
+    pub message: Option<String>,
+
+    /// The target of the error (for example, the name of the property in error).
+    pub target: Option<String>,
+
+    /// Additional details about the error.
+    #[serde(default)]
+    pub details: Vec<ErrorDetail>,
+
+    /// An inner error that may have more specific information about the root cause of the error.
+    #[serde(rename = "innererror")]
+    pub inner_error: Option<InnerError>,
+
+    /// Additional properties that may be returned with the error.
+    #[serde(flatten)]
+    pub additional_properties: HashMap<String, serde_json::Value>,
+}
+
+/// Inner error information about an error returned from a service.
+///
+/// Implements a standard "InnerError" as described in the [API guidelines](https://github.com/microsoft/api-guidelines/blob/vNext/azure/Guidelines.md#handling-errors).
+#[derive(Debug, Deserialize)]
+#[serde(rename_all = "camelCase")]
+pub struct InnerError {
+    /// A more specific error than was contained in the containing error.
+    pub code: Option<String>,
+
+    /// An object containing more specific information than the current object about the error.
+    #[serde(rename = "innererror")]
+    pub inner_error: Option<Box<InnerError>>,
+}
+
+/// Internal struct to help with deserialization without allocating Strings.
+#[derive(Debug, Deserialize)]
+struct ErrorResponseInternal<'a> {
+    #[serde(borrow)]
+    error: ErrorDetailsInternal<'a>,
+}
+
+#[derive(Debug, Deserialize)]
+struct ErrorDetailsInternal<'a> {
+    code: Option<&'a str>,
+    message: Option<&'a str>,
+}
+
+/// Checks if the response is a success and if not, creates an appropriate error.
+///
+/// # Arguments
+/// * `response` - The HTTP response to check.
+///
+/// # Returns
+/// * `Ok(RawResponse)` if the response is a success.
+/// * `Err(Error)` if the response is an error, with details extracted from the response
+///   body if possible.
+///
+pub async fn check_success(response: BufResponse) -> crate::Result<BufResponse> {
+    let status = response.status();
+    if status.is_success() {
+        return Ok(response);
+    }
+
+    let 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));
+        return Err(Error::message(error_kind, status.to_string()));
+    }
+    let internal_response =
+        serde_json::de::from_slice::<ErrorResponseInternal>(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(
+                status,
+                Some(code.map_or_else(|| response.status().to_string(), str::to_owned)),
+            );
+            return Err(Error::message(
+                error_kind,
+                format!(
+                    "{}: {}",
+                    status,
+                    str::from_utf8(response.body()).unwrap_or("(invalid utf-8 in body)")
+                ),
+            ));
+        }
+    };
+
+    // 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));
+
+    Err(Error::message(
+        error_kind,
+        internal_response
+            .error
+            .message
+            .map_or_else(|| status.to_string(), |m| m.to_owned()),
+    ))
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::http::{headers, headers::Headers, StatusCode};
+    use crate::Bytes;
+
+    #[tokio::test]
+    async fn matching_against_http_error() {
+        let mut headers = Headers::new();
+        headers.insert(headers::CONTENT_TYPE, "application/json".to_string());
+        let response = BufResponse::from_bytes(
+            StatusCode::ImATeapot,
+            headers,
+            Bytes::from_static(br#"{"error": {"code":"teapot","message":"I'm a teapot"}}"#),
+        );
+
+        let err = check_success(response).await.unwrap_err();
+        let kind = err.kind();
+        assert!(matches!(
+            kind,
+            ErrorKind::HttpResponse {
+                status: StatusCode::ImATeapot,
+                error_code,
+                raw_response: None
+            }
+            if error_code.as_deref() == Some("teapot")
+        ));
+    }
+
+    #[tokio::test]
+    async fn matching_against_http_error_no_body() {
+        let mut headers = Headers::new();
+        headers.insert(headers::ERROR_CODE, "testError".to_string());
+        let response = BufResponse::from_bytes(StatusCode::ImATeapot, headers, Bytes::new());
+
+        let err = check_success(response).await.unwrap_err();
+        let kind = err.kind();
+        assert_eq!(
+            *kind,
+            ErrorKind::HttpResponse {
+                status: StatusCode::ImATeapot,
+                error_code: Some("testError".to_string()),
+                raw_response: None
+            }
+        );
+    }
+
+    #[tokio::test]
+    async fn matching_against_http_error_invalid_body() {
+        let mut headers = Headers::new();
+        headers.insert(headers::ERROR_CODE, "testError".to_string());
+        let response = BufResponse::from_bytes(
+            StatusCode::ImATeapot,
+            headers,
+            Bytes::from_static(br#"{"json": "error"}"#),
+        );
+
+        let err = check_success(response).await.unwrap_err();
+        let kind = err.kind();
+        assert_eq!(
+            *kind,
+            ErrorKind::HttpResponse {
+                status: StatusCode::ImATeapot,
+                error_code: Some("testError".to_string()),
+                raw_response: None
+            }
+        );
+        assert!(err.to_string().contains(r#"{"json": "error"}"#));
+    }
+
+    #[test]
+    fn deserialize_to_error_response() {
+        let err : ErrorResponse = serde_json::from_slice (br#"{"error":{"code":"InvalidRequest","message":"The request object is not recognized.","innererror":{"code":"InvalidKey"},"key":"foo"}}"#)
+            .expect("Parse success.");
+        err.error.as_ref().expect("error should be set");
+
+        println!("{:?}", &err);
+        assert_eq!(
+            err.error.as_ref().unwrap().code,
+            Some("InvalidRequest".to_string())
+        );
+        assert_eq!(
+            err.error.as_ref().unwrap().message,
+            Some("The request object is not recognized.".to_string())
+        );
+        assert!(err.error.as_ref().unwrap().inner_error.is_some());
+        assert_eq!(
+            err.error
+                .as_ref()
+                .unwrap()
+                .inner_error
+                .as_ref()
+                .unwrap()
+                .code,
+            Some("InvalidKey".to_string())
+        );
+        assert!(err
+            .error
+            .as_ref()
+            .unwrap()
+            .additional_properties
+            .contains_key("key"));
+    }
+
+    #[tokio::test]
+    async fn convert_error_to_error_response() -> crate::Result<()> {
+        {
+            let err: Error = Error::from(ErrorKind::HttpResponse {
+                status: StatusCode::BadRequest,
+                error_code: Some("testError".to_string()),
+                raw_response: None,
+            });
+            let _error_response = ErrorResponse::try_from(err)
+                .expect_err("expected an error because there is no raw_response");
+        }
+        {
+            let buf_response = BufResponse::from_bytes(
+                StatusCode::BadRequest,
+                Headers::new(),
+                Bytes::from_static(br#"{"error":{"code":"InvalidRequest","message":"The request object is not recognized.","innererror":{"code":"InvalidKey"},"key":"foo"}}"#),
+            );
+            let err: Error = Error::from(ErrorKind::HttpResponse {
+                status: StatusCode::BadRequest,
+                error_code: Some("testError".to_string()),
+                raw_response: Some(Box::new(buf_response.try_into_raw_response().await?)),
+            });
+            let error_response = ErrorResponse::try_from(err).expect("expected an ErrorResponse");
+            error_response.error.as_ref().expect("error should be set");
+            println!("{:?}", &error_response);
+            assert_eq!(
+                error_response.error.as_ref().unwrap().code,
+                Some("InvalidRequest".to_string())
+            );
+        }
+        Ok(())
+    }
+
+    #[tokio::test]
+    async fn convert_buf_response_to_error_response() -> crate::Result<()> {
+        {
+            let buf_response = BufResponse::from_bytes(
+                StatusCode::BadRequest,
+                Headers::new(),
+                Bytes::from_static(br#"{"error":{"code":"InvalidRequest","message":"The request object is not recognized.","innererror":{"code":"InvalidKey"},"key":"foo"}}"#),
+            );
+            let error_response: ErrorResponse = buf_response
+                .into_body()
+                .json()
+                .await
+                .expect("expected an ErrorResponse");
+            error_response.error.as_ref().expect("error should be set");
+            println!("{:?}", &error_response);
+            assert_eq!(
+                error_response.error.as_ref().unwrap().code,
+                Some("InvalidRequest".to_string())
+            );
+        }
+        Ok(())
+    }
+
+    #[tokio::test]
+    async fn deserialize_to_error_response_internal() {
+        let err :ErrorResponseInternal = serde_json::from_slice (br#"{"error":{"code":"InvalidRequest","message":"The request object is not recognized.","innererror":{"code":"InvalidKey","key":"foo"}}}"#)
+            .expect("Parse success.");
+        println!("{:?}", &err);
+
+        assert_eq!(err.error.code, Some("InvalidRequest"));
+        assert_eq!(
+            err.error.message,
+            Some("The request object is not recognized.")
+        );
+    }
+}

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

@@ -0,0 +1,7 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+pub use typespec_client_core::error::*;
+mod error_response;
+
+pub use error_response::*;

sdk/core/azure_core/src/http/mod.rs

@@ -26,5 +26,6 @@ pub use typespec_client_core::http::{
     Method, NoFormat, StatusCode, Url,
 };
 
+pub use crate::error::check_success;
 #[cfg(feature = "xml")]
 pub use typespec_client_core::http::XmlFormat;

sdk/core/azure_core/src/http/pipeline.rs

@@ -3,7 +3,7 @@
 
 use super::policies::ClientRequestIdPolicy;
 use crate::http::{
-    headers::{ERROR_CODE, RETRY_AFTER_MS, X_MS_RETRY_AFTER_MS},
+    headers::{RETRY_AFTER_MS, X_MS_RETRY_AFTER_MS},
     policies::{
         Policy, PublicApiInstrumentationPolicy, RequestInstrumentationPolicy, UserAgentPolicy,
     },
@@ -106,7 +106,6 @@ impl Pipeline {
         let pipeline_options = pipeline_options.unwrap_or_else(|| PipelineOptions {
             retry_headers: RetryHeaders {
                 retry_headers: vec![X_MS_RETRY_AFTER_MS, RETRY_AFTER_MS, RETRY_AFTER],
-                error_header: Some(ERROR_CODE),
             },
         });