Skip to content

Commit 7658f79

Browse files
acoultonacoulton
committed
feat: More strictly check patch structural validity during fromJson
Improve test coverage of attempting to create a PatchOperationList from JSON with invalid data. Update the implementation so that the majority of potential issues will be detected and thrown as InvalidPatchException or InvalidPatchOperationException.
1 parent 028168c commit 7658f79

2 files changed

Lines changed: 156 additions & 17 deletions

File tree

‎src/operations/PatchOperationList.php‎

Lines changed: 82 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,10 @@
66
use blancks\JsonPatch\exceptions\InvalidPatchOperationException;
77
use blancks\JsonPatch\json\handlers\BasicJsonHandler;
88
use blancks\JsonPatch\json\handlers\JsonHandlerInterface;
9+
use ArgumentCountError;
10+
use Error;
911
use stdClass;
12+
use TypeError;
1013

1114
final readonly class PatchOperationList implements \JsonSerializable
1215
{
@@ -45,23 +48,94 @@ public static function fromJson(
4548

4649
return new PatchOperationList(
4750
...array_map(
48-
function (stdClass $patch) use ($classes) {
49-
// The top-level patch entries should be identical as objects or arrays, cast to array to allow
50-
// spreading the properties into the DTO constructors (which are assumed to match the JSON objects)
51-
$patch = (array) $patch;
52-
$op = $patch['op'];
53-
unset($patch['op']);
51+
function (mixed $patch) use ($classes) {
52+
[$op, $values] = self::extractPatchOpAndValues($patch);
5453
if (!isset($classes[$op])) {
5554
throw new InvalidPatchOperationException(sprintf('Unknown operation "%s"', $op));
5655
}
57-
58-
return new $classes[$op](...$patch);
56+
return self::createPatchDtoFromValues($op, $classes[$op], $values);
5957
},
6058
$patches
6159
),
6260
);
6361
}
6462

63+
/**
64+
* @phpstan-return array{string, array<string, mixed>}
65+
*/
66+
private static function extractPatchOpAndValues(mixed $patch): array
67+
{
68+
if (!$patch instanceof stdClass) {
69+
throw new InvalidPatchOperationException(
70+
sprintf('Each patch item must be an object, got %s', get_debug_type($patch))
71+
);
72+
}
73+
74+
if (!isset($patch->op)) {
75+
throw new InvalidPatchOperationException('Each patch item must specify "op"');
76+
}
77+
78+
if (!is_string($patch->op)) {
79+
throw new InvalidPatchOperationException(
80+
sprintf('Patch "op" must be a string, got %s', get_debug_type($patch->op))
81+
);
82+
}
83+
84+
$op = $patch->op;
85+
unset($patch->op);
86+
87+
$values = [];
88+
foreach ((array) $patch as $key => $value) {
89+
// We rely on the properties being strings, to ensure that PHP will enforce that all named properties are
90+
// present / defined when creating the arbitrary DTO object. Ensure this is the case
91+
if (!is_string($key)) {
92+
throw new InvalidPatchOperationException('All patch operation properties must have string names');
93+
}
94+
$values[$key] = $value;
95+
}
96+
97+
return [$op, $values];
98+
}
99+
100+
/**
101+
* @phpstan-param class-string<PatchOperation> $class
102+
* @param array<string, mixed> $values
103+
* @return PatchOperation
104+
*
105+
* @throws InvalidPatchOperationException
106+
*/
107+
private static function createPatchDtoFromValues(string $op, string $class, array $values): PatchOperation
108+
{
109+
try {
110+
return new $class(...$values);
111+
} catch (ArgumentCountError $e) {
112+
// We can be relatively confident that this was triggered for the DTO constructor. If the DTO was calling a
113+
// method (e.g. a parent method / internal helper method) with incorrect argument counts that should have
114+
// been detected by its own tests.
115+
throw new InvalidPatchOperationException(
116+
sprintf('Missing required param(s) for %s operation as %s: %s', $op, $class, $e->getMessage()),
117+
previous: $e,
118+
);
119+
} catch (TypeError $e) {
120+
// We can be relatively confident that this relates to the property types passed to the DTO constructor.
121+
// If the DTO constructor has a type signature that does not match the expected supported values, that
122+
// should have been detected by its own tests.
123+
throw new InvalidPatchOperationException(
124+
sprintf('Invalid param(s) for %s operation as %s: %s', $op, $class, $e->getMessage()),
125+
previous: $e,
126+
);
127+
} catch (Error $e) {
128+
if (str_contains($e->getMessage(), 'Unknown named parameter')) {
129+
// This does not have a dedicated error type so we have to match on the message.
130+
throw new InvalidPatchOperationException(
131+
sprintf('Unexpected param(s) for %s operation as %s: %s', $op, $class, $e->getMessage()),
132+
previous: $e,
133+
);
134+
}
135+
throw $e;
136+
}
137+
}
138+
65139
/**
66140
* @no-named-arguments
67141
*/

‎tests/operations/PatchOperationListTest.php‎

Lines changed: 74 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -29,13 +29,13 @@
2929
use Throwable;
3030

3131
#[CoversClass(PatchOperationList::class)]
32-
#[UsesClass(Add::class)]
33-
#[UsesClass(Copy::class)]
34-
#[UsesClass(Move::class)]
35-
#[UsesClass(Remove::class)]
36-
#[UsesClass(Replace::class)]
37-
#[UsesClass(Test::class)]
38-
#[UsesClass(PatchOperation::class)]
32+
#[CoversClass(Add::class)]
33+
#[CoversClass(Copy::class)]
34+
#[CoversClass(Move::class)]
35+
#[CoversClass(Remove::class)]
36+
#[CoversClass(Replace::class)]
37+
#[CoversClass(Test::class)]
38+
#[CoversClass(PatchOperation::class)]
3939
#[UsesClass(FastJsonPatchExceptionTrait::class)]
4040
#[UsesClass(InvalidPatchException::class)]
4141
#[UsesClass(InvalidPatchOperationException::class)]
@@ -192,7 +192,7 @@ public function testItCanDecodeWithCustomOperationClasses(): void
192192
$result = PatchOperationList::fromJson(
193193
<<<'JSON'
194194
[
195-
{"op": "add", "path": "/greeting", "value": "Hello" },
195+
{"op": "add", "path": "/greeting", "value": "Hello", "custom": "my own var" },
196196
{"op": "append", "path": "/greeting", "suffix": " World" },
197197
{"op": "copy", "path": "/whatever", "from": "/greeting"}
198198
]
@@ -205,7 +205,7 @@ public function testItCanDecodeWithCustomOperationClasses(): void
205205

206206
$this->assertEquals(
207207
new PatchOperationList(
208-
new CustomAdd('/greeting', 'Hello'),
208+
new CustomAdd('/greeting', 'Hello', 'my own var'),
209209
new Append('/greeting', ' World'),
210210
new Copy(path: '/whatever', from: '/greeting'),
211211
),
@@ -234,11 +234,75 @@ public static function invalidJsonProvider(): array
234234
InvalidPatchException::class,
235235
'Invalid patch structure (expected list, got stdClass)',
236236
],
237+
'patch item is not an object (example 1)' => [
238+
'[["foo"]]',
239+
InvalidPatchOperationException::class,
240+
'Each patch item must be an object, got array',
241+
],
242+
'patch item is not an object (example 2)' => [
243+
'[{"op": "remove", "path": "/some/path"}, true]',
244+
InvalidPatchOperationException::class,
245+
'Each patch item must be an object, got bool',
246+
],
247+
'patch without op property' => [
248+
'[{"path": "/some/path", "value": "anything"}]',
249+
InvalidPatchOperationException::class,
250+
'Each patch item must specify "op"',
251+
],
252+
'op is invalid type' => [
253+
'[{"op": true}]',
254+
InvalidPatchOperationException::class,
255+
'Patch "op" must be a string, got bool',
256+
],
237257
'unknown operation' => [
238258
'[{"op": "scramble", "path": "/anywhere"}]',
239259
InvalidPatchOperationException::class,
240260
'Unknown operation "scramble"',
241261
],
262+
'unexpected extra operation property' => [
263+
'[{"op":"add", "path": "/foo", "value": "bar", "custom": "things"}]',
264+
InvalidPatchOperationException::class,
265+
// The message also contains the original PHP message but this varies between versions so we do not
266+
// include it in the assertion.
267+
sprintf(
268+
'Unexpected param(s) for add operation as %s',
269+
Add::class,
270+
),
271+
],
272+
'missing operation property' => [
273+
'[{"op":"add", "path": "/foo"}]',
274+
InvalidPatchOperationException::class,
275+
// The message also contains the original PHP message but this varies between versions so we do not
276+
// include it in the assertion.
277+
sprintf(
278+
'Missing required param(s) for add operation as %s',
279+
Add::class,
280+
),
281+
],
282+
'operation property has incorrect type' => [
283+
'[{"op":"add", "path": true, "value": "bar"}]',
284+
InvalidPatchOperationException::class,
285+
// The message also contains the original PHP message but this varies between versions so we do not
286+
// include it in the assertion.
287+
sprintf(
288+
'Invalid param(s) for add operation as %s',
289+
Add::class,
290+
),
291+
],
292+
'operation with mixed string and int keys (example 1)' => [
293+
// PHP would internally convert this into positional args and accept it even though there's no guarantee
294+
// we have the right keys in the right sequence / have no extra properties.
295+
'[{"op":"add", "0": "bar", "1": "/from"}]',
296+
InvalidPatchOperationException::class,
297+
'All patch operation properties must have string names'
298+
],
299+
'operation with mixed string and int keys (example 2)' => [
300+
// PHP would internally convert this into positional args and accept it even though there's no guarantee
301+
// we have the right keys in the right sequence / have no extra properties.
302+
'[{"op":"add", "1": "bar", "2": "/from"}]',
303+
InvalidPatchOperationException::class,
304+
'All patch operation properties must have string names'
305+
]
242306
];
243307
}
244308

@@ -272,6 +336,7 @@ public function __construct(
272336
public function __construct(
273337
public string $path,
274338
public mixed $value,
339+
public mixed $custom,
275340
) {
276341
parent::__construct('add');
277342
}

0 commit comments

Comments
 (0)