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": "dev-master"
Copy link
Member

Choose a reason for hiding this comment

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

We would like to keep stable packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Completely agreed, alas last release from ruflin/elastica is from waaay back

Release 6.0.2 @ruflin ruflin released this on May 29 · 8 commits to master since this release

See link for reference https://github.com/ruflin/Elastica/releases

And the commit that's needed for this PR, is a fairly recent one on master

ruflin/Elastica@d6f1475

I'm not sure what's the best way around this? ask ruflin to release a new version?

Copy link

Choose a reason for hiding this comment

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

@jandom Is a new release needed from master?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ruflin nothing would make me more happy – but i'm not sure what your release cycle is like/don't want to bother if you're waiting for something else to merge in

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ruflin yeah let's have a new ruflin/Elastica release, if that's okay – it'll push this PR forward

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Next week is totally appropriate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ruflin super-gentle nudge if there are any updates on the new release of ruflin/Elastica?

Copy link

Choose a reason for hiding this comment

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

Thanks for the nudge ;-) Just opened the PR to prepare the release. Now I only need a review for it: ruflin/Elastica#1547

Copy link

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yaaas thank you!

},
"require-dev": {
"doctrine/orm": "^2.5",
Expand Down
11 changes: 10 additions & 1 deletion src/Elastica/Client.php
Expand Up @@ -24,6 +24,8 @@
*/
class Client extends BaseClient
{
private $forbiddenHttpCodes = [403, 404, 400];
Copy link
Member

Choose a reason for hiding this comment

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

We should make this configurable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, let me implement this and add some tests

Choose a reason for hiding this comment

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

Please note that by intercepting 404 you break various exists functions in Elastica which send HEAD requests and check for 200 vs 404 responses.

  • Elastica\Type::exists()
  • Elastica\Index::exists()
  • Elastica\IndexTemplate::exists()


/**
* Stores created indexes to avoid recreation.
*
Expand All @@ -48,8 +50,15 @@ public function request($path, $method = Request::GET, $data = [], array $query
}

$response = parent::request($path, $method, $data, $query, $contentType);
$responseData = $response->getData();

$transportInfo = $response->getTransferInfo();
if (isset($transportInfo['http_code']) && in_array($transportInfo['http_code'], $this->forbiddenHttpCodes)) {
$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
45 changes: 37 additions & 8 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,28 @@

class ClientTest extends TestCase
{
public function testRequestsAreLogged()
private function getClientMock(Response $response = null)
{
$transport = new NullTransport();
if ($response) {
$transport->setResponse($response);
}

$connection = $this->createMock(Connection::class);
$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 +60,30 @@ 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);

$client = $this->getClientMock($response);

$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');
}
}