aws / aws-sdk

Landing page for the AWS SDKs on GitHub
https://aws.amazon.com/tools/
Other
72 stars 14 forks source link

S3: `NotFound` error of `HeadObject` isn't handled properly #255

Open harai opened 3 years ago

harai commented 3 years ago

Confirm by changing [ ] to [x] below to ensure that it's a bug:

Describe the bug

    res, err := s3Client.HeadObject(ctx, &s3.HeadObjectInput{Bucket: &s3Bucket, Key: &s3Key})
    if err != nil {
        var notFoundError *type.NotFound
        if errors.As(err, &notFoundError) {
            // not executed
        }
    }

When the specified key doesn't exist, HeadObject returns Not Found. However, the error can't be handled properly using the code above.

Version of AWS SDK for Go?

1.2.0

Version of Go (go version)?

go version go1.15.3 linux/amd64

To Reproduce (observed behavior)

The code described above should be enough.

Expected behavior

errors.As(err, &notFoundError) returns true when the specified key doesn't exist.

skmcgrail commented 3 years ago

The HeadObject operation is special in that you can't use the the modeled NotFound exception type due to the limitations of the HTTP protocol. HTTP HEAD method responses can't contain response bodies per specification. To handle this error specifically in the SDK for this operation you will need to use the ResponseError type from smithy-go instead, and use the HTTPStatusCode method to determine if the error was a 404.

harai commented 3 years ago

Thanks. That inconsistency should be fixed. Until then, detailed documentation about this behavior will help us.

shawnHartsell commented 3 years ago

Thanks. That inconsistency should be fixed. Until then, detailed documentation about this behavior will help us.

I am porting a service over from V1 to V2 and noticed that other function calls return ResponseError from smithy/http such as GetObject and CopyObject. In addition, the exact error type seems to be OperationError. I discovered this by trying to write a switch based type assertion.

This would be a breaking change, but I would consider not leaking types that come from a dependent package. This prevents callers from having to explicitly import the smithy package in their code. It also ties the SDK to a particular version of smithy. If a breaking change was made to the dependent type, all callers would need to upgrade 😢

oakad commented 3 years ago

Smithy error type is not helpful if we need to distinguish between failure types (403 vs 404) and such. The only option right now is to cast to http.ResponseError and go from there.

This behavior may result in very non obvious bugs in cases where GetObject and HeadObject error paths get merged (not an uncommon use case) and thus must be documented in bold letters.

kellertk commented 5 months ago
P126358787