Add filePath Info in MultipartUploadException by cjyclaire · Pull Request #1140 · aws/aws-sdk-php

cjyclaire · 2016-12-13T00:37:30Z

Addressing issue #1015
Making Bucket/Key information accessible from MultipartUploadException instead of introducing complexity into S3Transfer. This introduces a more user-friendly way in getting file path information instead of parsing objectUrl in the error message.

@mtdowling

mtdowling

I like where this is headed. I think there's some more work to do in figuring out how to not couple this generic exception to S3 since it's used in both Glacier and S3.

mtdowling · 2016-12-14T18:38:05Z

+    /**
+     * Get file path of the transfer
+     *
+     * @return array


Shouldn't this be string|null?

mtdowling · 2016-12-14T18:38:17Z

+     *
+     * @return array
+     */
+    public function getFilePaths()


Singular, right? getFilePath

mtdowling · 2016-12-14T18:40:14Z

                $msg .= "- Part {$part}: " . $error->getMessage(). "\n";
+                $this->collectPathInfo($error->getCommand());
            }
        } elseif ($prev instanceof AwsException) {


What happens if this if/else branch and the branch above do not evaluate? The filePath value is not set. What exceptions would come through here that aren't an array or AwsException? Any random exception thrown in a promise I guess?

mtdowling · 2016-12-14T18:43:42Z

+    private $filePath;

    /**
     * @param UploadState      $state Upload state at time of the exception.


Side note: It's really weird that $prev might be an array. We should look at this and see if we can make $prev always an exception in a backwards compatible way so it's a bit more predictable.

mtdowling · 2016-12-14T18:46:23Z

+     *
+     * @return array
+     */
+    public function getFilePaths()


Maybe add some docs to state that the filePath may be null if the bucket and key could not be ascertained from the previous exception. Which, by the way... This class is coupling itself to S3 now even though it's meant to be generically used for both S3 and Glacier.

mtdowling · 2016-12-14T18:47:31Z

+     */
+    private function collectPathInfo(CommandInterface $cmd)
+    {
+        if (empty($this->filePath) && isset($cmd['Bucket']) && isset($cmd['Key'])) {


I don't think this will work... This is now tying this class to S3, but it's also used in Glacier: https://github.com/aws/aws-sdk-php/blob/master/src/Glacier/MultipartUploader.php#L6. Maybe the filePath needs to be injected into the exception in the constructor instead of extracted from a previous exception.

This is still coupling the generic MultipartUploadException to S3. Can you refactor this as previously suggested?

mtdowling · 2017-01-04T20:52:24Z

        if ($this->promise) {
            return $this->promise;
        }
+        $exception = $this->client->getApi()->getServiceName() === 's3'


This is coupling concrete exceptions to the abstract class. This isn't a good way to achieve what you want.

Considering this class is marked as internal, why not make this an argument passed in to the constructor instead?

Agreed, I added documentation and changed this to the constructor in the latest commit :)

mtdowling · 2017-01-04T20:53:02Z

 use GuzzleHttp\Psr7;
 use InvalidArgumentException as IAE;
 use Psr\Http\Message\RequestInterface;
+use Aws\S3\Exception\S3MultipartUploadException;


This looks like it isn't needed

mtdowling · 2017-01-04T20:53:37Z

+            $this->collectPathInfo($prev->getCommand());
+        }
+        parent::__construct($state, $prev);
+


Extra new line

mtdowling · 2017-01-04T20:54:18Z

+
+    /**
+     * @param UploadState      $state Upload state at time of the exception.
+     * @param \Exception|array $prev  Exception being thrown.


This seems to be missing information about when and why an array would be provided.

mtdowling · 2017-01-04T20:54:54Z

+    /**
+     * Get file path of S3 transfer
+     *
+     * @return string|null


Please document when or why would this be null.

mtdowling · 2017-01-05T01:29:03Z

     * @param array  $config
     */
-    public function __construct(Client $client, array $config = [])
+    public function __construct(Client $client, array $config = [], $exception = null)


Why not use $config instead? It's a bit awkward to put a scalar argument after an associative array argument. Can you also call it exceptionClass if an argument or exception_class if a config value?

Yeah, I also feel weird about leaving the scalar argument there. I hesitated to put it in config array originally because all options for config is documented publicly, and I'm not that sure whether exception_class parameter is appropriate (or make sense?) to document there.

I moved it to the config class with renaming in the latest commit :)

