Page MenuHomePhabricator

Not possible to edit items via wbeditentity if they have same label and description
Closed, ResolvedPublic

Description

Attempting https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Wikibase/+/506182 introduced a blocker to editors as it was no longer possible to edit entities that already had the same label and description. Even if this edit is to deliberately make them different. It failed with:

{"error":{"code":"modification-failed","info":"Label and description for language code en-gb can not have the same value.","messages":[{"name":"wikibase-validator-label-equals-description","parameters":["en-gb"],"html":{"*":"Label and description for language code en-gb can not have the same value."}}],"*":"See http://default.web.mw.localhost:8080/mediawiki/api.php for API usage. Subscribe to the mediawiki-api-announce mailing list at <https://lists.wikimedia.org/mailman/listinfo/mediawiki-api-announce> for notice of API deprecations and breaking changes."},"servedby":"66c7ba2ea93a"}

Acceptance Criteria

Given an entity that have same label and description in a language
When an editor changes the label or description of that language but they are still equal
And saves their changes
Then the save attempt should fail

Given an entity that have same label and description in a language
When an editor changes the label or description of that language so they are not equal
And saves their changes
Then the save attempt should not fail
And the new label and descriptions should be persisted

Given an entity that have same label and description in a language
When an editor do valid changes to labels, descriptions or aliases in other languages only (a.k.a the editor didn't touch the language with the equal label/description at all)
And no equal label/description pairs per language were introduced in the touched languages
And saves their changes
Then the save attempt should not fail
And the new label and descriptions should be persisted

Event Timeline

Tarrow created this task.May 6 2019, 2:49 PM
Restricted Application added a project: Wikidata. · View Herald TranscriptMay 6 2019, 2:49 PM
Restricted Application added subscribers: Liuxinyu970226, Aklapper. · View Herald Transcript
Tarrow added a comment.May 6 2019, 3:22 PM

@Jakob_WMDE I think if you wanted you could maybe make this a train blocker.

It's not the most crucial thing in the world because hopefully we don't have that many entities that have a matching label and description. Also, using wbsetdescription does work as a workaround. On the other hand if we can be sure to catch it before the train that would probably be for the best.

Jdforrester-WMF triaged this task as Unbreak Now! priority.May 6 2019, 4:30 PM
Jdforrester-WMF added a subscriber: Jdforrester-WMF.

This is either UBN! or not a train blocker. Please pick. :-)

I notice that this is still blocking the train, but no one has addressed this yet.

From the task description:

I checked this is the case by reverting just this commit.

Is that a reasonable course of action we could take to remove this as a train blocker? i.e. reverting https://gerrit.wikimedia.org/r/506182 ?

@thcipriani yes it has been reverted already

alaa_wmde moved this task from incoming to ready to go on the Wikidata board.
alaa_wmde moved this task from Incoming to Ready to estimate on the Wikidata-Campsite board.
alaa_wmde lowered the priority of this task from Unbreak Now! to High.May 7 2019, 3:04 PM

High because this is the last piece of a solution that is ready to go apart from this case. Let's fix this case and ship the working solution.

@thcipriani yes it has been reverted already

ah, hadn't realized, thank you!

alaa_wmde updated the task description. (Show Details)May 8 2019, 8:51 AM
alaa_wmde updated the task description. (Show Details)May 8 2019, 8:53 AM
alaa_wmde updated the task description. (Show Details)
alaa_wmde updated the task description. (Show Details)
alaa_wmde updated the task description. (Show Details)
alaa_wmde updated the task description. (Show Details)May 8 2019, 8:55 AM
Restricted Application added a project: User-Ladsgroup. · View Herald TranscriptMay 9 2019, 6:01 PM

I went through the code and couldn't find anything that does a full check on the entity. I even wrote some tests to make sure it's not happening (Failed TDD I guess), then cherry-picked the given change that got reverted and couldn't reproduce it locally (tried, changing another language, tried changing it to different label in the same language that has the issue, tried introducing new issue), all work as expected (I didn't test anything in API, I tested everything in desktop GUI). Can I get more info on how to reproduce the issue?

