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

Allow handling of NotFound responses as an inner Err in a nested Result #1010

Closed
2 tasks
jakoblell opened this issue Dec 15, 2023 · 4 comments
Closed
2 tasks
Labels
closed-for-staleness feature-request A feature should be added or improved. p2 This is a standard priority issue response-requested Waiting on additional info and feedback. Will move to 'closing-soon' in 7 days.

Comments

@jakoblell
Copy link

Describe the feature

For some applications it is required to explicitly handle a NotFound response for some requests (such as an S3 head_object) as part of the application logic while other errors returned by the same API can be considered sufficiently unlikely to just abort with an expect/unwrap or pass the error up to the caller with a ?. At the moment it requires quite a bit of boilerplate code to go through the nested error struct in order to distinguish a NotFound response from other errors.

From my point of view the best option would be to have a nested result with the outer Result indicating some form of hard error (network connectivity, permission issue, invalid server response etc.) and an inner Result just containing the expected response or a NotFound error. That way the application can directly handle the NotFound error after getting rid of the outer error via a ? or an unwrap/expect. In order to not break existing applications this should probably be optional, see the section "Proposed solution" on how this could be implemented.

Use Case

When using S3 it is sometimes required to check whether an object exists or not. At the moment it is pretty frustrating to go through the error response in order to find out whether it is an actual error or just a NotFound response which can be handled by the application logic.

Proposed Solution

Changing the default behavior is likely not an option since this would break pretty much all existing applications. However it may be an option to add something like an extract_not_found method to extract the NotFound error out of the SdkError and move it to an inner Result. Here is a prototype implementation I have written for the S3 head_object API:

trait GetNotFound{
    ///
    /// Retrieves the `NotFound` error if this error is of the `NotFound` variant, returns `None`
    /// for other errors.
    ///
    fn get_not_found(&self) -> Option<aws_sdk_s3::types::error::NotFound>;
}

impl GetNotFound for aws_sdk_s3::operation::head_object::HeadObjectError{
    fn get_not_found(&self) -> Option<aws_sdk_s3::types::error::NotFound>{
        if let aws_sdk_s3::operation::head_object::HeadObjectError::NotFound(not_found) = &self{
            Some(not_found.clone())
        } else{
            None
        }
    }
}

trait ExtractNotFound<OkType, ErrorType>{
    ///
    /// Take the `NotFound` error out of the nested error into an inner result which can be evaluated
    /// after `unwrap`ing outer errors such as connectivity issues or server errors.
    ///
    fn extract_not_found(self) -> std::result::Result<std::result::Result<OkType, aws_sdk_s3::types::error::NotFound>, aws_sdk_s3::error::SdkError<ErrorType>>;
}

impl<OkType, ErrorType> ExtractNotFound<OkType, ErrorType> for std::result::Result<
    OkType,
    aws_sdk_s3::error::SdkError<ErrorType>,
>
where ErrorType: GetNotFound
{
    fn extract_not_found(self) -> std::result::Result<std::result::Result<OkType, aws_sdk_s3::types::error::NotFound>, aws_sdk_s3::error::SdkError<ErrorType>>{
        match self{
            Ok(x) => {
                Ok(Ok(x))
            },
            Err(sdk_error) => {
                if let aws_sdk_s3::error::SdkError::ServiceError(service_error) = sdk_error{
                    if let Some(not_found) = service_error.err().get_not_found(){
                        Ok(Err(not_found))
                    } else{
                        Err(aws_sdk_s3::error::SdkError::ServiceError(service_error))
                    }
                } else{
                    Err(sdk_error)
                }
            }
        }
    }
}

The ExtractNotFound implementation is pretty generic and could therefore directly be used for a large range of APIs. It will however require a type-specific implementation of the GetNotFound trait for all the different auto-generated error types for the different API requests, so integrating this will likely require a change to the code generator.

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
@jakoblell jakoblell added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Dec 15, 2023
@rcoh
Copy link
Contributor

rcoh commented Dec 16, 2023

Have you tried err.into_service_error().is_not_found()? Fairly concise option. You might also find it convenient to use the conversion into aws_sdk_s3::error::Error

@jakoblell
Copy link
Author

Have you tried err.into_service_error().is_not_found()? Fairly concise option. You might also find it convenient to use the conversion into aws_sdk_s3::error::Error

Things get a little bit more complicated than calling err.into_service_error().is_not_found() if you want to pass up the error, into_service_error consumes the err so you need to store it in a temporary variable so that you can pass on the error if is_not_found() returns false. Also this solution comes with the drawback that it will convert other errors to a HeadObjectError::Unhandled (which is marked as deprecated), so other errors such as timeouts/connectivity issues will get messed up by this (and less easy to handle in upper layers when passed upwards via thiserror).

So I still think the solution proposed in this issue makes the typical pattern of explicitly handling a NotFound error while passing up all other errors via anyhow/thiserror quite a bit easier to implement (and more concise/readable).

@rcoh
Copy link
Contributor

rcoh commented Dec 20, 2023

oh gotcha! Yeah I think something like as_service_error() would generally be very beneficial and minimal code to add. That would allow:

let not_found = err.as_service_error().map(|err|err.is_not_found()).unwrap_or_default()

fn as_service_error(&self) -> Option<&E> { ... }

Thoughts?

@jmklix jmklix added response-requested Waiting on additional info and feedback. Will move to 'closing-soon' in 7 days. p2 This is a standard priority issue and removed needs-triage This issue or PR still needs to be triaged. labels Jan 5, 2024
Copy link

github-actions bot commented Jan 5, 2024

Greetings! It looks like this issue hasn’t been active in longer than a week. We encourage you to check if this is still an issue in the latest release. Because it has been longer than a week since the last update on this, and in the absence of more information, we will be closing this issue soon. If you find that this is still a problem, please feel free to provide a comment or add an upvote to prevent automatic closure, or if the issue is already closed, please feel free to open a new one.

@github-actions github-actions bot added closing-soon This issue will automatically close in 4 days unless further comments are made. closed-for-staleness and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Jan 5, 2024
@github-actions github-actions bot closed this as completed Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-for-staleness feature-request A feature should be added or improved. p2 This is a standard priority issue response-requested Waiting on additional info and feedback. Will move to 'closing-soon' in 7 days.
Projects
None yet
Development

No branches or pull requests

3 participants