Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle Elastica Transport errors #1465

Merged
merged 11 commits into from Jan 29, 2019
2 changes: 1 addition & 1 deletion composer.json
Expand Up @@ -19,7 +19,7 @@
"symfony/property-access": "^3.2|^4",
"pagerfanta/pagerfanta": "^1.0.5|^2.0",
"psr/log": "^1.0",
"ruflin/elastica": "^5.2.1|^6.0"
"ruflin/elastica": "^5.2.1|^6.1"
},
"require-dev": {
"doctrine/orm": "^2.5",
Expand Down
9 changes: 9 additions & 0 deletions src/DependencyInjection/Configuration.php
Expand Up @@ -456,6 +456,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()
Expand Down
13 changes: 12 additions & 1 deletion src/Elastica/Client.php
Expand Up @@ -48,8 +48,19 @@ 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 = isset($transportInfo['body']) ? $transportInfo['body'] : 'blank';
$message = sprintf('Error in transportInfo: response code is %s, response body is %s', $transportInfo['http_code'], $body);
throw new \Exception($message);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For debugging reasons we should throw a different exception. Maybe one of https://github.com/ruflin/Elastica/tree/master/lib/Elastica/Exception ? @ruflin ?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM. The part I'm worried here is that this could be a breaking change in Elastica? Want to open a PR against Elastica to discuss it there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the exception from the global Exception to ClientException, good point!

Gents, thank you both so much for your help and let me know if something more is needed

}

$responseData = $response->getData();
if (isset($responseData['took']) && isset($responseData['hits'])) {
$this->logQuery($path, $method, $data, $query, $response->getQueryTime(), $response->getEngineTime(), $responseData['hits']['total']);
} else {
Expand Down
24 changes: 24 additions & 0 deletions tests/Unit/DependencyInjection/ConfigurationTest.php
Expand Up @@ -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);
Expand Down
70 changes: 60 additions & 10 deletions tests/Unit/Elastica/ClientTest.php
Expand Up @@ -11,7 +11,9 @@

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;
Expand All @@ -21,14 +23,36 @@

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())
Expand All @@ -44,17 +68,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(\Exception::class);
$this->expectExceptionMessage($desiredMessage);
$response = $client->request('foo');
}
}