Add SimpleS3Client::copy by nmuntyanov · Pull Request #1592 · async-aws/aws

nmuntyanov · 2023-10-17T14:59:05Z

SimpleS3Client::copy method added

Method resolve which api it should use by itself (copyObject as atomic or copy by parts)

…simple-client

nmuntyanov · 2023-10-17T15:17:37Z

parent PR https://github.com/async-aws/aws/pull/1590/files

src/Integration/Aws/SimpleS3/src/SimpleS3Client.php

stof · 2023-10-25T14:27:26Z

src/Integration/Aws/SimpleS3/src/SimpleS3Client.php

+            $error = null;
+            foreach ($responses as $idx => $response) {
+                try {
+                    /** @var CopyPartResult $copyPartResult */


this is totally useless. The getter already define its return type properly.

If the static analysis or the IDE does not properly detect the type of $response based on assignments in the array, the acceptable thing would be added @var array<int, UploadPartCopyOutput> $responses on the initialization of the empty array.

here is the same situation,
exception is handled in the same place

My comment was not about the error handling. It was about the @var comment

src/Integration/Aws/SimpleS3/src/SimpleS3Client.php

stof · 2023-10-25T14:29:23Z

src/Integration/Aws/SimpleS3/src/SimpleS3Client.php

+            return;
+        }
+
+        /** @var string $uploadId */


this is useless. getUploadId already define its proper return type.

Hello.
How can I handle this errors
https://github.com/async-aws/aws/actions/runs/6642786472/job/18048337300?pr=1592
without doc blocs?

Well, if the AWS API can indeed return string|null for the upload id, you must actually handle the case of null properly instead of pretending that the type inference is wrong and forcing the type checkers to consider the variable as having the type string.
If the AWS API actually returns null, the code would still be broken at runtime and the type override added there only prevents the type checker for detecting the broken code. It does not make it less broken.

As I understand from AWS DOC (https://docs.aws.amazon.com/AmazonS3/latest/API/API_CreateMultipartUpload.html)
If the action is successful, the service sends back an HTTP 200 response.
Upload Id will be always string, otherwise exception would be thrown

underlying client handle case with uploadId == null and throws an exception.
I don't see why this code should be broken, exception will be thown any way

the solution was taken from here
https://github.com/async-aws/aws/blob/master/src/Integration/Aws/SimpleS3/src/SimpleS3Client.php#L104

@stof any updates on this?

stof · 2023-10-25T16:12:02Z

src/Integration/Aws/SimpleS3/src/SimpleS3Client.php

+            foreach ($responses as $idx => $response) {
+                try {
+                    $copyPartResult = $response->getCopyPartResult();
+                    $parts[] = new CompletedPart(['ETag' => $copyPartResult->getEtag(), 'PartNumber' => $idx]);


The weird thing about the PossiblyNullReference error reported by Psalm here is that https://docs.aws.amazon.com/AmazonS3/latest/API/API_UploadPartCopy.html#API_UploadPartCopy_ResponseElements documents CopyPartResult as required in the output. So it is weird that the operation was generated with a nullable type. Maybe the AWS SDK has metadata that does not match the documentation there.

It seems there are no "required" fields for outputs
https://raw.githubusercontent.com/aws/aws-sdk-php/3.283.11/src/data/s3/2006-03-01/api-2.json

I suggest reporting this to AWS as it looks like a mismatch between the human-readable documentation and the machine-readable data used to generate the SDKs.

https://repost.aws/questions/QU6D3R02ZGSgG5C2-yPYw-qw/aws-php-sdk-api-2-json-is-not-synchronised-with-docs-aws-amazon-com

what should I do?
Add additional checking? Or will wait for AWS fix?
We are waiting for this functionality

@stof any updates on this?

Hi @stof
I wanted to bring to your attention an ongoing issue regarding AWS that was posted approximately 4 months ago. It seems that progress on resolving this matter has been slower than anticipated.

Considering the delay in fixing the manifests, I'd like to propose a potential solution. Would it be feasible to remove the annotations related to this issue from the code and handle the error suppression in the Psalm baseline instead? I've come across a few similar issues in the baseline that could potentially benefit from this approach.

I believe this adjustment could provide a temporary workaround while we await a resolution from the AWS side. However, I'm open to discussing alternative solutions or any concerns you may have regarding this proposal.

Given that AWS has not yet clarified what is right between their online documentation and the metadata used to generate all their official SDKs (and then reused to generate our async-aws SDK), I think we should not blindly assume that the online doc is the right one. Instead, we should handle null values properly as required by static analysis (you're also doing it for the upload id above btw).
Note that we don't have any comparison point in official SDKs that are properly typed (I'm excluding aws-sdk-php from that definition) to see how they handle CopyPartResult being absent in the result:

the TransferManager in the official Java SDK has a copy method but it is not actually implemented: https://github.com/aws/aws-sdk-java-v2/blob/master/services-custom/s3-transfer-manager/src/main/java/software/amazon/awssdk/transfer/s3/S3TransferManager.java#L685-L687

equivalent features in other SDKs (the @aws-sdk/lib-storage of the JS SDK or the TransferManager classes of some other languages) don't have a copy function at all.

melya · 2024-02-14T13:01:19Z

Nice feature to have 👍

nmuntyanov · 2024-08-08T18:58:55Z

ping

jderusse · 2024-08-09T08:09:44Z

src/Integration/Aws/SimpleS3/src/SimpleS3Client.php

+     *   CacheControl?: string,
+     *   Metadata?: array<string, string>,
+     *   PartSize?: positive-int,
+     *   Concurrency?: positive-int,


please use int, this syntax is not a standard, therefore not understood by all IDE.

jderusse · 2024-08-09T08:10:30Z

src/Integration/Aws/SimpleS3/src/SimpleS3Client.php

+        $sourceHead = $this->headObject(['Bucket' => $srcBucket, 'Key' => $srcKey]);
+        $contentLength = (int) $sourceHead->getContentLength();
+        $options['ContentType'] = $sourceHead->getContentType();
+        $concurrency = (int) ($options['Concurrency'] ?? 10);


no need to cast, as it's an int by contract. Let people deal with PHP runtime errors if they misuse the method.

jderusse · 2024-08-09T08:11:45Z

docs/integration/simple-s3.md

 $s3->upload('my-image-bucket', 'photos/cat_2.txt', 'I like this cat');

+// Copy objects between buckets
+$s3->copy('source-bucket', 'source-key', 'destination-bucket', 'destination-key');


Options parameters deserve a small documentation IMHO

jderusse · 2024-08-09T08:13:34Z

src/Integration/Aws/SimpleS3/src/SimpleS3Client.php

+     *   Metadata?: array<string, string>,
+     *   PartSize?: positive-int,
+     *   Concurrency?: positive-int,
+     *   mupThreshold?: positive-int,


For consistency

Suggested change

* mupThreshold?: positive-int,

* MupThreshold?: positive-int,

jderusse · 2024-08-09T08:18:36Z

src/Integration/Aws/SimpleS3/src/SimpleS3Client.php

+        unset($options['PartSize']);
+
+        // If file is less than multipart upload threshold, use normal atomic copy
+        if ($contentLength < $mupThreshold) {


do we really need a parameter for this?
Is it an issue to split the file even if it is smaller than the default 2GB?

jderusse · 2024-08-09T08:24:11Z

src/Integration/Aws/SimpleS3/src/SimpleS3Client.php

+         * files smaller than 64 * 10 000 = 640GB. If you are coping larger files,
+         * please set PartSize to a higher number, like 128, 256 or 512. (Max 4096).
+         */
+        $partSize = ($options['PartSize'] ?? 64) * $megabyte;


If PartSize is not defined, I'd rather default to max(64 * $megabyte , 2 ** ceil(log($contentLength / 10000, 2)));

jderusse · 2024-08-09T08:37:27Z

src/Integration/Aws/SimpleS3/src/SimpleS3Client.php

+                } catch (\Throwable $e) {
+                    $error = $e;
+
+                    break;
+                }


you could move the try/catch outside the loop for more readability, and removing the error variable.

try { foreach ($responses as $idx => $response) { $parts[] = new CompletedPart(['ETag' => $response->getCopyPartResult()->getEtag(), 'PartNumber' => $idx]); } catch (\Throwable $e) { foreach ($responses as $response) { try { $response->cancel(); } catch (\Throwable $e) { continue; } } $this->abortMultipartUpload(AbortMultipartUploadRequest::create(['Bucket' => $destBucket, 'Key' => $destKey, 'UploadId' => $uploadId])); }

jderusse · 2024-08-09T08:52:09Z

src/Integration/Aws/SimpleS3/src/SimpleS3Client.php

+                --$parallelChunks;
+            }
+            $error = null;
+            foreach ($responses as $idx => $response) {


this is not efficient. We define 10 concurents upload, but in fact, we are uploading 10, waiting for the 10 to finish, and uploading 10 more, ...
That means:

we are waiting for the 10th to finish before starting the 9 previous.

if 1 upload takes time to finish, we don't leverage the 9 others.

A Beter implementation would be to use a pool (or buffer), to fill the pool with 10 responses, and once 1 response is over, add a new one before checking the other responses.

Better: leverage async processing, by not waiting for the end of a response, but checking if the response is over, if not check the next one.

while not all part uploaded while pool not full => start new upload while pool full foreach response in pull if response not finished continue else => process response => remove response from pool foreach response in pull => wait for response to finish => process response => remove response from pool

stof · 2025-04-18T15:17:59Z

src/Integration/Aws/SimpleS3/src/SimpleS3Client.php

+        unset($options['PartSize']);
+
+        // If file is less than multipart upload threshold, use normal atomic copy
+        if ($contentLength < $mupThreshold) {


this threshold should not be configurable IMO. upload does not let the caller choose the threshold for switching to a simple upload instead of a multipart one.

stof · 2025-04-18T15:18:43Z

src/Integration/Aws/SimpleS3/src/SimpleS3Client.php

+            $error = null;
+            foreach ($responses as $idx => $response) {
+                try {
+                    /** @var CopyPartResult $copyPartResult */


My comment was not about the error handling. It was about the @var comment

stof · 2025-04-18T15:52:03Z

src/Integration/Aws/SimpleS3/src/SimpleS3Client.php

+            foreach ($responses as $idx => $response) {
+                try {
+                    $copyPartResult = $response->getCopyPartResult();
+                    $parts[] = new CompletedPart(['ETag' => $copyPartResult->getEtag(), 'PartNumber' => $idx]);


Given that AWS has not yet clarified what is right between their online documentation and the metadata used to generate all their official SDKs (and then reused to generate our async-aws SDK), I think we should not blindly assume that the online doc is the right one. Instead, we should handle null values properly as required by static analysis (you're also doing it for the upload id above btw).
Note that we don't have any comparison point in official SDKs that are properly typed (I'm excluding aws-sdk-php from that definition) to see how they handle CopyPartResult being absent in the result:

the TransferManager in the official Java SDK has a copy method but it is not actually implemented: https://github.com/aws/aws-sdk-java-v2/blob/master/services-custom/s3-transfer-manager/src/main/java/software/amazon/awssdk/transfer/s3/S3TransferManager.java#L685-L687

equivalent features in other SDKs (the @aws-sdk/lib-storage of the JS SDK or the TransferManager classes of some other languages) don't have a copy function at all.

nmuntyanov added 4 commits October 17, 2023 15:47

s3Client::uploadPartCopy method added

d8e70c1

SimpleS3Client::copy method added

c66b0f9

Merge branch 'master' of github.com:async-aws/aws into issue-1589/s3-…

d5d8e83

…simple-client

Fix client

7587b8e

nmuntyanov added 4 commits October 18, 2023 12:31

Copy multipart concurrency added

90121f1

phpstan/psalm fix

83ac70a

phpstan/psalm fix

132c58e

phpstan/psalm fix

1575060

nmuntyanov marked this pull request as ready for review October 25, 2023 07:19

stof requested changes Oct 25, 2023

View reviewed changes

stof changed the title ~~Issue 1589/s3 simple client~~ Add SimpleS3Client::copy Oct 25, 2023

stof mentioned this pull request Oct 25, 2023

Multipart copy method #1590

Closed

nmuntyanov added 2 commits October 25, 2023 18:38

Fix doc blocks

fd8f7fb

Require new s3 version

54091e1

stof reviewed Oct 25, 2023

View reviewed changes

nmuntyanov added 7 commits October 30, 2023 18:34

Get Content type and Length from source object

57666cb

Test fix

d67d689

Test fix

91447f5

Psalm fix

6bf1c47

Test fix

6a065bd

Test fix

a350cb5

Code style fix

b1e9178

nmuntyanov requested a review from stof January 19, 2024 07:50

jderusse reviewed Aug 9, 2024

View reviewed changes

stof reviewed Apr 18, 2025

View reviewed changes

	* mupThreshold?: positive-int,
	* MupThreshold?: positive-int,

Uh oh!

Conversation

nmuntyanov commented Oct 17, 2023

Uh oh!

nmuntyanov commented Oct 17, 2023

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!

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!

Choose a reason for hiding this comment

Uh oh!

nmuntyanov Oct 25, 2023 • 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!

nmuntyanov Oct 26, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

melya commented Feb 14, 2024

Uh oh!

nmuntyanov commented Aug 8, 2024

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!

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nmuntyanov Oct 25, 2023 •

edited

Loading

nmuntyanov Oct 26, 2023 •

edited

Loading