From 99d7cd147c469e20e0c8e550c431712edda4dd19 Mon Sep 17 00:00:00 2001 From: ruflin Date: Thu, 31 Dec 2015 09:00:22 +0100 Subject: [PATCH 1/2] Refactor response exception and make it more BC compatible * Revert getError changes in Response object and make it better BC compatible. See comment https://github.com/ruflin/Elastica/commit/41a7a2075837320bc9bd3bca4150e05a1ec9a115#commitcomment-15136374 * Update tests to work with above change * Introduce getFullError() for ResponseSet * Update composer dev dependencies to most recent versions --- CHANGELOG.md | 3 +- Makefile | 2 +- env/elastica/Docker56 | 7 ++-- env/elastica/composer.json | 13 +++--- lib/Elastica/Bulk/ResponseSet.php | 19 +++++++++ lib/Elastica/Query/BoolQuery.php | 2 +- lib/Elastica/Query/Boosting.php | 2 +- lib/Elastica/Query/MoreLikeThis.php | 4 +- lib/Elastica/QueryBuilder/DSL/Query.php | 1 + lib/Elastica/Response.php | 40 ++++++++++++++++--- lib/Elastica/Type/Mapping.php | 1 + test/lib/Elastica/Test/BulkTest.php | 2 +- test/lib/Elastica/Test/ClientTest.php | 2 +- .../Elastica/Test/Cluster/SettingsTest.php | 4 +- .../Test/Exception/ResponseExceptionTest.php | 6 +-- .../Elastica/Test/Filter/BoolFilterTest.php | 2 +- test/lib/Elastica/Test/Index/SettingsTest.php | 4 +- test/lib/Elastica/Test/IndexTest.php | 6 +-- test/lib/Elastica/Test/Query/FilteredTest.php | 5 +-- test/lib/Elastica/Test/ResponseTest.php | 12 ------ test/lib/Elastica/Test/SearchTest.php | 2 +- test/lib/Elastica/Test/TypeTest.php | 6 +-- 22 files changed, 92 insertions(+), 53 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8358e87cbc..60aac6c07a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,10 +7,11 @@ All notable changes to this project will be documented in this file based on the ### Backward Compatibility Breaks ### Bugfixes -- Function score query: corrected the `score_method` `average` to `avg` #975 +- Function score query: corrected the `score_method` `average` to `avg` #975 - Set `json_decode()` assoc parameter to true in `Elastica\Response` #1005 - Add `bigintConversion` to keys passed to connection config in `Elastica\Client` #1005 - Use POST instead of PUT to send bulk requests #1010 +- Revert getError changes in Response object and make it better BC compatible. See comment [here](https://github.com/ruflin/Elastica/commit/41a7a2075837320bc9bd3bca4150e05a1ec9a115#commitcomment-15136374). ### Added - Elastica\Query\MultiMatch::setFuzziness now supports being set to `AUTO` with the const `MultiMatch::FUZZINESS_AUTO` diff --git a/Makefile b/Makefile index 41432434ab..8bfe688107 100644 --- a/Makefile +++ b/Makefile @@ -5,7 +5,7 @@ TARGET?=56 # By default docker environment is used to run commands. To run without the predefined environment, set RUN_ENV=" " either as parameter or as environment variable ifndef RUN_ENV - RUN_ENV = docker run --entrypoint="" -v $(shell pwd):/elastica ruflin/elastica + RUN_ENV = docker run --workdir="/elastica" -v $(shell pwd):/elastica ruflin/elastica-dev-base endif .PHONY: clean diff --git a/env/elastica/Docker56 b/env/elastica/Docker56 index f63361d85d..805e2af841 100644 --- a/env/elastica/Docker56 +++ b/env/elastica/Docker56 @@ -8,10 +8,11 @@ RUN apt-get update && apt-get install -y \ git \ graphviz \ nano \ - php5-xsl + php5-xsl \ + zlib1g-dev # XSL and Graphviz for PhpDocumentor -RUN docker-php-ext-install sockets +RUN docker-php-ext-install sockets zip RUN rm -r /var/lib/apt/lists/* @@ -35,4 +36,4 @@ ENV PATH=/root/composer/vendor/bin:$PATH COPY composer.json /root/composer/ # Install development tools -RUN composer global install --prefer-source +RUN composer global install #--prefer-source diff --git a/env/elastica/composer.json b/env/elastica/composer.json index 4410cdbdbd..4156a94a06 100644 --- a/env/elastica/composer.json +++ b/env/elastica/composer.json @@ -12,15 +12,14 @@ } ], "require": { - "fabpot/php-cs-fixer":"1.10.*", + "phpdocumentor/phpdocumentor":"~2.8", + "fabpot/php-cs-fixer":"1.11.*", "mayflower/php-codebrowser":"~1.1", - "pdepend/pdepend":"~2.0", + "pdepend/pdepend":"~2.2", "phploc/phploc":"~2.1", - "phpdocumentor/template-zend":"~1.3", - "phpdocumentor/phpdocumentor":"~2.8", - "phpmd/phpmd":"~2.2", - "phpunit/phpunit":"~4.7", + "phpmd/phpmd":"~2.3", + "phpunit/phpunit":"~4.8", "sebastian/phpcpd":"~2.0", - "squizlabs/php_codesniffer":"~2.3" + "squizlabs/php_codesniffer":"~2.5" } } diff --git a/lib/Elastica/Bulk/ResponseSet.php b/lib/Elastica/Bulk/ResponseSet.php index f3fdd31fd8..66b0039d75 100644 --- a/lib/Elastica/Bulk/ResponseSet.php +++ b/lib/Elastica/Bulk/ResponseSet.php @@ -53,6 +53,25 @@ public function getError() return $error; } + /** + * Returns first found error (full array). + * + * @return array|string + */ + public function getFullError() + { + $error = ''; + + foreach ($this->getBulkResponses() as $bulkResponse) { + if ($bulkResponse->hasError()) { + $error = $bulkResponse->getFullError(); + break; + } + } + + return $error; + } + /** * @return bool */ diff --git a/lib/Elastica/Query/BoolQuery.php b/lib/Elastica/Query/BoolQuery.php index 2aed522a9c..c06b88d327 100644 --- a/lib/Elastica/Query/BoolQuery.php +++ b/lib/Elastica/Query/BoolQuery.php @@ -55,7 +55,7 @@ public function addMustNot($args) * @param \Elastica\Filter\AbstractFilter $filter Filter object * * @return $this - */ + */ public function addFilter(AbstractFilter $filter) { return $this->addParam('filter', $filter); diff --git a/lib/Elastica/Query/Boosting.php b/lib/Elastica/Query/Boosting.php index 26c1ad3d07..6f02eccf04 100644 --- a/lib/Elastica/Query/Boosting.php +++ b/lib/Elastica/Query/Boosting.php @@ -39,7 +39,7 @@ public function setNegativeQuery(AbstractQuery $query) /** * Set the negative_boost parameter for this Boosting Query. * - * @param Float $negativeBoost + * @param float $negativeBoost * * @return $this */ diff --git a/lib/Elastica/Query/MoreLikeThis.php b/lib/Elastica/Query/MoreLikeThis.php index e99df079f1..0a46b27ab6 100644 --- a/lib/Elastica/Query/MoreLikeThis.php +++ b/lib/Elastica/Query/MoreLikeThis.php @@ -31,7 +31,7 @@ public function setFields(array $fields) * @param array $ids Document ids * * @deprecated Option "ids" deprecated as of ES 2.0.0-beta1 and will be removed in further Elastica releases. Use "like" instead. - + * @return \Elastica\Query\MoreLikeThis Current object */ public function setIds(array $ids) @@ -57,7 +57,7 @@ public function setLike($like) * @param string $likeText * * @deprecated Option "like_text" deprecated as of ES 2.0.0-beta1 and will be removed at further Elastica releases. Use "like" instead. - + * @return $this */ public function setLikeText($likeText) diff --git a/lib/Elastica/QueryBuilder/DSL/Query.php b/lib/Elastica/QueryBuilder/DSL/Query.php index 59f8caf68c..0254f6ac11 100644 --- a/lib/Elastica/QueryBuilder/DSL/Query.php +++ b/lib/Elastica/QueryBuilder/DSL/Query.php @@ -199,6 +199,7 @@ public function field() public function filtered(AbstractQuery $query = null, AbstractFilter $filter = null) { trigger_error('Use bool() instead. Filtered query is deprecated since ES 2.0.0-beta1 and this method will be removed in further Elastica releases.', E_USER_DEPRECATED); + return new Filtered($query, $filter); } diff --git a/lib/Elastica/Response.php b/lib/Elastica/Response.php index c9375cbbeb..f24783123c 100644 --- a/lib/Elastica/Response.php +++ b/lib/Elastica/Response.php @@ -81,18 +81,48 @@ public function __construct($responseString, $responseStatus = null) /** * Error message. * - * @return array Error object + * @return string Error message */ public function getError() { - $error = array(); + $error = $this->getFullError(); + + if (!$error) { + return ''; + } + + if (is_string($error)) { + return $error; + } + + if (isset($error['root_cause'][0])) { + $error = $error['root_cause'][0]; + } + + $message = $error['reason']; + if (isset($error['index'])) { + $message .= ' [index: '.$error['index'].']'; + } + + return $message; + } + + /** + * A keyed array representing any errors that occured. + * + * In case of http://localhost:9200/_alias/test the error is a string + * + * @return array|string Error data + */ + public function getFullError() + { $response = $this->getData(); if (isset($response['error'])) { - $error = $response['error']; + return $response['error']; } - return $error; + return; } /** @@ -180,7 +210,7 @@ public function isOk() return true; } - return (isset($data['ok']) && $data['ok']); + return isset($data['ok']) && $data['ok']; } /** diff --git a/lib/Elastica/Type/Mapping.php b/lib/Elastica/Type/Mapping.php index 0240e89f46..4f6de4cd4e 100644 --- a/lib/Elastica/Type/Mapping.php +++ b/lib/Elastica/Type/Mapping.php @@ -262,6 +262,7 @@ public function toArray() * Submits the mapping and sends it to the server. * * @param array $query Query string parameters to send with mapping + * * @return \Elastica\Response Response object */ public function send(array $query = array()) diff --git a/test/lib/Elastica/Test/BulkTest.php b/test/lib/Elastica/Test/BulkTest.php index bdb68fd5fc..4e33a6aa69 100644 --- a/test/lib/Elastica/Test/BulkTest.php +++ b/test/lib/Elastica/Test/BulkTest.php @@ -388,7 +388,7 @@ public function testErrorRequest() $bulk->send(); $bulk->fail('3rd document create should produce error'); } catch (ResponseException $e) { - $error = $e->getResponseSet()->getError(); + $error = $e->getResponseSet()->getFullError(); $this->assertContains('document_already_exists_exception', $error['type']); $failures = $e->getFailures(); $this->assertInternalType('array', $failures); diff --git a/test/lib/Elastica/Test/ClientTest.php b/test/lib/Elastica/Test/ClientTest.php index 31c252eb38..09930a8e24 100644 --- a/test/lib/Elastica/Test/ClientTest.php +++ b/test/lib/Elastica/Test/ClientTest.php @@ -1167,7 +1167,7 @@ public function testRemoveHeader() */ public function testPassBigIntSettingsToConnectionConfig() { - $client = new Client(['bigintConversion' => true]); + $client = new Client(array('bigintConversion' => true)); $this->assertTrue($client->getConnection()->getConfig('bigintConversion')); } diff --git a/test/lib/Elastica/Test/Cluster/SettingsTest.php b/test/lib/Elastica/Test/Cluster/SettingsTest.php index 75c9af5fe1..d052745240 100644 --- a/test/lib/Elastica/Test/Cluster/SettingsTest.php +++ b/test/lib/Elastica/Test/Cluster/SettingsTest.php @@ -84,7 +84,7 @@ public function testSetReadOnly() $index1->getType('test')->addDocument($doc3); $this->fail('should throw read only exception'); } catch (ResponseException $e) { - $error = $e->getResponse()->getError(); + $error = $e->getResponse()->getFullError(); $this->assertContains('cluster_block_exception', $error['type']); $this->assertContains('cluster read-only', $error['reason']); } @@ -93,7 +93,7 @@ public function testSetReadOnly() $index2->getType('test')->addDocument($doc4); $this->fail('should throw read only exception'); } catch (ResponseException $e) { - $error = $e->getResponse()->getError(); + $error = $e->getResponse()->getFullError(); $this->assertContains('cluster_block_exception', $error['type']); $this->assertContains('cluster read-only', $error['reason']); } diff --git a/test/lib/Elastica/Test/Exception/ResponseExceptionTest.php b/test/lib/Elastica/Test/Exception/ResponseExceptionTest.php index 67bffc3a77..c89dbf0ae6 100644 --- a/test/lib/Elastica/Test/Exception/ResponseExceptionTest.php +++ b/test/lib/Elastica/Test/Exception/ResponseExceptionTest.php @@ -17,7 +17,7 @@ public function testCreateExistingIndex() $this->_createIndex('woo', false); $this->fail('Index created when it should fail'); } catch (ResponseException $ex) { - $error = $ex->getResponse()->getError(); + $error = $ex->getResponse()->getFullError(); $this->assertEquals('index_already_exists_exception', $error['type']); $this->assertEquals(400, $ex->getElasticsearchException()->getCode()); } @@ -43,7 +43,7 @@ public function testBadType() ))); $this->fail('Indexing with wrong type should fail'); } catch (ResponseException $ex) { - $error = $ex->getResponse()->getError(); + $error = $ex->getResponse()->getFullError(); $this->assertEquals('mapper_parsing_exception', $error['type']); $this->assertEquals(400, $ex->getElasticsearchException()->getCode()); } @@ -60,7 +60,7 @@ public function testWhatever() try { $index->search(); } catch (ResponseException $ex) { - $error = $ex->getResponse()->getError(); + $error = $ex->getResponse()->getFullError(); $this->assertEquals('index_not_found_exception', $error['type']); $this->assertEquals(404, $ex->getElasticsearchException()->getCode()); } diff --git a/test/lib/Elastica/Test/Filter/BoolFilterTest.php b/test/lib/Elastica/Test/Filter/BoolFilterTest.php index 2a93bdc76c..aa6443a38c 100644 --- a/test/lib/Elastica/Test/Filter/BoolFilterTest.php +++ b/test/lib/Elastica/Test/Filter/BoolFilterTest.php @@ -82,7 +82,7 @@ public function getTestToArrayData() * @group unit * @dataProvider getTestToArrayData() * - * @param Bool $bool + * @param bool $bool * @param array $expectedArray */ public function testToArray(BoolFilter $bool, $expectedArray) diff --git a/test/lib/Elastica/Test/Index/SettingsTest.php b/test/lib/Elastica/Test/Index/SettingsTest.php index 6ec8622543..cb4d25f417 100644 --- a/test/lib/Elastica/Test/Index/SettingsTest.php +++ b/test/lib/Elastica/Test/Index/SettingsTest.php @@ -231,7 +231,7 @@ public function testSetReadOnly() $type->addDocument($doc2); $this->fail('Should throw exception because of read only'); } catch (ResponseException $e) { - $error = $e->getResponse()->getError(); + $error = $e->getResponse()->getFullError(); $this->assertContains('cluster_block_exception', $error['type']); $this->assertContains('index write', $error['reason']); @@ -333,7 +333,7 @@ public function testNotFoundIndex() $settings = $index->getSettings()->get(); $this->fail('Should throw exception because of index not found'); } catch (ResponseException $e) { - $error = $e->getResponse()->getError(); + $error = $e->getResponse()->getFullError(); $this->assertContains('index_not_found_exception', $error['type']); } } diff --git a/test/lib/Elastica/Test/IndexTest.php b/test/lib/Elastica/Test/IndexTest.php index 648a8b3aec..adddb7d9c3 100644 --- a/test/lib/Elastica/Test/IndexTest.php +++ b/test/lib/Elastica/Test/IndexTest.php @@ -26,7 +26,7 @@ public function testMapping() $type = $index->getType('test'); $mapping = array('id' => array('type' => 'integer', 'store' => true), 'email' => array('type' => 'string', 'store' => 'no'), - 'username' => array('type' => 'string', 'store' => 'no'), 'test' => array('type' => 'integer', 'store' => 'no'),); + 'username' => array('type' => 'string', 'store' => 'no'), 'test' => array('type' => 'integer', 'store' => 'no'), ); $type->setMapping($mapping); $type->addDocument($doc); @@ -247,7 +247,7 @@ public function testExcludeFileSource() { $this->_checkPlugin('elasticsearch-mapper-attachments'); $indexMapping = array('file' => array('type' => 'attachment'), 'text' => array('type' => 'string', 'store' => true), - 'title' => array('type' => 'string', 'store' => true),); + 'title' => array('type' => 'string', 'store' => true), ); $indexParams = array('index' => array('number_of_shards' => 1, 'number_of_replicas' => 0)); @@ -669,7 +669,7 @@ public function testIndexGetMapping() $type = $index->getType('test'); $mapping = array('id' => array('type' => 'integer', 'store' => true), 'email' => array('type' => 'string', 'store' => false), - 'username' => array('type' => 'string', 'store' => false), 'test' => array('type' => 'integer', 'store' => false),); + 'username' => array('type' => 'string', 'store' => false), 'test' => array('type' => 'integer', 'store' => false), ); $type->setMapping($mapping); $index->refresh(); diff --git a/test/lib/Elastica/Test/Query/FilteredTest.php b/test/lib/Elastica/Test/Query/FilteredTest.php index b2ea70ec80..c3da239a1b 100644 --- a/test/lib/Elastica/Test/Query/FilteredTest.php +++ b/test/lib/Elastica/Test/Query/FilteredTest.php @@ -9,8 +9,7 @@ class FilteredTest extends BaseTest { - - private $errors = []; + private $errors = array(); private function setErrorHandler() { @@ -27,7 +26,7 @@ private function restoreErrorHandler() $this->assertCount(1, $this->errors); $this->assertEquals(E_USER_DEPRECATED, $this->errors[0][0]); $this->assertEquals('Use BoolQuery instead. Filtered query is deprecated since ES 2.0.0-beta1 and this class will be removed in further Elastica releases.', $this->errors[0][1]); - $this->errors = []; + $this->errors = array(); } } diff --git a/test/lib/Elastica/Test/ResponseTest.php b/test/lib/Elastica/Test/ResponseTest.php index 66f62a5544..8d36b2b559 100644 --- a/test/lib/Elastica/Test/ResponseTest.php +++ b/test/lib/Elastica/Test/ResponseTest.php @@ -128,18 +128,6 @@ public function testIsOkBulkItemsWithOkField() $this->assertTrue($response->isOk()); } - /** - * @group unit - */ - public function testArrayErrorMessage() - { - $response = new Response(json_encode(array( - 'error' => array('a', 'b'), - ))); - - $this->assertEquals(json_encode(array('a', 'b')), $response->getErrorMessage()); - } - /** * @group unit */ diff --git a/test/lib/Elastica/Test/SearchTest.php b/test/lib/Elastica/Test/SearchTest.php index 563bfdd8ab..7e211c47b6 100644 --- a/test/lib/Elastica/Test/SearchTest.php +++ b/test/lib/Elastica/Test/SearchTest.php @@ -578,7 +578,7 @@ public function testIgnoreUnavailableOption() } catch (ResponseException $e) { $exception = $e; } - $error = $exception->getResponse()->getError(); + $error = $exception->getResponse()->getFullError(); $this->assertEquals('index_not_found_exception', $error['type']); $results = $search->search($query, array(Search::OPTION_SEARCH_IGNORE_UNAVAILABLE => true)); diff --git a/test/lib/Elastica/Test/TypeTest.php b/test/lib/Elastica/Test/TypeTest.php index 02ce6a204d..104b287d95 100644 --- a/test/lib/Elastica/Test/TypeTest.php +++ b/test/lib/Elastica/Test/TypeTest.php @@ -638,7 +638,7 @@ public function testUpdateDocumentWithParameter() try { $type->updateDocument($script, array('version' => 999)); // Wrong version number to make the update fail } catch (ResponseException $e) { - $error = $e->getResponse()->getError(); + $error = $e->getResponse()->getFullError(); $this->assertContains('version_conflict_engine_exception', $error['type']); } $updatedDoc = $type->getDocument($id)->getData(); @@ -750,7 +750,7 @@ public function testUpdateDocumentWithoutSource() $type->updateDocument($script); $this->fail('Update request should fail because source is disabled. Fields param is not set'); } catch (ResponseException $e) { - $error = $e->getResponse()->getError(); + $error = $e->getResponse()->getFullError(); $this->assertContains('document_source_missing_exception', $error['type']); } @@ -760,7 +760,7 @@ public function testUpdateDocumentWithoutSource() $type->updateDocument($newDocument); $this->fail('Update request should fail because source is disabled. Fields param is set to _source'); } catch (ResponseException $e) { - $error = $e->getResponse()->getError(); + $error = $e->getResponse()->getFullError(); $this->assertContains('document_source_missing_exception', $error['type']); } } From 4562b40b8bc4530d649395a686639ebf5d848063 Mon Sep 17 00:00:00 2001 From: ruflin Date: Mon, 4 Jan 2016 13:29:48 +0100 Subject: [PATCH 2/2] Readd testArrayErrorMessage test --- lib/Elastica/Response.php | 2 -- test/lib/Elastica/Test/ResponseTest.php | 14 ++++++++++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/lib/Elastica/Response.php b/lib/Elastica/Response.php index f24783123c..e96c217651 100644 --- a/lib/Elastica/Response.php +++ b/lib/Elastica/Response.php @@ -121,8 +121,6 @@ public function getFullError() if (isset($response['error'])) { return $response['error']; } - - return; } /** diff --git a/test/lib/Elastica/Test/ResponseTest.php b/test/lib/Elastica/Test/ResponseTest.php index 8d36b2b559..3b6b8c33c3 100644 --- a/test/lib/Elastica/Test/ResponseTest.php +++ b/test/lib/Elastica/Test/ResponseTest.php @@ -140,6 +140,20 @@ public function testStringErrorMessage() $this->assertEquals('a', $response->getErrorMessage()); } + /** + * @group unit + */ + public function testArrayErrorMessage() + { + $response = new Response(json_encode(array( + 'error' => array('a', 'b'), + ))); + + $this->assertEquals(array('a', 'b'), $response->getFullError()); + } + + + /** * @group unit */