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.

Sign up for GitHub

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

Jump to bottom

Implement std::error::Error::source() properly for [service]::Error enums. #784

Closed
1 of 2 tasks
abusch opened this issue Apr 11, 2023 · 7 comments
Closed
1 of 2 tasks

Implement std::error::Error::source() properly for [service]::Error enums. #784

abusch opened this issue Apr 11, 2023 · 7 comments
Labels
feature-request A feature should be added or improved.

Comments

@abusch
Copy link
Contributor

abusch commented Apr 11, 2023 •
edited
Loading

Describe the feature

The Error enums in the different services implement std::error::Error like so: impl std::error::Error for Error {}. I believe they should implement Error::source() properly by delegating to the contained error struct. This would be particularly valuable for the Unhandled variant.

Use Case

I was trying to decrypt something using KMS and kept getting an Unhandled error without any other information. I thought something in my request was wrong but it turned out all it was was a missing permission, but it was hard to realise that as the source of the original error was lost (I was using anyhow and relied on it to properly display the chain of causes).

Proposed Solution

I think the impl std::error::Error for Error{} line in error_meta.rs should be changed to something like:

impl std::error::Error for Error {
  fn source(&self) -> Option<&(dyn Error + 'static)> {
        match self {
            Self::Variant1(variant1) => variant1.source(),
            ...
        }
    }

Other Information

No response

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change

A note for the community

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue, please leave a comment
@abusch abusch added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Apr 11, 2023
@abusch
Copy link
Contributor Author

abusch commented Apr 11, 2023 •
edited
Loading

Having a quick look at smithy-rs, am I correct in thinking that this might be already fixed? https://github.com/awslabs/smithy-rs/blob/main/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/error/OperationErrorGenerator.kt#L218

Never mind, I was looking in the wrong place: https://github.com/awslabs/smithy-rs/blob/main/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/error/ServiceErrorGenerator.kt#L83

But I assume the fix would be exactly the same.

@abusch abusch mentioned this issue Apr 11, 2023
Merged
2 tasks
@Velfi
Copy link
Contributor

Velfi commented Apr 11, 2023

However we decide to solve this, I think we should relate/contrast the final design to the existing Errors RFC.

I know that we changed how we expose source because it was causing error reporters to log the "causal chain" multiple times. I want to make sure we're not undoing that work.

@Velfi Velfi removed the needs-triage This issue or PR still needs to be triaged. label Apr 11, 2023
@abusch
Copy link
Contributor Author

abusch commented Apr 11, 2023

The PR above only exposes the source (opaquely) for the top-level error enum via the standard Error::source() method. It doesn't change any of the existing errors, or any of the Display implementation, so hopefully that's OK?

Was there any particular issue I should be testing for?

@Velfi
Copy link
Contributor

Velfi commented Apr 13, 2023

I know that we changed how we expose source because it was causing error reporters to log the "causal chain" multiple times. I want to make sure we're not undoing that work.

We'll need to send an error to an error reporter and make sure it looks good. That's all.

rcoh pushed a commit to smithy-lang/smithy-rs that referenced this issue Apr 17, 2023
@abusch
Implement StdError::source() for Error enum (#2564)
fc63800 
## Motivation and Context
This is an attempt at fixing
awslabs/aws-sdk-rust#784.

The service-level `Error` enum implements `std::error::Error` but does
not implement its `source()` method. This means that an error library
like `anyhow` or `eyre` won't be able to display the root cause of an
error, which is especially problematic for the `Unhandled` variant.

## Description
I modified `ServiceErrorGenerator` in the `codegen-client` crate and
replaced the line that output `impl std::error::Error for Error {}` with
an impl block that implements the `source()` method by delegating to the
inner error structure.

## Testing
I've added a simple unit test to `ServiceErrorGeneratorTest`.

## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [x] I have updated `CHANGELOG.next.toml` if I made changes to the
smithy-rs codegen or runtime crates
- [x] I have updated `CHANGELOG.next.toml` if I made changes to the AWS
SDK, generated SDK code, or SDK runtime crates

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
Velfi pushed a commit to smithy-lang/smithy-rs that referenced this issue Apr 17, 2023
@abusch @Velfi
Implement StdError::source() for Error enum (#2564)
657d05e 
## Motivation and Context
This is an attempt at fixing
awslabs/aws-sdk-rust#784.

The service-level `Error` enum implements `std::error::Error` but does
not implement its `source()` method. This means that an error library
like `anyhow` or `eyre` won't be able to display the root cause of an
error, which is especially problematic for the `Unhandled` variant.

## Description
I modified `ServiceErrorGenerator` in the `codegen-client` crate and
replaced the line that output `impl std::error::Error for Error {}` with
an impl block that implements the `source()` method by delegating to the
inner error structure.

## Testing
I've added a simple unit test to `ServiceErrorGeneratorTest`.

## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [x] I have updated `CHANGELOG.next.toml` if I made changes to the
smithy-rs codegen or runtime crates
- [x] I have updated `CHANGELOG.next.toml` if I made changes to the AWS
SDK, generated SDK code, or SDK runtime crates

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
unexge pushed a commit to smithy-lang/smithy-rs that referenced this issue Apr 24, 2023
@abusch @unexge
Implement StdError::source() for Error enum (#2564)
81011a9 
## Motivation and Context
This is an attempt at fixing
awslabs/aws-sdk-rust#784.

The service-level `Error` enum implements `std::error::Error` but does
not implement its `source()` method. This means that an error library
like `anyhow` or `eyre` won't be able to display the root cause of an
error, which is especially problematic for the `Unhandled` variant.

## Description
I modified `ServiceErrorGenerator` in the `codegen-client` crate and
replaced the line that output `impl std::error::Error for Error {}` with
an impl block that implements the `source()` method by delegating to the
inner error structure.

## Testing
I've added a simple unit test to `ServiceErrorGeneratorTest`.

## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [x] I have updated `CHANGELOG.next.toml` if I made changes to the
smithy-rs codegen or runtime crates
- [x] I have updated `CHANGELOG.next.toml` if I made changes to the AWS
SDK, generated SDK code, or SDK runtime crates

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
rcoh pushed a commit to smithy-lang/smithy-rs that referenced this issue Apr 24, 2023
@abusch @rcoh
Implement StdError::source() for Error enum (#2564)
82c602b 
## Motivation and Context
This is an attempt at fixing
awslabs/aws-sdk-rust#784.

The service-level `Error` enum implements `std::error::Error` but does
not implement its `source()` method. This means that an error library
like `anyhow` or `eyre` won't be able to display the root cause of an
error, which is especially problematic for the `Unhandled` variant.

## Description
I modified `ServiceErrorGenerator` in the `codegen-client` crate and
replaced the line that output `impl std::error::Error for Error {}` with
an impl block that implements the `source()` method by delegating to the
inner error structure.

## Testing
I've added a simple unit test to `ServiceErrorGeneratorTest`.

## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [x] I have updated `CHANGELOG.next.toml` if I made changes to the
smithy-rs codegen or runtime crates
- [x] I have updated `CHANGELOG.next.toml` if I made changes to the AWS
SDK, generated SDK code, or SDK runtime crates

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
@imp imp mentioned this issue Apr 25, 2023
Closed
2 tasks
aws-sdk-rust-ci pushed a commit that referenced this issue May 1, 2023
@abusch @aws-sdk-rust-ci
[smithy-rs] Implement StdError::source() for Error enum (#2564)
29b3114 
## Motivation and Context
This is an attempt at fixing
#784.

The service-level `Error` enum implements `std::error::Error` but does
not implement its `source()` method. This means that an error library
like `anyhow` or `eyre` won't be able to display the root cause of an
error, which is especially problematic for the `Unhandled` variant.

## Description
I modified `ServiceErrorGenerator` in the `codegen-client` crate and
replaced the line that output `impl std::error::Error for Error {}` with
an impl block that implements the `source()` method by delegating to the
inner error structure.

## Testing
I've added a simple unit test to `ServiceErrorGeneratorTest`.

## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [x] I have updated `CHANGELOG.next.toml` if I made changes to the
smithy-rs codegen or runtime crates
- [x] I have updated `CHANGELOG.next.toml` if I made changes to the AWS
SDK, generated SDK code, or SDK runtime crates

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
@abusch
Copy link
Contributor Author

abusch commented May 5, 2023

This should be fixed in 0.27.0.

@abusch
Copy link
Contributor Author

abusch commented Jul 18, 2023

Taking the liberty of closing this as this has been implemented.

@abusch abusch closed this as completed Jul 18, 2023
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
feature-request A feature should be added or improved.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@abusch @Velfi