Page MenuHomePhabricator

💥️ Decide how to deal with missing/invalid field errors in patched statements
Closed, ResolvedPublic

Description

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

Event Timeline

Change 867528 had a related patch set uploaded (by Jakob; author: Jakob):

[mediawiki/extensions/Wikibase@master] REST: Improve invalid field errors, keeping the patcher

https://gerrit.wikimedia.org/r/867528

Change 867603 had a related patch set uploaded (by Jakob; author: Jakob):

[mediawiki/extensions/Wikibase@master] REST: Improve invalid field errors, removing the patcher

https://gerrit.wikimedia.org/r/867603

Change 867567 had a related patch set uploaded (by Jakob; author: Jakob):

[mediawiki/extensions/Wikibase@master] REST: Improve invalid field errors, moving the patcher

https://gerrit.wikimedia.org/r/867567

Change 867603 abandoned by Jakob:

[mediawiki/extensions/Wikibase@master] REST: Improve invalid field errors, removing the patcher

Reason:

https://gerrit.wikimedia.org/r/867603

Change 867567 abandoned by Jakob:

[mediawiki/extensions/Wikibase@master] REST: Improve invalid field errors, moving the patcher

Reason:

https://gerrit.wikimedia.org/r/867567

Change 867528 abandoned by Jakob:

[mediawiki/extensions/Wikibase@master] REST: Improve invalid field errors, keeping the patcher

Reason:

https://gerrit.wikimedia.org/r/867528