4) Make ChangeOp::apply() return ChangeOpResult

Date of submission: 2019-06-21

Last updated: 2019-07-04

Discussion Deadline: 2019-07-09

Status

accepted

Context

wbeditentity summary message for Wikibase Items and Properties provides very little information on what have been changed on the entity, with the main message in the summary being "Item Changed".

In T220696, we want to update those summary messages generated for Wikibase Items and Properties when using wbeditentity to be more detailed and informative.

Wikibase Lexeme achieved generating detailed summary messages by:

1. making its implementations update the summary object passed to the change op instance with granular information about the change.
2. implementing SummaryAggregator class and using it internally in some ChangeOp implementations (some of those that are non-leaf nodes and contain further change ops) in order to combine those summary messages generated by leaf nodes (the effective change ops in the tree).

Considered actions

I. Reusing current pattern

We considered doing the same thing for Wikibase, but that seemed a little off with regards to clean design.

The main two places where this off-feeling came from are:

1. a tree hierarchy is often implemented for structural purposes, and then accessed through visitors for logic (implementing the Visitor Pattern).

2. currently ChangeOp is concerned with:

   a. representing a change that can be applied to an entity

   b. applying that change to an entity

   c. generating summary messages as a result of applying it to an entity

II. Visitor pattern design

We then considered:

1. using Visitor Pattern to implement our detailed summary generation for Wikibase Items and Properties;
2. changing ChangeOp design in a way that:
   1. allows a visitor class to operate over change ops hierarchy and attain the information it needs from it to complete its job. These information include whether the change op has been applied or discarded (no-op).
   2. is not a breaking change (WikibaseLexeme should continue to operate with no touch to it).
   3. does not extract the logic of applying a change op to an entity outside of it (remember logic can be implemented as a visitor) as it is out of scope and it isn't clear whether it will have enough justification.

This solution comes with some cost of initial implementation and refactoring of current ChangeOp design and relevant implementations within Wikibase and WikibaseLexeme.

The benefits of this solution is that it:

1. uses a more standard pattern with tree structures.
2. extracts the concern of summary messages generation out of ChangeOp, simplifying its design and implementations and scoping down the domain and dependencies of ChangeOp.

Decision

We decided to go with the second option, the visitor pattern design.

In order to achieve that, we will make ChangeOp::apply() return ChangeOpResult that is defined by the following IDL:

// encapsulates the result of applying a change op to an entity document
interface ChangeOpResult {
    // the id of the entity document that the change op was applied to
    EntityId getEntityId();
 
    // whether the entity document was actually changed in any way
    // as a result of applying the change op to it
    bool isEntityChanged();
}

Next steps

1. Change ChangeOp::apply() to return ChangeOpResult

2. Provide needed implementations of ChangeOpResult that can capture the result of applying the different ChangeOp implementations.

   Example:

   ChangeOpLabel will probably return something like ChangeOpLabelResult that is defined as:

   interface ChangeOpLabelResult : ChangeOpResult {
       string getOldLabel();
       string getNewLabel();
       string getLanguageCode();
   }

3. Update implementations of ChangeOp in Wikibase and WikibaseLexeme to conform with the changes to the interface.

Consequences

There is one consequence of going with visitor pattern design. Wikibase and WikibaseLexeme will have two separate ways to achieve similar requirements.

This should be treated as short-term inconsistency of design and should be mitigated by a follow-up refactoring task that changes Lexeme implementation into reusing the visitor pattern similar to Wikibase.

Notes

Changing ChangeOp interface in Wikibase will result in failing CI jobs due to the fact that Wikibase CI will also run WikibaseLexeme tests as part of its pipeline. This creates a circular situation for implementations of ChangeOp interface.

The way to go around it is to go in this order:

1. update the changed methods in ChangeOp interface in WikibaseLexeme implementations of that interface.
2. update the methods in ChangeOp interface in Wikibase, and make that change depend on the change from 1 above. Implementations of ChangeOp in Wikibase itself can be updated with the new methods in this step or a separate follow up step.

Although this situation is not good enough as a reason, but it slightly hints that ChangeOp concepts might better be moved out into a separate library that can then be versioned and depended on as per proper mechanisms from Wikibase and WikibaseLexeme.

---