Page MenuHomePhabricator

Undo/revert allows having several properties with the same label in the same language
Closed, ResolvedPublic8 Estimated Story Points

Description

As a Wikidata editor, I want property labels to be unique per language at all times, so that different properties don’t get confused for one another (and so that the {{#statements:propertyName}} parser function is unambiguous).
As a Wikibase developer, I want hard constraints to actually be enforced as strictly as they’re supposed to be.

Problem:
Currently, reverting an edit to a property label (or restoring a previous revision with a different label) is allowed, even if another property got the same label in the meantime, which means that after the revert/restore, two properties have the same label. This is not supposed to be possible (if you try to edit a property label so it’s the same as another property, you get an error).

Example:

  • Create P1, label it A
  • Create P2, label it A2
  • Relabel P1 to A1
  • Relabel P2 to A
  • Revert edit to P1

Both P1 and P2 will have the label A.

Note that the other main hard constraint, sitelink uniqueness, is enforced on undo/restore:

Screenshot 2021-08-23 at 11-41-55 Error.png (181×960 px, 18 KB)

BDD
GIVEN a property with label x in a language
AND another property with label y in the same language
WHEN I try to set the first property’s label to y, whether as a regular edit, as an undo, or as a restore
THEN the edit is rejected

Acceptance criteria:

  • It’s not possible to have multiple properties with the same label in the same language at the same time by reverting an edit.

Event Timeline

Addshore set the point value for this task to 8.Sep 21 2021, 1:06 PM

Change 722640 had a related patch set uploaded (by Tobias Andersson; author: Tobias Andersson):

[mediawiki/extensions/Wikibase@master] Wip

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

Change 722866 had a related patch set uploaded (by Tobias Andersson; author: Tobias Andersson):

[mediawiki/extensions/Wikibase@master] Add LabelUniquenessValidator to check properties

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

Change 722866 abandoned by Tobias Andersson:

[mediawiki/extensions/Wikibase@master] Add LabelUniquenessValidator to check properties

Reason:

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

Change 722640 abandoned by Tobias Andersson:

[mediawiki/extensions/Wikibase@master] Add LabelUniquenessValidator to check properties

Reason:

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

Change 722866 restored by Tobias Andersson:

[mediawiki/extensions/Wikibase@master] Add LabelUniquenessValidator to check properties

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

Change 724083 had a related patch set uploaded (by Tobias Andersson; author: Tobias Andersson):

[mediawiki/extensions/Wikibase@master] Add LabelUniquenessValidator to check properties

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

Change 724348 had a related patch set uploaded (by Tobias Andersson; author: Tobias Andersson):

[mediawiki/extensions/Wikibase@master] Remove duplicate property label validation

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

Change 722866 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Add TermsCollisionDetector::detectLabelsCollision

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

Change 724083 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Add LabelUniquenessValidator to check properties

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

Change 724348 had a related patch set uploaded (by Tobias Andersson; author: Tobias Andersson):

[mediawiki/extensions/Wikibase@master] Remove duplicate property label validation

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

This change cleans up the code so that we don’t check the uniqueness multiple times. However, it has one observable consequence: with that change, the uniqueness of the label of a new property is only validated after the property has already had an ID assigned, so if that check fails, the ID is still consumed. Previously, this doesn’t happen, and if someone tries to create a property with the label of an existing property, no property ID is skipped. Is it acceptable to skip property IDs in this case, or should we leave some duplicate code to perform the check before assigning an ID?

This change cleans up the code so that we don’t check the uniqueness multiple times. However, it has one observable consequence: with that change, the uniqueness of the label of a new property is only validated after the property has already had an ID assigned, so if that check fails, the ID is still consumed. Previously, this doesn’t happen, and if someone tries to create a property with the label of an existing property, no property ID is skipped. Is it acceptable to skip property IDs in this case, or should we leave some duplicate code to perform the check before assigning an ID?

If possible we'd avoid skipping IDs unnecessarily.

Lucas_Werkmeister_WMDE added a subscriber: toan.

Alright, then we need to keep the Special:NewProperty version of the code. (We can probably still remove the other part, somewhere in change ops IIRC.)

It turns out that sending wbeditentity API requests with duplicate labels already consumes a property ID (at least if you have the right to create properties), even with the duplicate code still in place. Also, sending valid labels but an unknown datatype consumes a property ID too. So we can still remove the FingerprintUniquenessValidator part, I’d say.

Change 724348 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Remove some duplicate property label validation

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