feat: Support building & using patches as a list of DTOs #14

acoulton · 2025-11-30T18:21:03Z

Build on the existing implementation to allow users to specify patches as a list of operation objects, rather than as a JSON string.

Unlike the JSON form, the patch operation DTOs are strict about the parameters they accept. If a user wishes to include custom properties, they can implement a class extending the base PatchOperation.

Patch DTOs can be serialised to and from JSON, for convenience.

Completes #12

** note ** I have based this branch off the branch for #13 to avoid merge conflicts. The first 3 commits in this diff can therefore be ignored. Once #13 is merged I will rebase as required.

src/operations/handlers/AddHandler.php

src/operations/PatchOperation.php

src/operations/PatchOperationList.php

tests/operations/PatchOperationListTest.php

acoulton · 2025-11-30T23:42:08Z

tests/FastJsonPatchTest.php

+    #[DataProvider('validOperationsForDTOsProvider')]
+    public function testValidJsonPatchesAsDTOs(string $json, string $patches, string $expected): void


Potentially this test runs more cases than is strictly necessary to prove the DTOs are correctly read & handled.

However it seemed the most efficient way to have confidence that they support all the expected properties / types and work correctly with all handlers and to keep that in sync with any changes to the pure JSON objects over time.

I'm okay with your assumptions here, but I would move the tests into a dedicated class for each operation DTO. I suggest this because I think each DTO also needs a test that ensures it fails when incorrect properties are passed. This is important because the PatchOperationList::fromJson method bases its validation workflow (both for patch validation and invalid DTOs) on the assumption that the constructor throws an exception when invalid input is provided.

I'm not sure about creating separate test classes for the individual DTOs - they have no logic of their own, only a constructor and some promoted properties. I wouldn't usually unit test things like that other than through the code that uses them - I think for example we can trust that PHP will handle a user doing something explicitly wrong like new Add() or new Add(op: new DateTimeImmutable) without adding a test to show that?

Also just to be clear I wasn't imagining the DTOs would be validated as such (other than basic type-safety / property naming). For example PHP will natively prevent you creating a DTO with an array for the path, but you could pass any string even if it's not a valid JSON pointer. Actual validation only happens if you pass the DTO to FastJsonPatch. Otherwise the DTOs start to have dependencies like the jsonPointerHandler and stop being DTOs.

