From 50d2f034f0ce5c91d96ac0bfa47c839ec0f29297 Mon Sep 17 00:00:00 2001 From: Kim Pepper Date: Wed, 29 Jan 2025 17:00:28 +0900 Subject: [PATCH] Split out decider and add tests Signed-off-by: Kim Pepper --- CHANGELOG.md | 3 +- composer.json | 5 +- src/OpenSearch/GuzzleHttpClientFactory.php | 83 ------------------- .../HttpClient/GuzzleHttpClientFactory.php | 58 +++++++++++++ .../HttpClient/GuzzleRetryDecider.php | 51 ++++++++++++ .../HttpClientFactoryInterface.php | 4 +- .../SymfonyHttpClientFactory.php | 21 +++-- .../GuzzleHttpClientFactoryTest.php | 12 ++- tests/HttpClient/GuzzleRetryDeciderTest.php | 73 ++++++++++++++++ .../SymfonyHttpClientFactoryTest.php | 10 +-- 10 files changed, 212 insertions(+), 108 deletions(-) delete mode 100644 src/OpenSearch/GuzzleHttpClientFactory.php create mode 100644 src/OpenSearch/HttpClient/GuzzleHttpClientFactory.php create mode 100644 src/OpenSearch/HttpClient/GuzzleRetryDecider.php rename src/OpenSearch/{ => HttpClient}/HttpClientFactoryInterface.php (74%) rename src/OpenSearch/{ => HttpClient}/SymfonyHttpClientFactory.php (75%) rename tests/{ => HttpClient}/GuzzleHttpClientFactoryTest.php (62%) create mode 100644 tests/HttpClient/GuzzleRetryDeciderTest.php rename tests/{ => HttpClient}/SymfonyHttpClientFactoryTest.php (65%) diff --git a/CHANGELOG.md b/CHANGELOG.md index b59f12c4..311ae66b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,8 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) - Added a test for the AWS signing client decorator - Added PHPStan Deprecation rules and baseline - Added PHPStan PHPUnit extensions and rules -- Added Guzzle and Symfony HTTP client factories +- Added Guzzle and Symfony HTTP client factories. +- Added 'colinodell/psr-testlogger' as a dev dependency. ### Changed - Switched to PSR Interfaces - Increased PHP min version to 8.1 diff --git a/composer.json b/composer.json index 8b31a67a..8e8fe44a 100644 --- a/composer.json +++ b/composer.json @@ -38,6 +38,7 @@ "require-dev": { "ext-zip": "*", "aws/aws-sdk-php": "^3.0", + "colinodell/psr-testlogger": "^1.3", "friendsofphp/php-cs-fixer": "^v3.64", "guzzlehttp/psr7": "^2.7", "mockery/mockery": "^1.6", @@ -53,7 +54,9 @@ }, "suggest": { "monolog/monolog": "Allows for client-level logging and tracing", - "aws/aws-sdk-php": "Required (^3.0.0) in order to use the SigV4 handler" + "aws/aws-sdk-php": "Required (^3.0.0) in order to use the AWS Signing Client Decorator", + "guzzlehttp/psr7": "Required (^2.7) in order to use the Guzzle HTTP client", + "symfony/http-client": "Required (^6.4|^7.0) in order to use the Symfony HTTP client" }, "autoload": { "psr-4": { diff --git a/src/OpenSearch/GuzzleHttpClientFactory.php b/src/OpenSearch/GuzzleHttpClientFactory.php deleted file mode 100644 index 8c4d8ed4..00000000 --- a/src/OpenSearch/GuzzleHttpClientFactory.php +++ /dev/null @@ -1,83 +0,0 @@ - [ - 'Accept' => 'application/json', - 'Content-Type' => 'application/json', - 'User-Agent' => sprintf('opensearch-php/%s (%s; PHP %s)', Client::VERSION, PHP_OS, PHP_VERSION), - ], - ]; - - $logger = $options['logger'] ?? null; - unset($options['logger']); - - $maxRetries = $options['max_retries'] ?? 0; - unset($options['max_retries']); - - // Merge the default options with the provided options. - $config = array_merge_recursive($defaults, $options); - - $stack = HandlerStack::create(); - - // Handle retries if max_retries is set. - if ($maxRetries > 0) { - $stack->push(Middleware::retry(function ($retries, $request, $response, $exception) use ($maxRetries, $logger) { - if ($retries >= $maxRetries) { - return false; - } - if ($exception instanceof ConnectException) { - $logger?->warning( - 'Retrying request {retries} of {maxRetries}: {exception}', - [ - 'retries' => $retries, - 'maxRetries' => $maxRetries, - 'exception' => $exception->getMessage(), - ] - ); - return true; - } - if ($response && $response->getStatusCode() >= 500) { - $logger?->warning( - 'Retrying request {retries} of {maxRetries}: Status code {status}', - [ - 'retries' => $retries, - 'maxRetries' => $maxRetries, - 'status' => $response->getStatusCode(), - ] - ); - return true; - } - return false; - })); - } - - $config['handler'] = $stack; - - return new GuzzleClient($config); - } - -} diff --git a/src/OpenSearch/HttpClient/GuzzleHttpClientFactory.php b/src/OpenSearch/HttpClient/GuzzleHttpClientFactory.php new file mode 100644 index 00000000..35682d8e --- /dev/null +++ b/src/OpenSearch/HttpClient/GuzzleHttpClientFactory.php @@ -0,0 +1,58 @@ + [ + 'Accept' => 'application/json', + 'Content-Type' => 'application/json', + 'User-Agent' => sprintf('opensearch-php/%s (%s; PHP %s)', Client::VERSION, PHP_OS, PHP_VERSION), + ], + ]; + + // Merge the default options with the provided options. + $config = array_merge_recursive($defaults, $options); + + $stack = HandlerStack::create(); + + // Handle retries if max_retries is set. + if ($this->maxRetries > 0) { + $decider = new GuzzleRetryDecider($this->maxRetries, $this->logger); + $stack->push(Middleware::retry($decider(...))); + } + + $config['handler'] = $stack; + + return new GuzzleClient($config); + } + +} diff --git a/src/OpenSearch/HttpClient/GuzzleRetryDecider.php b/src/OpenSearch/HttpClient/GuzzleRetryDecider.php new file mode 100644 index 00000000..59d0a1ef --- /dev/null +++ b/src/OpenSearch/HttpClient/GuzzleRetryDecider.php @@ -0,0 +1,51 @@ += $this->maxRetries) { + return false; + } + if ($exception instanceof ConnectException) { + $this->logger?->warning( + 'Retrying request {retries} of {maxRetries}: {exception}', + [ + 'retries' => $retries, + 'maxRetries' => $this->maxRetries, + 'exception' => $exception->getMessage(), + ] + ); + return true; + } + if ($response && $response->getStatusCode() >= 500) { + $this->logger?->warning( + 'Retrying request {retries} of {maxRetries}: Status code {status}', + [ + 'retries' => $retries, + 'maxRetries' => $this->maxRetries, + 'status' => $response->getStatusCode(), + ] + ); + return true; + } + // We only retry if there is a 500 or a ConnectException. + return false; + } +} diff --git a/src/OpenSearch/HttpClientFactoryInterface.php b/src/OpenSearch/HttpClient/HttpClientFactoryInterface.php similarity index 74% rename from src/OpenSearch/HttpClientFactoryInterface.php rename to src/OpenSearch/HttpClient/HttpClientFactoryInterface.php index 9cd7b1dd..cd1a78cf 100644 --- a/src/OpenSearch/HttpClientFactoryInterface.php +++ b/src/OpenSearch/HttpClient/HttpClientFactoryInterface.php @@ -2,7 +2,7 @@ declare(strict_types=1); -namespace OpenSearch; +namespace OpenSearch\HttpClient; use Psr\Http\Client\ClientInterface; @@ -16,6 +16,6 @@ interface HttpClientFactoryInterface * * @param array $options */ - public static function create(array $options): ClientInterface; + public function create(array $options): ClientInterface; } diff --git a/src/OpenSearch/SymfonyHttpClientFactory.php b/src/OpenSearch/HttpClient/SymfonyHttpClientFactory.php similarity index 75% rename from src/OpenSearch/SymfonyHttpClientFactory.php rename to src/OpenSearch/HttpClient/SymfonyHttpClientFactory.php index 5a95ae6f..e70e4378 100644 --- a/src/OpenSearch/SymfonyHttpClientFactory.php +++ b/src/OpenSearch/HttpClient/SymfonyHttpClientFactory.php @@ -2,9 +2,11 @@ declare(strict_types=1); -namespace OpenSearch; +namespace OpenSearch\HttpClient; +use OpenSearch\Client; use Psr\Http\Client\ClientInterface; +use Psr\Log\LoggerInterface; use Symfony\Component\HttpClient\HttpClient; use Symfony\Component\HttpClient\Psr18Client; use Symfony\Component\HttpClient\RetryableHttpClient; @@ -14,10 +16,16 @@ */ class SymfonyHttpClientFactory implements HttpClientFactoryInterface { + public function __construct( + protected int $maxRetries = 0, + protected ?LoggerInterface $logger = null, + ) { + } + /** * {@inheritdoc} */ - public static function create(array $options): ClientInterface + public function create(array $options): ClientInterface { if (!isset($options['base_uri'])) { throw new \InvalidArgumentException('The base_uri option is required.'); @@ -31,16 +39,11 @@ public static function create(array $options): ClientInterface ], ]; $options = array_merge_recursive($defaults, $options); - $maxRetries = $options['max_retries'] ?? 0; - unset($options['max_retries']); - - $logger = $options['logger'] ?? null; - unset($options['logger']); $symfonyClient = HttpClient::create()->withOptions($options); - if ($maxRetries > 0) { - $symfonyClient = new RetryableHttpClient($symfonyClient, null, $maxRetries, $logger); + if ($this->maxRetries > 0) { + $symfonyClient = new RetryableHttpClient($symfonyClient, null, $this->maxRetries, $this->logger); } return new Psr18Client($symfonyClient); diff --git a/tests/GuzzleHttpClientFactoryTest.php b/tests/HttpClient/GuzzleHttpClientFactoryTest.php similarity index 62% rename from tests/GuzzleHttpClientFactoryTest.php rename to tests/HttpClient/GuzzleHttpClientFactoryTest.php index ba81f20f..2e7f02ad 100644 --- a/tests/GuzzleHttpClientFactoryTest.php +++ b/tests/HttpClient/GuzzleHttpClientFactoryTest.php @@ -2,28 +2,26 @@ declare(strict_types=1); -namespace OpenSearch\Tests; +namespace OpenSearch\Tests\HttpClient; -use OpenSearch\GuzzleHttpClientFactory; +use OpenSearch\HttpClient\GuzzleHttpClientFactory; use PHPUnit\Framework\TestCase; use Psr\Http\Client\ClientInterface; -use Psr\Log\NullLogger; /** * Test the Guzzle HTTP client factory. * - * @coversDefaultClass \OpenSearch\GuzzleHttpClientFactory + * @coversDefaultClass \OpenSearch\HttpClient\GuzzleHttpClientFactory */ class GuzzleHttpClientFactoryTest extends TestCase { public function testCreate() { - $client = GuzzleHttpClientFactory::create([ + $factory = new GuzzleHttpClientFactory(2); + $client = $factory->create([ 'base_uri' => 'http://example.com', 'verify' => true, - 'max_retries' => 2, 'auth' => ['username', 'password'], - 'logger' => new NullLogger(), ]); $this->assertInstanceOf(ClientInterface::class, $client); diff --git a/tests/HttpClient/GuzzleRetryDeciderTest.php b/tests/HttpClient/GuzzleRetryDeciderTest.php new file mode 100644 index 00000000..7d827f75 --- /dev/null +++ b/tests/HttpClient/GuzzleRetryDeciderTest.php @@ -0,0 +1,73 @@ +assertFalse($decider(2, null, null, null)); + } + + public function test500orNoExceptionDoesNotRetry(): void + { + $decider = new GuzzleRetryDecider(2); + $this->assertFalse($decider(0, null, null, null)); + } + + /** + * @covers ::__invoke + */ + public function testConnectExceptionRetries(): void + { + $logger = new TestLogger(); + $decider = new GuzzleRetryDecider(2, $logger); + $this->assertTrue($decider(0, null, null, new ConnectException('Error', $this->createMock(RequestInterface::class)))); + $this->assertTrue($logger->hasWarning([ + 'level' => 'warning', + 'message' => 'Retrying request {retries} of {maxRetries}: {exception}', + 'context' => [ + 'retries' => 0, + 'maxRetries' => 2, + 'exception' => 'Error', + ], + ])); + } + + public function testStatus500Retries(): void + { + $logger = new TestLogger(); + $decider = new GuzzleRetryDecider(2, $logger); + $response = $this->createMock(ResponseInterface::class); + $response->method('getStatusCode')->willReturn(500); + + $this->assertTrue($decider(0, null, $response, null)); + $this->assertTrue($logger->hasWarning([ + 'level' => 'warning', + 'message' => 'Retrying request {retries} of {maxRetries}: Status code {status}', + 'context' => [ + 'retries' => 0, + 'maxRetries' => 2, + 'status' => 500, + ], + ])); + } +} diff --git a/tests/SymfonyHttpClientFactoryTest.php b/tests/HttpClient/SymfonyHttpClientFactoryTest.php similarity index 65% rename from tests/SymfonyHttpClientFactoryTest.php rename to tests/HttpClient/SymfonyHttpClientFactoryTest.php index 87050388..1724f6c8 100644 --- a/tests/SymfonyHttpClientFactoryTest.php +++ b/tests/HttpClient/SymfonyHttpClientFactoryTest.php @@ -2,25 +2,25 @@ declare(strict_types=1); -namespace OpenSearch\Tests; +namespace OpenSearch\Tests\HttpClient; -use OpenSearch\SymfonyHttpClientFactory; +use OpenSearch\HttpClient\SymfonyHttpClientFactory; use PHPUnit\Framework\TestCase; use Psr\Http\Client\ClientInterface; /** * Test the Symfony HTTP client factory. * - * @coversDefaultClass \OpenSearch\SymfonyHttpClientFactory + * @coversDefaultClass \OpenSearch\HttpClient\SymfonyHttpClientFactory */ class SymfonyHttpClientFactoryTest extends TestCase { public function testCreate() { - $client = SymfonyHttpClientFactory::create([ + $factory = new SymfonyHttpClientFactory(2); + $client = $factory->create([ 'base_uri' => 'http://example.com', 'verify_peer' => false, - 'max_retries' => 2, 'auth_basic' => ['username', 'password'], ]);