Fix reading bool index setting & deprecate ElasticsearchException (#1251

) * Use Response::getStatus and fix reading of bool index settings * Deprecate ElasticsearchException which is irrelevant since elasticsearch now exposes the errors as a structured array instead of a single string * Add test to prove #457 is solved by now
ruflin · Feb 20, 2017 · cad28fd · cad28fd
1 parent 07a7701
commit cad28fd
Show file tree

Hide file tree

Showing 15 changed files with 72 additions and 88 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,12 +7,17 @@ All notable changes to this project will be documented in this file based on the
 
 ### Bugfixes
 
+- Fix reading bool index settings like `\Elastica\Index\Settings::getBlocksWrite`. Elasticsearch returns all settings as strings and does not normalize bool values.
+  The getters now return the right bool value for whichever string representation is used like 'true', '1', 'on', 'yes'.
+
 ### Added
 
 ### Improvements
 
 ### Deprecated
 
+- Deprecated `\Elastica\Exception\ElasticsearchException` which is irrelevant since Elasticsearch now exposes the errors as a structured array instead of a single string.
+  Use `\Elastica\Exception\ResponseException::getResponse::getFullError` instead.
 
 ## [5.1.0](https://github.com/ruflin/Elastica/compare/5.0.0...5.1.0)
 

diff --git a/lib/Elastica/Exception/ElasticsearchException.php b/lib/Elastica/Exception/ElasticsearchException.php
@@ -1,6 +1,8 @@
 <?php
 namespace Elastica\Exception;
 
+trigger_error('Elastica\Exception\ElasticsearchException is deprecated. Use Elastica\Exception\ResponseException::getResponse::getFullError instead.', E_USER_DEPRECATED);
+
 /**
  * Elasticsearch exception.
  *
@@ -28,15 +30,13 @@ class ElasticsearchException extends \Exception implements ExceptionInterface
     /**
      * Constructs elasticsearch exception.
      *
-     * @param int   $code  Error code
-     * @param array $error Error object from elasticsearch
+     * @param int    $code  Error code
+     * @param string $error Error message from elasticsearch
      */
     public function __construct($code, $error)
     {
-        $this->_error = $error;
-        // TODO: es2 improve as now an array
-        $this->_parseError(json_encode($error));
-        parent::__construct(json_encode($error), $code);
+        $this->_parseError($error);
+        parent::__construct($error, $code);
     }
 
     /**

diff --git a/lib/Elastica/Exception/ResponseException.php b/lib/Elastica/Exception/ResponseException.php
@@ -62,9 +62,7 @@ public function getResponse()
     public function getElasticsearchException()
     {
         $response = $this->getResponse();
-        $transfer = $response->getTransferInfo();
-        $code = array_key_exists('http_code', $transfer) ? $transfer['http_code'] : 0;
 
-        return new ElasticsearchException($code, $response->getErrorMessage());
+        return new ElasticsearchException($response->getStatus(), $response->getErrorMessage());
     }
 }
diff --git a/lib/Elastica/Index.php b/lib/Elastica/Index.php
@@ -283,9 +283,8 @@ public function create(array $args = [], $options = null)
     public function exists()
     {
         $response = $this->getClient()->request($this->getName(), Request::HEAD);
-        $info = $response->getTransferInfo();
 
-        return (bool) ($info['http_code'] == 200);
+        return $response->getStatus() === 200;
     }
 
     /**

diff --git a/lib/Elastica/Index/Settings.php b/lib/Elastica/Index/Settings.php
@@ -102,6 +102,25 @@ public function get($setting = '')
         }
     }
 
+    /**
+     * Returns a setting interpreted as a bool.
+     *
+     * One can use a real bool, int(0), int(1) to set bool settings.
+     * But Elasticsearch stores and returns all settings as strings and does
+     * not normalize bool values. This method ensures a bool is returned for
+     * whichever string representation is used like 'true', '1', 'on', 'yes'.
+     *
+     * @param string $setting Setting name to return
+     *
+     * @return bool
+     */
+    public function getBool($setting)
+    {
+        $data = $this->get($setting);
+
+        return 'true' === $data || '1' === $data || 'on' === $data || 'yes' === $data;
+    }
+
     /**
      * Sets the number of replicas.
      *
@@ -111,11 +130,7 @@ public function get($setting = '')
      */
     public function setNumberOfReplicas($replicas)
     {
-        $replicas = (int) $replicas;
-
-        $data = ['number_of_replicas' => $replicas];
-
-        return $this->set($data);
+        return $this->set(['number_of_replicas' => (int) $replicas]);
     }
 
     /**
@@ -127,25 +142,23 @@ public function setNumberOfReplicas($replicas)
      */
     public function setReadOnly($readOnly = true)
     {
-        return $this->set(['blocks.write' => $readOnly]);
+        return $this->set(['blocks.read_only' => $readOnly]);
     }
 
     /**
-     * getReadOnly.
-     *
      * @return bool
      */
     public function getReadOnly()
     {
-        return $this->get('blocks.write') === 'true'; // ES returns a string for this setting
+        return $this->getBool('blocks.read_only');
     }
 
     /**
      * @return bool
      */
     public function getBlocksRead()
     {
-        return (bool) $this->get('blocks.read');
+        return $this->getBool('blocks.read');
     }
 
     /**
@@ -155,8 +168,6 @@ public function getBlocksRead()
      */
     public function setBlocksRead($state = true)
     {
-        $state = $state ? 1 : 0;
-
         return $this->set(['blocks.read' => $state]);
     }
 