mtdowling · 2017-01-06T01:05:44Z

+     * @return string|null Returns null when 'Bucket' or 'Key' information
+     *                     are unavailable.
+     */
+    public function getFilePath()


Is there a reason to not just have a getBucket and getKey method instead of returning these as a single concatenated string?

Actually, no. Taking a second thought, providing Bucket and Key is actually more useful, they could be concatenated easily anyways. I changed those to Bucket and Key in the last commit :)

mtdowling · 2017-01-06T17:51:20Z

It looks like from the referenced issue, the customer wants to know which file that was being uploaded failed to upload. It's nice to also provide the bucket and key that we were attempting to upload to, but we should also try to capture the filename of the upload. You might be able to get this from stream metadata.

cjyclaire · 2017-01-06T22:06:51Z

@mtdowling Totally agreed on providing the filename there, and thanks for the suggestion. I made changes in the latest commit.

Yet I kind feel I'm "polluting" the constructor or config array in the way I'm doing right now, since determineSource is private within AbstractUploader, do you have any suggestions to make these better?

mtdowling · 2017-01-06T22:20:58Z

    public function __construct(Client $client, $source, array $config = [])
    {
        $this->source = $this->determineSource($source);
+        $config['exception_params']['file_name'] = $this->source->getMetadata('uri');


I'd just push this responsibility off onto instances of MultipartUploadException. Have them pull it out of the $prev array just like how they're pulling key and bucket out.

$prev should always be full of UploadPart operations which, if I recall correctly, have a Body parameter that must be a stream. You could use that if present, pull out the metadata, and populate the filename.

That's correct, and makes perfect sense to me, thanks Michael. I fixed it and did renaming in the latest commit.

I have also done some manually testing with "real" Exceptions and checked this work as expected :)

mtdowling · 2017-01-06T22:23:16Z

