feat: s3 transfer manager v2 #3079
Conversation
This is an initial phase for the s3 transfer manager v2, which includes: - Progress Tracker with a default Console Progres Bar. - Dedicated Multipart Download Listener for listen to events specificly to multipart download. - Generic Transfer Listener that will be used in either a multipart upload or a multipart download. The progress tracker is dependant on the Generic Transfer Listener, and when enabled it uses the same parameter to be provided as the progress tracker. This is important because if there is a need for listening to transfer specific events and also track the progress then, a custom implementation must be done that incorporate those two needs together, otherwise one of each other must be used. - Single Object Download - Multipart Objet Download This initial implementation misses the test cases.
- Refactor set a single argument, even when not exists, in the console progress bar. - Add a specific parameter for showing the progress rendering defaulted to STDOUT. - Add test cases for ConsoleProgressBar. - Add test cases for DefaultProgressTracker. - Add test cases for ObjectProgressTracker. - Add test cases for TransferListener.
- Add test cases for multipart download listener.
- Add a trait to the MultipartDownloader implementation to keep the main implementatio cleaner. - Add test cases for multipart downloader, in specific testing part and range get multipart downloader.
Refactor: - Moves opening braces into a new line. - Make requestArgs an optional argument. - Remove unnecessary traits. - Use traditional declarations. Adds: - Download directory feature.
Refactor: - Add a message placeholder for progress status. For example in case of errors. Adds: - Upload feature, missing multipart functionality.
- Add upload directory feature
- Add a dedicated multipart upload implementation - Add transfer progress to multipart upload - Add upload directory with the required options. - Create specific response models for upload, and upload directory. - Add multipart upload test cases. - Fix transfer listener completation eval.
Short namespace from `Aws\S3\Features\S3Transfer` to `Aws\S3\S3Transfer`.
There was a problem hiding this comment.
A few more comments- I think a few from the last round were left addressed also. I'd do another check for function opening braces (needing to be moved to a new line) and new files that are missing a newline at the end. More test classes needed as well but I'm assuming those are on the way
| /** | ||
| * @return Closure|ProgressBarFactory | ||
| */ | ||
| private function defaultProgressBarFactory(): Closure| ProgressBarFactory |
There was a problem hiding this comment.
Does this ever return an instance of ProgressBarFactory?
- Implement progress tracker based on SEP spec. - Add a default progress bar implementation. - Add different progress tracker formats: -- Plain progress format: [|progress_bar|] |percent|% -- Transfer progress format: [|progress_bar|] |percent|% |transferred|/|tobe_transferred| |unit| -- Colored progress format: |object_name|:\n\033|color_code|[|progress_bar|] |percent|% |transferred|/|tobe_transferred| |unit| |message|\033[0m - Add a default single progress tracker implementation. - Add a default multi progress tracker implementation for tracking directory transfers. - Include tests unit just for console progress bar.
- Fixes current test cases for: - MultipartUploader - MultipartDownloader - ProgressTracker
- Remove progress bar color enum since the colors were moved into the specific format that requires them.
TransferListener must be tested from the implementations that extends and use this abstract class.
Add nullable type to listenerNotifier property in the MultipartUploader implementation.
- Tests for MultiProgressTracker - Tests for SingleProgressTracker - Tests for ProgressBarFormat - Tests for TransferProgressSnapshot - Tests for TransferListenerNotifier
- Refactor code to address some styling related feedback. - Add upload and uploadDirectory unit tests.
- Fix MultipartUpload tests by increasing the part size from 1024 to 10240000 so it gets between the allowed part size range 5MB-5GBs. - Rename tobe to to_be in the progress formatting.
- Add download tests - Add download directory tests - Minor naming refactor
src/S3/S3Transfer/Progress/ColoredTransferProgressBarFormat.php
Outdated
Show resolved
Hide resolved
src/S3/S3Transfer/Progress/ColoredTransferProgressBarFormat.php
Outdated
Show resolved
Hide resolved
- Add upload integ tests for: - Single uploads - Multipart uploads - Checksum in single uploads - Checksum in multipart uploads - Add download integ tests for: - Single downloads - Multipart downloads
- Add integ tests for directory uploads - Add integ tests for directory downloads
There was a problem hiding this comment.
Mostly nits, but the most important requests are: adding upload() and download() methods on the uploader and downloader classes that call promise() (similar to the old implementation)and testing the resolvesOutsideTargetDirectory logic.
I would do an audit of hard-coded values that can be moved to classes, line length (max 85 char) and all new files that do not end with a newline.
There was a problem hiding this comment.
Similarly, can we add an upload() method that calls the promise() method?
There was a problem hiding this comment.
What are your thoughts about abstracting some of the shared logic between this and the 🤐 class (when ready) into something like AbstractMultipartUploadManager or something like that
- Move some fixed values out of the methods into consts. - Address a line exceeded 80 chars. - Declare keys used across different implementations as consts.
- Fix keys declaration in TransferListener.php - Make use of DIRECTORY_SEPARATOR const instead of hardcoding `/`
- Some implementations using TransferListener were missing the import statement.
- Add test cases for if a file being download `resolvesOutsideTargetDirectory`.
There was a problem hiding this comment.
What are your thoughts about abstracting some of the shared logic between this and the 🤐 class (when ready) into something like AbstractMultipartUploadManager or something like that
- Improve how the parts are created in multipart uploader so that it looks cleaner. - Create single class tests for PartGetMultipartDownloader and RangeGetMultipartDownloader. - Add tests for TransferListenerNotifier from MultipartUploader and MultipartDownloader implementations.
When a part download fails we trigger downloadFailed so it can be propagated to the listeners, and then we retrhow the exception, however, we also have a global exception catching for if something else fails during a multipart download also gets caught and propagated to the listeners as well, however, this causes the downloadFailed to be called twice. To prevent this we just check if the current snapshot has already a error message present there.
- In MultipartUploader, when a part upload fails, the exception should be thrown, and it was not being to.
| ] | ||
| ]; | ||
| if ($this->containsChecksum($this->createMultipartArgs)) { | ||
| $completeMultipartUploadArgs['ChecksumType'] = 'FULL_OBJECT'; |
There was a problem hiding this comment.
We should have some assertions on the resulting x-amz-checksum-type header ensuring this value is provided. Also, I think we need to set this in the CreateMultipartUpload call as well.
There was a problem hiding this comment.
We should also ensure we set the checksum in the appropriate header (x-amz-checksum-<lowercase_algorithm_name>) when calling CompleteMultipartUpload. This might already be done by middleware, so something to check on. In any case, we'll need to make some assertions on that key and value as well.
| private int $calculatedObjectSize; | ||
|
|
||
| /** @var array */ | ||
| private array $deferFns = []; |
There was a problem hiding this comment.
I used it for making sure some streams when necessary are closed and are not left dangling.
| * | ||
| * @return PromiseInterface | ||
| */ | ||
| public function download( |
There was a problem hiding this comment.
I believe we should have recursive download functionality as well
There was a problem hiding this comment.
Sorry, what do you mean with recursive download? this API method is intended for single file downloads.
There was a problem hiding this comment.
Just wanted to add from the comment I made in the other file about the checksum headers/values— we should have assertions here and also in the unit tests (where they apply).
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.