@@ -165,7 +176,7 @@ public function setBlocksRead($state = true)
      */
     public function getBlocksWrite()
     {
-        return (bool) $this->get('blocks.write');
+        return $this->getBool('blocks.write');
     }
 
     /**
@@ -175,8 +186,6 @@ public function getBlocksWrite()
      */
     public function setBlocksWrite($state = true)
     {
-        $state = $state ? 1 : 0;
-
         return $this->set(['blocks.write' => $state]);
     }
 
@@ -185,13 +194,12 @@ public function setBlocksWrite($state = true)
      */
     public function getBlocksMetadata()
     {
-        // TODO will have to be replace by block.metadata.write once https://github.com/elasticsearch/elasticsearch/pull/9203 has been fixed
-        // the try/catch will have to be remove too
+        // When blocks.metadata is enabled, reading the settings is not possible anymore.
+        // So when a cluster_block_exception happened it must be enabled.
         try {
-            return (bool) $this->get('blocks.write');
+            return $this->getBool('blocks.metadata');
         } catch (ResponseException $e) {
-            if (strpos($e->getMessage(), 'ClusterBlockException') !== false) {
-                // hacky way to test if the metadata is blocked since bug 9203 is not fixed
+            if ($e->getResponse()->getFullError()['type'] === 'cluster_block_exception') {
                 return true;
             }
 
@@ -200,15 +208,15 @@ public function getBlocksMetadata()
     }
 
     /**
+     * Set to true to disable index metadata reads and writes.
+     *
      * @param bool $state OPTIONAL (default = true)
      *
      * @return \Elastica\Response
      */
     public function setBlocksMetadata($state = true)
     {
-        $state = $state ? 1 : 0;
-
-        return $this->set(['blocks.write' => $state]);
+        return $this->set(['blocks.metadata' => $state]);
     }
 
     /**

diff --git a/lib/Elastica/IndexTemplate.php b/lib/Elastica/IndexTemplate.php
@@ -78,9 +78,8 @@ public function create(array $args = [])
     public function exists()
     {
         $response = $this->request(Request::HEAD);
-        $info = $response->getTransferInfo();
 
-        return (bool) ($info['http_code'] == 200);
+        return $response->getStatus() === 200;
     }
 
     /**

diff --git a/lib/Elastica/Query.php b/lib/Elastica/Query.php
@@ -46,13 +46,13 @@ public function __construct($query = null)
     }
 
     /**
-     * Transforms a string or an array to a query object.
+     * Transforms the argument to a query object.
      *
-     * If query is empty,
+     * For example, an empty argument will return a \Elastica\Query with a \Elastica\Query\MatchAll.
      *
      * @param mixed $query
      *
-     * @throws \Elastica\Exception\NotImplementedException
+     * @throws InvalidException For an invalid argument
      *
      * @return self
      */
@@ -77,8 +77,7 @@ public static function create($query)
 
         }
 
-        // TODO: Implement queries without
-        throw new NotImplementedException();
+        throw new InvalidException('Unexpected argument to create a query for.');
     }
 
     /**

diff --git a/lib/Elastica/Response.php b/lib/Elastica/Response.php
@@ -27,13 +27,6 @@ class Response
      */
     protected $_responseString = '';
 