+    }
+
+    /**
+     * Get the filename of the transfer object


Maybe call this getSourceFilename to better clarify that this is the filename of the data that was being uploaded.

cjyclaire · 2017-02-13T19:32:34Z

@mtdowling I have updated this PR with S3 namespace exception, it will be great if you could have another look at it when you have time : )

jeskew · 2017-02-13T19:47:24Z

        parent::__construct($client, $source, array_change_key_case($config) + [
            'bucket' => null,
            'key'    => null,
+            'exception_class' => 'Aws\S3\Exception\S3MultipartUploadException'


Should be S3MultipartUploadException::class

Maybe my naming here is kind of confusing? What I'm trying to use the class namespace directly like this, using S3MultipartUploadException::class doesn't seem to work.

Maybe you are suggesting better way that I did get? Could you explain a bit about it? Or perhaps I can do better naming for the parameter there? : )

If you import the exception with a use statement, ::class should just expand the classname into a fully-qualified one. Example

Good to know, thanks! I'll fix it

jeskew · 2017-02-13T19:47:33Z

        parent::__construct($client, $source, $config + [
            'bucket' => null,
            'key'    => null,
+            'exception_class' => 'Aws\S3\Exception\S3MultipartUploadException'


Should be S3MultipartUploadException::class

jeskew · 2017-02-17T17:10:53Z

+    public function __construct(UploadState $state, $prev = null) {
+        if (is_array($prev)) {
+            foreach ($prev as $part => $error) {
+                $this->collectPathInfo($error->getCommand());


$this->collectPathInfo overwrites the $bucket, $key, and $filename each time it is called. Since exceptions encountered when performing a multipart upload would always pertain to the same bucket and key, I don't think this is a bug so much as it is an inefficiency.

Would it make sense just have a compound condition for the if statement that ensures the array is not empty and collects path info from either the head or the tail of that array, whichever represents the most recently encountered error? E.g.:

if (is_array($prev) && $error = array_pop($prev) { $this->collectPathInfo($error->getCommand); }

Make sense, I used to make it exactly like MultipartException scenario, taking a second look, it does provides enough info. in this way. I'll fix that : )

@jeskew Actually I thought about the this behavior inefficiency there, so I did empty check for those values in case I overwrote them. Though this still doesn't avoid unnecessary iterates.

Taking a look at array_pop(), it will remove the element from prev, which will leave incomplete messaging. Looks like php doesn't have something like array_peek? I didn't find a clean way to do peek(). Thoughts? : )

Ah, I missed that $prev gets passed on to the parent constructor. You could use each($prev)[1].

Awesome, thanks! I'll fix this

cjyclaire · 2017-02-17T21:37:44Z

@jeskew All fixed : ) Ready for another review. Online PHP editor is fun : )

jeskew

LGTM!

Please make sure to squash before merging.

* Fix CI Warning caused by PHPUnit Api deprecation (aws#1037) * When PHP Fatal Error is not surfaced, disable retry on certain errors (aws#1053) * When PHP Fatal Error is not surfaced, disable retry on certain errors * add error class check * use Throwable * update models for next release * 3.18.34 release * Update doc * update models for next release * 3.18.35 release * Disable autoload when checking class existence (aws#1056) * Disable autoload when checking class existence * remove unnecessary check * update models for next release * 3.18.36 release * update models for next release * 3.18.37 release * update models for next release * 3.18.38 release * Added the DynamoDB Marshaler argument mapAsObject to unmarshalItem method (aws#1067) * [Doc-Only]Add Error code handling documentation and split project information (aws#1062) * Add Error code handling documentation and split project information * Add standalone feature * fix upcase * Adding support for S3 IPv6 (aws#1068) * Add support for dual stack endpoint * enhance test, remove parameter check * Path style host * update models for next release * 3.18.39 release * update models for next release * 3.19.0 release * Fix ElasticLoadBalancingV2 endpoints resolving issue (aws#1072) * Fix ElasticLoadBalancingV2 endpoints resolving issue * move to constructor * Fix documentation * update models for next release * 3.19.1 release * update models for next release * 3.19.2 release * Update model for next release * 3.19.3 release * update models for next release * 3.19.4 release * 3.19.5 release * update models for next release * 3.19.6 release * update models for next release * 3.19.7 release * 3.19.8 release * Removed undefined namespace (aws#1087) * update models for next release * 3.19.9 release * update models for next release * 3.19.10 release * Properly set security token for upload (aws#1093) * Adding a rejected promise handler to determineBucketRegionAsync (aws#1075) * Adding a rejected promise handler to determineBucketRegionAsync * add another test case * update models for next release * 3.19.11 release * update models for next release * 3.19.12 release * Fix duplicate EC2 in Doc quicklink (aws#1095) * update models for next release * 3.19.13 release * Add support for s3-accelerate.dualstack Endpoint (aws#1103) * Adding support for s3-accelerate.dualstack endpoint * Add S3EndpointMiddleware instead * Add pattern const with number & renaming * fix indentation * update models for next release * 3.19.13 release * fix tag * 3.19.14 release * update model for next release * 3.19.15 release * update model for next release * 3.19.16 release * update model for next release * 3.19.17 release * update model for next release * 3.19.18 release * removesdb in endpoint * update model for next release * 3.19.19 release * update model for next release * 3.19.20 release * update model for next release * 3.19.21 release * Remove (optional) in documentation (aws#1108) * update model for next release * 3.19.22 release * update model for next release * 3.19.23 release * 3.19.24 release * update model for next release * 3.19.25 release * update model for next release * 3.19.26 release * update model for next release * 3.19.27 release * update model for next release * 3.19.28 release * update model for next release * 3.19.29 release * update model for next release * 3.19.30 release * update model for next release * 3.19.31 release * Fix PHPUnit Mock object failing (5.6, 7.0) (aws#1123) * Fix PHPUnit Mock object failing (5.6, 7.0) * Remove mock * clean up * update model for next release * 3.19.32 release * Fix DynamoDB session handler sleep time (aws#1122) Between attempts to access a locked session, the handler is meant to wait for a configurable number of microseconds. The handler code was previously multiplying the wait time by 10^-6 before passing it to usleep, even though the values were already in microseconds. For the default min and max wait time values of 10000 and 50000, the usleep argument would always be less than one and so the handler wouldn't wait at all between attempts. This commit removes the multiplication by 10^-6, so the handler sleeps for the intended amount of time between requests. * Fix ECS credential throws exception outside of a RejectedPromise (aws#1126) * change us-standard to us-east-1 (aws#1127) * update model for next release * 3.19.33 release * update model for next release * 3.20.0 release * update model for next release * 3.20.1 release * Fix PostObjectV4 ignoring customized virtual-style endpoint (aws#1131) * Fix PostObjectV4 ignoring customized virtual-style endpoint * checking using prefix * test bucket contains dot * fix prefix * update model for next release * 3.20.2 release * update model for next release * 3.20.3 release * update model for next release * 3.20.4 release * Add remote credentials provider (aws#1133) * Add remote credentials provider * Always chain instance profile in default credential provider * Fix tests * Add Doc cross linking support (aws#1139) * Add doc cross linking support, generating rewrite file * fix html * fix name * update model for next release * 3.20.5 release * Blacklist 'x-amzn-trace-id' header (aws#1141) * update model for next release * 3.20.6 release * update model for next release * 3.20.7 release * Adding new release changes * 3.20.8 release * 3.20.9 release * Update CHANGELOG.md * Update CHANGELOG.md * 3.20.10 release * 3.20.11 release * Update CHANGELOG.md * 3.20.12 release * Fixed typo (aws#1147) * 3.20.13 release * Fix sed to work on GNU ``` sed -i '' -e "s/VERSION = '.*'/VERSION = '3.20.15'/" src/Sdk.php ``` The above command would fail on GNU. The update works fine on both GNU and OSX 10.9+ Reference: - http://www.grymoire.com/Unix/Sed.html#uh-62h - http://stackoverflow.com/questions/4247068/sed-command-with-i-option-failing-on-mac-but-works-on-linux * 3.20.14 release * 3.20.14 release * 3.20.15 release * Fix manifest * Update Changelog * 3.20.16 release * Fix AWS Documentation files * Update Example Json * Adds support for AssumeRole and ./aws/config ini credentials (aws#1137) * Add shared config * fix php5.5 * Camel case, renaming, formatting fix etc. * Add AssumeRoleResolver * seperate from default chain, and documentation and small fix etc. * clean up * rebase pre * fix typo * Add support for assume role profile and ./aws/config * Fix doc * fix format * Remove trait file * fix indent * Add doc * Fix indent * require client parameter * Fix documentation * Fix exception * Skip invalid example snippets instead of break the doc * clean up * update model for next release * 3.21.0 release * update models for release * 3.21.1 release * update model for release * 3.21.2 release * update models for release * 3.21.3 release * Update models for release * 3.21.4 release * Update models for release * 3.21.5 release * Link docs for S3ClientTrait methods to the S3ClientInterface methods they implement * Update models for release * 3.21.6 release * Update Readme Update Readme.md with instructions on getting help and opening issues * Update README.md * Delete copy-model.sh * Update models for release * 3.22.0 release * Fixed typo in docblock comment * Update models for release * 3.22.1 release * Update models for release * 3.22.2 release * Fix Marshaler leaving dangling references (aws#1168) Some PHP versions will create references to keys instead of copying them after doing a foreach-by-reference, which can cause unexpected state mutations. Remove those foreach-by-reference calls to prevent that from happening * Update models for release * 3.22.3 release * Update models for release * 3.22.4 release * Update models for release * 3.22.5 release * Automate changelog (aws#1169) * Script to automate Release Automation * Update comments * add method to fix endpoints files * Fix code formatting and add unit tests * Fix code formatting and add unit tests * Fix unit tests and indentation * Fix tests formatting and code fix * Remove Newline * Fix indentation and typos * PSR2 compatible code and code fix * Fix call for object instantiation * Fix endpoints file indentation * Update resources json * Update unit tests * Update class and test file * Remove extra newlines from code * rename verbose flag * update main class and builder class * remove unused var * update class and builder * made changes * Fixes aws#1181 (aws#1182) * fix typo * Update models for release * 3.22.6 release * Update models for release * 3.22.7 release * Fix retry jitter logic (aws#1187) * Add filePath Info in MultipartUploadException (aws#1140) * Add $filePath Info in MultipartUploadException * Pass endpoint prefix rather than service identifier to EndpointProvider (aws#1192) * Ignore isRegionalized if no partitionEndpoint is provided * Do not patch SDB signatureVersions -- they are unresolvable by the PartitionEndpointProvider, which should return `null` * Pass endpoint prefix rather than service identifier to the EndpointProvider * Remove constructor customization from ELBv2 client * Update models for release * 3.22.8 release * Move idempotency token generation from EC2 and SSM customizations to ClientResolver * Allow serialization of XML structure with name but no locationName * Sync protocol tests * Update models for release * 3.22.9 release * Update models for release * 3.22.10 release * Prefer the signing name provided by the service model to that provided by the endpoint provider * Update models for release * 3.22.11 release * Update models for release * 3.23.0 release * Update models for release * 3.23.1 release * Update version constraint on guzzlehttp/psr7 to allow 1.4.1 * Add unsigned Payload capability * add tests for unsigned signature v4 * Update models for release * 3.23.2 release * Update code and add more tests * remove variable from s3signv4 * Fix minore typos * Update code and remove typos * Update models for release * 3.23.3 release * Update models for release * 3.24.0 release * Update models for release * 3.24.1 release * Update models for release * 3.24.2 release * change null comparison to not empty * Force usage of the latest version of guzzlehttp/psr7 due to BC concerns * Update models for release * 3.24.3 release * Update code and add unit tests for Invalid Security Token when using Access/Secret key from environment: * Update models for release * 3.24.4 release * Bug Fix * created CloudWatch example section and added section on creating alarms * Add tests and update code * Update code * Fix formatting * changing topic title and filename * update code and fix formatting * update test and unsign payload only for https requests * Add more tests and update code * Update models for release * 3.24.5 release * Update models for release * 3.24.6 release * Update models for release * 3.24.7 release

Add $filePath Info in MultipartUploadException

69e2bb8

cjyclaire mentioned this pull request Dec 13, 2016

Add S3 TransferException to track failed file source #1038

Closed

cjyclaire changed the title ~~Add $filePath Info in MultipartUploadException~~ Add filePath Info in MultipartUploadException Dec 13, 2016

cjyclaire added the ready-for-review label Dec 13, 2016

mtdowling suggested changes Dec 14, 2016

View reviewed changes

cjyclaire added investigating This issue is being investigated and/or work is in progress to resolve the issue. and removed ready-for-review labels Dec 14, 2016

cjyclaire added 5 commits December 15, 2016 14:33

fix null case

b7f136c

fix test

ef61c83

fix test

74dce10

change empty check to null check

efab4a0

revert

e4bed4c

cjyclaire added ready-for-review investigating This issue is being investigated and/or work is in progress to resolve the issue. and removed investigating This issue is being investigated and/or work is in progress to resolve the issue. ready-for-review labels Dec 16, 2016

cjyclaire added 2 commits January 3, 2017 14:40

Create S3MultipartUploadException

384b041

remove an extra line

dbab3a8

cjyclaire added ready-for-review and removed investigating This issue is being investigated and/or work is in progress to resolve the issue. labels Jan 3, 2017

mtdowling suggested changes Jan 4, 2017

View reviewed changes

cjyclaire removed the ready-for-review label Jan 4, 2017

Move the exception to constructor

50d421d

cjyclaire added the ready-for-review label Jan 4, 2017

mtdowling suggested changes Jan 5, 2017

View reviewed changes

cjyclaire removed the ready-for-review label Jan 5, 2017

cjyclaire added 3 commits January 5, 2017 08:31

change exception to exception_class in config

7344d0b

clean up code

caeae1a

removing an extra space

f4cdf87

cjyclaire added the ready-for-review label Jan 5, 2017

mtdowling reviewed Jan 6, 2017

View reviewed changes

Collect bucket and key info instead

c7011cd

cjyclaire added investigating This issue is being investigated and/or work is in progress to resolve the issue. and removed ready-for-review labels Jan 6, 2017

cjyclaire added 2 commits January 6, 2017 13:53

Add filename information

48a71d6

clean up

8dc7894

mtdowling suggested changes Jan 6, 2017

View reviewed changes

cjyclaire added 3 commits January 6, 2017 15:23

Use stream in 'Body' param

bb69989

Better documentation

b78b712

Clean

321baae

cjyclaire added ready-for-review and removed investigating This issue is being investigated and/or work is in progress to resolve the issue. labels Jan 6, 2017

jeskew suggested changes Feb 17, 2017

View reviewed changes

cjyclaire removed the ready-for-review label Feb 17, 2017

Clean up code

4e4ebdc

jeskew approved these changes Feb 21, 2017

View reviewed changes

cjyclaire merged commit f859d7d into aws:master Feb 21, 2017

Conversation

cjyclaire commented Dec 13, 2016

Uh oh!

mtdowling left a comment

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!

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!

mtdowling commented Jan 6, 2017

Uh oh!

cjyclaire commented Jan 6, 2017

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!

cjyclaire commented Feb 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjyclaire Feb 17, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjyclaire Feb 17, 2017 • 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!

cjyclaire commented Feb 17, 2017

Uh oh!

jeskew left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

cjyclaire Feb 17, 2017 •

edited

Loading

cjyclaire Feb 17, 2017 •

edited

Loading

jeskew left a comment •

edited

Loading