Add missing DiffOpChange to ListPatcher

[thiemowmde]

Found while working on wmde/WikibaseDataModelServices#120. With this, it would be possible to reuse this as a service in FingerprintPatcher.

Bug: T78298

[mariushoch]

+1 I can't see why this shouldn't be done (with tests of course), but I think someone with more knowledge of this component should chime in (@JeroenDeDauw).

[JeroenDeDauw]

At first glance I'm inclined to say that the list stuff is meant to be non-positional, and that change operations are thus not allowed here. Been some time since I looked at this library though, will double check later.

Did you look at MapPatcher?

[thiemowmde]

I don't see what's "positional" here. A change is a remove and an add combined. The code of the two existing remove and add operations is literally duplicated.

As said, I would like to reuse this implementation in the FingerprintPatcher but can't because it misses this operation.

[JeroenDeDauw]

Oops, forgot to look at this. On my TODO list again now

[JeroenDeDauw]

Ok, looking at this now. Too bad you did not answer my question, that'd have saved some time.

[JeroenDeDauw]

This patcher was created to mirror ListDiffer, which it does, as this one never creates change operations. That does not mean adding support for change operations here is bad, though I'd like to understand where those are coming from first.

What code in the linked DMS PR is this supposed to replace? getPatchedAliases? (This is a sincere question, please answer it.) As far as I can tell, the old code there did not deal with change operations (as it ended up using this ListPatcher). Did something change that now change operations are created for alias lists? And if so, where is this code?

[thiemowmde]

The refactored FingerprintPatcher in wmde/WikibaseDataModelServices#120 works with DataModel objects instead of arrays. Therefore it can not use generic array patchers any more. But there is one exception: A set of aliases is nothing but an array of strings. This could still use ListPatcher.

I would like to replace the line $this->getPatchedAliases( $aliases, $patch ) (see https://github.com/wmde/WikibaseDataModelServices/pull/120/files#diff-2025a7dc62b3bce3d57afbac4038d098R141) with $this->listPatcher->patch( $aliases, $patch ). But when I do this a test fails.

The reason for this is hidden in the current MapPatcher implementation. It applies all kinds of change ops (add, change and remove) recursively (see https://github.com/wmde/Diff/blob/master/src/Patcher/MapPatcher.php#L149). You can construct all kinds of aliases diffs (that's what I did in wmde/WikibaseDataModelServices#123) that do not forward to ListPatcher. But with the refactoring I did there is no recursion any more. This means all aliases diffs are forwarded to the same getPatchedAliases method, including diffs ListPatcher never supported. It didn't had to. MapPatcher took care of these.

[thiemowmde]

Added tests.

[thiemowmde]

For completeness:

Too bad you did not answer my question

Are you referring to this?

Did you look at MapPatcher?

I really can't tell what you want to know. I probably looked at all code at some point. I looked at this class while working on FingerprintPatcher. So the answer is yes, I guess? Does this answer your question? How could this answer have saved time?

[JeroenDeDauw]

Are the only tests that are failing new ones added in https://github.com/wmde/WikibaseDataModelServices/pull/123/files (for instance at line 190)? I'm not understanding where in the production code change operations for aliases would come from, as those are a list, not a map.

It's now clear to me that the change you made, at least in its current form, goes against the intention of the modified class. As per README "The Diff class can be set to be either associative or non-associative. In case of the later, only DiffOpAdd and DiffOpRemove are allowed in it". And in ListPatcher itself: "ListPatcher can only patch using non-associative diffs". Again, this does not mean there should be no changes in Diff. I can't say without understanding where these change operations are coming from.

[JeroenDeDauw]

How could this answer have saved time?

I was wondering if what you where looking for might be MapPatcher. Now I know better what you are trying to do, this is clearly not the case. I did not know that at the time, so had to verify myself this was not relevant. This answer would have been great: "I want to patch a list of aliases [link code]", and would also have saved me from asking the next question.

[thiemowmde]

I wanted to try a refactoring of FingerprintPatcher to make it work on DataModel objects instead of arrays, but found that lots of features are uncovered. I was more fearless a year ago, but nowadays refactoring with insufficient coverage scares me.

While collecting tests in https://github.com/wmde/WikibaseDataModelServices/pull/123/files#diff-083f80d290824bee8a9622736e379faaR89 I found that all possible (and actually supported) types of alias changes make sense:

1. 'en' => new DiffOpChange( [ 'a', 'c' ], [ 'b', 'c' ] ) means somebody changed the English alias "a" to "b". This is a perfectly fine diff, but it's not minimal. When an other user edits "c" the same time, this causes an edit conflict.
2. 'en' => new Diff( [ new DiffOpChange( 'a', 'b' ) ] ) is the same edit, but minimal. An edit to "c" can't cause an edit conflict, but a concurrent edit to "a" is guaranteed to cause an edit conflict.
3. 'en' => new Diff( [ new DiffOpRemove( 'a' ), new DiffOpAdd( 'b' ) ] ) is not guaranteed to cause an edit conflict. Two users can change "a" the same time.

Currently we use number 3. We may switch to number 2. Other users of this component may use these other possibilities. And since the class supports them all, I did not wanted to make my refactoring a breaking change.

This answer would have been great: "I want to patch a list of aliases

I do have the feeling you still miss a critical point: MapPatcher does not always call ListPatcher to apply diffs on non-associative, numerically indexed arrays. Sometimes MapPatcher applies diffs on numerical arrays itself, without calling ListPatcher. This happens in FingerprintPatcher.

The interface of FingerprintPatcher does not say it is limited to only one specific type of alias diff. It does support all three.

[JeroenDeDauw]

Can we close this?

[thiemowmde]

You mean merge? This would still help making FingerprintPatcher less messy.

[JeroenDeDauw]

My understanding is still that this patch is going again the intent of the modified code. IIRC we talked about this in person at some point, where I explained this, and you went "oh, I was not aware of that part".

[thiemowmde]

I don't remember what we talked about. I did not documented it, unfortunately. I just read this thread again and still believe it applies.

Aliases are organized in lists. We do apply change operations on lists. This class is supposed to do this job but silently ignores change operations. It doesn't even tell you. For the moment FingerprintPatcher works around this issue by basically holding a copy of the fixed version of this class. I would like to fix this properly.

[JeroenDeDauw]

As indicated by the check in this patcher, it is not meant to deal with associative diffs. And as indicated in the README, non-associative diffs are not supposed to have change operations.

[thiemowmde]

Denying a problem exists does not make it go away.

[JeroenDeDauw]

Please reopen if still relevant