From f94afc49442bd4c06f3ca6ea1950ff24dd28ee15 Mon Sep 17 00:00:00 2001 From: Yenfry Herrera Feliz Date: Thu, 8 Aug 2024 18:09:50 -0700 Subject: [PATCH] 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 --- .../Parser/GetBucketLocationResultMutator.php | 11 ++- src/S3/Parser/S3Parser.php | 34 ++++++--- .../ValidateResponseChecksumResultMutator.php | 11 ++- src/S3/S3Client.php | 10 ++- .../GetBucketLocationResultMutatorTest.php | 28 ++++++++ tests/S3/Parser/S3ParserTest.php | 69 ++++++++++++------- 6 files changed, 121 insertions(+), 42 deletions(-) diff --git a/src/S3/Parser/GetBucketLocationResultMutator.php b/src/S3/Parser/GetBucketLocationResultMutator.php index 9ca16a3ba2..f319f55aaf 100644 --- 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 index a9cfd0dcb5..b6e661c911 100644 --- 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*/'; + static $pattern = '/<\?xml version="1\.0" encoding="UTF-8"\?>\s*/'; // 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 index e7cb7883fb..2c5cb8eae1 100644 --- 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 index aa22612b54..bcd0bbcc6f 100644 --- 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 index bba530d651..551d5bccdc 100644 --- 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, [], "$bucketTestLocation"); + $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 index 6e249e6e5f..ba98076fcd 100644 --- 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);