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

s3 HeadObject still doesn't return types.NotFound error #2084

Closed
sachevl opened this issue Apr 6, 2023 · 3 comments · Fixed by #2139 or 88labs/go-utils#212
Closed

s3 HeadObject still doesn't return types.NotFound error #2084

sachevl opened this issue Apr 6, 2023 · 3 comments · Fixed by #2139 or 88labs/go-utils#212
Assignees
Labels
bug This issue is a bug. p2 This is a standard priority issue pending-release This issue will be fixed by an approved PR that hasn't been released yet.

Comments

@sachevl
Copy link

sachevl commented Apr 6, 2023

Describe the bug

issue #1049 claims to add NotFound as an error for HeadBucket and HeadObject methods. However proper error is not returned still

Expected Behavior

_, err := client.HeadObject(ctx, &s3.HeadObjectInput{
  Bucket: aws.String(bucketName),
  Key:    aws.String(key),
})
if err != nil {
  var nf *types.NotFound
  if errors.As(err, &nf) {   // <---------------- Expected to be true on http 404
  }
}

Current Behavior

Current behavior doesn't cast err to types.NotFound when object doesn't exist

Reproduction Steps

run the code from expected behavior against non existing s3 object

Possible Solution

Fix smithy model. Add unit tests to ensure correct error casting

Additional Information/Context

No response

AWS Go SDK V2 Module Versions Used

the latest

Compiler and Version used

go version go1.20.3 linux/amd64

Operating System and version

AL2

@sachevl sachevl added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Apr 6, 2023
@jmklix jmklix added needs-review This issue or pull request needs review from a core team member. p2 This is a standard priority issue and removed needs-triage This issue or PR still needs to be triaged. labels Apr 10, 2023
@RanVaknin RanVaknin self-assigned this Apr 17, 2023
@RanVaknin RanVaknin added investigating This issue is being investigated and/or work is in progress to resolve the issue. and removed needs-review This issue or pull request needs review from a core team member. labels Apr 17, 2023
@RanVaknin
Copy link
Contributor

Hi @sachevl ,

Thanks for opening the issue. I'm able to confirm this is an issue. This occurs because we parse the error type from the request body. Since HeadObject and HeadBucket requests don't return a body, the SDK should check the headers for the error type, and it doesn't do that yet.

This requires customization. I'll add to the backlog.

Thanks,
Ran~

@RanVaknin RanVaknin added needs-review This issue or pull request needs review from a core team member. and removed investigating This issue is being investigated and/or work is in progress to resolve the issue. labels Apr 19, 2023
@pkwarren
Copy link

pkwarren commented May 9, 2023

I found this works as expected for HeadBucket, not for HeadObject. It appears the deserializer for HeadObject is missing this part:

case strings.EqualFold("NotFound", errorCode):
return awsRestxml_deserializeErrorNotFound(response, errorBody)

https://github.com/aws/aws-sdk-go-v2/blob/a86beba126393fdecee20c8d3fc6ef21064121f5/service/s3/deserializers.go#L6829C2-L6837

I think to preserve backwards compatibility, the SDK would need to continue wrapping the current error in types.NotFound (or the other way around). We're currently working around this issue with the following:

// Unfortunately HeadObject doesn't consistently wrap errors in s3.NotFound
// https://github.com/aws/aws-sdk-go-v2/issues/2084
var apiError smithy.APIError
if errors.As(err, &apiError) && apiError.ErrorCode() == "NotFound" {
	...
}

@aajtodd aajtodd self-assigned this May 25, 2023
@RanVaknin RanVaknin added pending-release This issue will be fixed by an approved PR that hasn't been released yet. and removed needs-review This issue or pull request needs review from a core team member. labels Jun 1, 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
Labels
bug This issue is a bug. p2 This is a standard priority issue pending-release This issue will be fixed by an approved PR that hasn't been released yet.
Projects
None yet
5 participants