From 8900b8e31e4541bea3a88844a7188574eefc20e5 Mon Sep 17 00:00:00 2001 From: Ollie <43674967+outdooracorn@users.noreply.github.com> Date: Mon, 17 Oct 2022 14:41:07 +0100 Subject: [PATCH 1/5] Throw more specific exceptions in `JsonPatch::import()` (#1) - UnknownOperationException - MissingFieldException Co-authored-by: sihe --- src/JsonPatch.php | 14 +++++---- src/MissingFieldException.php | 47 +++++++++++++++++++++++++++++++ src/UnknownOperationException.php | 36 +++++++++++++++++++++++ tests/src/JsonPatchTest.php | 6 ++-- 4 files changed, 96 insertions(+), 7 deletions(-) create mode 100644 src/MissingFieldException.php create mode 100644 src/UnknownOperationException.php diff --git a/src/JsonPatch.php b/src/JsonPatch.php index 62a80f2..ccd4c99 100644 --- a/src/JsonPatch.php +++ b/src/JsonPatch.php @@ -60,11 +60,15 @@ public static function import(array $data) $operation = (object)$operation; } + if (!is_object($operation)) { + throw new Exception( 'Invalid patch operation - should be a JSON object' ); + } + if (!isset($operation->op)) { - throw new Exception('Missing "op" in operation data'); + throw new MissingFieldException('op', $operation); } if (!isset($operation->path)) { - throw new Exception('Missing "path" in operation data'); + throw new MissingFieldException('path', $operation); } $op = null; @@ -88,18 +92,18 @@ public static function import(array $data) $op = new Test(); break; default: - throw new Exception('Unknown "op": ' . $operation->op); + throw new UnknownOperationException($operation); } $op->path = $operation->path; if ($op instanceof OpPathValue) { if (property_exists($operation, 'value')) { $op->value = $operation->value; } else { - throw new Exception('Missing "value" in operation data'); + throw new MissingFieldException('value', $operation); } } elseif ($op instanceof OpPathFrom) { if (!isset($operation->from)) { - throw new Exception('Missing "from" in operation data'); + throw new MissingFieldException('from', $operation); } $op->from = $operation->from; } diff --git a/src/MissingFieldException.php b/src/MissingFieldException.php new file mode 100644 index 0000000..915f1dc --- /dev/null +++ b/src/MissingFieldException.php @@ -0,0 +1,47 @@ +missingField = $missingField; + $this->operation = $operation; + } + + /** + * @return string + */ + public function getMissingField() + { + return $this->missingField; + } + + /** + * @return object + */ + public function getOperation() + { + return $this->operation; + } +} diff --git a/src/UnknownOperationException.php b/src/UnknownOperationException.php new file mode 100644 index 0000000..0855bb2 --- /dev/null +++ b/src/UnknownOperationException.php @@ -0,0 +1,36 @@ +op, $code, $previous); + $this->operation = $operation; + } + + /** + * @return object + */ + public function getOperation() + { + return $this->operation; + } +} diff --git a/tests/src/JsonPatchTest.php b/tests/src/JsonPatchTest.php index 0870442..ce73ab6 100644 --- a/tests/src/JsonPatchTest.php +++ b/tests/src/JsonPatchTest.php @@ -5,7 +5,9 @@ use Swaggest\JsonDiff\Exception; use Swaggest\JsonDiff\JsonDiff; use Swaggest\JsonDiff\JsonPatch; +use Swaggest\JsonDiff\MissingFieldException; use Swaggest\JsonDiff\PatchTestOperationFailedException; +use Swaggest\JsonDiff\UnknownOperationException; class JsonPatchTest extends \PHPUnit_Framework_TestCase { @@ -75,7 +77,7 @@ public function testNull() public function testMissingOp() { - $this->setExpectedException(get_class(new Exception()), 'Missing "op" in operation data'); + $this->setExpectedException(MissingFieldException::class, 'Missing "op" in operation data'); JsonPatch::import(array((object)array('path' => '/123'))); } @@ -87,7 +89,7 @@ public function testMissingPath() public function testInvalidOp() { - $this->setExpectedException(get_class(new Exception()), 'Unknown "op": wat'); + $this->setExpectedException(UnknownOperationException::class, 'Unknown "op": wat'); JsonPatch::import(array((object)array('op' => 'wat', 'path' => '/123'))); } From e618e58a7be36a7b3c5e069a394c2a2a78fd29f0 Mon Sep 17 00:00:00 2001 From: Jakob Warkotsch Date: Mon, 17 Oct 2022 16:15:01 +0200 Subject: [PATCH 2/5] Improve JsonPatch::import exception tests --- tests/src/JsonPatchTest.php | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/tests/src/JsonPatchTest.php b/tests/src/JsonPatchTest.php index ce73ab6..da0fd45 100644 --- a/tests/src/JsonPatchTest.php +++ b/tests/src/JsonPatchTest.php @@ -77,8 +77,16 @@ public function testNull() public function testMissingOp() { - $this->setExpectedException(MissingFieldException::class, 'Missing "op" in operation data'); - JsonPatch::import(array((object)array('path' => '/123'))); + $operation = (object)array('path' => '/123'); + try { + JsonPatch::import(array($operation)); + $this->fail('Expected exception was not thrown'); + } catch (Exception $exception) { + $this->assertInstanceOf(MissingFieldException::class, $exception); + $this->assertSame('Missing "op" in operation data', $exception->getMessage()); + $this->assertSame('op', $exception->getMissingField()); + $this->assertSame($operation, $exception->getOperation()); + } } public function testMissingPath() @@ -89,8 +97,15 @@ public function testMissingPath() public function testInvalidOp() { - $this->setExpectedException(UnknownOperationException::class, 'Unknown "op": wat'); - JsonPatch::import(array((object)array('op' => 'wat', 'path' => '/123'))); + $operation = (object)array('op' => 'wat', 'path' => '/123'); + try { + JsonPatch::import(array($operation)); + $this->fail('Expected exception was not thrown'); + } catch (Exception $exception) { + $this->assertInstanceOf(UnknownOperationException::class, $exception); + $this->assertSame('Unknown "op": wat', $exception->getMessage()); + $this->assertSame($operation, $exception->getOperation()); + } } public function testMissingFrom() @@ -154,4 +169,4 @@ public function testTestOperationFailed() $this->assertInstanceOf(PatchTestOperationFailedException::class, $errors[0]); } -} \ No newline at end of file +} From 5f297cbf75faac321810dc19c8da558dfa647a2b Mon Sep 17 00:00:00 2001 From: Jakob Warkotsch Date: Mon, 17 Oct 2022 16:27:43 +0200 Subject: [PATCH 3/5] Improve PatchTestOperationFailedException This adds the failed operation and the actual value that failed the test operation check to the exception. --- src/JsonPatch.php | 5 ++- src/PatchTestOperationFailedException.php | 43 ++++++++++++++++++++++- tests/src/JsonPatchTest.php | 13 ++++--- 3 files changed, 53 insertions(+), 8 deletions(-) diff --git a/src/JsonPatch.php b/src/JsonPatch.php index ccd4c99..c9f679c 100644 --- a/src/JsonPatch.php +++ b/src/JsonPatch.php @@ -174,8 +174,7 @@ public function apply(&$original, $stopOnError = true) $diff = new JsonDiff($operation->value, $value, JsonDiff::STOP_ON_DIFF); if ($diff->getDiffCnt() !== 0) { - throw new PatchTestOperationFailedException('Test operation ' . json_encode($operation, JSON_UNESCAPED_SLASHES) - . ' failed: ' . json_encode($value)); + throw new PatchTestOperationFailedException($operation, $value); } break; } @@ -189,4 +188,4 @@ public function apply(&$original, $stopOnError = true) } return $errors; } -} \ No newline at end of file +} diff --git a/src/PatchTestOperationFailedException.php b/src/PatchTestOperationFailedException.php index 8fc7431..c2ded02 100644 --- a/src/PatchTestOperationFailedException.php +++ b/src/PatchTestOperationFailedException.php @@ -3,6 +3,47 @@ namespace Swaggest\JsonDiff; +use Throwable; + class PatchTestOperationFailedException extends Exception { -} \ No newline at end of file + /** @var object */ + private $operation; + /** @var string */ + private $actualValue; + + /** + * @param object $operation + * @param string $actualValue + * @param int $code + * @param Throwable|null $previous + */ + public function __construct( + $operation, + $actualValue, + $code = 0, + Throwable $previous = null + ) + { + parent::__construct('Test operation ' . json_encode($operation, JSON_UNESCAPED_SLASHES) + . ' failed: ' . json_encode($actualValue), $code, $previous); + $this->operation = $operation; + $this->actualValue = $actualValue; + } + + /** + * @return object + */ + public function getOperation() + { + return $this->operation; + } + + /** + * @return string + */ + public function getActualValue() + { + return $this->actualValue; + } +} diff --git a/tests/src/JsonPatchTest.php b/tests/src/JsonPatchTest.php index da0fd45..07d288c 100644 --- a/tests/src/JsonPatchTest.php +++ b/tests/src/JsonPatchTest.php @@ -162,11 +162,16 @@ public function testApplyNonExistentLevelOne() public function testTestOperationFailed() { - $data = array('abc' => 'xyz'); + $actualValue = 'xyz'; + $data = array('abc' => $actualValue); + $operation = new JsonPatch\Test('/abc', 'def'); + $p = new JsonPatch(); - $p->op(new JsonPatch\Test('/abc', 'def')); - $errors = $p->apply($data, false); - $this->assertInstanceOf(PatchTestOperationFailedException::class, $errors[0]); + $p->op($operation); + $testError = $p->apply($data, false)[0]; + $this->assertInstanceOf(PatchTestOperationFailedException::class, $testError); + $this->assertSame($operation, $testError->getOperation()); + $this->assertSame($actualValue, $testError->getActualValue()); } } From eae12565a03f25d358c2b86571e282ac27cab788 Mon Sep 17 00:00:00 2001 From: sihe Date: Tue, 18 Oct 2022 12:46:12 +0200 Subject: [PATCH 4/5] Introduce JsonPointerException Replace all occurences of the generic Exception in JsonPointer with the more specific JsonPointerException so that the origin of the error is made explicit and can be handled by consuming code. --- src/JsonPointer.php | 30 +++++++++++++++--------------- src/JsonPointerException.php | 5 +++++ tests/src/JsonPointerTest.php | 5 +++-- 3 files changed, 23 insertions(+), 17 deletions(-) create mode 100644 src/JsonPointerException.php diff --git a/src/JsonPointer.php b/src/JsonPointer.php index d9fc2e0..0b950a4 100644 --- a/src/JsonPointer.php +++ b/src/JsonPointer.php @@ -66,7 +66,7 @@ public static function splitPath($path) return self::splitPathURIFragment($pathItems); } else { if ($first !== '') { - throw new Exception('Path must start with "/": ' . $path); + throw new JsonPointerException('Path must start with "/": ' . $path); } return self::splitPathJsonString($pathItems); } @@ -105,7 +105,7 @@ public static function add(&$holder, $pathItems, $value, $flags = self::RECURSIV while (null !== $key = array_shift($pathItems)) { if ($ref instanceof \stdClass || is_object($ref)) { if (PHP_VERSION_ID < 70100 && '' === $key) { - throw new Exception('Empty property name is not supported by PHP <7.1', + throw new JsonPointerException('Empty property name is not supported by PHP <7.1', Exception::EMPTY_PROPERTY_NAME_UNSUPPORTED); } @@ -113,7 +113,7 @@ public static function add(&$holder, $pathItems, $value, $flags = self::RECURSIV $ref = &$ref->$key; } else { if (!isset($ref->$key) && count($pathItems)) { - throw new Exception('Non-existent path item: ' . $key); + throw new JsonPointerException('Non-existent path item: ' . $key); } else { $ref = &$ref->$key; } @@ -126,7 +126,7 @@ public static function add(&$holder, $pathItems, $value, $flags = self::RECURSIV $ref = new \stdClass(); $ref = &$ref->{$key}; } else { - throw new Exception('Non-existent path item: ' . $key); + throw new JsonPointerException('Non-existent path item: ' . $key); } } elseif ([] === $ref && 0 === ($flags & self::STRICT_MODE) && false === $intKey && '-' !== $key) { $ref = new \stdClass(); @@ -138,7 +138,7 @@ public static function add(&$holder, $pathItems, $value, $flags = self::RECURSIV } else { if (false === $intKey) { if (0 === ($flags & self::TOLERATE_ASSOCIATIVE_ARRAYS)) { - throw new Exception('Invalid key for array operation'); + throw new JsonPointerException('Invalid key for array operation'); } $ref = &$ref[$key]; continue; @@ -148,9 +148,9 @@ public static function add(&$holder, $pathItems, $value, $flags = self::RECURSIV } if (0 === ($flags & self::TOLERATE_ASSOCIATIVE_ARRAYS)) { if ($intKey > count($ref) && 0 === ($flags & self::RECURSIVE_KEY_CREATION)) { - throw new Exception('Index is greater than number of items in array'); + throw new JsonPointerException('Index is greater than number of items in array'); } elseif ($intKey < 0) { - throw new Exception('Negative index'); + throw new JsonPointerException('Negative index'); } } @@ -203,7 +203,7 @@ public static function get($holder, $pathItems) while (null !== $key = array_shift($pathItems)) { if ($ref instanceof \stdClass) { if (PHP_VERSION_ID < 70100 && '' === $key) { - throw new Exception('Empty property name is not supported by PHP <7.1', + throw new JsonPointerException('Empty property name is not supported by PHP <7.1', Exception::EMPTY_PROPERTY_NAME_UNSUPPORTED); } @@ -211,22 +211,22 @@ public static function get($holder, $pathItems) if (self::arrayKeyExists($key, $vars)) { $ref = self::arrayGet($key, $vars); } else { - throw new Exception('Key not found: ' . $key); + throw new JsonPointerException('Key not found: ' . $key); } } elseif (is_array($ref)) { if (self::arrayKeyExists($key, $ref)) { $ref = $ref[$key]; } else { - throw new Exception('Key not found: ' . $key); + throw new JsonPointerException('Key not found: ' . $key); } } elseif (is_object($ref)) { if (isset($ref->$key)) { $ref = $ref->$key; } else { - throw new Exception('Key not found: ' . $key); + throw new JsonPointerException('Key not found: ' . $key); } } else { - throw new Exception('Key not found: ' . $key); + throw new JsonPointerException('Key not found: ' . $key); } } return $ref; @@ -260,19 +260,19 @@ public static function remove(&$holder, $pathItems, $flags = 0) if (property_exists($ref, $key)) { $ref = &$ref->$key; } else { - throw new Exception('Key not found: ' . $key); + throw new JsonPointerException('Key not found: ' . $key); } } elseif (is_object($ref)) { if (isset($ref->$key)) { $ref = &$ref->$key; } else { - throw new Exception('Key not found: ' . $key); + throw new JsonPointerException('Key not found: ' . $key); } } else { if (array_key_exists($key, $ref)) { $ref = &$ref[$key]; } else { - throw new Exception('Key not found: ' . $key); + throw new JsonPointerException('Key not found: ' . $key); } } } diff --git a/src/JsonPointerException.php b/src/JsonPointerException.php new file mode 100644 index 0000000..480a301 --- /dev/null +++ b/src/JsonPointerException.php @@ -0,0 +1,5 @@ +assertSame('null', json_encode(JsonPointer::get($json, JsonPointer::splitPath('/l1/l2/non-existent')))); - } catch (Exception $exception) { + } catch (JsonPointerException $exception) { $this->assertSame('Key not found: non-existent', $exception->getMessage()); } @@ -89,7 +90,7 @@ public function testGetSetDeleteObject() try { JsonPointer::get($s, ['one', 'two']); $this->fail('Exception expected'); - } catch (Exception $e) { + } catch (JsonPointerException $e) { $this->assertEquals('Key not found: two', $e->getMessage()); } $this->assertEquals(null, $s->one->two); From 478e61893b30689bf8491f76b95d557161b56dae Mon Sep 17 00:00:00 2001 From: sihe Date: Tue, 18 Oct 2022 12:46:12 +0200 Subject: [PATCH 5/5] Introduce PathException for JsonPatch This exception is thrown in JsonPatch::apply whenever a JsonPointer call fails. --- src/JsonPatch.php | 20 ++++++++- src/PathException.php | 51 ++++++++++++++++++++++ tests/src/JsonPatchTest.php | 86 +++++++++++++++++++++++++++++++++++++ 3 files changed, 156 insertions(+), 1 deletion(-) create mode 100644 src/PathException.php diff --git a/src/JsonPatch.php b/src/JsonPatch.php index c9f679c..5b07e2c 100644 --- a/src/JsonPatch.php +++ b/src/JsonPatch.php @@ -61,7 +61,7 @@ public static function import(array $data) } if (!is_object($operation)) { - throw new Exception( 'Invalid patch operation - should be a JSON object' ); + throw new Exception('Invalid patch operation - should be a JSON object'); } if (!isset($operation->op)) { @@ -145,20 +145,26 @@ public function apply(&$original, $stopOnError = true) $errors = array(); foreach ($this->operations as $operation) { try { + // track the current pointer field so we can use it for a potential PathException + $pointerField = 'path'; $pathItems = JsonPointer::splitPath($operation->path); switch (true) { case $operation instanceof Add: JsonPointer::add($original, $pathItems, $operation->value, $this->flags); break; case $operation instanceof Copy: + $pointerField = 'from'; $fromItems = JsonPointer::splitPath($operation->from); $value = JsonPointer::get($original, $fromItems); + $pointerField = 'path'; JsonPointer::add($original, $pathItems, $value, $this->flags); break; case $operation instanceof Move: + $pointerField = 'from'; $fromItems = JsonPointer::splitPath($operation->from); $value = JsonPointer::get($original, $fromItems); JsonPointer::remove($original, $fromItems, $this->flags); + $pointerField = 'path'; JsonPointer::add($original, $pathItems, $value, $this->flags); break; case $operation instanceof Remove: @@ -178,6 +184,18 @@ public function apply(&$original, $stopOnError = true) } break; } + } catch (JsonPointerException $jsonPointerException) { + $pathException = new PathException( + $jsonPointerException->getMessage(), + $operation, + $pointerField, + $jsonPointerException->getCode() + ); + if ($stopOnError) { + throw $pathException; + } else { + $errors[] = $pathException; + } } catch (Exception $exception) { if ($stopOnError) { throw $exception; diff --git a/src/PathException.php b/src/PathException.php new file mode 100644 index 0000000..c96f83f --- /dev/null +++ b/src/PathException.php @@ -0,0 +1,51 @@ +operation = $operation; + $this->field = $field; + } + + /** + * @return object + */ + public function getOperation() + { + return $this->operation; + } + + /** + * @return string + */ + public function getField() + { + return $this->field; + } +} diff --git a/tests/src/JsonPatchTest.php b/tests/src/JsonPatchTest.php index 07d288c..e15c910 100644 --- a/tests/src/JsonPatchTest.php +++ b/tests/src/JsonPatchTest.php @@ -5,8 +5,10 @@ use Swaggest\JsonDiff\Exception; use Swaggest\JsonDiff\JsonDiff; use Swaggest\JsonDiff\JsonPatch; +use Swaggest\JsonDiff\JsonPatch\OpPath; use Swaggest\JsonDiff\MissingFieldException; use Swaggest\JsonDiff\PatchTestOperationFailedException; +use Swaggest\JsonDiff\PathException; use Swaggest\JsonDiff\UnknownOperationException; class JsonPatchTest extends \PHPUnit_Framework_TestCase @@ -174,4 +176,88 @@ public function testTestOperationFailed() $this->assertSame($actualValue, $testError->getActualValue()); } + public function testPathExceptionContinueOnError() + { + $actualValue = 'xyz'; + $data = array('abc' => $actualValue); + $patch = new JsonPatch(); + + $operation1 = new JsonPatch\Test('/abc', 'def'); + $patch->op($operation1); + + $operation2 = new JsonPatch\Move('/target', '/source'); + $patch->op($operation2); + + $errors = $patch->apply($data, false); + + $this->assertInstanceOf(PatchTestOperationFailedException::class, $errors[0]); + $this->assertSame($operation1, $errors[0]->getOperation()); + + $this->assertInstanceOf(PathException::class, $errors[1]); + $this->assertSame($operation2, $errors[1]->getOperation()); + $this->assertSame('from', $errors[1]->getField()); + } + + public function pathExceptionProvider() { + return [ + 'splitPath_path' => [ + new JsonPatch\Copy('invalid/path', '/valid/from'), + 'Path must start with "/": invalid/path', + 'path' + ], + 'splitPath_from' => [ + new JsonPatch\Copy('/valid/path', 'invalid/from'), + 'Path must start with "/": invalid/from', + 'from' + ], + 'add' => [ + new JsonPatch\Add('/some/path', 22), + 'Non-existent path item: some', + 'path' + ], + 'get_from' => [ + new JsonPatch\Copy('/target', '/source'), + 'Key not found: source', + 'from' + ], + 'get_path' => [ + new JsonPatch\Replace('/some/path', 23), + 'Key not found: some', + 'path' + ], + 'remove_from' => [ + new JsonPatch\Move('/target', '/source'), + 'Key not found: source', + 'from' + ], + 'remove_path' => [ + new JsonPatch\Remove('/some/path'), + 'Key not found: some', + 'path' + ] + ]; + } + + /** + * @param OpPath $operation + * @param string $expectedMessage + * @param string $expectedField + * + * @dataProvider pathExceptionProvider + */ + public function testPathException(OpPath $operation, $expectedMessage, $expectedField) { + $data = new \stdClass(); + $patch = new JsonPatch(); + + $patch->op($operation); + + try { + $patch->apply($data ); + $this->fail('PathException expected'); + } catch (Exception $ex) { + $this->assertInstanceOf(PathException::class, $ex); + $this->assertEquals($expectedMessage, $ex->getMessage()); + $this->assertEquals($expectedField, $ex->getField()); + } + } }