Page MenuHomePhabricator

Fix wbeditentity API ignoring setting aliases to an empty set
Open, NormalPublic5 Story Points

Description

According to the docs, "you can either add or remove aliases individually, or send the full set of aliases for a language". But it seems like sending the full set of aliases is not supported if the set is empty?

Steps to reproduce:

Make a wbeditentity request with data set to {"aliases":{"en":[]}}

Actual result:

No changes are made.

Expected result:

Existing English aliases are removed.

Acceptance criteria

  • API behaviour is changed to match the expected one, as described above
  • documentation of the API input is changed to make the behaviour for empty list input explicit

Deployment Blocker

This change will be announced as a breaking change to avoid tools that has been using the current behavior in any way from starting to delete aliases unintentionally. Wait until sub-task is done T223079

Hints
Probably a good place to start is: \Wikibase\Repo\ChangeOp\Deserialization\AliasesChangeOpDeserializer::getIndexedAliasesChangeOps

NOTE: This is now a tracking ticket. See the subtasks for the actual implementation and deployment.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 1 2018, 9:15 PM
Tarrow added a subscriber: Tarrow.May 7 2019, 11:21 AM
Restricted Application added a subscriber: Liuxinyu970226. · View Herald TranscriptMay 7 2019, 11:21 AM
Tarrow renamed this task from The wbeditentity API does not support setting aliases to an empty set to Fix wbeditentity API ignoring setting aliases to an empty set.May 8 2019, 7:59 AM
Tarrow updated the task description. (Show Details)
Tarrow added a subscriber: Lea_WMDE.

@Lea_WMDE : I think we agreed in the daily that the hike wouldn't instantly pick this up and instead we'd maybe see if camp would be looking at it as an API bug. In that case I've de-tagged the hike.

@Tarrow @Jakob_WMDE @Pablo-WMDE @Matthias_Geisler_WMDE after talking to @thiemowmde: He also doesn't see a reason why the behavior should be kept this way, but suggests making a git bisect to understand since when the behavior exists and to see if this was maybe introduced as a fail save.

thiemowmde triaged this task as Low priority.May 10 2019, 11:48 AM
thiemowmde moved this task from incoming to needs discussion or investigation on the Wikidata board.
thiemowmde added a project: good first bug.

Here is the line of code that is responsible for the current behavior: https://phabricator.wikimedia.org/diffusion/EWBA/browse/master/repo/includes/ChangeOp/Deserialization/AliasesChangeOpDeserializer.php$125.

if ( $aliasesToSet !== [] ) {

This file exists since https://gerrit.wikimedia.org/r/334142 (not linked to a Phabricator ticket), mostly copied from the EditEntity class.

In EditEntity the line was originally introduced in 2013 via https://gerrit.wikimedia.org/r/73322, bug T52983. The bug was about the UI accidentally removing all aliases when it shouldn't. This means:

  • No, the fact an empty list of aliases is ignored is not an intentional product feature. It was a bugfix for a misbehavior.
  • As long as we can make sure the bug does not come back, I believe we can remove the special case and allow users to pass an empty list of aliases to the API.

Proposed fix:

if ( $aliasesChangeOps->getChangeOps() === [] || $aliasesToSet !== [] ) {
alaa_wmde raised the priority of this task from Low to Normal.May 10 2019, 12:20 PM
alaa_wmde added a project: Wikidata-Campsite.
alaa_wmde moved this task from Incoming to Ready to go on the Wikidata-Campsite board.
alaa_wmde updated the task description. (Show Details)May 10 2019, 1:11 PM

The documentation does not explicitly mention the current behavior, neither it would be implicitly concluded from that documentation. This is a bug that need to be fixed.

Though, it can be that some api users found out about it by coincidence and used it to achieve some shortcut implementation for some case they have.

Check the logs for such calls (where { ... aliases: { 'lang': [] } } ... } , i.e on a single language) to get a better feeling whether this might been used as a feature

In case you are referring to the documentation in docs/change-op-serializations.wiki, note I wrote this very late (in 2017), based on experiments and information I was able to extract directly from the code at this time. I simply missed the special case that ignores an empty list of aliases. Otherwise that special case would have become part of the documentation.

I still agree we should continue as if it is a bug. The fact it did not made it in any documentation is helpful now. ;-)

alaa_wmde added a comment.EditedMay 10 2019, 2:26 PM

@thiemowmde that's great! let's hope no one have caught it and relied on it since then :)

I checked some logs (in hadoop, thanks @Ladsgroup for your help getting them).

Last month, the first 50 requests to wbeditentity that contained any empty array for some language in aliases did not seem like this "bug" has been used as a "feature" in them. The following example items were submitted with an empty array for aliases, but that was because they did have no aliases in the corresponding languages: https://www.wikidata.org/wiki/Q62003178, https://www.wikidata.org/wiki/Q7294838, https://www.wikidata.org/wiki/Q13634732, https://www.wikidata.org/wiki/Q13634732, https://www.wikidata.org/wiki/Q12681550, https://www.wikidata.org/wiki/Q62061893, https://www.wikidata.org/wiki/Q12681509, https://www.wikidata.org/wiki/Q1590383, https://www.wikidata.org/wiki/Q62016440.

As it is also not so obvious how this "bug" can be actually useful as a "feature" in the first place, I think we should just treat as a bug and go on fixing it without the need for any breaking change announcement. @Lydia_Pintscher @Lea_Lacroix_WMDE if that sounds sensible to you, I'll move it to ready to go on top today so that we take it in story time tomorrow.

Lea_Lacroix_WMDE added a comment.EditedMay 13 2019, 6:57 AM

Edit: after talking with Lea, we decided to go for a breaking change, just to be safe and make sure that we don't accidentally change the behaviour of editors' tools.

alaa_wmde updated the task description. (Show Details)May 13 2019, 9:38 AM
alaa_wmde updated the task description. (Show Details)May 13 2019, 9:42 AM
alaa_wmde updated the task description. (Show Details)
Lea_Lacroix_WMDE added a comment.EditedMay 14 2019, 10:16 AM

Breaking change process running:

  • May 14th: announcement
  • May 28th or earlier: bugfix deployed on beta
  • June 17th: bugfix deployed on wikidata.org
WMDE-leszek updated the task description. (Show Details)May 14 2019, 12:55 PM
WMDE-leszek updated the task description. (Show Details)
WMDE-leszek set the point value for this task to 8.May 14 2019, 12:57 PM
WMDE-leszek changed the point value for this task from 8 to 5.
alaa_wmde removed alaa_wmde as the assignee of this task.May 14 2019, 1:30 PM
Michael claimed this task.May 14 2019, 2:34 PM
Michael added a project: User-Michael.
Michael moved this task from 🗃️ Incoming to ⏳ In progress on the User-Michael board.

Change 510204 had a related patch set uploaded (by Michael Große; owner: Michael Große):
[operations/mediawiki-config@master] Add feature flag config for breaking Wikibase API change

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

Change 510205 had a related patch set uploaded (by Michael Große; owner: Michael Große):
[mediawiki/extensions/Wikibase@master] Rename method to better reflect actual function

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

Change 510206 had a related patch set uploaded (by Michael Große; owner: Michael Große):
[mediawiki/extensions/Wikibase@master] Allow set aliases to empty with wbeditentity

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

Michael removed Michael as the assignee of this task.
Michael updated the task description. (Show Details)
Michael added a subscriber: Michael.

Update: because Operations team have their offsite next week, the deployment on wikidata.org is postponed to June 17th.

The change is now deployed on wikidata.org!