Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revise unhandled error variant according to RFC-39 #3191

Merged
merged 16 commits into from
Nov 15, 2023
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 58 additions & 0 deletions CHANGELOG.next.toml
Original file line number Diff line number Diff line change
Expand Up @@ -119,3 +119,61 @@ message = "Remove deprecated error kind type aliases."
references = ["smithy-rs#3189"]
meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "client" }
author = "jdisanti"

[[aws-sdk-rust]]
message = """
Unhandled errors have been made opaque to ensure code is written in a future-proof manner. Where previously, you
might have:
```rust
match service_error.err() {
GetStorageError::StorageAccessNotAuthorized(_) => { /* ... */ }
GetStorageError::Unhandled(unhandled) if unhandled.code() == Some("SomeUnmodeledErrorCode") {
// unhandled error handling
}
_ => { /* ... */ }
}
```
It should now look as follows:
```rust
match service_error.err() {
GetStorageError::StorageAccessNotAuthorized(_) => { /* ... */ }
err if err.code() == Some("SomeUnmodeledErrorCode") {
// unhandled error handling
}
_ => { /* ... */ }
}
```
The `Unhandled` variant should never be referenced directly.
"""
references = ["smithy-rs#3191"]
meta = { "breaking" = true, "tada" = false, "bug" = false }
author = "jdisanti"

[[smithy-rs]]
message = """
Unhandled errors have been made opaque to ensure code is written in a future-proof manner. Where previously, you
might have:
```rust
match service_error.err() {
GetStorageError::StorageAccessNotAuthorized(_) => { /* ... */ }
GetStorageError::Unhandled(unhandled) if unhandled.code() == Some("SomeUnmodeledErrorCode") {
// unhandled error handling
}
_ => { /* ... */ }
}
```
It should now look as follows:
```rust
match service_error.err() {
GetStorageError::StorageAccessNotAuthorized(_) => { /* ... */ }
err if err.code() == Some("SomeUnmodeledErrorCode") {
// unhandled error handling
}
_ => { /* ... */ }
}
```
The `Unhandled` variant should never be referenced directly.
"""
references = ["smithy-rs#3191"]
meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "client" }
author = "jdisanti"
2 changes: 2 additions & 0 deletions aws/rust-runtime/aws-inlineable/src/s3_request_id.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use aws_smithy_runtime_api::http::{Headers, Response};
use aws_smithy_types::error::metadata::{
Builder as ErrorMetadataBuilder, ErrorMetadata, ProvideErrorMetadata,
};
#[allow(deprecated)]
use aws_smithy_types::error::Unhandled;

const EXTENDED_REQUEST_ID: &str = "s3_extended_request_id";
Expand Down Expand Up @@ -36,6 +37,7 @@ impl RequestIdExt for ErrorMetadata {
}
}

#[allow(deprecated)]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is where I'd consider using Introspect but this is fine, I don't think it's a huge deal if we do it this way.

impl RequestIdExt for Unhandled {
fn extended_request_id(&self) -> Option<&str> {
self.meta().extended_request_id()
Expand Down
3 changes: 3 additions & 0 deletions aws/rust-runtime/aws-types/src/request_id.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ use aws_smithy_runtime_api::http::Response;
use aws_smithy_types::error::metadata::{
Builder as ErrorMetadataBuilder, ErrorMetadata, ProvideErrorMetadata,
};

#[allow(deprecated)]
use aws_smithy_types::error::Unhandled;

/// Constant for the [`ErrorMetadata`] extra field that contains the request ID
Expand Down Expand Up @@ -38,6 +40,7 @@ impl RequestId for ErrorMetadata {
}
}

#[allow(deprecated)]
impl RequestId for Unhandled {
fn request_id(&self) -> Option<&str> {
self.meta().request_id()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ abstract class BaseRequestIdDecorator : ClientCodegenDecorator {
val sym = codegenContext.symbolProvider.toSymbol(error)
rust("Self::${sym.name}(e) => #T,", wrapped)
}
rust("Self::Unhandled(e) => e.$accessorFunctionName(),")
rust("Self::Unhandled(e) => e.meta.$accessorFunctionName(),")
}
}
}
Expand Down
1 change: 1 addition & 0 deletions aws/sdk/integration-tests/lambda/tests/request_id.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use aws_sdk_lambda::operation::RequestId;
use aws_sdk_lambda::{Client, Config};
use aws_smithy_runtime::client::http::test_util::infallible_client_fn;

