-
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
Add update object metadata API #8253
Conversation
Hidden - this is an experimental feature.
♻️ PR Preview 9a17670 has been successfully destroyed since this PR has been closed. 🤖 By surge-preview |
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.
Looks great to me! Just wondering on 201 successful response for a PUT request
schema: | ||
$ref: "#/components/schemas/UpdateObjectUserMetadata" | ||
responses: | ||
201: |
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.
Is 201 Created customary for a PUT that updates an existing resource? (not asking that you'd change it, legitimately asking!)
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.
MDN says we're good:
If the target resource does have a current representation and that representation is successfully modified with the state in the request, the origin server must send either a 200 OK or a 204 No Content to indicate successful completion of the request
Here we're always on the modification flow, you cannot PUT this is the object doesn't exist.
|
||
var errExpectedKV = errors.New("expected <key>=<value>") | ||
|
||
var fsUpdateUserMetadataCmd = &cobra.Command{ |
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.
Going above and beyond!
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.
Easiest way to develop the code:-)
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.
Thanks!
@N-o-Z ... can you review or should I get someone else on this?
schema: | ||
$ref: "#/components/schemas/UpdateObjectUserMetadata" | ||
responses: | ||
201: |
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.
MDN says we're good:
If the target resource does have a current representation and that representation is successfully modified with the state in the request, the origin server must send either a 200 OK or a 204 No Content to indicate successful completion of the request
Here we're always on the modification flow, you cannot PUT this is the object doesn't exist.
|
||
var errExpectedKV = errors.New("expected <key>=<value>") | ||
|
||
var fsUpdateUserMetadataCmd = &cobra.Command{ |
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.
Easiest way to develop the code:-)
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.
LGTM
Thanks!
api/swagger.yml
Outdated
- objects | ||
- experimental | ||
operationId: updateObjectUserMetadata | ||
summary: update object metadata |
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.
Maybe we should mention this will override the current metadata
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, thanks!
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.
Thanks, pulling...
api/swagger.yml
Outdated
- objects | ||
- experimental | ||
operationId: updateObjectUserMetadata | ||
summary: update object metadata |
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, thanks!
This is an experimental feature. When reviewing please comment about its usability, but focus mainly on correctness.
The code in the controller re-uses an existing physical address, which is supposedly a no-no. However this case is fine: the physical address is used by the same object being updated. So the only way for UGC to remove it would be to lose these races:
Thread 3 is UGC so it only cleans up objects that were abandoned several hours ago. Our UGC model allows us to lose the object in this (extreme) case - this is not a new lossage.
Fixes #8251.