johannesboyne / gofakes3

A simple fake AWS S3 object storage (used for local test-runs against AWS S3 APIs)
MIT License
361 stars 84 forks source link

add multi delete for versioned buckets, omit delete markers on list operation #69

Closed kucukaslan closed 2 years ago

kucukaslan commented 2 years ago

Ordinary multi delete couldn't be used to permanently delete multiple objects from a versioned bucket. The backend.DeleteMulti ignored the version IDs and requested delete without version IDs which resulted in adding "delete markers" instead of permanent deletion of the objects.

In a versioned bucket, ordinary list/head/get requests shouldn't return the "delete marker"s. Added conditions to eliminate "delete marker"s from response.

kucukaslan commented 2 years ago
  1. A little bit context for multi delete issue quoting from my comment

    • There is was a bug when I try to delete from gofakes3 server using s5cmd rm. Despite using version-id/all-versions flags, the server does not permanently delete the corresponding objects and just adds delete marker to them. Interestingly:
    • this bug does not happen when I use aws s3api delete-object to connect gofakes3 server.
    • this bug does not happen when I use s5cmd rm to connect real AWS S3 server.
    • other subcommands of s5cmd works as expected. I'm currently trying to identify root cause of this bug and to fix it.

      Note It turned out that gofakes3 does not support multidelete for versioned objects.

  2. Listing (getting) an object whose current version is a delete marker:

    If you try to get an object and its current version is a delete marker, Amazon S3 responds with the following:

shabbyrobe commented 2 years ago

This appears to only add support to the memory backend? Do you have plans to add this to the other backends too? Can it even be supported in those?

kucukaslan commented 2 years ago

This appears to only add support to the memory backend? Do you have plans to add this to the other backends too? Can it even be supported in those?

Yes it only supports the memory backend.

I think neither of the changes is relevant (possible to implement) for other backends: 1) Support multi delete for versioned objects:

Other backends don't have versioned objects. So there is no need to specify the version to delete. 2) Hide delete markers from Get/Head/List requests. Other backends don't have versioning, and, so, concept of delete markers. So there is no need (or way) to check for delete markers.

johannesboyne commented 2 years ago

@Kucukaslan sorry, have not been available for 2 weeks! Thanks a lot for your contribution. From my perspective this looks good to go. Agree, it is probably not directly possible to implement elsewhere.

sonarcloud[bot] commented 2 years ago

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
4.9% 4.9% Duplication

codecov-commenter commented 2 years ago

Codecov Report

Merging #69 (d42c943) into master (c3ac35d) will decrease coverage by 0.84%. The diff coverage is 48.78%.

@@            Coverage Diff             @@
##           master      #69      +/-   ##
==========================================
- Coverage   67.45%   66.60%   -0.85%     
==========================================
  Files          28       28              
  Lines        2553     2590      +37     
==========================================
+ Hits         1722     1725       +3     
- Misses        583      613      +30     
- Partials      248      252       +4     
Impacted Files Coverage Δ
backend.go 91.30% <ø> (ø)
backend/s3mem/bucket.go 80.85% <0.00%> (-2.37%) :arrow_down:
gofakes3.go 63.85% <44.44%> (-0.55%) :arrow_down:
backend/s3mem/backend.go 68.24% <57.14%> (-6.02%) :arrow_down:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

kucukaslan commented 2 years ago

It would be awesome, to even get a test case for this. See c45911a#diff-9d57a47136e4e1b766663f750472ffd2714f8f6b3f04cba3ccc82fc18e225f85 how we do this.

Would you be up for it?

Hi, I wrote tests[^how] to cover two cases:

  1. Ordinary delete request places a delete marker and ordinary list does not return that object
  2. It is possible to delete multiple objects at a single request.

[^how]: adapted from your tests.