feat: implement conditional delete feature for s3 objects #13330
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
There was a problem hiding this comment.
Welcome to LocalStack! Thanks for raising your first Pull Request and landing in your contributions. Our team will reach out with any reviews or feedbacks that we have shortly. We recommend joining our Slack Community and share your PR on the #community channel to share your contributions with us. Please make sure you are following our contributing guidelines and our Code of Conduct.
|
I have read the CLA Document and I hereby sign the CLA |
bentsku
added
aws:s3
There was a problem hiding this comment.
Hello @mfurqaan31 and thanks for your contribution!
We have some contribution guidelines that details how a PR should look like and what it should contain. It needs to have tests that are validated against AWS to show how the feature behaves.
In the current state of this PR, the pipeline will fail because we were testing the previous behavior. See all the tests in tests.aws.services.s3.test_s3_api.TestS3DeletePrecondition
Currently, the behavior implemented is not correct: only Directory Buckets support x-amz-if-match-last-modified-time and x-amz-if-match-size, so the behavior that you changed around them is not correct (LocalStack does not support Directory Buckets yet). The link you provided only talks about AWS S3 supporting IfMatch headers.
Conditional writes and deletes come from a concurrency problem (to avoid overwriting files, etc), and this part is not addressed in your PR. We have systems in place in S3 with locks (as in PutObject) to make sure concurrent writes cannot happen.
Let me know if you want to push further in this PR, or if you'll need any help. Thanks again for the contribution!
|
Thanks for the feedback, I just want to confirm my next steps before updating the PR: Add tests to validate the conditional delete behavior against AWS. Update the implementation so that only the If-Match conditional delete is supported for non-directory buckets, while the other preconditions (If-Match-Last-Modified-Time and If-Match-Size) remain unsupported (since directory buckets are not implemented yet). Review and handle any potential concurrency concerns, similar to how PutObject uses locking in S3. Please confirm if this plan aligns with what you expect for this PR. |
|
Yes, those steps look good! This might be a lot of work and a complex feature, so feel free to ping me if there's anything I can help you with. |
|
@bentsku can you please review this. |
|
@mfurqaan31 seems like there are linting issues, could you please run |
3 participants
Motivation
This PR addresses issue #13323, which requests support for conditional delete operations in S3. These conditions allow object deletions to occur only when specific criteria are met.
Changes
--if-match,--if-match-last-modified-time, and--if-match-sizeconditions for S3 delete-object operations.