From 921a88512439f511a7e4e90556a5d31c118eef3b Mon Sep 17 00:00:00 2001 From: Lena Orobei Date: Fri, 30 Aug 2019 10:59:55 -0500 Subject: [PATCH 1/3] Contract for JSON encoding/decoding --- design-documents/data-format/json.md | 245 +++++++++++++++++++++++++++ 1 file changed, 245 insertions(+) create mode 100644 design-documents/data-format/json.md diff --git a/design-documents/data-format/json.md b/design-documents/data-format/json.md new file mode 100644 index 000000000..da77d221e --- /dev/null +++ b/design-documents/data-format/json.md @@ -0,0 +1,245 @@ +# Contract for JSON encoding/decoding + +This proposal is based on [Design Document for changing SerializerInterface](https://github.com/magento-engcom/msi/wiki/Design-Document-for-changing-SerializerInterface) written by [Igor Miniailo](https://github.com/maghamed). + +## Current state + +Magento 2.2 changed the default serialization implementation in favour of JSON encoding in order to make code more secure and avoid vulnerabilities related to PHP `unserialize` function. + +For this purpose `SerializerInterface` was introduced. + +``` php +/** + * Interface for serializing + * + * @api + * @since 100.2.0 + */ +interface SerializerInterface +{ + /** + * Serialize data into string + * + * @param string|int|float|bool|array|null $data + * @return string|bool + * @throws \InvalidArgumentException + * @since 100.2.0 + */ + public function serialize($data); + + /** + * Unserialize the given string + * + * @param string $string + * @return string|int|float|bool|array|null + * @throws \InvalidArgumentException + * @since 100.2.0 + */ + public function unserialize($string); +} +``` + +Its default implementation of `Magento\Framework\Serialize\Serializer\Json` became an independent contract for all the business logic where `json_encode`/`json_decode` can be applied. + +``` php +/** + * Serialize data to JSON, unserialize JSON encoded data + * + * @api + * @since 100.2.0 + */ +class Json implements SerializerInterface +{ + /** + * @inheritDoc + * @since 100.2.0 + */ + public function serialize($data) + { + $result = json_encode($data); + if (false === $result) { + throw new \InvalidArgumentException("Unable to serialize value. Error: " . json_last_error_msg()); + } + return $result; + } + + /** + * @inheritDoc + * @since 100.2.0 + */ + public function unserialize($string) + { + $result = json_decode($string, true); + if (json_last_error() !== JSON_ERROR_NONE) { + throw new \InvalidArgumentException("Unable to unserialize value. Error: " . json_last_error_msg()); + } + return $result; + } +} +``` + +## Problem overview +JSON as data format can be used in other different from serialization contexts so `Magento\Framework\Serialize\Serializer\Json` class implementation is not sufficient enough to cover all those scenarios, e.g. it is impossible to pass `$options` or `$depth`. + +Native PHP [json_encode](https://www.php.net/manual/en/function.json-encode.php) function: +``` php +string json_encode ( mixed $value [, int $options = 0 [, int $depth = 512 ]] ) +``` + +Native PHP [json_decode](https://www.php.net/manual/en/function.json-decode.php) function: +``` php +mixed json_decode ( string $json [, bool $assoc = FALSE [, int $depth = 512 [, int $options = 0 ]]] ) +``` + +On the other hand native PHP implementation requires us to call `json_last_error` in order to handle errors instead of throwing an exception. It was [changed in PHP 7.3](https://wiki.php.net/rfc/json_throw_on_error), but this is still not the default behaviour. + +## Solution + +Since we want to protect our platform from any possible security vulnerabilities related to the code we do not control and have expected implementation we propose to add new dedicated contract for JSON. + +``` php +interface JsonEncoderInterface +{ + /** + * Encode data into JSON string + * + * @param string|int|float|bool|array|null $data + * @param int $option + * @param int $depth + * @return string + * @throws \InvalidArgumentException + */ + public function encode($data, int $option, int $depth); + + /** + * Decode the given JSON string + * + * @param string $string + * @param bool $assoc + * @param int $option + * @param int $depth + * @return string|int|float|bool|array|null + * @throws \InvalidArgumentException + */ + public function decode($string, bool $assoc, int $option, int $depth); + + //it's possible to add some other methods +} +``` + +Implementation would use low-level PHP `json_encode`/`json_decode` under the hood and will throw `InvalidArgumentException` if encoding/decoding cannot be performed. +New interface introduction means that we need to refactor all the existing business logic which currently depends on JSON class, and substitute it with this new service `JsonEncoderInterface`, because having two contracts means that it's incorrect to depend directly on JSON object (particular implementation) for JSON encoding purposes, that's a violation of polymorphism. + +#### Pros +- Correct from OOP point of view as we have two independent interfaces for two different business operations. +- Follows the Interface Segregation Principle (SOLID). +- Get us closer to the desired state of serializer/encoding design. +- Proper error handling. + +#### Cons +- Backward incompatible as introduces a lot of refactoring substituting existing direct usages of JSON object with newly introduced contract `JsonEncoderInterface`. + +### Other possible solutions +1. [Extending JSON class contract](https://github.com/magento-engcom/msi/wiki/Design-Document-for-changing-SerializerInterface#extending-json-class-contract---option-1) + + `Magento\Framework\Serialize\Serializer\Json` class implementation + + ``` php + /** + * Serialize data to JSON, unserialize JSON encoded data + * + * @api + * @since 100.2.0 + */ + class Json implements SerializerInterface + { + /** + * {@inheritDoc} + * @since 100.2.0 + */ + public function serialize($data, int $option = 0) //Optional parameter added + { + $result = json_encode($data, $option); + if (false === $result) { + throw new \InvalidArgumentException('Unable to serialize value.'); + } + return $result; + } + + /** + * {@inheritDoc} + * @since 100.2.0 + */ + public function unserialize($string, int $option = 0) //Optional parameter added + { + $result = json_decode($string, true, 512, $option); + if (json_last_error() !== JSON_ERROR_NONE) { + throw new \InvalidArgumentException('Unable to unserialize value.'); + } + return $result; + } + } + ``` +2. [Add Constructor and Use Virtual Types for JSON](https://github.com/magento-engcom/msi/wiki/Design-Document-for-changing-SerializerInterface#add-constructor-and-use-virtual-types-for-json---option-2) + + `Magento\Framework\Serialize\Serializer\Json` class implementation + + ``` php + /** + * Serialize data to JSON, unserialize JSON encoded data + * + * @api + * @since 100.2.0 + */ + class Json implements SerializerInterface + { + /** + * @var int + */ + private $option; + + /** + * @param int $option + */ + public function __construct(int $option) + { + $this->option = $option; + } + + + /** + * {@inheritDoc} + * @since 100.2.0 + */ + public function serialize($data) + { + $result = json_encode($data, $this->option); + if (false === $result) { + throw new \InvalidArgumentException('Unable to serialize value.'); + } + return $result; + } + + /** + * {@inheritDoc} + * @since 100.2.0 + */ + public function unserialize($string) + { + $result = json_decode($string, true, 512, $this->option); + if (json_last_error() !== JSON_ERROR_NONE) { + throw new \InvalidArgumentException('Unable to unserialize value.'); + } + return $result; + } + } + ``` + `di.xml` configuration + ``` + + + JSON_PRETTY_PRINT + + + ``` +3. Do not use abstraction at all and use low-level PHP `json_encode`/`json_decode`. From 36cafb3408e5cca419d3165aba6ec51f3f5ea2d0 Mon Sep 17 00:00:00 2001 From: Lena Orobei Date: Fri, 8 Nov 2019 14:50:55 -0600 Subject: [PATCH 2/3] Contract for JSON encoding/decoding - review updates --- design-documents/data-format/json.md | 61 +++++++++++++++++++--------- 1 file changed, 42 insertions(+), 19 deletions(-) diff --git a/design-documents/data-format/json.md b/design-documents/data-format/json.md index da77d221e..dbdbc4a4e 100644 --- a/design-documents/data-format/json.md +++ b/design-documents/data-format/json.md @@ -95,10 +95,10 @@ On the other hand native PHP implementation requires us to call `json_last_error ## Solution -Since we want to protect our platform from any possible security vulnerabilities related to the code we do not control and have expected implementation we propose to add new dedicated contract for JSON. +Since we want to protect our platform from any possible security vulnerabilities related to the code we do not control and have expected implementation we propose to add new `@api` class for JSON. ``` php -interface JsonEncoderInterface +class JsonEncoder { /** * Encode data into JSON string @@ -109,7 +109,10 @@ interface JsonEncoderInterface * @return string * @throws \InvalidArgumentException */ - public function encode($data, int $option, int $depth); + public function encode($data, int $options = 0, int $depth = 512) :string + { + //implementation + }; /** * Decode the given JSON string @@ -118,29 +121,49 @@ interface JsonEncoderInterface * @param bool $assoc * @param int $option * @param int $depth - * @return string|int|float|bool|array|null + * @return array|null * @throws \InvalidArgumentException */ - public function decode($string, bool $assoc, int $option, int $depth); - - //it's possible to add some other methods + public function decode(string $string, bool $assoc = false, int $options = 0, int $depth = 512): ?array + { + //implementation + }; } ``` -Implementation would use low-level PHP `json_encode`/`json_decode` under the hood and will throw `InvalidArgumentException` if encoding/decoding cannot be performed. -New interface introduction means that we need to refactor all the existing business logic which currently depends on JSON class, and substitute it with this new service `JsonEncoderInterface`, because having two contracts means that it's incorrect to depend directly on JSON object (particular implementation) for JSON encoding purposes, that's a violation of polymorphism. +Methods implementation would use low-level PHP `json_encode`/`json_decode` under the hood and will throw `InvalidArgumentException` if encoding/decoding cannot be performed. -#### Pros -- Correct from OOP point of view as we have two independent interfaces for two different business operations. -- Follows the Interface Segregation Principle (SOLID). -- Get us closer to the desired state of serializer/encoding design. -- Proper error handling. +### Other possible solutions +1. [New interface for JSON encoding-decoding operation](https://github.com/magento/inventory/wiki/Design-Document-for-changing-SerializerInterface#introduce-new-dedicated-contract-for-json-encoding-decoding-operation---option-3) + ``` + interface JsonEncoderInterface + { + /** + * Serialize data into string + * + * @param string|int|float|bool|array|null $data + * @param int + * @param int + * @return string + * @throws \InvalidArgumentException + */ + public function encode($data, int $option, int $depth); -#### Cons -- Backward incompatible as introduces a lot of refactoring substituting existing direct usages of JSON object with newly introduced contract `JsonEncoderInterface`. + /** + * Unserialize the given string + * + * @param string $string + * @param int + * @param int + * @return string|int|float|bool|array|null + * @throws \InvalidArgumentException + */ + public function decode($string, int $option, int $depth); -### Other possible solutions -1. [Extending JSON class contract](https://github.com/magento-engcom/msi/wiki/Design-Document-for-changing-SerializerInterface#extending-json-class-contract---option-1) + //it's possible there are some other methods inside + } + ``` +2. [Extending JSON class contract](https://github.com/magento-engcom/msi/wiki/Design-Document-for-changing-SerializerInterface#extending-json-class-contract---option-1) `Magento\Framework\Serialize\Serializer\Json` class implementation @@ -180,7 +203,7 @@ New interface introduction means that we need to refactor all the existing busin } } ``` -2. [Add Constructor and Use Virtual Types for JSON](https://github.com/magento-engcom/msi/wiki/Design-Document-for-changing-SerializerInterface#add-constructor-and-use-virtual-types-for-json---option-2) +3. [Add Constructor and Use Virtual Types for JSON](https://github.com/magento-engcom/msi/wiki/Design-Document-for-changing-SerializerInterface#add-constructor-and-use-virtual-types-for-json---option-2) `Magento\Framework\Serialize\Serializer\Json` class implementation From 0695616357376718698ba9c6b3d28cb3cd4bb533 Mon Sep 17 00:00:00 2001 From: Lena Orobei Date: Wed, 13 Nov 2019 09:39:22 -0600 Subject: [PATCH 3/3] Contract for JSON encoding/decoding --- design-documents/data-format/json.md | 97 +++++++++++++++++++++------- 1 file changed, 73 insertions(+), 24 deletions(-) diff --git a/design-documents/data-format/json.md b/design-documents/data-format/json.md index dbdbc4a4e..90ed39b7f 100644 --- a/design-documents/data-format/json.md +++ b/design-documents/data-format/json.md @@ -95,46 +95,92 @@ On the other hand native PHP implementation requires us to call `json_last_error ## Solution -Since we want to protect our platform from any possible security vulnerabilities related to the code we do not control and have expected implementation we propose to add new `@api` class for JSON. +- Deprecate the `Magento\Framework\Serialize\SerializerInterface` and add annotation `@see use Magento\Framework\Serialize\Serializer\Json instead`. +- Add two new methods to the `Magento\Framework\Serialize\Serializer\Json`. Implementation will use low-level PHP `json_encode`/`json_decode` under the hood and will throw `InvalidArgumentException` if encoding/decoding cannot be performed. ``` php -class JsonEncoder +/** + * Serialize data to JSON, unserialize JSON encoded data + * + * @api + */ +class Json implements SerializerInterface { + // already existing methods + public function serialize($data); + public function unserialize($string); + /** - * Encode data into JSON string - * - * @param string|int|float|bool|array|null $data - * @param int $option + * @param $value + * @param int $options * @param int $depth - * @return string * @throws \InvalidArgumentException + * @return string */ - public function encode($data, int $options = 0, int $depth = 512) :string + public function serializeWithOptions($value, int $options = 0, int $depth = 512) : string { - //implementation - }; + $result = json_encode($value, $options, $depth); + if (false === $result) { + throw new \InvalidArgumentException('Unable to serialize value. Error: ' . json_last_error_msg()); + } + return $result; + } /** - * Decode the given JSON string - * - * @param string $string + * @param $json * @param bool $assoc - * @param int $option * @param int $depth - * @return array|null + * @param int $options * @throws \InvalidArgumentException + * @return mixed */ - public function decode(string $string, bool $assoc = false, int $options = 0, int $depth = 512): ?array + public function unserializeWithOptions($json, bool $assoc = true, int $depth = 512, int $options = 0) { - //implementation - }; + $result = json_decode($json, $assoc, $depth, $options); + if (json_last_error() !== JSON_ERROR_NONE) { + throw new \InvalidArgumentException('Unable to unserialize value. Error: ' . json_last_error_msg()); + } + return $result; + } } ``` -Methods implementation would use low-level PHP `json_encode`/`json_decode` under the hood and will throw `InvalidArgumentException` if encoding/decoding cannot be performed. - ### Other possible solutions -1. [New interface for JSON encoding-decoding operation](https://github.com/magento/inventory/wiki/Design-Document-for-changing-SerializerInterface#introduce-new-dedicated-contract-for-json-encoding-decoding-operation---option-3) +1. Add new `@api` class for JSON encoding-decoding without interface. + ``` php + class JsonEncoder + { + /** + * Encode data into JSON string + * + * @param string|int|float|bool|array|null $data + * @param int $option + * @param int $depth + * @return string + * @throws \InvalidArgumentException + */ + public function encode($data, int $options = 0, int $depth = 512) :string + { + //implementation + }; + /** + * Decode the given JSON string + * + * @param string $string + * @param bool $assoc + * @param int $option + * @param int $depth + * @return array|null + * @throws \InvalidArgumentException + */ + public function decode(string $string, bool $assoc = false, int $options = 0, int $depth = 512): ?array + { + //implementation + }; + } + ``` + +2. [New interface for JSON encoding/decoding operation](https://github.com/magento/inventory/wiki/Design-Document-for-changing-SerializerInterface#introduce-new-dedicated-contract-for-json-encoding-decoding-operation---option-3) ``` interface JsonEncoderInterface { @@ -163,7 +209,8 @@ Methods implementation would use low-level PHP `json_encode`/`json_decode` under //it's possible there are some other methods inside } ``` -2. [Extending JSON class contract](https://github.com/magento-engcom/msi/wiki/Design-Document-for-changing-SerializerInterface#extending-json-class-contract---option-1) + +3. [Extending JSON class contract](https://github.com/magento-engcom/msi/wiki/Design-Document-for-changing-SerializerInterface#extending-json-class-contract---option-1) `Magento\Framework\Serialize\Serializer\Json` class implementation @@ -203,7 +250,8 @@ Methods implementation would use low-level PHP `json_encode`/`json_decode` under } } ``` -3. [Add Constructor and Use Virtual Types for JSON](https://github.com/magento-engcom/msi/wiki/Design-Document-for-changing-SerializerInterface#add-constructor-and-use-virtual-types-for-json---option-2) + +4. [Add Constructor and Use Virtual Types for JSON](https://github.com/magento-engcom/msi/wiki/Design-Document-for-changing-SerializerInterface#add-constructor-and-use-virtual-types-for-json---option-2) `Magento\Framework\Serialize\Serializer\Json` class implementation @@ -265,4 +313,5 @@ Methods implementation would use low-level PHP `json_encode`/`json_decode` under ``` -3. Do not use abstraction at all and use low-level PHP `json_encode`/`json_decode`. + +5. Do not use abstraction at all and use low-level PHP `json_encode`/`json_decode`.