From ec0b06d1ad38a3bc30ea5b0441ea6b1826d1997d Mon Sep 17 00:00:00 2001 From: Russell Cohen Date: Wed, 27 Jan 2021 20:07:34 -0500 Subject: [PATCH] Top level error shape improvements (#172) * Fallback to a generic error representation for unmodeled errors * DRY out some dependencies * Fix name is instant_iso8601 resource * Top level error shape improvements This commit adds `code` and `message` as inherent methods to top level error shapes (eg. `ListTablesError`). If the error is unmodeled, it uses downcasting to extract a message. * Fix clippy warning --- codegen-test/dynamo-it/Cargo.lock | 43 +++++-- codegen-test/dynamo-it/src/main.rs | 5 +- .../rust/codegen/smithy/RuntimeTypes.kt | 3 + .../generators/CombinedErrorGenerator.kt | 105 ++++++++++++++++-- .../generators/CombinedErrorGeneratorTest.kt | 15 +++ rust-runtime/smithy-types/src/lib.rs | 5 + 6 files changed, 150 insertions(+), 26 deletions(-) diff --git a/codegen-test/dynamo-it/Cargo.lock b/codegen-test/dynamo-it/Cargo.lock index 925946178..84c3d06af 100644 --- a/codegen-test/dynamo-it/Cargo.lock +++ b/codegen-test/dynamo-it/Cargo.lock @@ -12,12 +12,12 @@ version = "0.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "85293e31cbf209322718ff220c48470f5dd31f39f652220bba6f3ceb2579cc81" dependencies = [ - "bytes", + "bytes 0.5.6", "chrono", "eliza_error", "hex", "http", - "http-body", + "http-body 0.3.1", "httparse", "hyper", "ring", @@ -44,6 +44,12 @@ version = "0.5.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "0e4cec68f03f32e44924783795810fa50a7035d8c8ebe78580ad7e6c703fba38" +[[package]] +name = "bytes" +version = "1.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b700ce4376041dcd0a327fd0097c41095743c4c8af8887265942faf1100bd040" + [[package]] name = "cc" version = "1.0.66" @@ -188,7 +194,7 @@ version = "0.2.7" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "5e4728fd124914ad25e99e3d15a9361a879f6620f63cb56bbb08f95abb97a535" dependencies = [ - "bytes", + "bytes 0.5.6", "fnv", "futures-core", "futures-sink", @@ -225,11 +231,11 @@ checksum = "644f9158b2f133fd50f5fb3242878846d9eb792e445c893805ff0e3824006e35" [[package]] name = "http" -version = "0.2.1" +version = "0.2.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "28d569972648b2c512421b5f2a405ad6ac9666547189d0c5477a3f200f3e02f9" +checksum = "7245cd7449cc792608c3c8a9eaf69bd4eabbabf802713748fd739c98b82f0747" dependencies = [ - "bytes", + "bytes 1.0.1", "fnv", "itoa", ] @@ -240,7 +246,17 @@ version = "0.3.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "13d5ff830006f7646652e057693569bfe0d51760c0085a071769d142a205111b" dependencies = [ - "bytes", + "bytes 0.5.6", + "http", +] + +[[package]] +name = "http-body" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2861bd27ee074e5ee891e8b539837a9430012e249d7f0ca2d795650f579c1994" +dependencies = [ + "bytes 1.0.1", "http", ] @@ -262,13 +278,13 @@ version = "0.13.9" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f6ad767baac13b44d4529fcf58ba2cd0995e36e7b435bc5b039de6f47e880dbf" dependencies = [ - "bytes", + "bytes 0.5.6", "futures-channel", "futures-core", "futures-util", "h2", "http", - "http-body", + "http-body 0.3.1", "httparse", "httpdate", "itoa", @@ -307,7 +323,7 @@ version = "0.1.0" dependencies = [ "aws-sigv4", "http", - "http-body", + "http-body 0.3.1", "hyper", "pin-utils", "tokio", @@ -708,6 +724,9 @@ checksum = "c111b5bd5695e56cffe5129854aa230b39c93a305372fdbb2668ca2394eea9f8" name = "smithy-http" version = "0.0.1" dependencies = [ + "bytes 1.0.1", + "http", + "http-body 0.4.0", "smithy-types", ] @@ -779,7 +798,7 @@ version = "0.2.24" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "099837d3464c16a808060bb3f02263b412f6fafcb5d01c533d309985fbeebe48" dependencies = [ - "bytes", + "bytes 0.5.6", "fnv", "futures-core", "iovec", @@ -814,7 +833,7 @@ version = "0.3.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "be8242891f2b6cbef26a2d7e8605133c2c554cd35b3e4948ea892d6d68436499" dependencies = [ - "bytes", + "bytes 0.5.6", "futures-core", "futures-sink", "log", diff --git a/codegen-test/dynamo-it/src/main.rs b/codegen-test/dynamo-it/src/main.rs index 5e1439490..be1208afc 100644 --- a/codegen-test/dynamo-it/src/main.rs +++ b/codegen-test/dynamo-it/src/main.rs @@ -58,7 +58,7 @@ async fn main() -> Result<(), Box> { .build()]) .provisioned_throughput( ProvisionedThroughput::builder() - .read_capacity_units(100) + //.read_capacity_units(100) .write_capacity_units(100) .build(), ) @@ -76,7 +76,8 @@ async fn main() -> Result<(), Box> { ); println!("{} was created", table_name); } - _ => println!("{:?}", response.raw), + Some(Err(e)) => println!("{}", e), + _ => println!("something else") } let tables = io_v0::dispatch!( diff --git a/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/RuntimeTypes.kt b/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/RuntimeTypes.kt index 6a045a7e0..77d43c8b8 100644 --- a/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/RuntimeTypes.kt +++ b/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/RuntimeTypes.kt @@ -66,6 +66,9 @@ data class RuntimeType(val name: String?, val dependency: RustDependency?, val n fun Instant(runtimeConfig: RuntimeConfig) = RuntimeType("Instant", CargoDependency.SmithyTypes(runtimeConfig), "${runtimeConfig.cratePrefix}_types") + fun GenericError(runtimeConfig: RuntimeConfig) = + RuntimeType("Error", CargoDependency.SmithyTypes(runtimeConfig), "${runtimeConfig.cratePrefix}_types") + fun Blob(runtimeConfig: RuntimeConfig) = RuntimeType("Blob", CargoDependency.SmithyTypes(runtimeConfig), "${runtimeConfig.cratePrefix}_types") diff --git a/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/CombinedErrorGenerator.kt b/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/CombinedErrorGenerator.kt index ec105d43f..988bfd35d 100644 --- a/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/CombinedErrorGenerator.kt +++ b/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/CombinedErrorGenerator.kt @@ -5,6 +5,7 @@ package software.amazon.smithy.rust.codegen.smithy.generators +import software.amazon.smithy.codegen.core.Symbol import software.amazon.smithy.codegen.core.SymbolProvider import software.amazon.smithy.model.Model import software.amazon.smithy.model.knowledge.OperationIndex @@ -13,9 +14,13 @@ import software.amazon.smithy.rust.codegen.rustlang.Attribute import software.amazon.smithy.rust.codegen.rustlang.Derives import software.amazon.smithy.rust.codegen.rustlang.RustMetadata 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.writable import software.amazon.smithy.rust.codegen.smithy.RuntimeType +import software.amazon.smithy.rust.codegen.smithy.RustSymbolProvider +import software.amazon.smithy.rust.codegen.smithy.generators.config.Section /** * For a given Operation ([this]), return the symbol referring to the unified error? This can be used @@ -38,7 +43,7 @@ fun OperationShape.errorSymbol(symbolProvider: SymbolProvider): RuntimeType { */ class CombinedErrorGenerator( model: Model, - private val symbolProvider: SymbolProvider, + private val symbolProvider: RustSymbolProvider, private val operation: OperationShape ) { @@ -67,12 +72,8 @@ class CombinedErrorGenerator( } writer.rustBlock("impl #T for ${symbol.name}", RuntimeType.StdFmt("Display")) { rustBlock("fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result") { - rustBlock("match self") { - errors.forEach { - val errorSymbol = symbolProvider.toSymbol(it) - rust("""${symbol.name}::${errorSymbol.name}(inner) => inner.fmt(f),""") - } - rust("${symbol.name}::Unhandled(inner) => inner.fmt(f)") + delegateToVariants { + writable { rust("_inner.fmt(f)") } } } } @@ -81,16 +82,96 @@ class CombinedErrorGenerator( writer.rustBlock("pub fn unhandled>>(err: E) -> Self", RuntimeType.StdError) { write("${symbol.name}::Unhandled(err.into())") } + + // Consider if this should actually be `Option>`. 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") } + } + } + } + + 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()") } + } + } + } } writer.rustBlock("impl #T for ${symbol.name}", RuntimeType.StdError) { rustBlock("fn source(&self) -> Option<&(dyn #T + 'static)>", RuntimeType.StdError) { - rustBlock("match self") { - errors.forEach { - val errorSymbol = symbolProvider.toSymbol(it) - rust("""${symbol.name}::${errorSymbol.name}(inner) => Some(inner),""") + delegateToVariants { + writable { + when (it) { + is VariantMatch.Unhandled -> rust("Some(_inner.as_ref())") + is VariantMatch.Generic, is VariantMatch.Modeled -> rust("Some(_inner)") + } + } + } + } + } + } + + sealed class VariantMatch(name: String) : Section(name) { + object Unhandled : VariantMatch("Unhandled") + object Generic : VariantMatch("Generic") + data class Modeled(val symbol: Symbol) : VariantMatch("Modeled") + } + + /** + * Generates code to delegate behavior to the variants, for example: + * ```rust + * match self { + * 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, + * } + * } + * ``` + * + * [handler] is passed an instance of [VariantMatch]—a [writable] should be returned containing the content to be + * written for this variant. + * + * The field will always be bound as `_inner`. + */ + private fun RustWriter.delegateToVariants( + handler: (VariantMatch) -> Writable + ) { + val errors = operationIndex.getErrors(operation) + val symbol = operation.errorSymbol(symbolProvider) + val genericError = RuntimeType.GenericError(symbolProvider.config().runtimeConfig) + rustBlock("match self") { + errors.forEach { + val errorSymbol = symbolProvider.toSymbol(it) + rust("""${symbol.name}::${errorSymbol.name}(_inner) => """) + handler(VariantMatch.Modeled(errorSymbol))(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) + } } - rust("${symbol.name}::Unhandled(inner) => Some(inner.as_ref())") + } else { + // If the handlers are the same, skip the downcast + genericHandler(this) } } } diff --git a/codegen/src/test/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/CombinedErrorGeneratorTest.kt b/codegen/src/test/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/CombinedErrorGeneratorTest.kt index 9fa34acdb..ef5a93d98 100644 --- a/codegen/src/test/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/CombinedErrorGeneratorTest.kt +++ b/codegen/src/test/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/CombinedErrorGeneratorTest.kt @@ -49,6 +49,21 @@ internal class CombinedErrorGeneratorTest { """ let error = GreetingError::InvalidGreeting(InvalidGreeting::builder().message("an error").build()); assert_eq!(format!("{}", error), "InvalidGreeting: an error"); + assert_eq!(error.message(), Some("an error")); + assert_eq!(error.code(), Some("InvalidGreeting")); + + + // unhandled variants properly delegate message + let error = GreetingError::Unhandled(Box::new(smithy_types::Error { + code: None, + message: Some("hello".to_string()), + request_id: None + })); + assert_eq!(error.message(), Some("hello")); + + let error = GreetingError::unhandled("some other error"); + assert_eq!(error.message(), None); + assert_eq!(error.code(), None); """ ) } diff --git a/rust-runtime/smithy-types/src/lib.rs b/rust-runtime/smithy-types/src/lib.rs index 31cd74a57..6b7e6085e 100644 --- a/rust-runtime/smithy-types/src/lib.rs +++ b/rust-runtime/smithy-types/src/lib.rs @@ -63,6 +63,11 @@ impl Error { pub fn code(&self) -> Option<&str> { self.code.as_deref() } + + pub fn message(&self) -> Option<&str> { + self.message.as_deref() + } + pub fn request_id(&self) -> Option<&str> { self.request_id.as_deref() } } impl Display for Error { -- GitLab