Change 509135 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/extensions/Wikibase@master] Add tests in LabelDescriptionNotEqualValidator for cases that it's only checking one language

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

Sorry for the lack of explicit reproduction :/. The steps below use httpie and the mediawiki-docker-dev setup.

I didn't test anything in API, I tested everything in desktop GUI

I think in desktop GUI different endpoints are used (wbsetlabel, wbsetdescription) not wbeditentity. Here is are steps to reproduce; sorry that they weren't clearly there before.

  1. in Wikibase git checkout 336207887deea6ad3644c222124233a132db7f58 At the time of writing this is: origin/wmf/1.34.0-wmf.4
  2. make a new item e.g. http -f POST default.web.mw.localhost:8080/mediawiki/api.php action==wbeditentity new==item data=='{ "labels": { "en":{"language": "en", "value": "foo"} }, "descriptions": { "en":{"language": "en", "value": "foo"} } }' token='+\'
  3. Apply the reverted patch git cherry-pick b71b5b2af611e593816bbe6b949e216b88e1dcf8
  4. try and change the duplicated terms: http -f POST default.web.mw.localhost:8080/mediawiki/api.php action==wbeditentity id=Q314 data=='{ "labels": { "en":{"language": "en", "value": "foo"} }, "descriptions": { "en":{"language": "en", "value": "NOTfoo"} } }' token='+\'
  5. Get returned error with info Label and description for language code en can not have the same value.

@alaa_wmde and I tested it with wmf.4 and master and with all the given acceptance criteria, also followed your steps to reproduce (did the API call using API sandbox though but it shouldn't make any difference) and still we can't reproduce it :'(

I was able to reproduce the issue, I needed to remove clear= bit. I double check where this is coming from

Thanks for digging into it so much. Seems perhaps somehow I wandered into a very edge case set-up by mistake :(. As I mentioned I only found it by testing our new termbox on a random item on beta; seems impossibly bad/good luck that I ran into it with the first request I made.

I dug deep into the issue and it's really interesting, first, if you try to edit another language, it doesn't fail using wbeditentity. This is an example I got:

{
    "entity": {
        "aliases": {},
        "claims": {},
        "descriptions": {
            "de": {
                "language": "de",
                "value": "foo"
            },
            "en": {
                "language": "en",
                "value": "foo0"
            }
        },
        "id": "Q29",
        "labels": {
            "de": {
                "language": "de",
                "value": "nfoo"
            },
            "en": {
                "language": "en",
                "value": "foo0"
            }
        },
        "lastrevid": 111,
        "sitelinks": {},
        "type": "item"
    },
    "success": 1
}

(I added de using httpie and wbeditentity later)

It also doesn't fail if instead of { "labels": { "en":{"language": "en", "value": "foo"} }, "descriptions": { "en":{"language": "en", "value": "NOTfoo"} } }, you set '{ "labels": { "en":{"language": "en", "value": "NOTfoo"} }, "descriptions": { "en":{"language": "en", "value": "foo"} } } which gives away why it's happening, the changeOps are atomic, the system tries to apply the changeOp that doesn't change anything (in our case label, and it fails. If you apply the description first, the entity is untangled and then you can simply apply the empty changeop. I can add a basic sanity check to shorten out and don't try to apply an empty change op and that would fix our very funny edge case.

Change 509373 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/extensions/Wikibase@master] Do not validate against same label and description when change is noop

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

Change 509374 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/extensions/Wikibase@master] Revert "Revert "Use LabelDescriptionNotEqualValidator in TermValidatorFactory""

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

Change 509373 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Do not validate against same label and description when change is noop

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

Change 509374 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Revert "Revert "Use LabelDescriptionNotEqualValidator in TermValidatorFactory""

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

Change 509135 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Add tests in LabelDescriptionNotEqualValidator for cases that it's only checking one language

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