-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat(storage): add folder deletion support to remove API #14671
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
base: main
Are you sure you want to change the base?
Conversation
packages/storage/__tests__/providers/s3/utils/validateRemovePath.test.ts
Outdated
Show resolved
Hide resolved
packages/storage/src/providers/s3/utils/generateDeleteObjectsXml.ts
Outdated
Show resolved
Hide resolved
…omise for remove operation.
4e2d753 to
1849103
Compare
|
Did you also create E2E tests? |
Not yet. But will do in a follow up PR. BTW the existing e2e test already verifying this change is working (for files) and backward compatible. |
| delete: '', | ||
| }).toString(); | ||
|
|
||
| const objects = input |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better use generateDeleteObjectsXml helper function
packages/storage/src/providers/s3/utils/client/s3data/deleteObjects.ts
Outdated
Show resolved
Hide resolved
| }).toString(); | ||
|
|
||
| const objects = input | ||
| .Delete!.Objects?.map(obj => `<Object><Key>${obj.Key}</Key></Object>`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Objects is not optional. the ? is not needed here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also (the optional) VersionId is missing here
| export interface DeleteObjectsCommandInput { | ||
| Bucket: string; | ||
| Delete: { | ||
| Objects: Array<{ Key: string; VersionId?: string }>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alternative to what I mentioned further up, remove the VersionId from the types
| Quiet: false, | ||
| }, | ||
| ExpectedBucketOwner: expectedBucketOwner, | ||
| ContentMD5: await calculateContentMd5(xmlBody), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move this to the serializer
| } | ||
|
|
||
| const batch = listResult.Contents.map(obj => ({ Key: obj.Key! })); | ||
| const xmlBody = generateDeleteObjectsXml(batch, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be done in the serializer
| * Represents an ongoing remove operation with cancellation and state tracking capabilities | ||
| * @template T - The type of the result (RemoveWithPathOutput | RemoveOutput) | ||
| */ | ||
| export interface RemoveOperation<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better update this type to
interface NonPausableTransferTask<T>
extends Omit<TransferTask<T>, 'pause' | 'resume' | 'state'> {
state: Omit<TransferTask<T>['state'], 'PAUSED'>;
}
interface RemoveOperation<T> extends NonPausableTransferTask<T>, Promise<T> {}
| ); | ||
| then: wrappedPromise.then.bind(wrappedPromise), | ||
| catch: wrappedPromise.catch.bind(wrappedPromise), | ||
| finally: wrappedPromise.finally.bind(wrappedPromise), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use Object.assign here, to assign these operation props to the wrappedPromise and then you can drop the then/catch/finally
| logger.debug(`removing object in path "${finalKey}"`); | ||
| } | ||
| ): RemoveOperation<RemoveOutput | RemoveWithPathOutput> { | ||
| const cancellationToken = new CancellationToken(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here an AbortController is better to use as it is built-in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ultimately check if the AbortController can be passed all the way down to the transferHandler/fetchHandler.
| let state: RemoveTaskState = 'IN_PROGRESS'; | ||
|
|
||
| const resultPromise = executeRemove(amplify, input, cancellationToken); | ||
| const wrappedPromise = resultPromise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see if it makes sense to create a factory function for "AbortablePromise / AbortableTask", which consolidates the result/cancel function with promises.
But some tasks might not have pause/resume on them aka "NonPausableTransferTask"
|
|
||
| await deleteObject( | ||
| { | ||
| ...s3Config, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here you can add the abortSignal
Description of changes
This PR adds folder deletion support to the Storage remove API.
Key Features:
Technical Implementation:
deleteObjectsS3 client operation for batch deletiondeleteFolderContentsutility for recursive folder deletion with paginationisPathFolderutility to detect folders vs filesIssue #, if available
Closes #5841
Description of how you validated changes
Checklist
yarn testpassesChecklist for repo maintainers
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.