Split out decider and add tests

Signed-off-by: Kim Pepper <[email protected]>

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

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": {

src/OpenSearch/HttpClient/GuzzleHttpClientFactory.php

@@ -0,0 +1,58 @@
+<?php
+
+declare(strict_types=1);
+
+namespace OpenSearch\HttpClient;
+
+use GuzzleHttp\Client as GuzzleClient;
+use GuzzleHttp\HandlerStack;
+use GuzzleHttp\Middleware;
+use OpenSearch\Client;
+use Psr\Http\Client\ClientInterface;
+use Psr\Log\LoggerInterface;
+
+/**
+ * Builds an OpenSearch client using Guzzle.
+ */
+class GuzzleHttpClientFactory implements HttpClientFactoryInterface
+{
+    public function __construct(
+        protected int $maxRetries = 0,
+        protected ?LoggerInterface $logger = null,
+    ) {
+    }
+
+    /**
+     * {@inheritdoc}
+     */
+    public function create(array $options): ClientInterface
+    {
+        if (!isset($options['base_uri'])) {
+            throw new \InvalidArgumentException('The base_uri option is required.');
+        }
+        // Set default configuration.
+        $defaults = [
+            'headers' => [
+                '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);
+    }
+
+}

src/OpenSearch/HttpClient/GuzzleRetryDecider.php

@@ -0,0 +1,51 @@
+<?php
+
+namespace OpenSearch\HttpClient;
+
+use GuzzleHttp\Exception\ConnectException;
+use Psr\Http\Message\RequestInterface;
+use Psr\Http\Message\ResponseInterface;
+use Psr\Log\LoggerInterface;
+
+/**
+ * Retry decider for Guzzle HTTP Client.
+ */
+class GuzzleRetryDecider
+{
+    public function __construct(
+        protected ?int $maxRetries = 0,
+        protected ?LoggerInterface $logger = null,
+    ) {
+    }
+
+    public function __invoke(int $retries, ?RequestInterface $request, ?ResponseInterface $response, $exception): bool
+    {
+        if ($retries >= $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;
+    }
+}

src/OpenSearch/HttpClientFactoryInterface.php → 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<string,mixed> $options
      */
-    public static function create(array $options): ClientInterface;
+    public function create(array $options): ClientInterface;
 
 }

src/OpenSearch/SymfonyHttpClientFactory.php → 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);

tests/GuzzleHttpClientFactoryTest.php → 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);

tests/HttpClient/GuzzleRetryDeciderTest.php

@@ -0,0 +1,73 @@
+<?php
+
+declare(strict_types=1);
+
+namespace OpenSearch\Tests\HttpClient;
+
+use ColinODell\PsrTestLogger\TestLogger;
+use GuzzleHttp\Exception\ConnectException;
+use OpenSearch\HttpClient\GuzzleRetryDecider;
+use PHPUnit\Framework\TestCase;
+use Psr\Http\Message\RequestInterface;
+use Psr\Http\Message\ResponseInterface;
+
+/**
+ * Test the Guzzle retry decider.
+ *
+ * @coversDefaultClass \OpenSearch\HttpClient\GuzzleRetryDecider
+ */
+class GuzzleRetryDeciderTest extends TestCase
+{
+    /**
+     * @covers ::__invoke
+     */
+    public function testMaxRetriesDoesNotRetry(): void
+    {
+        $decider = new GuzzleRetryDecider(2);
+        $this->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,
+            ],
+        ]));
+    }
+}