Add SimpleS3Client::copy #1592
Add SimpleS3Client::copy #1592
Conversation
| $error = null; | ||
| foreach ($responses as $idx => $response) { | ||
| try { | ||
| /** @var CopyPartResult $copyPartResult */ |
There was a problem hiding this comment.
this is totally useless. The getter already define its return type properly.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
here is the same situation,
exception is handled in the same place
| return; | ||
| } | ||
|
|
||
| /** @var string $uploadId */ |
There was a problem hiding this comment.
this is useless. getUploadId already define its proper return type.
There was a problem hiding this comment.
Hello.
How can I handle this errors
https://github.com/async-aws/aws/actions/runs/6642786472/job/18048337300?pr=1592
without doc blocs?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@stof any updates on this?
| foreach ($responses as $idx => $response) { | ||
| try { | ||
| $copyPartResult = $response->getCopyPartResult(); | ||
| $parts[] = new CompletedPart(['ETag' => $copyPartResult->getEtag(), 'PartNumber' => $idx]); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
There was a problem hiding this comment.
what should I do?
Add additional checking? Or will wait for AWS fix?
We are waiting for this functionality
There was a problem hiding this comment.
@stof any updates on this?
There was a problem hiding this comment.
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.
|
Nice feature to have 👍 |
SimpleS3Client::copy method added
Method resolve which api it should use by itself (copyObject as atomic or copy by parts)