Rework JSON Serializer to Throw Useful Exceptions

Closes #175 Previously `SmartSerializer` was the only class that sometimes threw exceptions when something went wrong during JSON deserialization. This changes that so all serializers throw. - `JsonSerializationError` is thrown when something goes wrong during `json_encode` - `JsonDeserializationError` is thrown when something goes wrong during `json_decode` Both are subclasses of `JsonErrorException`, so anyone already catching that should be able to continue doing so. There's a also a bit of restructuring here and some additional tests to cover some more paths in the serializers.
elastic · Mar 2, 2015 · 850240a · 850240a
1 parent a273a95
commit 850240a
Show file tree

Hide file tree

Showing 9 changed files with 190 additions and 103 deletions.
diff --git a/src/Elasticsearch/Common/Exceptions/Serializer/JsonDeserializationError.php b/src/Elasticsearch/Common/Exceptions/Serializer/JsonDeserializationError.php
@@ -0,0 +1,15 @@
+<?php
+
+namespace Elasticsearch\Common\Exceptions\Serializer;
+
+/**
+ * Thrown when there's an error unserializing a JSON string to a PHP array.
+ *
+ * @author   Christopher Davis <[email protected]>
+ * @license  http://www.apache.org/licenses/LICENSE-2.0 Apache2
+ * @link     http://elasticsearch.org
+ */
+final class JsonDeserializationError extends JsonErrorException
+{
+    // noop
+}
diff --git a/src/Elasticsearch/Common/Exceptions/Serializer/JsonSerializationError.php b/src/Elasticsearch/Common/Exceptions/Serializer/JsonSerializationError.php
@@ -0,0 +1,16 @@
+<?php
+
+namespace Elasticsearch\Common\Exceptions\Serializer;
+
+/**
+ * Thrown when there's an error serialization a PHP object or array to a JSON
+ * string.
+ *
+ * @author   Christopher Davis <[email protected]>
+ * @license  http://www.apache.org/licenses/LICENSE-2.0 Apache2
+ * @link     http://elasticsearch.org
+ */
+final class JsonSerializationError extends JsonErrorException
+{
+    // noop
+}
diff --git a/src/Elasticsearch/Serializers/AbstractJsonSerializer.php b/src/Elasticsearch/Serializers/AbstractJsonSerializer.php
@@ -0,0 +1,62 @@
+<?php
+
+namespace Elasticsearch\Serializers;
+
+use Elasticsearch\Common\Exceptions\Serializer\JsonSerializationError;
+use Elasticsearch\Common\Exceptions\Serializer\JsonDeserializationError;
+
+/**
+ * An abstract base class for serializers that go to/from JSON.
+ *
+ * @author   Christopher Davis <[email protected]>
+ * @license  http://www.apache.org/licenses/LICENSE-2.0 Apache2
+ * @link     http://elasticsearch.org
+ */
+abstract class AbstractJsonSerializer implements SerializerInterface
+{
+    /**
+     * Encode a php object or array to a JSON string
+     *
+     * @param   object|array $value
+     * @throws  JsonSerializationError if something goes wrong during encoding
+     * @return  string
+     */
+    protected static function jsonEncode($value)
+    {
+        $result = json_encode($value);
+
+        if (static::hasJsonError()) {
+            throw new JsonSerializationError(json_last_error(), $value, $result);
+        }
+
+        return '[]' === $result ? '{}' : $result;
+    }
+
+    /**
+     * Decode a JSON string to a PHP array.
+     *
+     * @param   string $json
+     * @throws  JsonDeserializationError if something goes wrong during decoding
+     * @return  array
+     */
+    protected static function jsonDecode($json)
+    {
+        $result = json_decode($json, true);
+
+        if (static::hasJsonError()) {
+            throw new JsonDeserializationError(json_last_error(), $json, $result);
+        }
+
+        return $result;
+    }
+
+    /**
+     * Check to see if the last `json_{encode,decode}` call produced an error.
+     *
+     * @return  boolean
+     */
+    protected static function hasJsonError()
+    {
+        return json_last_error() !== JSON_ERROR_NONE;
+    }
+}
diff --git a/src/Elasticsearch/Serializers/ArrayToJSONSerializer.php b/src/Elasticsearch/Serializers/ArrayToJSONSerializer.php
@@ -16,10 +16,8 @@
  * @license  http://www.apache.org/licenses/LICENSE-2.0 Apache2
  * @link     http://elasticsearch.org
  */
