Refactor errors to expose a kind & meta field separately (#249)

* Refactor errors to expose a kind & meta field separately

* More simplification of errors

* Simplify the retryable_error_kind implementation

* Error generator cleanups

* Fix aws-hyper feature issue

* Small test-util refactoring to improve debug output

aws/rust-runtime/aws-hyper/Cargo.toml

@@ -36,3 +36,7 @@ protocol-test-helpers = { path = "../../../rust-runtime/protocol-test-helpers",
 tokio = { version = "1", features = ["full", "test-util"] }
 tower-test = "0.4.0"
 aws-types = { path = "../aws-types" }
+
+[[test]]
+name = "e2e_test"
+required-features = ["test-util"]

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

@@ -136,21 +136,13 @@ where
 
 #[cfg(test)]
 mod tests {
-    use crate::test_connection::TestConnection;
-    use crate::{conn, Client};
+    use crate::Client;
 
     #[test]
     fn construct_default_client() {
         let _ = Client::https();
     }
 
-    #[test]
-    fn construct_test_client() {
-        let test_conn = TestConnection::<String>::new(vec![]);
-        let client = Client::new(conn::Standard::new(test_conn));
-        is_send_sync(client);
-    }
-
     #[test]
     fn client_debug_includes_retry_info() {
         let client = Client::https();

aws/rust-runtime/aws-hyper/src/test_connection.rs

@@ -129,6 +129,7 @@ impl<B: Into<hyper::Body>> tower::Service<http::Request<SdkBody>> for TestConnec
 #[cfg(test)]
 mod tests {
     use crate::test_connection::TestConnection;
+    use crate::{conn, Client};
     use smithy_http::body::SdkBody;
     use tower::BoxError;
 
@@ -145,4 +146,13 @@ mod tests {
         }
         let _ = check();
     }
+
+    fn is_send_sync<T: Send + Sync>(_: T) {}
+
+    #[test]
+    fn construct_test_client() {
+        let test_conn = TestConnection::<String>::new(vec![]);
+        let client = Client::new(conn::Standard::new(test_conn));
+        is_send_sync(client);
+    }
 }

aws/sdk/integration-tests/kms/Cargo.toml

@@ -12,7 +12,7 @@ kms = { path = "../../build/aws-sdk/kms" }
 smithy-http = { path = "../../build/aws-sdk/smithy-http" }
 smithy-types = { path = "../../build/aws-sdk/smithy-types" }
 http = "0.2.3"
-aws-hyper = { path = "../../build/aws-sdk/aws-hyper" }
+aws-hyper = { path = "../../build/aws-sdk/aws-hyper", features = ["test-util"] }
 aws-auth = { path = "../../build/aws-sdk/aws-auth" }
 aws-http = { path = "../../build/aws-sdk/aws-http" }
 tokio = { version = "1", features = ["full"]}

aws/sdk/integration-tests/kms/tests/integration.rs

@@ -6,11 +6,12 @@
 use aws_auth::Credentials;
 use aws_http::user_agent::AwsUserAgent;
 use aws_hyper::test_connection::TestConnection;
-use aws_hyper::Client;
+use aws_hyper::{Client, SdkError};
 use http::Uri;
+use kms::error::GenerateRandomErrorKind;
 use kms::operation::GenerateRandom;
-use smithy_http::body::SdkBody;
 use kms::{Config, Region};
+use smithy_http::body::SdkBody;
 use std::time::{Duration, UNIX_EPOCH};
 
 // TODO: having the full HTTP requests right in the code is a bit gross, consider something
@@ -137,13 +138,17 @@ async fn generate_random_keystore_not_found() {
     let client = Client::new(conn.clone());
     let err = client.call(op).await.expect_err("key store doesn't exist");
     let inner = match err {
-        aws_hyper::SdkError::ServiceError {
-            err: kms::error::GenerateRandomError::CustomKeyStoreNotFoundError(e),
-            ..
-        } => e,
+        SdkError::ServiceError { err, .. } => err,
         other => panic!("Incorrect error received: {:}", other),
     };
-    assert_eq!(inner.message, None);
+    assert!(matches!(
+        inner.kind,
+        GenerateRandomErrorKind::CustomKeyStoreNotFoundError(_)
+    ));
+    assert_eq!(
+        inner.request_id(),
+        Some("bfe81a0a-9a08-4e71-9910-cdb5ab6ea3b6")
+    );
     assert_eq!(conn.requests().len(), 1);
     for validate_request in conn.requests().iter() {
         validate_request.assert_matches(vec![]);

aws/sdk/integration-tests/kms/tests/sensitive-it.rs

@@ -3,14 +3,13 @@
  * SPDX-License-Identifier: Apache-2.0.
  */
 
-use kms::error::{CreateAliasError, LimitExceededError};
 use kms::operation::CreateAlias;
-use kms::output::{CreateAliasOutput, GenerateRandomOutput};
+use kms::output::GenerateRandomOutput;
 use kms::Blob;
-use smithy_http::result::{SdkError, SdkSuccess};
+use smithy_http::middleware::ResponseBody;
+use smithy_http::result::SdkError;
 use smithy_http::retry::ClassifyResponse;
 use smithy_types::retry::{ErrorKind, RetryKind};
-use smithy_http::middleware::ResponseBody;
 
 #[test]
 fn validate_sensitive_trait() {
@@ -23,19 +22,22 @@ fn validate_sensitive_trait() {
     );
 }
 
+/// Parse a semi-real response body and assert that the correct retry status is returned
 #[test]
 fn errors_are_retryable() {
-    let err = CreateAliasError::LimitExceededError(LimitExceededError::builder().build());
-    assert_eq!(err.code(), Some("LimitExceededException"));
     let conf = kms::Config::builder().build();
-
-    let op = CreateAlias::builder().build(&conf);
-    let err = Result::<SdkSuccess<CreateAliasOutput>, SdkError<CreateAliasError>>::Err(
-        SdkError::ServiceError {
-            raw: http::Response::builder().body(ResponseBody::from_static("resp")).unwrap(),
-            err,
-        },
-    );
-    let retry_kind = op.retry_policy().classify(err.as_ref());
+    let (_, parts) = CreateAlias::builder().build(&conf).into_request_response();
+    let http_response = http::Response::builder()
+        .status(400)
+        .body(r#"{ "code": "LimitExceededException" }"#)
+        .unwrap();
+    let err = parts
+        .response_handler
+        .parse_response(&http_response)
+        .map_err(|e| SdkError::ServiceError {
+            err: e,
+            raw: http_response.map(ResponseBody::from_static),
+        });
+    let retry_kind = parts.retry_policy.classify(err.as_ref());
     assert_eq!(retry_kind, RetryKind::Error(ErrorKind::ThrottlingError));
 }

codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/rustlang/RustWriter.kt

@@ -102,7 +102,12 @@ fun <T : CodeWriter> T.rustTemplate(
 /*
  * Writes a Rust-style block, demarcated by curly braces
  */
-fun <T : CodeWriter> T.rustBlock(header: String, vararg args: Any, block: T.() -> Unit): T {
+fun <T : CodeWriter> T.rustBlock(
+    @Language("Rust", prefix = "macro_rules! foo { () =>  {{ ", suffix = "}}}")
+    header: String,
+    vararg args: Any,
+    block: T.() -> Unit
+): T {
     openBlock("$header {", *args)
     block(this)
     closeBlock("}")

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

@@ -19,6 +19,7 @@ import software.amazon.smithy.rust.codegen.rustlang.RustWriter
 import software.amazon.smithy.rust.codegen.rustlang.Writable
 import software.amazon.smithy.rust.codegen.rustlang.rust
 import software.amazon.smithy.rust.codegen.rustlang.rustBlock
+import software.amazon.smithy.rust.codegen.rustlang.rustTemplate
 import software.amazon.smithy.rust.codegen.rustlang.writable
 import software.amazon.smithy.rust.codegen.smithy.RuntimeType
 import software.amazon.smithy.rust.codegen.smithy.RustSymbolProvider
@@ -50,6 +51,8 @@ class CombinedErrorGenerator(
 ) {
 
     private val operationIndex = OperationIndex.of(model)
+    private val runtimeConfig = symbolProvider.config().runtimeConfig
+    private val genericError = RuntimeType.GenericError(symbolProvider.config().runtimeConfig)
     fun render(writer: RustWriter) {
         val errors = operationIndex.getErrors(operation)
         val symbol = operation.errorSymbol(symbolProvider)
@@ -58,15 +61,26 @@ class CombinedErrorGenerator(
             additionalAttributes = listOf(Attribute.NonExhaustive),
             public = true
         )
+
+        meta.render(writer)
+        writer.rustBlock("struct ${symbol.name}") {
+            rust(
+                """
+                pub kind: ${symbol.name}Kind,
+                pub (crate) meta: #T
+            """,
+                RuntimeType.GenericError(runtimeConfig)
+            )
+        }
         meta.render(writer)
-        writer.rustBlock("enum ${symbol.name}") {
+        writer.rustBlock("enum ${symbol.name}Kind") {
             errors.forEach { errorVariant ->
                 val errorSymbol = symbolProvider.toSymbol(errorVariant)
                 write("${errorSymbol.name}(#T),", errorSymbol)
             }
             rust(
                 """
-                /// An unexpected error, eg. invalid JSON returned by the service
+                /// An unexpected error, eg. invalid JSON returned by the service or an unknown error code
                 Unhandled(Box<dyn #T>),
             """,
                 RuntimeType.StdError
@@ -90,56 +104,67 @@ class CombinedErrorGenerator(
             }
 
             rustBlock("fn retryable_error_kind(&self) -> Option<#T>", errorKindT) {
-                delegateToVariants {
-                    when (it) {
-                        is VariantMatch.Modeled -> writable {
-                            if (it.shape.hasTrait(RetryableTrait::class.java)) {
-                                rust("Some(_inner.retryable_error_kind())")
-                            } else {
-                                rust("None")
-                            }
+                val retryableVariants = errors.filter { it.hasTrait(RetryableTrait::class.java) }
+                if (retryableVariants.isEmpty()) {
+                    rust("None")
+                } else {
+                    rustBlock("match &self.kind") {
+                        retryableVariants.forEach {
+                            val errorSymbol = symbolProvider.toSymbol(it)
+                            rust("${symbol.name}Kind::${errorSymbol.name}(inner) => Some(inner.retryable_error_kind()),")
                         }
-                        is VariantMatch.Generic -> writable { rust("_inner.retryable_error_kind()") }
-                        is VariantMatch.Unhandled -> writable { rust("None") }
+                        rust("_ => None")
                     }
                 }
             }
         }
 
-        writer.rustBlock("impl ${symbol.name}") {
-            writer.rustBlock("pub fn unhandled<E: Into<Box<dyn #T>>>(err: E) -> Self", RuntimeType.StdError) {
-                write("${symbol.name}::Unhandled(err.into())")
-            }
+        writer.rustTemplate(
+            """
+            impl ${symbol.name} {
+                pub fn new(kind: ${symbol.name}Kind, meta: #{generic_error}) -> Self {
+                    Self { kind, meta }
+                }
 
-            // 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.
-            writer.rustBlock("pub fn message(&self) -> Option<&str>") {
-                delegateToVariants {
-                    when (it) {
-                        is VariantMatch.Generic, is VariantMatch.Modeled -> writable { rust("_inner.message()") }
-                        else -> writable { rust("None") }
+                pub fn unhandled(err: impl Into<Box<dyn #{std_error}>>) -> Self {
+                    Self {
+                        kind: ${symbol.name}Kind::Unhandled(err.into()),
+                        meta: Default::default()
                     }
                 }
-            }
 
-            writer.rustBlock("pub fn code(&self) -> Option<&str>") {
-                delegateToVariants {
-                    when (it) {
-                        is VariantMatch.Unhandled -> writable { rust("None") }
-                        is VariantMatch.Modeled -> writable { rust("Some(_inner.code())") }
-                        is VariantMatch.Generic -> writable { rust("_inner.code()") }
+                pub fn generic(err: #{generic_error}) -> Self {
+                    Self {
+                        meta: err.clone(),
+                        kind: ${symbol.name}Kind::Unhandled(err.into()),
                     }
                 }
+
+            // 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()
+            }
+
+            pub fn request_id(&self) -> Option<&str> {
+                self.meta.request_id.as_deref()
+            }
+
+            pub fn code(&self) -> Option<&str> {
+                self.meta.code.as_deref()
             }
         }
+        """,
+            "generic_error" to genericError, "std_error" to RuntimeType.StdError
+        )
 
         writer.rustBlock("impl #T for ${symbol.name}", RuntimeType.StdError) {
             rustBlock("fn source(&self) -> Option<&(dyn #T + 'static)>", RuntimeType.StdError) {
                 delegateToVariants {
                     writable {
                         when (it) {
                             is VariantMatch.Unhandled -> rust("Some(_inner.as_ref())")
-                            is VariantMatch.Generic, is VariantMatch.Modeled -> rust("Some(_inner)")
+                            is VariantMatch.Modeled -> rust("Some(_inner)")
                         }
                     }
                 }
@@ -149,7 +174,6 @@ class CombinedErrorGenerator(
 
     sealed class VariantMatch(name: String) : Section(name) {
         object Unhandled : VariantMatch("Unhandled")
-        object Generic : VariantMatch("Generic")
         data class Modeled(val symbol: Symbol, val shape: Shape) : VariantMatch("Modeled")
     }
 
@@ -160,10 +184,7 @@ class CombinedErrorGenerator(
      *    GreetingWithErrorsError::InvalidGreeting(_inner) => inner.fmt(f),
      *    GreetingWithErrorsError::ComplexError(_inner) => inner.fmt(f),
      *    GreetingWithErrorsError::FooError(_inner) => inner.fmt(f),
-     *    GreetingWithErrorsError::Unhandled(_inner) => match inner.downcast_ref::<::smithy_types::Error>() {
-     *      Some(_inner) => inner.message(),
-     *      None => None,
-     *    }
+     *    GreetingWithErrorsError::Unhandled(_inner) => _inner.fmt(f),
      *  }
      *  ```
      *
@@ -177,30 +198,16 @@ class CombinedErrorGenerator(
     ) {
         val errors = operationIndex.getErrors(operation)
         val symbol = operation.errorSymbol(symbolProvider)
-        val genericError = RuntimeType.GenericError(symbolProvider.config().runtimeConfig)
-        rustBlock("match self") {
+        rustBlock("match &self.kind") {
             errors.forEach {
                 val errorSymbol = symbolProvider.toSymbol(it)
-                rust("""${symbol.name}::${errorSymbol.name}(_inner) => """)
+                rust("""${symbol.name}Kind::${errorSymbol.name}(_inner) => """)
                 handler(VariantMatch.Modeled(errorSymbol, it))(this)
                 write(",")
             }
-            val genericHandler = handler(VariantMatch.Generic)
             val unhandledHandler = handler(VariantMatch.Unhandled)
-            rustBlock("${symbol.name}::Unhandled(_inner) =>") {
-                if (genericHandler != unhandledHandler) {
-                    rustBlock("match _inner.downcast_ref::<#T>()", genericError) {
-                        rustBlock("Some(_inner) => ") {
-                            genericHandler(this)
-                        }
-                        rustBlock("None => ") {
-                            unhandledHandler(this)
-                        }
-                    }
-                } else {
-                    // If the handlers are the same, skip the downcast
-                    genericHandler(this)
-                }
+            rustBlock("${symbol.name}Kind::Unhandled(_inner) =>") {
+                unhandledHandler(this)
             }
         }
     }