Page MenuHomePhabricator

Undefined index error in CachingResultsBuilder when checking constraints on empty entity
Closed, ResolvedPublic

Description

If an entity has no statements, then we don’t have any CheckResults to store in the result array (not even NullResults), so CheckingResultsBuilder returns a completely empty array. This is already a violation of the wbcheckconstraints API, I think (consumers should probably be able to expect the entity ID and an empty claims array, at least), but it also results in an error in CachingResultsBuilder when trying to access the results for that entity to cache them:

Notice: Undefined index: Q50580715 in /srv/mediawiki/php-1.31.0-wmf.25/extensions/WikibaseQualityConstraints/src/Api/CachingResultsBuilder.php on line 242

Not sure what the best solution for this will be… probably some kind of NullContextCursor (only entity ID, no statement etc.) that we can plug into the NullResult we already have from T178160?

Patch-For-Review (two chains):

Event Timeline

Reedy created this task.Mar 15 2018, 11:41 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 15 2018, 11:41 PM
Lucas_Werkmeister_WMDE renamed this task from Notice: Undefined index: Q50580715 to Undefined index error in CachingResultsBuilder when checking constraints on empty entity.Mar 16 2018, 11:40 AM
Lucas_Werkmeister_WMDE updated the task description. (Show Details)

Change 420010 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/WikibaseQualityConstraints@master] Work around CheckingResultsBuilder bug

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

I uploaded a workaround to fix the PHP warning at least; do you think this is serious enough to warrant a backport?

This is already a violation of the wbcheckconstraints API, I think (consumers should probably be able to expect the entity ID and an empty claims array, at least)

Well, not quite… we actually work around this in CheckConstraints.php itself, so the API end-user never sees this :) but this fix belongs into the proper constraint checking machinery, not tacked onto it in the API wrapper like that, IMHO.

Change 420319 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/WikibaseQualityConstraints@master] Replace NullResult’s Context with ContextCursor

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

Change 420320 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/WikibaseQualityConstraints@master] Add EntityContextCursor

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

Change 420321 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/WikibaseQualityConstraints@master] Ensure that result is populated even for empty entities

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

Change 420010 merged by jenkins-bot:
[mediawiki/extensions/WikibaseQualityConstraints@master] Work around CheckingResultsBuilder bug

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

Change 420319 merged by jenkins-bot:
[mediawiki/extensions/WikibaseQualityConstraints@master] Replace NullResult’s Context with ContextCursor

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

Change 420320 merged by jenkins-bot:
[mediawiki/extensions/WikibaseQualityConstraints@master] Add EntityContextCursor

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

Change 420321 merged by jenkins-bot:
[mediawiki/extensions/WikibaseQualityConstraints@master] Ensure that result is populated even for empty entities

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

@Jonas why is this in the “blocked” column?

Change 420993 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/WikibaseQualityConstraints@master] Revert "Work around CheckingResultsBuilder bug"

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

Pushed a new commit anyways, so moving back to “needs review”.

Guys, please communicate. This back-and-forth does not look like fun.

Change 420993 abandoned by Lucas Werkmeister (WMDE):
Revert "Work around CheckingResultsBuilder bug"

Reason:
CachingResultsBuilder has been removed and we never copied this workaround to its successor, CachingResultsSource.

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

Lucas_Werkmeister_WMDE closed this task as Resolved.Mar 26 2018, 2:39 PM

I think this is resolved now, we’ve done a lot of refactorings in this area anyways (see T189593). The proper fix is still in place (though there are some more fixes for semi-related issues in T190684), and the workaround was removed as part of the refactoring already.

mmodell changed the subtype of this task from "Task" to "Production Error".Aug 28 2019, 11:09 PM