-
Notifications
You must be signed in to change notification settings - Fork 360
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
Handle no path for delete objects in gateway #1708
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1708 +/- ##
==========================================
- Coverage 41.64% 41.63% -0.01%
==========================================
Files 168 168
Lines 13316 13318 +2
==========================================
Hits 5545 5545
- Misses 7030 7032 +2
Partials 741 741
Continue to review full report at Codecov.
|
@@ -63,6 +63,11 @@ func (controller *DeleteObjects) Handle(w http.ResponseWriter, req *http.Request | |||
switch { | |||
case errors.Is(err, catalog.ErrNotFound): | |||
lg.Debug("tried to delete a non-existent object") | |||
case errors.Is(err, catalog.ErrRequiredValue) && resolvedPath.Path == "": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is risky: DeleteEntry
will also return ErrRequiredValue
if the repository or branch names are empty, and here you are possibly suppressing this error. Consider defining a new error type for an empty path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alt, you can just check if the path
we pass we parsed is empty and return success after successful get repo/branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nopcoder you mean get repo/branch here or in the cataloger?
If it's here, then it's a wasteful repeatable operation I would like to avoid.
If in the underlying cataloger then I'll need to rely or even modify behaviour for the gateway, which I think is suboptimal.
closes #1706