-class ArrayToJSONSerializer implements SerializerInterface
+class ArrayToJSONSerializer extends AbstractJsonSerializer
 {
-
-
     /**
      * Serialize assoc array into JSON string
      *
@@ -31,16 +29,9 @@ public function serialize($data)
     {
         if (is_string($data) === true) {
             return $data;
-        } else {
-            $data = json_encode($data);
-            if ($data === '[]') {
-                return '{}';
-            } else {
-                return $data;
-            }
         }
 
-
+        return $this->jsonEncode($data);
     }
 
 
@@ -54,7 +45,6 @@ public function serialize($data)
      */
     public function deserialize($data, $headers)
     {
-        return json_decode($data, true);
-
+        return $this->jsonDecode($data);
     }
-}
+}
diff --git a/src/Elasticsearch/Serializers/EverythingToJSONSerializer.php b/src/Elasticsearch/Serializers/EverythingToJSONSerializer.php
@@ -15,9 +15,8 @@
  * @license  http://www.apache.org/licenses/LICENSE-2.0 Apache2
  * @link     http://elasticsearch.org
  */
-class EverythingToJSONSerializer implements SerializerInterface
+class EverythingToJSONSerializer extends AbstractJsonSerializer
 {
-
     /**
      * Serialize assoc array into JSON string
      *
@@ -27,15 +26,9 @@ class EverythingToJSONSerializer implements SerializerInterface
      */
     public function serialize($data)
     {
-        $data = json_encode($data);
-        if ($data === '[]') {
-            return '{}';
-        } else {
-            return $data;
-        }
+        return $this->jsonEncode($data);
     }
 
-
     /**
      * Deserialize JSON into an assoc array
      *
@@ -46,7 +39,6 @@ public function serialize($data)
      */
     public function deserialize($data, $headers)
     {
-        return json_decode($data, true);
-
+        return $this->jsonDecode($data);
     }
-}
+}
diff --git a/src/Elasticsearch/Serializers/SmartSerializer.php b/src/Elasticsearch/Serializers/SmartSerializer.php
@@ -18,34 +18,8 @@
  * @license  http://www.apache.org/licenses/LICENSE-2.0 Apache2
  * @link     http://elasticsearch.org
  */
-class SmartSerializer implements SerializerInterface
+class SmartSerializer extends ArrayToJSONSerializer
 {
-
-
-    /**
-     * Serialize assoc array into JSON string
-     *
-     * @param string|array $data Assoc array to encode into JSON
-     *
-     * @return string
-     */
-    public function serialize($data)
-    {
-        if (is_string($data) === true) {
-            return $data;
-        } else {
-            $data = json_encode($data);
-            if ($data === '[]') {
-                return '{}';
-            } else {
-                return $data;
-            }
-        }
-
-
-    }
-
-
     /**
      * Deserialize by introspecting content_type. Tries to deserialize JSON,
      * otherwise returns string
@@ -58,42 +32,10 @@ public function serialize($data)
      */
     public function deserialize($data, $headers)
     {
-        if (isset($headers['content_type']) === true) {
-            if (strpos($headers['content_type'], 'json') !== false) {
-
-                return $this->decode($data);
-
-            } else {
-                //Not json, return as string
-                return $data;
-            }
-
-        } else {
-            //No content headers, assume json
-            return $this->decode($data);
-        }
-
-
-    }
-
-    /**
-     * @todo For 2.0, remove the E_NOTICE check before raising the exception.
-     *
-     * @param $data
-     *
-     * @return array
-     * @throws JsonErrorException
-     */
-    private function decode($data)
-    {
-        $result = @json_decode($data, true);
-
-        // Throw exception only if E_NOTICE is on to maintain backwards-compatibility on systems that silently ignore E_NOTICEs.
-        if (json_last_error() !== JSON_ERROR_NONE && (error_reporting() & E_NOTICE) === E_NOTICE) {
-            $e = new JsonErrorException(json_last_error(), $data, $result);
-            throw $e;
+        if (isset($headers['content_type']) && strpos($headers['content_type'], 'json') === false) {
+            return $data;
         }
 
-        return $result;
+        return $this->jsonDecode($data);
     }
-}
+}
diff --git a/tests/Elasticsearch/Tests/Serializers/ArrayToJSONSerializerTest.php b/tests/Elasticsearch/Tests/Serializers/ArrayToJSONSerializerTest.php
@@ -9,19 +9,13 @@
 
 use Elasticsearch\Serializers\ArrayToJSONSerializer;
 use PHPUnit_Framework_TestCase;
