Fix: Gateway wrong error description for copy object #7526
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
N-o-Z
added
bug
E2E Test Results - DynamoDB Local - Local Block Adapter |
itaiad200
requested changes
Mar 3, 2024
| @@ -31,12 +31,12 @@ func (controller *DeleteObject) HandleAbortMultipartUpload(w http.ResponseWriter | |||
| mpu, err := o.MultipartTracker.Get(ctx, uploadID) | |||
| if err != nil { | |||
| o.Log(req).WithError(err).Error("upload id not found in tracker") | |||
| _ = o.EncodeError(w, req, err, gatewayerrors.Codes.ToAPIErr(gatewayerrors.ErrNoSuchKey)) | |||
| _ = o.EncodeError(w, req, err, gatewayerrors.Codes.ToAPIErr(gatewayerrors.ErrNoSuchKey, nil)) | |||
There was a problem hiding this comment.
Any reason not to pass the actual error instead of nil to all of these calls?
There was a problem hiding this comment.
We decided to limit the scope of the current fix to the CopyObject API.
We need to consider a more holistic approach for the actual solution that might require a deeper fix
pkg/gateway/errors/errors.go
Outdated
| @@ -176,20 +176,24 @@ const ( | |||
|
|
|||
| type errorCodeMap map[APIErrorCode]APIError | |||
|
|
|||
| func (e errorCodeMap) ToAPIErr(errCode APIErrorCode) APIError { | |||
| func (e errorCodeMap) ToAPIErr(errCode APIErrorCode, err error) APIError { | |||
There was a problem hiding this comment.
Maybe add a function
func (e errorCodeMap) ToAPIErrWithInternalError(errCode APIErrorCode, err error) APIError {
It could leverage the existing ToAPIErr func.
That way you won't have dozens of calls to ToAPIErr with nil as the err, although an actual error does exist. I also wonder what the next caller to this funciton would do, pass an err or nil?
esti/s3_gateway_test.go
Outdated
Comment on lines
546
to
547
| //destRepo := createRepositoryByName(ctx, t, destRepoName) | ||
| //defer deleteRepositoryIfAskedTo(ctx, destRepoName) |
guy-har
approved these changes
Mar 5, 2024
esti/s3_gateway_test.go
Outdated
| @@ -2,6 +2,7 @@ package esti | |||
|
|
|||
| import ( | |||
| "bytes" | |||
| "github.com/treeverse/lakefs/pkg/block" | |||
itaiad200
approved these changes
Mar 5, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #7452
Partial fix for the issue.
Fix error description for copy object errors in S3 GW