feat: Support building & using patches as a list of DTOs #14
Conversation
acoulton
force-pushed
the
3.x-patch-dtos
branch
3 times, most recently
from
ebe3f7b to
a25fa9c
Compare
acoulton
force-pushed
the
3.x-patch-dtos
branch
2 times, most recently
from
075e62b to
efa8e0d
Compare
acoulton
force-pushed
the
3.x-patch-dtos
branch
from
efa8e0d to
9b8d4ff
Compare
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.
acoulton
force-pushed
the
3.x-patch-dtos
branch
from
ae61ce3 to
7658f79
Compare
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
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.
|
One other thought: I originally only wrote the code for 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 first of all, thank you for the amazing effort you're putting into this project and sorry that I'm only getting to review this now. The helper method is actually a nice addition to the library, and I'm happy with it being public and used in tests as well. Going down the reflection route feels a bit redundant to me. Letting DTO instantiation fail naturally is good enough in this case, especially now that we have tests explicitly covering that behaviour. If we ever need stricter or more explicit validation later on, introducing a dedicated hydrator as a separate concern would make sense, but I don't think that's necessary right now. |
There was a problem hiding this comment.
@acoulton I would add the PatchOperationList type to the isValidPatch method here as well.
This should be the very last thing before merging the PR 😃
There was a problem hiding this comment.
Thanks @blancks and no need to apologise for the review timing :)
That's a good suggestion, I've added that now.
Internally, this is already implemented - it just required a change to the typehint for `isValidPatch`. I've refactored the existing `isValidPatch` tests to use a DataProvider to make the overlap between examples for strings and DTOs clearer. As part of this I added a couple of new examples of valid / invalid string patches. This is because some of the existing string cases (missing params / structural issues) cannot happen with DTOs.
acoulton
force-pushed
the
3.x-patch-dtos
branch
from
7c5d983 to
3747bce
Compare
| $FastJsonPatch = FastJsonPatch::fromJson('{"foo":"bar"}'); | ||
| $this->assertFalse($FastJsonPatch->isValidPatch('{"op":"test","path":"/foo","value":"bar"}')); | ||
| $this->assertFalse($FastJsonPatch->isValidPatch('[{"op":"add"}]')); |
There was a problem hiding this comment.
I refactored the existing isValidPatch tests to use a data provider, to make it easier to see the overlap / differences between the string and DTO examples that are tested.
Some of the previous string examples (structural issues / missing parameters) cannot be represented as DTOs. So I also added a couple of new string examples to prove that these behave consistently with both a string patch and a DTO.
| 'DTO patch - invalid (invalid path)' => [ | ||
| new PatchOperationList(new Remove(path: 'not a path')), | ||
| false, | ||
| ], |
There was a problem hiding this comment.
This (invalid JSON pointer string) was actually the only case I could think of where a DTO created in code could actually fail validation.
2 participants
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.