Handle Elastica Transport errors (#1465)

* Elastica/Client uses getTransportInfo and throws an exception if couldn't connect to ES host * Pass errored response body in transportInfo * Revert changes is composer.json - no longer point to a fork of ruflin/Elistica * Switch to ruflin/elastica 6.1 in composer.json * Factor out the forbidden HTTP codes Put in configuration instead of hard-coding in the Client class Unit test the configuration Defaults are 400, 403, 404 Refactor unit tests and mocks for the Client * Switch from Exception to Elastica\Exception\ClientException Update unit test * Refactor following discussion in ruflin/Elastica * Remove double carrige return * Handle responseData being a string or an array * Update composer.json
FriendsOfSymfony · Jan 29, 2019 · 8b30ba6 · 8b30ba6
1 parent c2c5e06
commit 8b30ba6
Show file tree

Hide file tree

Showing 5 changed files with 106 additions and 11 deletions.
diff --git a/composer.json b/composer.json
@@ -19,7 +19,7 @@
         "symfony/property-access": "^3.4|^4",
         "pagerfanta/pagerfanta": "^1.0.5|^2.0",
         "psr/log": "^1.0",
-        "ruflin/elastica": "^5.3|^6.1"
+        "ruflin/elastica": "^5.3.5|^6.1.1"
     },
     "require-dev": {
         "doctrine/orm": "^2.5",

diff --git a/src/DependencyInjection/Configuration.php b/src/DependencyInjection/Configuration.php
@@ -452,6 +452,15 @@ private function addClientsSection(ArrayNodeDefinition $rootNode)
                                         ->scalarNode('host')->end()
                                         ->scalarNode('port')->end()
                                         ->scalarNode('proxy')->end()
+                                        ->arrayNode('http_error_codes')
+                                            ->beforeNormalization()
+                                                ->ifTrue(function ($v) { return !is_array($v); })
+                                                ->then(function ($v) { return array($v); })
+                                            ->end()
+                                            ->cannotBeEmpty()
+                                            ->defaultValue([400, 403, 404])
+                                            ->prototype('scalar')->end()
+                                        ->end()
                                         ->scalarNode('aws_access_key_id')->end()
                                         ->scalarNode('aws_secret_access_key')->end()
                                         ->scalarNode('aws_region')->end()

diff --git a/src/Elastica/Client.php b/src/Elastica/Client.php
@@ -12,6 +12,7 @@
 namespace FOS\ElasticaBundle\Elastica;
 
 use Elastica\Client as BaseClient;
+use Elastica\Exception\ClientException;
 use Elastica\Request;
 use FOS\ElasticaBundle\Logger\ElasticaLogger;
 use Symfony\Component\Stopwatch\Stopwatch;
@@ -50,6 +51,16 @@ public function request($path, $method = Request::GET, $data = [], array $query
         $response = parent::request($path, $method, $data, $query, $contentType);
         $responseData = $response->getData();
 
+        $transportInfo = $response->getTransferInfo();
+        $connection = $this->getLastRequest()->getConnection();
+        $forbiddenHttpCodes = $connection->hasConfig('http_error_codes') ? $connection->getConfig('http_error_codes') : [];
+
+        if (isset($transportInfo['http_code']) && in_array($transportInfo['http_code'], $forbiddenHttpCodes, true)) {
+            $body = is_array($responseData) ? json_encode($responseData) : $responseData;
+            $message = sprintf('Error in transportInfo: response code is %s, response body is %s', $transportInfo['http_code'], $body);
+            throw new ClientException($message);
+        }
+
         if (isset($responseData['took']) && isset($responseData['hits'])) {
             $this->logQuery($path, $method, $data, $query, $response->getQueryTime(), $response->getEngineTime(), $responseData['hits']['total']);
         } else {

diff --git a/tests/Unit/DependencyInjection/ConfigurationTest.php b/tests/Unit/DependencyInjection/ConfigurationTest.php
@@ -310,6 +310,30 @@ public function testAWSConfig()
         $this->assertTrue($connection['ssl']);
     }
 
+    public function testHttpErrorCodesConfig()
+    {
+        // test defaults
+        $configuration = $this->getConfigs([
+            'clients' => [
+                'default' => [
+                ],
+            ],
+        ]);
+        $connection = $configuration['clients']['default']['connections'][0];
+        $this->assertSame([400, 403, 404], $connection['http_error_codes']);
+
+        // test custom
+        $configuration = $this->getConfigs([
+            'clients' => [
+                'default' => [
+                    'http_error_codes' => ['HTTP_ERROR_CODE']
+                ],
+            ],
+        ]);
+        $connection = $configuration['clients']['default']['connections'][0];
+        $this->assertSame(['HTTP_ERROR_CODE'], $connection['http_error_codes']);
+    }
+
     private function getConfigs(array $configArray)
     {
         $configuration = new Configuration(true);

diff --git a/tests/Unit/Elastica/ClientTest.php b/tests/Unit/Elastica/ClientTest.php
@@ -11,24 +11,49 @@
 
 namespace FOS\ElasticaBundle\Tests\Unit\Client;
 
+use Elastica\Client as BaseClient;
 use Elastica\Connection;
+use Elastica\JSON;
 use Elastica\Request;
 use Elastica\Response;
 use Elastica\Transport\NullTransport;
 use FOS\ElasticaBundle\Elastica\Client;
 use FOS\ElasticaBundle\Logger\ElasticaLogger;
 use PHPUnit\Framework\TestCase;
+use Elastica\Exception\ClientException;
 
 class ClientTest extends TestCase
 {
-    public function testRequestsAreLogged()
+    private function getConnectionMock()
+    {
+        $connection = $this->createMock(Connection::class);
+        $connection->expects($this->any())->method('toArray')->will($this->returnValue([]));
+        return $connection;
+    }
+
+    private function getClientMock(Response $response = null, $connection = null)
     {
         $transport = new NullTransport();
+        if ($response) {
+            $transport->setResponse($response);
+        }
 
-        $connection = $this->createMock(Connection::class);
+        if (!$connection) {
+            $connection = $this->getConnectionMock();
+        }
         $connection->expects($this->any())->method('getTransportObject')->will($this->returnValue($transport));
-        $connection->expects($this->any())->method('toArray')->will($this->returnValue([]));
 
+        $client = $this->getMockBuilder(Client::class)
+            ->setMethods(['getConnection'])
+            ->getMock();
+
+        $client->expects($this->any())->method('getConnection')->will($this->returnValue($connection));
+        return $client;
+    }
+
+    public function testRequestsAreLogged()
+    {
+        $client = $this->getClientMock();
         $logger = $this->createMock(ElasticaLogger::class);
         $logger
             ->expects($this->once())
@@ -44,17 +69,43 @@ public function testRequestsAreLogged()
                 $this->isType('array'),
                 $this->isType('array')
             );
-
-        $client = $this->getMockBuilder(Client::class)
-            ->setMethods(['getConnection'])
-            ->getMock();
-
-        $client->expects($this->any())->method('getConnection')->will($this->returnValue($connection));
-
         $client->setLogger($logger);
 
         $response = $client->request('foo');
 
         $this->assertInstanceOf(Response::class, $response);
     }
+
+    public function testRequestsWithTransportInfoErrorsRaiseExceptions()
+    {
+        $httpCode = 403;
+        $responseString = JSON::stringify(['message' => 'some AWS error']);
+        $transferInfo = [
+            'request_header' => 'bar',
+            'http_code' => $httpCode,
+            'body' => $responseString,
+        ];
+        $response = new Response($responseString);
+        $response->setTransferInfo($transferInfo);
+
+        $connection = $this->getConnectionMock();
+        $connection
+          ->expects($this->exactly(1))
+          ->method('hasConfig')
+          ->with('http_error_codes')
+          ->willReturn(true)
+        ;
+        $connection
+          ->expects($this->exactly(1))
+          ->method('getConfig')
+          ->with('http_error_codes')
+          ->willReturn([400, 403, 404])
+        ;
+        $client = $this->getClientMock($response, $connection);
+
+        $desiredMessage = sprintf('Error in transportInfo: response code is %d, response body is %s', $httpCode, $responseString);
+        $this->expectException(ClientException::class);
+        $this->expectExceptionMessage($desiredMessage);
+        $response = $client->request('foo');
+    }
 }