chore: address PR feedback

- Split statements into multiple lines to improve readability. - Make pattern variable for finding the Error element name to be static. - Add a test in the GetBucketLocationResultMutatorTest to make sure the mutator just applies to GetBucketLocation - Move some values into static variables in the GetBucketLocationMutator implementation
aws · Aug 12, 2024 · f94afc4 · f94afc4
1 parent 80a195c
commit f94afc4
Show file tree

Hide file tree

Showing 6 changed files with 121 additions and 42 deletions.
diff --git a/src/S3/Parser/GetBucketLocationResultMutator.php b/src/S3/Parser/GetBucketLocationResultMutator.php
@@ -18,14 +18,19 @@ final class GetBucketLocationResultMutator implements S3ResultMutator
     /**
      * @inheritDoc
      */
-    public function __invoke(ResultInterface $result, CommandInterface $command, ResponseInterface $response): ResultInterface
+    public function __invoke(
+        ResultInterface $result,
+        CommandInterface $command,
+        ResponseInterface $response
+    ): ResultInterface
     {
         if ($command->getName() !== 'GetBucketLocation') {
             return $result;
         }
 
-        $location = 'us-east-1';
-        if (preg_match('/>(.+?)<\/LocationConstraint>/', $response->getBody(), $matches)) {
+        static $location = 'us-east-1';
+        static $pattern = '/>(.+?)<\/LocationConstraint>/';
+        if (preg_match($pattern, $response->getBody(), $matches)) {
             $location = $matches[1] === 'EU' ? 'eu-west-1' : $matches[1];
         }
 

diff --git a/src/S3/Parser/S3Parser.php b/src/S3/Parser/S3Parser.php
@@ -55,11 +55,14 @@ public function __construct(
      * Parses a S3 response.
      *
      * @param CommandInterface $command The command that originated the request.
-     * @param ResponseInterface $response The response gotten from the service.
+     * @param ResponseInterface $response The response received from the service.
      *
      * @return ResultInterface|null
      */
-    public function __invoke(CommandInterface $command, ResponseInterface $response):? ResultInterface
+    public function __invoke(
+        CommandInterface $command,
+        ResponseInterface $response
+    ):? ResultInterface
     {
         // Check first if the response is an error
         $this->parse200Error($command, $response);
@@ -91,11 +94,15 @@ public function __invoke(CommandInterface $command, ResponseInterface $response)
      *
      * @return void
      */
-    private function parse200Error(CommandInterface $command, ResponseInterface $response)
+    private function parse200Error(
+        CommandInterface $command,
+        ResponseInterface $response
+    ): void
     {
         // This error parsing should be just for 200 error responses and operations where its output shape
         // does not have a streaming member.
-        if (200 !== $response->getStatusCode() || !$this->shouldBeConsidered200Error($command->getName())) {
+        if (200 !== $response->getStatusCode()
+            || !$this->shouldBeConsidered200Error($command->getName())) {
             return;
         }
 
@@ -157,13 +164,14 @@ private function shouldBeConsidered200Error($commandName): bool
      * when we should try to parse an error from a 200 response from s3.
      * It is recommended to make sure the stream given is seekable, otherwise
      * the rewind call will cause a user warning.
+     *
      * @param StreamInterface $responseBody
      *
      * @return bool
      */
     private function isFirstRootElementError(StreamInterface $responseBody): bool
     {
-        $pattern = '/<\?xml version="1\.0" encoding="UTF-8"\?>\s*<Error>/';
+        static $pattern = '/<\?xml version="1\.0" encoding="UTF-8"\?>\s*<Error>/';
         // To avoid performance overhead in large streams
         $reducedBodyContent = $responseBody->read(64);
         $foundErrorElement = preg_match($pattern, $reducedBodyContent);
@@ -207,7 +215,10 @@ private function executeS3ResultMutators(
     public function addS3ResultMutator($mutatorName, S3ResultMutator $s3ResultMutator): void
     {
         if (isset($this->s3ResultMutators[$mutatorName])) {
-            trigger_error("The S3 Result Mutator {$mutatorName} already exists!", E_USER_WARNING);
+            trigger_error(
+                "The S3 Result Mutator {$mutatorName} already exists!",
+                E_USER_WARNING
+            );
 
             return;
         }
@@ -224,7 +235,10 @@ public function addS3ResultMutator($mutatorName, S3ResultMutator $s3ResultMutato
     public function removeS3ResultMutator($mutatorName): void
     {
         if (!isset($this->s3ResultMutators[$mutatorName])) {
-            trigger_error("The S3 Result Mutator {$mutatorName} does not exist!", E_USER_WARNING);
+            trigger_error(
+                "The S3 Result Mutator {$mutatorName} does not exist!",
+                E_USER_WARNING
+            );
 
             return;
         }
@@ -244,6 +258,10 @@ public function getS3ResultMutators(): array
 
     public function parseMemberFromStream(StreamInterface $stream, StructureShape $member, $response)
     {
-        return $this->protocolParser->parseMemberFromStream($stream, $member, $response);
+        return $this->protocolParser->parseMemberFromStream(
+            $stream,
+            $member,
+            $response
+        );
     }
 }
diff --git a/src/S3/Parser/ValidateResponseChecksumResultMutator.php b/src/S3/Parser/ValidateResponseChecksumResultMutator.php
@@ -54,7 +54,8 @@ public function __invoke(
         $checksumModeEnabledMember = $checksumInfo['requestValidationModeMember'] ?? "";
         $checksumModeEnabled = $command[$checksumModeEnabledMember] ?? "";
         $responseAlgorithms = $checksumInfo['responseAlgorithms'] ?? [];
-        if (empty($responseAlgorithms) || strtolower($checksumModeEnabled) !== "enabled") {
+        if (empty($responseAlgorithms)
+            || strtolower($checksumModeEnabled) !== "enabled") {
             return $result;
         }
 
@@ -98,7 +99,10 @@ public function __invoke(
      *
      * @return array
      */
-    public function validateChecksum($checksumPriority, ResponseInterface $response): array
+    public function validateChecksum(
+        $checksumPriority,
+        ResponseInterface $response
+    ): array
     {
         $checksumToValidate = $this->chooseChecksumHeaderToValidate(
             $checksumPriority,
@@ -137,7 +141,8 @@ public function validateChecksum($checksumPriority, ResponseInterface $response)
     public function chooseChecksumHeaderToValidate(
         $checksumPriority,
         ResponseInterface $response
-    ):? string {
+    ):? string
+    {
         foreach ($checksumPriority as $checksum) {
             $checksumHeader = 'x-amz-checksum-' . $checksum;
             if ($response->hasHeader($checksumHeader)) {

diff --git a/src/S3/S3Client.php b/src/S3/S3Client.php
@@ -956,8 +956,14 @@ public static function _applyApiProvider($value, array &$args, HandlerList $list
             $args['api'],
             $args['exception_class']
         );
-        $s3Parser->addS3ResultMutator('get-bucket-location', new GetBucketLocationResultMutator());
-        $s3Parser->addS3ResultMutator('validate-response-checksum', new ValidateResponseChecksumResultMutator($args['api']));
+        $s3Parser->addS3ResultMutator(
+            'get-bucket-location',
+            new GetBucketLocationResultMutator()
+        );
+        $s3Parser->addS3ResultMutator(
+            'validate-response-checksum',
+            new ValidateResponseChecksumResultMutator($args['api'])
+        );
         $args['parser'] = $s3Parser;
     }
 

diff --git a/tests/S3/Parser/GetBucketLocationResultMutatorTest.php b/tests/S3/Parser/GetBucketLocationResultMutatorTest.php
@@ -10,6 +10,12 @@
 
 class GetBucketLocationResultMutatorTest extends TestCase
 {
+    /**
+     * Test that the bucket location is extracted from the response
+     * and added to the result as the LocationConstraint field.
+     *
+     * @return void
+     */
     public function testInjectsLocationConstraint()
     {
         $bucketTestLocation = 'us-test-1';
@@ -19,4 +25,26 @@ public function testInjectsLocationConstraint()
 
         $this->assertEquals($bucketTestLocation, $result['LocationConstraint']);
     }
+
+    /**
+     * Test that the GetBucketLocationResultMutator ignores any operation that is not
+     * GetBucketLocation.
+     *
+     * @return void
+     */
+    public function testMutatorDoesNotApplyToOtherOperations()
+    {
+        $bucketTestLocation = 'us-test-1';
+        $response = new Response(200, [], "<LocationConstraint>$bucketTestLocation</LocationConstraint>");
+        $mutator = new GetBucketLocationResultMutator();
+        $operations = [
+            'ListObjects',
+            'ListBuckets',
+            'GetObject'
+        ];
+        foreach ($operations as $operation) {
+            $result = $mutator(new Result(), new Command($operation), $response);
+            $this->assertEmpty($result['LocationConstraint']);
+        }
+    }
 }
diff --git a/tests/S3/Parser/S3ParserTest.php b/tests/S3/Parser/S3ParserTest.php
@@ -166,7 +166,11 @@ public function testS3ParserReturnsRetryableErrorOnNotParsableBody()
             $parser($command, $response);
         } catch (AwsException $e) {
             $this->assertTrue($e->isConnectionError());
-            $this->assertEquals("Error parsing response for ListBuckets: AWS parsing error: Error parsing XML: String could not be parsed as XML", $e->getMessage());
+            $errorMessage = [
+                'Error parsing response for ListBuckets: ',
+                'AWS parsing error: Error parsing XML: String could not be parsed as XML'
+            ];
+            $this->assertEquals(join('', $errorMessage), $e->getMessage());
         }
     }
 
@@ -176,29 +180,36 @@ public function testAddsS3ResultMutator()
         $testValue = 'TestValue';
         $s3MutatorName = 's3.test-mutator';
         $s3Parser = $this->getS3Parser();
-        $s3Parser->addS3ResultMutator($s3MutatorName, new class($testField, $testValue) implements S3ResultMutator
-        {
-            /**
-             * @var string $testField
-             */
-            private $testField;
-            /**
-             * @var string $testValue
-             */
-            private $testValue;
-            public function __construct($testField, $testValue)
+        $s3Parser->addS3ResultMutator(
+            $s3MutatorName,
+            new class($testField, $testValue) implements S3ResultMutator
             {
-                $this->testField = $testField;
-                $this->testValue = $testValue;
-            }
+                /**
+                 * @var string $testField
+                 */
+                private $testField;
+                /**
+                 * @var string $testValue
+                 */
+                private $testValue;
+                public function __construct($testField, $testValue)
+                {
+                    $this->testField = $testField;
+                    $this->testValue = $testValue;
+                }
 
-            public function __invoke(ResultInterface $result, CommandInterface $command, ResponseInterface $response): ResultInterface
-            {
-                $result[$this->testField] = $this->testValue;
+                public function __invoke(
+                    ResultInterface $result,
+                    CommandInterface $command,
+                    ResponseInterface $response
+                ): ResultInterface
+                {
+                    $result[$this->testField] = $this->testValue;
 
-                return $result;
+                    return $result;
+                }
             }
-        });
+        );
         $mutators = $s3Parser->getS3ResultMutators();
         $command = new Command('ListBuckets', [], new HandlerList());
         $response = new Response();
@@ -212,14 +223,20 @@ public function testRemovesS3ResultMutator()
     {
         $s3Parser = $this->getS3Parser();
         $s3MutatorName = 's3.test-mutator';
-        $s3Parser->addS3ResultMutator($s3MutatorName, new class implements S3ResultMutator
-        {
-
-            public function __invoke(ResultInterface $result, CommandInterface $command, ResponseInterface $response): ResultInterface
+        $s3Parser->addS3ResultMutator(
+            $s3MutatorName,
+            new class implements S3ResultMutator
             {
-                return $result;
+                public function __invoke(
+                    ResultInterface $result,
+                    CommandInterface $command,
+                    ResponseInterface $response
+                ): ResultInterface
+                {
+                    return $result;
+                }
             }
-        });
+        );
         $mutators = $s3Parser->getS3ResultMutators();
         $this->assertTrue(isset($mutators[$s3MutatorName]));
         $s3Parser->removeS3ResultMutator($s3MutatorName);