From c093eb2f990b877f4999e400b7b517dcbe1b2ac7 Mon Sep 17 00:00:00 2001 From: Volker Theile Date: Wed, 22 Nov 2023 18:07:34 +0100 Subject: [PATCH] Deleting an object deletes all following the same name base pattern Fixes: https://github.com/aquarist-labs/s3gw/issues/822 Signed-off-by: Volker Theile (cherry picked from commit c89c6dfaefebf5944edf8d5d91ee008bb87c31bb) Signed-off-by: Volker Theile --- src/backend/api/objects.py | 50 +++-- src/backend/api/types.py | 6 +- .../tests/unit/api/test_api_objects.py | 203 +++++++++++++++++- .../object-datatable-page.component.ts | 2 +- .../shared/services/api/s3-bucket.service.ts | 15 +- 5 files changed, 244 insertions(+), 32 deletions(-) diff --git a/src/backend/api/objects.py b/src/backend/api/objects.py index 9be11820..6791fa1c 100644 --- a/src/backend/api/objects.py +++ b/src/backend/api/objects.py @@ -33,7 +33,6 @@ CommonPrefixTypeDef, CopySourceTypeDef, DeleteMarkerEntryTypeDef, - DeleteObjectOutputTypeDef, DeleteObjectsOutputTypeDef, GetObjectOutputTypeDef, HeadObjectOutputTypeDef, @@ -585,29 +584,42 @@ async def restore_object( @router.delete( "/{bucket}/delete", - response_model=DeletedObject, + response_model=List[DeletedObject], responses=s3gw_client_responses(), ) async def delete_object( conn: S3GWClientDep, bucket: str, params: DeleteObjectRequest -) -> DeletedObject: +) -> List[DeletedObject]: """ See https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/s3/client/delete_object.html https://www.middlewareinventory.com/blog/recover-s3/ """ - async with conn.conn() as s3: - s3_res: DeleteObjectOutputTypeDef = await s3.delete_object( - Bucket=bucket, - Key=params.Key, - VersionId=params.VersionId, - ) - res = DeletedObject( - Key=params.Key, - VersionId=s3_res["VersionId"], - DeleteMarker=s3_res["DeleteMarker"], + + async def collect_objects() -> List[ObjectIdentifierTypeDef]: + api_res: Optional[List[ObjectVersion]] = await list_object_versions( + conn, + bucket, + ListObjectVersionsRequest(Prefix=params.Key, Delimiter=""), ) - return res + obj: ObjectVersion + res_objects: List[ObjectIdentifierTypeDef] = [] + for obj in api_res or []: + # Skip "virtual folders" and objects that do not match + # the given key. + if obj.Type != "OBJECT" or obj.Key != params.Key: + continue + version_id: str = obj.VersionId if obj.VersionId else "" + res_objects.append({"Key": obj.Key, "VersionId": version_id}) + return res_objects + + objects: List[ObjectIdentifierTypeDef] + if params.AllVersions: + objects = await collect_objects() + else: + objects = [{"Key": params.Key, "VersionId": params.VersionId}] + + return await delete_objects(conn, bucket, objects) @router.delete( @@ -628,7 +640,7 @@ async def delete_object_by_prefix( """ async def collect_objects(prefix: str) -> List[ObjectIdentifierTypeDef]: - res: Optional[List[ObjectVersion]] = await list_object_versions( + api_res: Optional[List[ObjectVersion]] = await list_object_versions( conn, bucket, ListObjectVersionsRequest( @@ -637,7 +649,7 @@ async def collect_objects(prefix: str) -> List[ObjectIdentifierTypeDef]: ) obj: ObjectVersion res_objects: List[ObjectIdentifierTypeDef] = [] - for obj in res or []: + for obj in api_res or []: if not (params.AllVersions or (obj.IsLatest and not obj.IsDeleted)): continue if obj.Type == "OBJECT": @@ -660,6 +672,12 @@ async def collect_objects(prefix: str) -> List[ObjectIdentifierTypeDef]: params.Prefix ) + return await delete_objects(conn, bucket, objects) + + +async def delete_objects( + conn: S3GWClientDep, bucket: str, objects: List[ObjectIdentifierTypeDef] +) -> List[DeletedObject]: async with conn.conn() as s3: s3_res: DeleteObjectsOutputTypeDef = await s3.delete_objects( Bucket=bucket, Delete={"Objects": objects} diff --git a/src/backend/api/types.py b/src/backend/api/types.py index c01cdf0c..6fd673b6 100644 --- a/src/backend/api/types.py +++ b/src/backend/api/types.py @@ -164,7 +164,11 @@ class RestoreObjectRequest(ObjectIdentifier): class DeleteObjectRequest(ObjectIdentifier): - pass + AllVersions: bool = Field( + False, + description="If `True`, all versions will be deleted, otherwise " + "only the specified one.", + ) class DeleteObjectByPrefixRequest(BaseModel): diff --git a/src/backend/tests/unit/api/test_api_objects.py b/src/backend/tests/unit/api/test_api_objects.py index 27fcaae8..b1a01fbd 100644 --- a/src/backend/tests/unit/api/test_api_objects.py +++ b/src/backend/tests/unit/api/test_api_objects.py @@ -1199,28 +1199,120 @@ async def test_restore_object_failure( @pytest.mark.anyio -async def test_delete_object( +async def test_delete_object_1( s3_client: S3GWClient, mocker: MockerFixture ) -> None: s3api_mock = S3ApiMock(s3_client, mocker) s3api_mock.patch( - "delete_object", + "delete_objects", + return_value=async_return( + { + "Deleted": [ + { + "Key": "x/y/file1.md", + "VersionId": "gNRRPXBPWNkRP7U3D-swsXuWvMC2kwA", + "DeleteMarker": True, + } + ] + } + ), + ) + + res: List[DeletedObject] = await objects.delete_object( + s3_client, + "test01", + DeleteObjectRequest(Key="x/y/file1.md", AllVersions=False), + ) + assert res[0].Key == "x/y/file1.md" + assert res[0].VersionId == "gNRRPXBPWNkRP7U3D-swsXuWvMC2kwA" + assert res[0].DeleteMarker is True + + +@pytest.mark.anyio +async def test_delete_object_2( + s3_client: S3GWClient, mocker: MockerFixture +) -> None: + s3api_mock = S3ApiMock(s3_client, mocker) + s3api_mock.patch( + "list_object_versions", + return_value=async_return( + { + "IsTruncated": False, + "KeyMarker": "", + "VersionIdMarker": "", + "Versions": [ + { + "ETag": '"8d1c84a7fbe1dbc559c6b5c63fa184fc-1"', + "Size": 26, + "StorageClass": "STANDARD", + "Key": "a/b/test", + "VersionId": "ueWoMAuiaqtPl0KrdF92Q0qHHVK8lqO", + "IsLatest": True, + "LastModified": datetime.datetime( + 2023, 11, 22, 15, 44, 44, 244000 + ), + "Owner": {"DisplayName": "M. Tester", "ID": "testid"}, + }, + { + "ETag": '"4805e085427dee36f8d1dd78c354e6eb-1"', + "Size": 79, + "StorageClass": "STANDARD", + "Key": "a/b/test", + "VersionId": "uqoQZvzUnxzTVwfQmItCkmYdLJrrKKr", + "IsLatest": False, + "LastModified": datetime.datetime( + 2023, 11, 22, 15, 44, 3, 178000 + ), + "Owner": {"DisplayName": "M. Tester", "ID": "testid"}, + }, + { + "ETag": '"327314f477e56f612a636181fecc40f5-1"', + "Size": 45, + "StorageClass": "STANDARD", + "Key": "a/b/test1", + "VersionId": "6Hc5vzd3PJGnBRfJ8F4Z2LYdE2tKtEY", + "IsLatest": True, + "LastModified": datetime.datetime( + 2023, 11, 22, 15, 40, 19, 418000 + ), + "Owner": {"DisplayName": "M. Tester", "ID": "testid"}, + }, + ], + "Name": "test02", + "Prefix": "a/b/test", + "MaxKeys": 1000, + "EncodingType": "url", + } + ), + ) + s3api_mock.patch( + "delete_objects", return_value=async_return( { - "VersionId": "gNRRPXBPWNkRP7U3D-swsXuWvMC2kwA", - "DeleteMarker": True, + "Deleted": [ + { + "Key": "a/b/test", + "VersionId": "ueWoMAuiaqtPl0KrdF92Q0qHHVK8lqO", + "DeleteMarker": True, + "DeleteMarkerVersionId": "ueWoMAuiaqtPl0KrdF92Q0qHHVK8lqO", # noqa: E501 + }, + { + "Key": "a/b/test", + "VersionId": "uqoQZvzUnxzTVwfQmItCkmYdLJrrKKr", + "DeleteMarker": True, + "DeleteMarkerVersionId": "uqoQZvzUnxzTVwfQmItCkmYdLJrrKKr", # noqa: E501 + }, + ], } ), ) - res: DeletedObject = await objects.delete_object( + res: List[DeletedObject] = await objects.delete_object( s3_client, "test01", - DeleteObjectRequest(Key="x/y/file1.md"), + DeleteObjectRequest(Key="a/b/test", AllVersions=True), ) - assert res.Key == "x/y/file1.md" - assert res.VersionId == "gNRRPXBPWNkRP7U3D-swsXuWvMC2kwA" - assert res.DeleteMarker is True + assert len(res) == 2 @pytest.mark.anyio @@ -1439,6 +1531,99 @@ async def test_delete_object_by_prefix_2( ) +@pytest.mark.anyio +async def test_delete_object_by_prefix_3( + s3_client: S3GWClient, mocker: MockerFixture +) -> None: + s3api_mock = S3ApiMock(s3_client, mocker) + s3api_mock.patch( + "list_object_versions", + return_value=async_return( + { + "IsTruncated": False, + "KeyMarker": "", + "VersionIdMarker": "", + "Versions": [ + { + "ETag": '"0d6c947604d695f58a4b844c3c0d4233"', + "Size": 423, + "Key": "file2.txt", + "VersionId": "vXrrVkZNXIbprzSpR4hdGt", + "IsLatest": False, + "LastModified": datetime.datetime( + 2023, + 8, + 7, + 12, + 49, + 30, + 506000, + ), + "Owner": {"DisplayName": "M. Tester", "ID": "testid"}, + }, + { + "ETag": '"d64d9775216d967b6d8d4726d54386ed"', + "Size": 1050, + "Key": "file3.txt", + "VersionId": "HkW2UWxXxASjRRlBhEfEsCL-", + "IsLatest": True, + "LastModified": datetime.datetime( + 2022, + 4, + 6, + 2, + 12, + 34, + 643000, + ), + "Owner": {"DisplayName": "M. Tester", "ID": "testid"}, + }, + ], + "DeleteMarkers": [ + { + "Key": "file2.txt", + "VersionId": "Drdct2tP8dnfNl2E2DYNv", + "IsLatest": True, + "LastModified": datetime.datetime( + 2023, + 8, + 7, + 12, + 49, + 19, + 667000, + ), + "Owner": {"DisplayName": "M. Tester", "ID": "testid"}, + }, + ], + "Name": "test01", + "Prefix": "", + "Delimiter": "/", + "MaxKeys": 1000, + } + ), + ) + s3api_mock.patch( + "delete_objects", + return_value=async_return( + {"Deleted": [{"Key": "file3.txt", "VersionId": ""}]} + ), + ) + + res: List[DeletedObject] = await objects.delete_object_by_prefix( + s3_client, + "test01", + DeleteObjectByPrefixRequest(Prefix="file", AllVersions=False), + ) + assert len(res) == 1 + assert res[0].Key == "file3.txt" + assert res[0].VersionId == "" + s3api_mock.mocked_fn["delete_objects"].assert_called_once_with( + Bucket="test01", + Delete={"Objects": [{"Key": "file3.txt", "VersionId": ""}]}, + ) + + @pytest.mark.anyio async def test_download_object( s3_client: S3GWClient, mocker: MockerFixture diff --git a/src/frontend/src/app/pages/user/object/object-datatable-page/object-datatable-page.component.ts b/src/frontend/src/app/pages/user/object/object-datatable-page/object-datatable-page.component.ts index a1e49bcd..ffb09b80 100644 --- a/src/frontend/src/app/pages/user/object/object-datatable-page/object-datatable-page.component.ts +++ b/src/frontend/src/app/pages/user/object/object-datatable-page/object-datatable-page.component.ts @@ -679,7 +679,7 @@ export class ObjectDatatablePageComponent implements OnInit { break; case 'OBJECT': sources.push( - this.s3BucketService.deleteObjectByPrefix(this.bid, item.Key!, allVersions) + this.s3BucketService.deleteObject(this.bid, item.Key!, undefined, allVersions) ); break; } diff --git a/src/frontend/src/app/shared/services/api/s3-bucket.service.ts b/src/frontend/src/app/shared/services/api/s3-bucket.service.ts index fc63aee3..030b9d47 100644 --- a/src/frontend/src/app/shared/services/api/s3-bucket.service.ts +++ b/src/frontend/src/app/shared/services/api/s3-bucket.service.ts @@ -557,7 +557,10 @@ export class S3BucketService { * @param bucket The name of the bucket. * @param key The object key. * @param versionId The version ID used to reference a specific version - * of the object. + * of the object to be deleted. If not specified, a delete marker is + * created instead. + * @param allVersions If `true`, all versions will be deleted, otherwise + * only the specified one. Defaults to `false`. * @param credentials The AWS credentials to sign requests with. Defaults * to the credentials of the currently logged-in user. */ @@ -566,16 +569,18 @@ export class S3BucketService { bucket: S3BucketName, key: S3ObjectKey, versionId?: S3ObjectVersionId, + allVersions?: boolean, credentials?: Credentials - ): Observable { + ): Observable { credentials = credentials ?? this.authSessionService.getCredentials(); const body: Record = { /* eslint-disable @typescript-eslint/naming-convention */ Key: key, - VersionId: versionId + VersionId: versionId, + AllVersions: allVersions /* eslint-enable @typescript-eslint/naming-convention */ }; - return this.s3gwApiService.delete(`objects/${bucket}/delete`, { + return this.s3gwApiService.delete(`objects/${bucket}/delete`, { body, credentials }); @@ -587,7 +592,7 @@ export class S3BucketService { * @param bucket The name of the bucket. * @param prefix The prefix of the objects to delete. Note, a prefix * like `a/b/` will delete all objects starting with that prefix, - * whereas `a/b` will only delete this specific object. + * whereas `a/b` will delete objects like `a/b`, `a/b1`, `a/baz`. * @param allVersions If `true`, all versions will be deleted, otherwise * only the latest one. Defaults to `false`. * @param credentials The AWS credentials to sign requests with.