-use Mockery as m;
 
 /**
  * Class ArrayToJSONSerializerTest
  * @package Elasticsearch\Tests\Serializers
  */
 class ArrayToJSONSerializerTest extends PHPUnit_Framework_TestCase
 {
-    public function tearDown()
-    {
-        m::close();
-    }
-
     public function testSerializeArray()
     {
         $serializer = new ArrayToJSONSerializer();
@@ -53,4 +47,4 @@ public function testDeserializeJSON()
         $body = json_decode($body, true);
         $this->assertEquals($body, $ret);
     }
-}
+}
diff --git a/tests/Elasticsearch/Tests/Serializers/EverythingToJSONSerializerTest.php b/tests/Elasticsearch/Tests/Serializers/EverythingToJSONSerializerTest.php
@@ -9,19 +9,13 @@
 
 use Elasticsearch\Serializers\EverythingToJSONSerializer;
 use PHPUnit_Framework_TestCase;
-use Mockery as m;
 
 /**
  * Class EverythingToJSONSerializerTest
  * @package Elasticsearch\Tests\Serializers
  */
 class EverythingToJSONSerializerTest extends PHPUnit_Framework_TestCase
 {
-    public function tearDown()
-    {
-        m::close();
-    }
-
     public function testSerializeArray()
     {
         $serializer = new EverythingToJSONSerializer();
@@ -33,6 +27,15 @@ public function testSerializeArray()
         $this->assertEquals($body, $ret);
     }
 
+    public function testSerializeEmptyArrayReturnsEmptyJsonObject()
+    {
+        $serializer = new EverythingToJSONSerializer();
+
+        $ret = $serializer->serialize(array());
+
+        $this->assertEquals('{}', $ret);
+    }
+
     public function testSerializeString()
     {
         $serializer = new EverythingToJSONSerializer();
@@ -44,6 +47,18 @@ public function testSerializeString()
         $this->assertEquals($body, $ret);
     }
 
+    /**
+     * @expectedException Elasticsearch\Common\Exceptions\Serializer\JsonSerializationError
+     */
+    public function testSerializeArrayWithInvalidUtf8ThrowsException()
+    {
+        $serializer = new EverythingToJSONSerializer();
+
+        $body = array("value" => "\xB1\x31"); // invalid UTF-8 byte sequence
+
+        $serializer->serialize($body);
+    }
+
     public function testDeserializeJSON()
     {
         $serializer = new EverythingToJSONSerializer();
@@ -54,4 +69,15 @@ public function testDeserializeJSON()
         $body = json_decode($body, true);
         $this->assertEquals($body, $ret);
     }
-}
+
+    /**
+     * @expectedException Elasticsearch\Common\Exceptions\Serializer\JsonDeserializationError
+     */
+    public function testDeserializeWithInvalidJsonThrowsException()
+    {
+        $serializer = new EverythingToJSONSerializer();
+        $body = '{"field":}';
+
+        $serializer->deserialize($body, array());
+    }
+}