Unverified Commit 43936cbb authored by Russell Cohen's avatar Russell Cohen Committed by GitHub
Browse files

Refactor smithy_types::Error to be more flexible (Breaking Change) (#426) 

* Refactor smithy_types::Error to be more flexible

S3 (and probably other) services have errors with an additional set of metadata that must be recorded. This diff does a few things:
1. Make the internal fields of smithy_error::Error private (as they should have been from the beginning)
2. Adds a builder to easily construct errors
3. Adds a property bag to enable storing additional fields on the error.

This will make it straightforward to add per-service customizations to record additional error metadata.

* Fix two missed refactorings
parent a6161140

aws/rust-runtime/aws-http/src/lib.rs

+1 −5
Original line number Diff line number Diff line
@@ -198,11 +198,7 @@ mod test { 


    #[test]

    fn classify_generic() {

        let err = smithy_types::Error {

            code: Some("SlowDown".to_string()),

            message: None,

            request_id: None,

        };

        let err = smithy_types::Error::builder().code("SlowDown").build();

        let test_response = http::Response::new("OK");

        let policy = AwsErrorRetryPolicy::new();

        assert_eq!(

codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/error/CombinedErrorGenerator.kt

+3 −3
Original line number Diff line number Diff line
@@ -143,15 +143,15 @@ class CombinedErrorGenerator( 
            // Consider if this should actually be `Option<Cow<&str>>`. This would enable us to use display as implemented

            // by std::Error to generate a message in that case.

            pub fn message(&self) -> Option<&str> {

                self.meta.message.as_deref()

                self.meta.message()

            }



            pub fn request_id(&self) -> Option<&str> {

                self.meta.request_id.as_deref()

                self.meta.request_id()

            }



            pub fn code(&self) -> Option<&str> {

                self.meta.code.as_deref()

                self.meta.code()

            }

        }

        """,

codegen/src/test/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/CombinedErrorGeneratorTest.kt

+2 −6
Original line number Diff line number Diff line
@@ -51,7 +51,7 @@ internal class CombinedErrorGeneratorTest { 
        writer.compileAndTest(

            """

            let kind = GreetingErrorKind::InvalidGreeting(InvalidGreeting::builder().message("an error").build());

            let error = GreetingError::new(kind, smithy_types::Error { code: Some("InvalidGreeting".to_string()), message: Some("an error".to_string()), request_id: None });

            let error = GreetingError::new(kind, smithy_types::Error::builder().code("InvalidGreeting").message("an error").build());

            assert_eq!(format!("{}", error), "InvalidGreeting: an error");

            assert_eq!(error.message(), Some("an error"));

            assert_eq!(error.code(), Some("InvalidGreeting"));
@@ -60,11 +60,7 @@ internal class CombinedErrorGeneratorTest { 




            // unhandled variants properly delegate message

            let error = GreetingError::generic(smithy_types::Error {

               code: None,

               message: Some("hello".to_string()),

               request_id: None

            });

            let error = GreetingError::generic(smithy_types::Error::builder().message("hello").build());

            assert_eq!(error.message(), Some("hello"));



            let error = GreetingError::unhandled("some other error");

rust-runtime/inlineable/src/aws_json_errors.rs

+23 −22
Original line number Diff line number Diff line
@@ -45,26 +45,30 @@ pub fn parse_generic_error<B>( 
    response: &http::Response<B>,

    body: &serde_json::Value,

) -> smithy_types::Error {

    let mut err_builder = smithy_types::Error::builder();

    let code = error_type_from_header(&response)

        .unwrap_or(Some("header was not valid UTF-8"))

        .or_else(|| error_type_from_body(body))

        .map(|s| sanitize_error_code(s).to_string());

        .map(|s| sanitize_error_code(s));

    if let Some(code) = code {

        err_builder.code(code);

    }

    let message = body

        .get("message")

        .or_else(|| body.get("Message"))

        .or_else(|| body.get("errorMessage"))

        .and_then(|v| v.as_str())

        .map(|s| s.to_string());

        .and_then(|v| v.as_str());

    if let Some(message) = message {

        err_builder.message(message);

    }

    let request_id = response

        .headers()

        .get("X-Amzn-Requestid")

        .and_then(|v| v.to_str().ok())

        .map(|s| s.to_string());

    smithy_types::Error {

        code,

        message,

        request_id,

        .and_then(|v| v.to_str().ok());

    if let Some(request_id) = request_id {

        err_builder.request_id(request_id);

    }

    err_builder.build()

}



#[cfg(test)]
@@ -84,11 +88,11 @@ mod test { 
            .unwrap();

        assert_eq!(

            parse_generic_error(&response, response.body()),

            Error {

                code: Some("FooError".to_string()),

                message: Some("Go to foo".to_string()),

                request_id: Some("1234".to_string()),

            }

            Error::builder()

                .code("FooError")

                .message("Go to foo")

                .request_id("1234")

                .build()

        )

    }
@@ -151,13 +155,10 @@ mod test { 
            .unwrap();

        assert_eq!(

            parse_generic_error(&response, response.body()),

            Error {

                code: Some("ResourceNotFoundException".to_string()),

                message: Some(

                    "Functions from 'us-west-2' are not reachable from us-east-1".to_string()

                ),

                request_id: None,

            }

        )

            Error::builder()

                .code("ResourceNotFoundException")

                .message("Functions from 'us-west-2' are not reachable from us-east-1")

                .build()

        );

    }

}

rust-runtime/inlineable/src/rest_xml_unwrapped_errors.rs

+11 −5
Original line number Diff line number Diff line
@@ -27,16 +27,22 @@ pub fn error_scope<'a, 'b>(doc: &'a mut Document<'b>) -> Result<ScopedDecoder<'b 
pub fn parse_generic_error(body: &[u8]) -> Result<smithy_types::Error, XmlError> {

    let mut doc = Document::try_from(body)?;

    let mut root = doc.root_element()?;

    let mut err = smithy_types::Error::default();

    let mut err = smithy_types::Error::builder();

    while let Some(mut tag) = root.next_tag() {

        match tag.start_el().local() {

            "Code" => err.code = Some(String::from(try_data(&mut tag)?)),

            "Message" => err.message = Some(String::from(try_data(&mut tag)?)),

            "RequestId" => err.request_id = Some(String::from(try_data(&mut tag)?)),

            "Code" => {

                err.code(try_data(&mut tag)?);

            }

            "Message" => {

                err.message(try_data(&mut tag)?);

            }

            "RequestId" => {

                err.request_id(try_data(&mut tag)?);

            }

            _ => {}

        }

    }

    Ok(err)

    Ok(err.build())

}



#[cfg(test)]