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

API delete will not return object not found #4886

Merged
merged 3 commits into from
Dec 29, 2022
Merged

Conversation

nopcoder
Copy link
Contributor

@nopcoder nopcoder commented Dec 28, 2022

Change in API delete object

The delete object(s) APIs will no longer return "not found" status code when object doesn't exists (similar to S3 behaviour).

The API will not return NotFound errors in the following cases:

  • when tombstone found on stage
  • when object not found on branch commit

Fix #4798

On the following cases the API will not return NotFound error:
- when tombstone found on stage
- when object not found on branch commit
@nopcoder nopcoder self-assigned this Dec 28, 2022
@nopcoder nopcoder added the area/API Improvements or additions to the API label Dec 28, 2022
@nopcoder nopcoder requested a review from arielshaqed December 28, 2022 10:58
@nopcoder nopcoder added the include-changelog PR description should be included in next release changelog label Dec 28, 2022
@nopcoder nopcoder enabled auto-merge (squash) December 28, 2022 13:08
@nopcoder nopcoder disabled auto-merge December 28, 2022 13:09
Copy link
Contributor

@arielshaqed arielshaqed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
This looks great, but I would like it to go much further! I want us to remove the 404 response entirely. Currently swagger.yml makes me think that I could synchronize multiple workers on 200-vs-404s, but in fact this is not true! 404 is not programmatically usable in the API.

pkg/graveler/graveler.go Outdated Show resolved Hide resolved
@nopcoder
Copy link
Contributor Author

Thanks! This looks great, but I would like it to go much further! I want us to remove the 404 response entirely. Currently swagger.yml makes me think that I could synchronize multiple workers on 200-vs-404s, but in fact this is not true! 404 is not programmatically usable in the API.

We require 404 for repository and branch not found

Co-authored-by: Ariel Shaqed (Scolnicov) <ariels@treeverse.io>
@nopcoder nopcoder requested a review from arielshaqed December 28, 2022 13:27
Copy link
Contributor

@arielshaqed arielshaqed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

I think we can try to improve the API documentation, and made some suggestions.

In any case it's a really useful change: as someone coding against it, an API that precisely describes what it does is really important to me.

@@ -3094,7 +3094,7 @@ paths:
tags:
- objects
operationId: deleteObject
summary: delete object
summary: delete object. Missing objects will not return a NotFound error.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
summary: delete object. Missing objects will not return a NotFound error.
summary: delete object.

I believe that this belongs on the descriptions of 204 and of 404, and in a positive not a negative.

        404:
          description: Repository or branch do not exist.
          $ref: "#/components/responses/NotFound"

(The same for deleteObjects, of course!)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried this one first. The description is part of the definition. Set it also on the status code - the code generators just ignore it.

@@ -3094,7 +3094,7 @@ paths:
tags:
- objects
operationId: deleteObject
summary: delete object
summary: delete object. Missing objects will not return a NotFound error.
responses:
204:
description: object deleted successfully
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
description: object deleted successfully
description: Success: Now there is no object at key. Also returned if the object did not exist in the first place.

@nopcoder nopcoder merged commit 2be610c into master Dec 29, 2022
@nopcoder nopcoder deleted the fix/del-obj-ret branch December 29, 2022 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/API Improvements or additions to the API include-changelog PR description should be included in next release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Graveler's Delete race condition on branch retry can return invalid response
2 participants