From f6f7d71fe66dcb2429fb4012b064a054141e9659 Mon Sep 17 00:00:00 2001 From: Indrek Date: Tue, 31 Dec 2024 09:29:29 +0200 Subject: [PATCH 1/7] fix: attempt to only remember received content when the header is in correct status --- Storage/src/Connection/Rest.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/Storage/src/Connection/Rest.php b/Storage/src/Connection/Rest.php index 2f89fe309a2c..b6eb3dbeec8c 100644 --- a/Storage/src/Connection/Rest.php +++ b/Storage/src/Connection/Rest.php @@ -316,7 +316,12 @@ public function downloadObject(array $args = []) &$attempt, ) { // if the exception has a response for us to use - if ($e instanceof RequestException && $e->hasResponse()) { + if ( + $e instanceof RequestException + && $e->hasResponse() + && $e->getResponse()->getStatusCode() >= 200 + && $e->getResponse()->getStatusCode() < 300 + ) { $msg = (string) $e->getResponse()->getBody(); $fetchedStream = Utils::streamFor($msg); From e7b117385d2b707b8221203449c842c53c58aabb Mon Sep 17 00:00:00 2001 From: Indrek Date: Wed, 8 Jan 2025 17:47:20 +0200 Subject: [PATCH 2/7] fix: add unittest to show how retryListener is supposed to work, reformat code a bit --- Storage/src/Connection/Rest.php | 6 +- Storage/tests/Unit/Connection/RestTest.php | 90 ++++++++++++++++++++++ 2 files changed, 92 insertions(+), 4 deletions(-) diff --git a/Storage/src/Connection/Rest.php b/Storage/src/Connection/Rest.php index 3abc0ba8162c..386a9bbbb4b2 100644 --- a/Storage/src/Connection/Rest.php +++ b/Storage/src/Connection/Rest.php @@ -324,12 +324,10 @@ public function downloadObject(array $args = []) &$attempt, ) { // if the exception has a response for us to use - if ( - $e instanceof RequestException + if ($e instanceof RequestException && $e->hasResponse() && $e->getResponse()->getStatusCode() >= 200 - && $e->getResponse()->getStatusCode() < 300 - ) { + && $e->getResponse()->getStatusCode() < 300) { $msg = (string) $e->getResponse()->getBody(); $fetchedStream = Utils::streamFor($msg); diff --git a/Storage/tests/Unit/Connection/RestTest.php b/Storage/tests/Unit/Connection/RestTest.php index 7bcc731fdddd..7a1fac96925b 100644 --- a/Storage/tests/Unit/Connection/RestTest.php +++ b/Storage/tests/Unit/Connection/RestTest.php @@ -17,6 +17,7 @@ namespace Google\Cloud\Storage\Tests\Unit\Connection; +use Google\Auth\HttpHandler\Guzzle7HttpHandler; use Google\Cloud\Core\RequestBuilder; use Google\Cloud\Core\RequestWrapper; use Google\Cloud\Core\Retry; @@ -25,6 +26,9 @@ use Google\Cloud\Core\Upload\ResumableUploader; use Google\Cloud\Core\Upload\StreamableUploader; use Google\Cloud\Storage\Connection\Rest; +use Google\Cloud\Storage\Connection\RetryTrait; +use GuzzleHttp\Client; +use GuzzleHttp\Exception\RequestException; use GuzzleHttp\Promise\Create; use GuzzleHttp\Promise\PromiseInterface; use GuzzleHttp\Psr7\Request; @@ -273,6 +277,92 @@ function ($args) use (&$actualRequest, $response) { ); } + public function downloadObjectWithResumeProvider() + { + $part1 = 'first part'; + $part2 = 'second part'; + $full = $part1 . $part2; + /** @see RetryTrait::$httpRetryCodes for list of status codes that are allowed to retry */ + return [ + [0, '', 200, $full, $full, []], + [200, $part1, 200, $part2, $full, ['Range' => 'bytes=10-']], + [408, 'Server Message', 200, $full, $full, []], + [429, 'Server Message', 200, $full, $full, []], + [500, 'Server Error', 200, $full, $full, []], + [502, 'Server Error', 200, $full, $full, []], + [503, 'Server Error', 200, $full, $full, []], + [504, 'Server Error', 200, $full, $full, []], + ]; + } + + /** + * @dataProvider downloadObjectWithResumeProvider + */ + public function testDownloadObjectWithResume(int $status1, + string $body1, + int $status2, + string $body2, + string $expectedResult, + array $expectedSecondRequestHeaders) + { + /** @var Request[] $actualRequests */ + $actualRequests = []; + $requestHeaders = []; + + $mockClient = $this->prophesize(Client::class); + $requestIndex = 0; + $mockClient->send( + Argument::type(RequestInterface::class), + Argument::type('array') + )->will( + function ($args) use (&$actualRequests, + &$requestHeaders, + $status1, + $body1, + $status2, + $body2, + &$requestIndex) { + $actualRequests[$requestIndex] = $args[0]; + $requestHeaders[$requestIndex] = $args[1]['headers'] ?? []; + if ($requestIndex++ === 0) { + throw new RequestException("Server error", $args[0], new Response($status1, [], $body1)); + } + return new Response($status2, [], $body2); + } + ); + $requestWrapper = new RequestWrapper([ + 'httpHandler' => new Guzzle7HttpHandler($mockClient->reveal()), + 'accessToken' => 'Fake token', + 'retries' => 3, + ]); + + $rest = new Rest(); + $rest->setRequestWrapper($requestWrapper); + + $options = self::$downloadOptions; + $options['retries'] = 3; + $actualBody = (string) $rest->downloadObject($options); + $actualUri1 = (string) $actualRequests[0]->getUri(); + $actualUri2 = (string) $actualRequests[1]->getUri(); + + $expectedUri = sprintf( + '%s/storage/v1/b/bigbucket/o/myfile.txt?generation=100&alt=media&userProject=myProject', + Rest::DEFAULT_API_ENDPOINT + ); + + $this->assertEquals( + $expectedUri, + $actualUri1 + ); + $this->assertEquals([], $requestHeaders[0]); + $this->assertEquals( + $expectedUri, + $actualUri2 + ); + $this->assertEquals($expectedSecondRequestHeaders, $requestHeaders[1]); + $this->assertEquals($expectedResult, $actualBody); + } + /** * @dataProvider insertObjectProvider */ From ac6afbb2a58316f63f3bb7f82d145bd250e5e6e2 Mon Sep 17 00:00:00 2001 From: indreka Date: Wed, 8 Jan 2025 19:24:00 +0200 Subject: [PATCH 3/7] code formatting update Co-authored-by: Brent Shaffer --- Storage/tests/Unit/Connection/RestTest.php | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/Storage/tests/Unit/Connection/RestTest.php b/Storage/tests/Unit/Connection/RestTest.php index 7a1fac96925b..2dc824a187ea 100644 --- a/Storage/tests/Unit/Connection/RestTest.php +++ b/Storage/tests/Unit/Connection/RestTest.php @@ -298,13 +298,14 @@ public function downloadObjectWithResumeProvider() /** * @dataProvider downloadObjectWithResumeProvider */ - public function testDownloadObjectWithResume(int $status1, - string $body1, - int $status2, - string $body2, - string $expectedResult, - array $expectedSecondRequestHeaders) - { + public function testDownloadObjectWithResume( + int $status1, + string $body1, + int $status2, + string $body2, + string $expectedResult, + array $expectedSecondRequestHeaders + ) { /** @var Request[] $actualRequests */ $actualRequests = []; $requestHeaders = []; From 2a9be6fd149b9363a3ad958e43e205a2c463c2ce Mon Sep 17 00:00:00 2001 From: indreka Date: Fri, 10 Jan 2025 20:58:40 +0200 Subject: [PATCH 4/7] Update Storage/tests/Unit/Connection/RestTest.php Co-authored-by: Vishwaraj Anand --- Storage/tests/Unit/Connection/RestTest.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Storage/tests/Unit/Connection/RestTest.php b/Storage/tests/Unit/Connection/RestTest.php index 2dc824a187ea..40e0e14d2e78 100644 --- a/Storage/tests/Unit/Connection/RestTest.php +++ b/Storage/tests/Unit/Connection/RestTest.php @@ -316,7 +316,8 @@ public function testDownloadObjectWithResume( Argument::type(RequestInterface::class), Argument::type('array') )->will( - function ($args) use (&$actualRequests, + function ($args) use ( + &$actualRequests, &$requestHeaders, $status1, $body1, From 79c92f1059247501948359e9deb1b3ea0705c593 Mon Sep 17 00:00:00 2001 From: indreka Date: Fri, 10 Jan 2025 20:58:50 +0200 Subject: [PATCH 5/7] Update Storage/tests/Unit/Connection/RestTest.php Co-authored-by: Vishwaraj Anand --- Storage/tests/Unit/Connection/RestTest.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Storage/tests/Unit/Connection/RestTest.php b/Storage/tests/Unit/Connection/RestTest.php index 40e0e14d2e78..d68dce273b9b 100644 --- a/Storage/tests/Unit/Connection/RestTest.php +++ b/Storage/tests/Unit/Connection/RestTest.php @@ -323,7 +323,8 @@ function ($args) use ( $body1, $status2, $body2, - &$requestIndex) { + &$requestIndex + ) { $actualRequests[$requestIndex] = $args[0]; $requestHeaders[$requestIndex] = $args[1]['headers'] ?? []; if ($requestIndex++ === 0) { From 5dd0ebde368f517febed98139eead3b5bab27a92 Mon Sep 17 00:00:00 2001 From: Brent Shaffer Date: Fri, 10 Jan 2025 11:38:43 -0800 Subject: [PATCH 6/7] test cleanup --- Storage/tests/Unit/Connection/RestTest.php | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/Storage/tests/Unit/Connection/RestTest.php b/Storage/tests/Unit/Connection/RestTest.php index d68dce273b9b..1a2205307d45 100644 --- a/Storage/tests/Unit/Connection/RestTest.php +++ b/Storage/tests/Unit/Connection/RestTest.php @@ -284,14 +284,14 @@ public function downloadObjectWithResumeProvider() $full = $part1 . $part2; /** @see RetryTrait::$httpRetryCodes for list of status codes that are allowed to retry */ return [ - [0, '', 200, $full, $full, []], + [0, '', 200, $full, $full], [200, $part1, 200, $part2, $full, ['Range' => 'bytes=10-']], - [408, 'Server Message', 200, $full, $full, []], - [429, 'Server Message', 200, $full, $full, []], - [500, 'Server Error', 200, $full, $full, []], - [502, 'Server Error', 200, $full, $full, []], - [503, 'Server Error', 200, $full, $full, []], - [504, 'Server Error', 200, $full, $full, []], + [408, 'Server Message', 200, $full, $full], + [429, 'Server Message', 200, $full, $full], + [500, 'Server Error', 200, $full, $full], + [502, 'Server Error', 200, $full, $full], + [503, 'Server Error', 200, $full, $full], + [504, 'Server Error', 200, $full, $full], ]; } @@ -304,7 +304,7 @@ public function testDownloadObjectWithResume( int $status2, string $body2, string $expectedResult, - array $expectedSecondRequestHeaders + array $expectedSecondRequestHeaders = [] ) { /** @var Request[] $actualRequests */ $actualRequests = []; @@ -344,6 +344,9 @@ function ($args) use ( $options = self::$downloadOptions; $options['retries'] = 3; + // ensure the tests execute quickly (no reason to wait for the delay) + $options['restDelayFunction'] = function () { + }; $actualBody = (string) $rest->downloadObject($options); $actualUri1 = (string) $actualRequests[0]->getUri(); $actualUri2 = (string) $actualRequests[1]->getUri(); From 14253216e2209f5782da9b8365dbc0bbf3730e87 Mon Sep 17 00:00:00 2001 From: Brent Shaffer Date: Fri, 10 Jan 2025 11:39:58 -0800 Subject: [PATCH 7/7] style cleanup --- Storage/src/Connection/Rest.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Storage/src/Connection/Rest.php b/Storage/src/Connection/Rest.php index 386a9bbbb4b2..a719b0bd1f69 100644 --- a/Storage/src/Connection/Rest.php +++ b/Storage/src/Connection/Rest.php @@ -327,7 +327,8 @@ public function downloadObject(array $args = []) if ($e instanceof RequestException && $e->hasResponse() && $e->getResponse()->getStatusCode() >= 200 - && $e->getResponse()->getStatusCode() < 300) { + && $e->getResponse()->getStatusCode() < 300 + ) { $msg = (string) $e->getResponse()->getBody(); $fetchedStream = Utils::streamFor($msg);