-    /**
-     * Error.
-     *
-     * @var bool Error
-     */
-    protected $_error = false;
-
     /**
      * Transfer info.
      *
@@ -113,11 +106,11 @@ public function getError()
     }
 
     /**
-     * A keyed array representing any errors that occured.
+     * A keyed array representing any errors that occurred.
      *
      * In case of http://localhost:9200/_alias/test the error is a string
      *
-     * @return array|string Error data
+     * @return array|string|null Error data or null if there is no error
      */
     public function getFullError()
     {
@@ -133,13 +126,7 @@ public function getFullError()
      */
     public function getErrorMessage()
     {
-        $error = $this->getError();
-
-        if (!is_string($error)) {
-            $error = json_encode($error);
-        }
-
-        return $error;
+        return $this->getError();
     }
 
     /**
@@ -227,18 +214,15 @@ public function getData()
     {
         if ($this->_response == null) {
             $response = $this->_responseString;
-            if ($response === false) {
-                $this->_error = true;
-            } else {
-                try {
-                    if ($this->getJsonBigintConversion()) {
-                        $response = JSON::parse($response, true, 512, JSON_BIGINT_AS_STRING);
-                    } else {
-                        $response = JSON::parse($response);
-                    }
-                } catch (JSONParseException $e) {
-                    // leave response as is if parse fails
+
+            try {
+                if ($this->getJsonBigintConversion()) {
+                    $response = JSON::parse($response, true, 512, JSON_BIGINT_AS_STRING);
+                } else {
+                    $response = JSON::parse($response);
                 }
+            } catch (JSONParseException $e) {
+                // leave response as is if parse fails
             }
 
             if (empty($response)) {

diff --git a/lib/Elastica/Status.php b/lib/Elastica/Status.php
@@ -106,9 +106,8 @@ public function getIndicesWithAlias($alias)
         try {
             $response = $this->_client->request('/_alias/'.$alias);
         } catch (ResponseException $e) {
-            $transferInfo = $e->getResponse()->getTransferInfo();
             // 404 means the index alias doesn't exist which means no indexes have it.
-            if ($transferInfo['http_code'] === 404) {
+            if ($e->getResponse()->getStatus() === 404) {
                 return [];
             }
             // If we don't have a 404 then this is still unexpected so rethrow the exception.

diff --git a/lib/Elastica/Type.php b/lib/Elastica/Type.php
@@ -522,8 +522,7 @@ public function setSerializer($serializer)
     public function exists()
     {
         $response = $this->getIndex()->request($this->getName(), Request::HEAD);
-        $info = $response->getTransferInfo();
 
-        return $info['http_code'] == 200;
+        return $response->getStatus() === 200;
     }
 }
diff --git a/test/lib/Elastica/Test/Bulk/ResponseSetTest.php b/test/lib/Elastica/Test/Bulk/ResponseSetTest.php
@@ -139,7 +139,7 @@ public function isOkDataProvider()
      */
     protected function _createResponseSet(array $responseData, array $actions)
     {
-        $client = $this->createMock('Elastica\\Client', ['request']);
+        $client = $this->createMock('Elastica\\Client');
 
         $client->expects($this->once())
             ->method('request')

diff --git a/test/lib/Elastica/Test/Exception/ElasticsearchExceptionTest.php b/test/lib/Elastica/Test/Exception/ElasticsearchExceptionTest.php
diff --git a/test/lib/Elastica/Test/Exception/ResponseExceptionTest.php b/test/lib/Elastica/Test/Exception/ResponseExceptionTest.php
@@ -19,7 +19,7 @@ public function testCreateExistingIndex()
         } catch (ResponseException $ex) {
             $error = $ex->getResponse()->getFullError();
             $this->assertEquals('index_already_exists_exception', $error['type']);
-            $this->assertEquals(400, $ex->getElasticsearchException()->getCode());
+            $this->assertEquals(400, $ex->getResponse()->getStatus());
         }
     }
 
@@ -45,7 +45,7 @@ public function testBadType()
         } catch (ResponseException $ex) {
             $error = $ex->getResponse()->getFullError();
             $this->assertEquals('mapper_parsing_exception', $error['type']);
-            $this->assertEquals(400, $ex->getElasticsearchException()->getCode());
+            $this->assertEquals(400, $ex->getResponse()->getStatus());
         }
     }
 
@@ -62,7 +62,7 @@ public function testWhatever()
         } catch (ResponseException $ex) {
             $error = $ex->getResponse()->getFullError();
             $this->assertEquals('index_not_found_exception', $error['type']);
-            $this->assertEquals(404, $ex->getElasticsearchException()->getCode());
+            $this->assertEquals(404, $ex->getResponse()->getStatus());
         }
     }
 }
diff --git a/test/lib/Elastica/Test/Index/SettingsTest.php b/test/lib/Elastica/Test/Index/SettingsTest.php
@@ -199,6 +199,7 @@ public function testSetReadOnly()
         // Try to add doc to read only index
         $index->getSettings()->setReadOnly(true);
         $this->assertTrue($index->getSettings()->getReadOnly());
+        $this->assertTrue($index->exists());
 
         try {
             $type->addDocument($doc2);
@@ -207,7 +208,7 @@ public function testSetReadOnly()
             $error = $e->getResponse()->getFullError();
 
             $this->assertContains('cluster_block_exception', $error['type']);
-            $this->assertContains('index write', $error['reason']);
+            $this->assertContains('read-only', $error['reason']);
         }
 
         // Remove read only, add document