#[allow(deprecated)]
async fn run_test(
response: impl Fn() -> http::Response<&'static str> + Send + Sync + 'static,
expect_error: bool,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

use aws_credential_types::provider::SharedCredentialsProvider;
use aws_sdk_s3::config::{Credentials, Region};
use aws_sdk_s3::operation::list_objects_v2::ListObjectsV2Error;
use aws_sdk_s3::{Client, Config};
use aws_smithy_runtime::client::http::test_util::capture_request;

Expand Down Expand Up @@ -58,8 +59,8 @@ async fn test_s3_signer_query_string_with_all_valid_chars() {
// test must be run against an actual bucket so we `ignore` it unless the runner specifically requests it
#[tokio::test]
#[ignore]
#[allow(deprecated)]
async fn test_query_strings_are_correctly_encoded() {
use aws_sdk_s3::operation::list_objects_v2::ListObjectsV2Error;
use aws_smithy_runtime_api::client::result::SdkError;

tracing_subscriber::fmt::init();
Expand All @@ -80,22 +81,19 @@ async fn test_query_strings_are_correctly_encoded() {
.send()
.await;
if let Err(SdkError::ServiceError(context)) = res {
match context.err() {
ListObjectsV2Error::Unhandled(e)
if e.to_string().contains("SignatureDoesNotMatch") =>
{
chars_that_break_signing.push(byte);
}
ListObjectsV2Error::Unhandled(e) if e.to_string().contains("InvalidUri") => {
chars_that_break_uri_parsing.push(byte);
}
ListObjectsV2Error::Unhandled(e) if e.to_string().contains("InvalidArgument") => {
chars_that_are_invalid_arguments.push(byte);
}
ListObjectsV2Error::Unhandled(e) if e.to_string().contains("InvalidToken") => {
panic!("refresh your credentials and run this test again");
}
e => todo!("unexpected error: {:?}", e),
let err = context.err();
let msg = err.to_string();
let unhandled = matches!(err, ListObjectsV2Error::Unhandled(_));
if unhandled && msg.contains("SignatureDoesNotMatch") {
chars_that_break_signing.push(byte);
} else if unhandled && msg.to_string().contains("InvalidUri") {
chars_that_break_uri_parsing.push(byte);
} else if unhandled && msg.to_string().contains("InvalidArgument") {
chars_that_are_invalid_arguments.push(byte);
} else if unhandled && msg.to_string().contains("InvalidToken") {
panic!("refresh your credentials and run this test again");
} else {
todo!("unexpected error: {:?}", err);
}
}
}
Expand Down
1 change: 1 addition & 0 deletions aws/sdk/integration-tests/s3/tests/request_id.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ async fn get_request_id_from_modeled_error() {
}

#[tokio::test]
#[allow(deprecated)]
async fn get_request_id_from_unmodeled_error() {
let (http_client, request) = capture_request(Some(
http::Response::builder()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import software.amazon.smithy.rust.codegen.client.smithy.ClientCodegenContext
import software.amazon.smithy.rust.codegen.client.smithy.ClientRustModule
import software.amazon.smithy.rust.codegen.core.rustlang.RustModule
import software.amazon.smithy.rust.codegen.core.rustlang.RustWriter
import software.amazon.smithy.rust.codegen.core.rustlang.Visibility
import software.amazon.smithy.rust.codegen.core.rustlang.Writable
import software.amazon.smithy.rust.codegen.core.rustlang.docs
import software.amazon.smithy.rust.codegen.core.rustlang.rust
Expand Down Expand Up @@ -75,12 +76,39 @@ data class InfallibleEnumType(
)
}

override fun additionalEnumImpls(context: EnumGeneratorContext): Writable = writable {
// `try_parse` isn't needed for unnamed enums
if (context.enumTrait.hasNames()) {
rustTemplate(
"""
impl ${context.enumName} {
/// Parses the enum value while disallowing unknown variants.
///
/// Unknown variants will result in an error.
pub fn try_parse(value: &str) -> #{Result}<Self, #{UnknownVariantError}> {
match Self::from(value) {
##[allow(deprecated)]
Self::Unknown(_) => #{Err}(#{UnknownVariantError}::new(value)),
known => Ok(known),
}
}
}
""",
*preludeScope,
"UnknownVariantError" to unknownVariantError(),
)
}
}

override fun additionalDocs(context: EnumGeneratorContext): Writable = writable {
renderForwardCompatibilityNote(context.enumName, context.sortedMembers, UnknownVariant, UnknownVariantValue)
}

override fun additionalEnumMembers(context: EnumGeneratorContext): Writable = writable {
docs("`$UnknownVariant` contains new variants that have been added since this code was generated.")
rust(
"""##[deprecated(note = "Don't directly match on `$UnknownVariant`. See the docs on this enum for the correct way to handle unknown variants.")]""",
)
rust("$UnknownVariant(#T)", unknownVariantValue(context))
}

Expand All @@ -93,10 +121,9 @@ data class InfallibleEnumType(
docs(
"""
Opaque struct used as inner data for the `Unknown` variant defined in enums in
the crate
the crate.

While this is not intended to be used directly, it is marked as `pub` because it is
part of the enums that are public interface.
This is not intended to be used directly.
""".trimIndent(),
)
context.enumMeta.render(this)
Expand Down Expand Up @@ -174,5 +201,35 @@ class ClientEnumGenerator(codegenContext: ClientCodegenContext, shape: StringSha
codegenContext.model,
codegenContext.symbolProvider,
shape,
InfallibleEnumType(ClientRustModule.primitives),
InfallibleEnumType(
RustModule.new(
"sealed_enum_unknown",
visibility = Visibility.PUBCRATE,
parent = ClientRustModule.primitives,
),
),
)

private fun unknownVariantError(): RuntimeType = RuntimeType.forInlineFun("UnknownVariantError", ClientRustModule.Error) {
rustTemplate(
"""
/// The given enum value failed to parse since it is not a known value.
##[derive(Debug)]
pub struct UnknownVariantError {
value: #{String},
}
impl UnknownVariantError {
pub(crate) fn new(value: impl #{Into}<#{String}>) -> Self {
Self { value: value.into() }
}
}
impl ::std::fmt::Display for UnknownVariantError {
fn fmt(&self, f: &mut ::std::fmt::Formatter<'_>) -> #{Result}<(), ::std::fmt::Error> {
write!(f, "unknown enum variant: '{}'", self.value)
}
}
impl ::std::error::Error for UnknownVariantError {}
""",
*preludeScope,
)
}
Loading
Loading