However you're right that I'm missing examples of attempting to build DTOs from invalid JSON (e.g. the items in the JSON list aren't patch objects / are missing properties / have extra properties / have properties with the wrong types). Even if we're ok with PHP causing native TypeError there, we should prove that's what happens. I think I should just add those to the cases in PatchOperationListTest::invalidJsonProvider covering PatchOperationList::fromJson() which is the only place I'm dynamically creating DTOs.

Can you let me know if you're happy with that and I'll update?

I wasn't implying adding validation logic to the DTOs, but since we're relying on their types as a safety mechanism, we need to prove that they're type-safe and not constructed with a mixed argument, for example.

I suggested separate test classes mainly because it felt cleaner, but I understand your point, and I'm fine testing this the way you proposed as long as we agree that the behaviour should be covered

I have added more cases to the PatchOperationListTest and marked that that class now also covers the individual DTOs (since it contains examples of creating every supported DTO with valid values, and invalid values for a sample of them).

In implementing this, I found a few other actual or theoretical cases where an invalid data structure in the JSON would cause a runtime error (e.g. op is not specified or is an invalid type to be an array key). I decided it was more robust to check for these and throw an appropriate InvalidPatchException / InvalidPatchOperationException.

That led me to also catching and wrapping exceptions when creating the actual DTOs. Having thought about it some more, I think this is cleaner and fairly robust - if we assume that the DTO has tests proving it can be created with valid values (ours do) then any TypeError / ArgumentCountError / incorrect named parameters errors at runtime are virtually guaranteed to be due to invalid values in the JSON.

Of course, if a user gets an InvalidPatchOperationException for something they think should be valid, they can open a bug (as they would if it was our own validation logic that was incorrect).

I'm not sure about your comment we need to prove they're type-safe and not constructed with a mixed argument, for example.

We can see that the individual DTOs are typed with string where they should be from their class definitions.

We could add a test for every individual DTO to prove that e.g. we get an exception with {... "path": true} - but that could still pass if the property type was changed to string|int. The only way to conclusively prove the DTO will only accept a string would be to have test cases for every parameter with every possible type of non-string value. That feels redundant to me given none of those cases depend on any logic of our own.

I also saw that the existing tests for the library don't go into that level of detail testing invalid property data types in the native stdClass unserialisation so I think I might be misunderstanding what you want here?

I think all we need to test is that a suitable exception is thrown if any DTO is created with any parameter of an incorrect type.

Therefore I've just added a test case for '[{"op":"add", "path": true, "value": "bar"}]' which IMO covers all cases where one or more parameters don't match the type declared in the DTO constructor.

Let me know what you think of this?

blancks · 2025-12-08T06:45:13Z

src/operations/Add.php

-
-        return ['op' => 'replace', 'path' => $patch->path, 'value' => $this->previous];
+    public function __construct(
+        public readonly string $path,


Since PHP 8.1 is reaching EOL on January 1st, I'm considering bumping the minimum required PHP version to 8.2 for the 3.0 release. This would allow us to use the class-level readonly keyword.

What do you think about that?

That sounds a good idea - I'm certainly fine with that personally, everything I would want to use this with is already on 8.4 or later.

I've updated the DTOs to be readonly classes, except for the abstract PatchOperation base class - if we're using that as the contract for users to add their own implementations, they should probably be able to decide whether to make them readonly or not? (the op property in PatchOperation is still defined as readonly though as a basic requirement).

I wasn't sure if you'd want to bump the PHP version in 2.x or 3.x - happy to do a PR to update the composer.json and github actions etc if you let me know whether to base off main or dev?

@acoulton I have created and merged PR #15, which bumps the min php version to 8.2 for the existing actions. You should be able to merge the updated dev branch into your branch without conflicts.

This will be part of the 3.0 release as well

Thanks :)

I've rebased onto dev. Of course, the PatchOperation base class also has to be readonly as a readonly class cannot extend a non-readonly class. On reflection, I think that's OK - it seems reasonable to enforce that end-user DTO classes must also be readonly.

src/operations/PatchOperationList.php

blancks · 2025-12-08T07:30:16Z

src/operations/PatchOperationList.php

+        JsonHandlerInterface $jsonHandler = new BasicJsonHandler(),
+        array $customClasses = [],
+    ): self {
+        $patches = $jsonHandler->decode($jsonOperations, ['associative' => true]);


Some JSON decoders may not support the associative array option at all because forcing objects into arrays introduces representation limitations that make it error-prone. Decoding the patch as objects instead is a more robust approach.

Of course, you're absolutely right. I hadn't quite thought through that although the top-level patches will always decode OK as an array, the value property is completely arbitrary and could easily differ between [] and {}

Fixed now and I've added a test case explicitly using an object value to prove that it is encoded and decoded to JSON as expected.

blancks · 2025-12-08T08:11:07Z

tests/FastJsonPatchTest.php

+    #[DataProvider('validOperationsForDTOsProvider')]
+    public function testValidJsonPatchesAsDTOs(string $json, string $patches, string $expected): void


I'm okay with your assumptions here, but I would move the tests into a dedicated class for each operation DTO. I suggest this because I think each DTO also needs a test that ensures it fails when incorrect properties are passed. This is important because the PatchOperationList::fromJson method bases its validation workflow (both for patch validation and invalid DTOs) on the assumption that the constructor throws an exception when invalid input is provided.

Build on the existing implementation to allow users to specify patches as a list of operation objects, rather than as a JSON string. Unlike the JSON form, the patch operation DTOs are strict about the parameters they accept. If a user wishes to include custom properties, they can implement a class extending the base `PatchOperation`. Patch DTOs can be serialised to and from JSON, for convenience.

Now that there are explicit operation classes for each standard operation, it feels appropriate to define the expected schema for unserialized operations alongside the DTO and import it to the handler. This more clearly shows the relationship between these objects.

Because although the top-level patch entries will be equivalent as arrays or objects, properties like the `value` key can contain any type of data.

Will break on 8.1, to be rebased once the project drops 8.1 support. I have intentionally left the base `PatchOperation` class as *not* readonly in case end-users want to extend it with mutable operation DTOs for any reason. Instead, I have just left `op` as a readonly property in the base class.

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.

codecov · 2025-12-16T09:54:32Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines	Coverage Δ	Complexity Δ
src/FastJsonPatch.php	`100.00% <100.00%> (ø)`	`23.00 <11.00> (+1.00)`
src/operations/Add.php	`100.00% <100.00%> (ø)`	`1.00 <1.00> (?)`
src/operations/Copy.php	`100.00% <100.00%> (ø)`	`1.00 <1.00> (?)`
src/operations/Move.php	`100.00% <100.00%> (ø)`	`1.00 <1.00> (?)`
src/operations/PatchOperation.php	`100.00% <100.00%> (ø)`	`1.00 <1.00> (?)`
src/operations/PatchOperationList.php	`100.00% <100.00%> (ø)`	`17.00 <17.00> (?)`
src/operations/Remove.php	`100.00% <100.00%> (ø)`	`1.00 <1.00> (?)`
src/operations/Replace.php	`100.00% <100.00%> (ø)`	`1.00 <1.00> (?)`
src/operations/Test.php	`100.00% <100.00%> (ø)`	`1.00 <1.00> (?)`
src/operations/handlers/AddHandler.php	`100.00% <ø> (ø)`	`6.00 <0.00> (ø)`
... and 5 more

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

It's essentially impossible for our own DTOs to trigger PHP errors other than the ones related to creating them with incorrect parameters. However, because our code has to catch `Error` to check for certain types of error, we need to prove that it rethrows if the error is not one that it expects to handle.

acoulton · 2025-12-16T10:27:16Z

src/operations/PatchOperationList.php

+        try {
+            return new $class(...$values);
+        } catch (ArgumentCountError $e) {


I have gone for catching & rethrowing (even though it is a little verbose) because it minimises runtime performance impact in the "happy case" which is likely the most common.

Alternatively we could use Reflection to get the list of expected constructor parameter names and types and strictly compare that to the $values array before we attempt to create the object.

However this would create a performance overhead and would likely need to be cached (at least in process, perhaps between processes). That would complicate the implementation & likely introduce dependencies.

Given we know that the objects are expected to be very simple value objects, that will almost always have the right data (and that are properly validated when they get to FastJsonPatch and the relevant handler) I think this is an OK solution?

acoulton · 2025-12-16T10:29:12Z

tests/operations/PatchOperationListTest.php

+    }
+}
+
+readonly class Append extends PatchOperation


Unfortunately anonymous classes cannot be readonly until PHP 8.3, so the custom classes used by these tests have to be defined as actual named classes.

acoulton · 2025-12-16T10:41:48Z

One other thought: I originally only wrote the code for PatchOperationList::fromJson as a helper method within a testcase to allow me to use the existing json test data for testing FastJsonPatch with the DTOs.

Then I thought it might be useful in the library itself so moved it into that public static constructor.

If you're not sure about the testing/robustness of that method and the validation etc I could easily move it back to being an internal test helper.

If we wanted to go down the route of properly checking things (e.g. with Reflection) it would be cleaner to do that with an instantiable hydrator class which could then have dependencies. That could be added later as a separate PR.

acoulton force-pushed the 3.x-patch-dtos branch 3 times, most recently from ebe3f7b to a25fa9c Compare November 30, 2025 23:18

acoulton changed the base branch from main to dev November 30, 2025 23:20