Request: expose the s3err package for more robust error comparison.

[Bjohnson131]

Is your feature request related to a problem? Please describe. Currently the only way to compare errors in the s3 package is to compare strings. This is slow, resource intensive, and indeterminate at best. I'd like to see the s3err package be removed from the /internal/ code so that go developers can more easily compare, and test for errors.

Describe the solution you'd like move aws-sdk-go/internal/s3shared/s3err out of internal, for example, to aws-sdk-go/service/s3/s3err, which will expose error types for developers to typeassert to, as well as organize more properly.

Describe alternatives you've considered It's hard to consider alternatives, as without reflection or string comparison, there's not many options currently.

Additional context Not much to say here, I just want to be able to implement proper, robust error handling with my code that uses AWS SDK

[Bjohnson131]

The PR https://github.com/aws/aws-sdk-go/pull/3916 is an example of the changes that I suggest.

[KaibaLopez]

Hi @Bjohnson131 , Thanks for the PR, but before that, and just so we have a better idea of the situation, can you give us an example on how you're trying to compare errors?

[Bjohnson131]

@KaibaLopez here is a sample of what I'm doing to compare errors currently:

if err != nil && strings.HasPrefix(err.Error(), "404") {
      // do something
}

or

myRegex := regexp.MustCompile(`myregex`)
if !myRegex.MatchString(err.Error()) {
	return nil, err
}

[Bjohnson131]

bumping. what are the thoughts on this? it would help a lot of Go developers to write more efficient code.

[github-actions[bot]]

We have noticed this issue has not received attention in 1 year. We will close this issue for now. If you think this is in error, please feel free to comment and reopen the issue.

[Bjohnson131]

Hello, This issue is still relevant.

[lucix-aws]

@Bjohnson131 --

Can you clarify what functionality within this internal s3err package you need? In general, you can handle errors in the v1 SDK in a more explicit manner than as demonstrated in the code snippets you've provided - see https://docs.aws.amazon.com/sdk-for-go/v1/developer-guide/handling-errors.html for a more detailed guide (with some examples for s3).

[Bjohnson131]

Hey @lucix-aws thanks for getting back to me. 😄 Happy to see this is getting some attention.

The answer is that it is not possible for every error, for example s3err (https://github.com/aws/aws-sdk-go/tree/main/internal/s3shared/s3err) to perform type assertion, as it's in the internal directory, and not accessible without vendoring the SDK.

[RanVaknin]

Hi @Bjohnson131 ,

Can you please provide some basic repro code that can raise the specific error you are trying to error handle on? Im having a hard time understanding what piece in that internal package is helpful to you when error handling.

Thanks, Ran~

[lucix-aws]

I'm only inferring what you're trying to do based on the samples you've provided, but I think there's some confusion about how the types are exposed here.

All errors returned when calling an AWS service should implement awserr.RequestFailure (and beneath that, they should all implement awserr.Error as described in the above error handling guide). If you assert into this type, you can access some additional metadata, such as the underlying HTTP status code, which is preferable to the sample you provided above:

// your example
if err != nil && strings.HasPrefix(err.Error(), "404") {
    // do something
}

// using awserr.RequestFailure
if requestErr, ok := && err.(awserr.RequestFailure); ok && requestErr.StatusCode() == http.StatusNotFound {
    // ...
}

As an aside, I would not consider the value of a native golang error's Error() returned by us to be stable. Pursuant to that fact, if you get an error returned whose code (err.(awserr.Error).Code()) is not a recognized error code for that service, do NOT attempt to branch based on inspection of the contents of the error message, whether that be through retrieving the underlying Error() string, or the message returned by the service in err.(awserr.Error).Message(). The value here could change without warning - we could either change the format of the string we report in Error(), or the service could even change the contents of the message they return. The values here are really only usable to be logged or captured for debugging purposes.

Now, for s3 specifically, there's an additional type -- s3.RequestFailure, which is simply a wrapper that embeds awserr.RequestFailure and includes an additional piece of information - the "host ID" - returned by s3 responses. If you do happen to need that value, you can access it through the appropriate type assertion, otherwise, the base awserr.RequestFailure will suffice.

The code you've come across (and the resident type s3err.RequestFailure) is nothing more than some internal shared logic (shared between s3 and s3control, in this case) to facilitate unmarshaling that host ID value from the raw HTTP response. It's not exposing any information about the underlying error that you otherwise don't have access to.

Given that there's no internalized information in this package, and that the "externalization" as PR'd would introduce a cross-service dependency (s3control would suddenly depend on s3, service packages should have no sibling relationship), we're not inclined to take this change.

[github-actions[bot]]

⚠️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.

[Bjohnson131]

@lucix-aws perhaps given the dependencies of both packages, s3error should go along awserr.go.

This would not introduce any additional dependencies. What do you think?

[lucix-aws]

The point about package dependencies was more meant as an aside- in the context of this specific issue, module hierarchy is really neither here nor there.

The fact of the matter is that the logic contained within the s3err package is internal by design, and as stated in previous discussion, the error type in the package does NOT contain any information that isn't accessible elsewhere, i.e. its internal status isn't preventing any calling code from doing something that it could otherwise were it to be made public. Therefore, there's simply no compelling reason to make this change.

[Bjohnson131]

@lucix-aws it's not the information inside that's useful, it's the type assertion that's useful in this case, and that is the compelling reason for this change, is that we're missing a way to assert that any error in our code is an s3err.