Add `DELETE /storage/user-bucket` endpoint by nss10 · Pull Request #66 · uc-cdis/gen3-workflow

nss10 · 2025-02-25T18:46:54Z

Link to JIRA ticket if there is one: MIDRC-945

New Features

Add new endpoint DELETE /storage/user-bucket that lets user delete the bucket along with everything in there`

Bug Fixes

Improvements

Dependency updates

Deployment changes

github-actions · 2025-02-25T18:47:27Z

The style in this PR agrees with black. ✔️

This formatting comment was generated automatically by a script in uc-cdis/wool.

coveralls · 2025-02-25T18:49:40Z

Pull Request Test Coverage Report for Build 14089800485

Details

0 of 0 changed or added relevant lines in 0 files are covered.
5 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.4%) to 84.06%

Files with Coverage Reduction	New Missed Lines	%
aws_utils.py	5	88.89%

Totals
Change from base Build 13315751395:	0.4%
Covered Lines:	501
Relevant Lines:	596

💛 - Coveralls

…alled multiple times

gen3workflow/aws_utils.py

paulineribeyre · 2025-03-06T18:09:40Z

gen3workflow/aws_utils.py

+                f"Deleting all contents from '{user_bucket_name}' for user '{user_id}' before deleting the bucket"
+            )
+            keys = [{"Key": obj.get("Key")} for obj in object_list["Contents"]]
+            s3_client.delete_objects(Bucket=user_bucket_name, Delete={"Objects": keys})


According to the docs this will fail if the list of keys is >1000.
You could delete in batches of 1000 manually, or try something like this (I'd recommend trying that first, it's probably optimized)

Thanks for catching this. I totally missed it. Will add batching to this approach.

your update looks good, i'm curious why not use bucket.objects.all().delete()?

No reason not to use the other approach. Since we were already using the s3_client reference, my initial approach was to maintain uniformity by working within that solution. If it hadn’t worked, I would have considered creating an s3_resource. But since this worked, I didn’t explore the other option further.

gen3workflow/aws_utils.py

gen3workflow/routes/storage.py

tests/test_misc.py

gen3workflow/aws_utils.py

paulineribeyre · 2025-03-13T17:42:20Z

tests/test_misc.py

+        assert (
+            e.value.response.get("ResponseMetadata", {}).get("HTTPStatusCode") == 404
+        ), f"Bucket exists: {e.value}"


i think the ClientError is raised before this assert statement runs?

tests/test_misc.py

paulineribeyre · 2025-03-13T17:45:16Z

tests/test_misc.py

+    # Verify the bucket is deleted
+    with pytest.raises(ClientError) as e:
+        aws_utils.s3_client.head_bucket(Bucket=bucket_name)
+    assert (


same comment here about ClientError vs assert statement

I think this was indented appropriately. No?

yeah sorry, i think i got this wrong, the assert statement can/should be inside of the with block here. I confused it with a try block. Could you revert that? my bad

I don't understand. I think the assert statement must be outside the with block. (reference here)

whichever is fine.

tests/test_misc.py

paulineribeyre · 2025-03-13T17:51:06Z

tests/test_misc.py

+    # Since we are unable to user put_object API due to a probable bug in moto,
+    # we will monkeypatch the list_objects_v2 API to get a list of objects to the bucket
+    # Add 1500 objects to the bucket, to ensure batching is working correctly
+    with patch.object(
+        aws_utils.s3_client, "list_objects_v2", get_dummy_object_list(bucket_name, 1500)


The problem with this approach is that we're not testing that the delete_object call actually works against a bucket with objects, because it's effectively called against an empty bucket. What's the problem with moto?

Also, does s3_client.delete_objects not raise an error if the object doesn't exist?

does s3_client.delete_objects not raise an error if the object doesn't exist?

Weirdly, no! I expected s3_client.delete_objects to raise an error if the object didn’t exist, but Moto doesn’t throw any errors in that case.

I wanted to add tests to verify that the batching logic works correctly without triggering errors. Initially, I considered mocking the delete_objects endpoint to throw an error when the input exceeds 1000 records, but that felt like overfitting, so I decided against it.

What's the problem with moto?

I also couldn’t create objects in the bucket for testing because Moto throws an error when using the putObject API with an encrypted bucket, even when making a signed request. (Relevant Code)

I see. Since in this case we're not testing the encryption, i think we could use an unencrypted bucket so we can test the deletion properly. I think the quick way would be to add a line to delete the bucket policy (and a comment linking to the previous issue so we know it's the same thing). A better way would be to reconfigure the service with disabled KMS for this test, but that sounds like a waste of time

I’m glad you brought this up—I realized that the list_objects_v2 function has a restriction of returning no more than 1,000 files per operation. I’ve added a function to properly fetch and delete them in batches.

Additionally, I’ve removed the bucket_policy enforcing encryption for the bucket in the unit test.

docs/authorization.md

paulineribeyre

Looks good, no new comments. Just a few conversations above we still need to address/finalize

… for list_objects_v2

tests/test_misc.py

gen3workflow/aws_utils.py

paulineribeyre · 2025-03-26T16:11:12Z

gen3workflow/aws_utils.py

+        user_id (str): The user's unique Gen3 ID.
+        user_bucket_name (str): The name of the S3 bucket.
+    """
+    object_list = get_all_bucket_objects(user_bucket_name)


This is fine because this endpoint is only meant to be used in integration tests for now. But in general, it would be better practice to fetch 1000 objects and delete them, and then fetch the next 1000, rather than store in memory the full list of objects and then loop over them in batches of 1000.

If you don't want to spend time making this improvement now, could you add a TODO comment before the call to get_all_bucket_objects so we know to do it if we ever need to use this endpoint in production?

Thanks! Initially, I considered making the get_all_bucket_objects function a generator that accepts a batch_size parameter (capped at 1,000) and yields results one batch at a time. This approach would avoid the batching logic we’re currently applying in the delete function and could also serve as a form of pagination for end users if needed.

However, given the narrow scope of this use case right now, it felt somewhat over-engineered. If you anticipate us using this functionality in the near future, I’d be happy to allocate time to implement it. Otherwise, I can leave a TODO as you suggested and probably we can backlog a ticket to revisit this later.

yea a comment is fine

gen3workflow/aws_utils.py

nss10 and others added 2 commits February 25, 2025 12:45

Add DELETE /storage/user-bucket endpoint

920fbda

Apply automatic documentation changes

bc55543

Add exception handling and 404 instead of 500 when bucket delete is c…

7f9fc27

…alled multiple times

paulineribeyre reviewed Feb 25, 2025

View reviewed changes

gen3workflow/aws_utils.py Outdated Show resolved Hide resolved

Fix: Breaking logic 'str' | None

2c7c687

nss10 requested a review from paulineribeyre February 25, 2025 23:00

nss10 marked this pull request as ready for review February 25, 2025 23:00

nss10 removed the request for review from paulineribeyre February 26, 2025 00:08

nss10 marked this pull request as draft February 26, 2025 00:08

Add unit tests to DELETE /storage/user-bucket endpoint

58a7a0b

nss10 marked this pull request as ready for review March 6, 2025 15:38

nss10 requested a review from paulineribeyre March 6, 2025 15:38

paulineribeyre requested changes Mar 6, 2025

View reviewed changes

Respond to PR comments

bacf71f

nss10 requested a review from paulineribeyre March 10, 2025 18:27

paulineribeyre reviewed Mar 13, 2025

View reviewed changes

gen3workflow/aws_utils.py Outdated Show resolved Hide resolved

paulineribeyre reviewed Mar 13, 2025

View reviewed changes

gen3workflow/aws_utils.py Outdated Show resolved Hide resolved

paulineribeyre reviewed Mar 13, 2025

View reviewed changes

nss10 added 2 commits March 13, 2025 20:47

Respond to PR comments

20807ec

Fix typo!

1032353

nss10 requested a review from paulineribeyre March 14, 2025 01:53

paulineribeyre reviewed Mar 20, 2025

View reviewed changes

docs/authorization.md Outdated Show resolved Hide resolved

docs/authorization.md Outdated Show resolved Hide resolved

docs/authorization.md Outdated Show resolved Hide resolved

Fix documentation

75610bc

nss10 requested a review from paulineribeyre March 23, 2025 08:23

paulineribeyre reviewed Mar 24, 2025

View reviewed changes

Remove bucket encryption for delete unit test. Add batched retrievals…

14784e9

… for list_objects_v2

nss10 requested a review from paulineribeyre March 24, 2025 21:39

paulineribeyre reviewed Mar 26, 2025

View reviewed changes

Fix typos and add neccessary comments

d077f7d

paulineribeyre reviewed Mar 26, 2025

View reviewed changes

gen3workflow/aws_utils.py Outdated Show resolved Hide resolved

Add TODO comments and change log level for few lines

068786c

nss10 requested a review from paulineribeyre March 26, 2025 19:07

paulineribeyre approved these changes Mar 26, 2025

View reviewed changes

nss10 merged commit 8de8a62 into master Mar 26, 2025
8 checks passed

nss10 deleted the chore/delete_storage_bucket branch July 21, 2025 21:18

Conversation

nss10 commented Feb 25, 2025 • edited by atlassian bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

New Features

Bug Fixes

Improvements

Dependency updates

Deployment changes

Uh oh!

github-actions bot commented Feb 25, 2025

Uh oh!

coveralls commented Feb 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 14089800485

Details

💛 - Coveralls

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nss10 Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

paulineribeyre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

nss10 commented Feb 25, 2025 •

edited by atlassian bot

Loading

coveralls commented Feb 25, 2025 •

edited

Loading

nss10 Mar 10, 2025 •

edited

Loading