This turned out to be tricky enough to probably warrant an ADR.
Problem: Patching is by nature tied to serialization/deserialization, but per the onion rules the StatementPatcher service must not depend on the Serialization namespace. The story we're working on is about user input issues (invalid/missing fields), which for other use cases get caught at *validation* time. For patching, they pop up in the patching process.
Dependencies: use cases -> validation -> serialization -> domain services -> domain
Error case control flow for add/replace statement: use case -> validator (via deserializer exception) -> validation error -> use case error response
Solution approaches
Redundant mapping of deserialization issues (https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Wikibase/+/867528)
Control flow: use case -> patcher -> patcher exception -> use case error response
Thoughts:
- bloats patcher exception handling and use case exception handling
- exception -> use case response mapping is redundant
- doesn't look that bad seeing it in code (few lines), but the amount of redundant code grows with finger grained error reports
Move the patcher into the use case namespace (https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Wikibase/+/867567)
Control flow: use case -> patcher (via validator) -> patcher exception with validation error -> validation error -> use case error response
Thoughts:
- eliminates the redundant error handling, which is nice
- looks out of place. What do we even call this kind of service that sits in the use case namespace, but isn't a use case itself?
Get rid of the patcher, do everything in the use case (https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Wikibase/+/867603)
Control flow: use case -> validator (validating the *patched* serialization) -> validation error -> use case error response
Thoughts:
- binds directly to the JsonDiff lib and its exceptions in the use case which is not great. We could probably work around that by wrapping it in a generic (i.e. array -> array, not specific to Statement or any entity) JsonPatcher, which comes at the cost of reintroduces some boilerplate (Durchlauferhitzer exception "mapping").
- removes lots of code
- acknowledges the fact that patching is an operation that acts on primitive arrays, not entities
- ^ means that the use case knows/does more than it did before, but maybe it should