Phan failing on Wikibase after ConnectionManager::getReadConnection() changed return type
Closed, ResolvedPublic
Actions

Description

Current status: CI fixed, follow-up changes also merged

In this MediaWiki core change, I changed the return type of ConnectionManager::getReadConnection() from IDatabase to IReadableDatabase. Apparently this broke Phan on Wikibase (example build):

client/includes/Hooks/ChangesListSpecialPageHookHandler.php:61 PhanTypeMismatchArgumentSuperType Argument 1 ($dbr) is $dbFactory->newLocalDb()->connections()->getReadConnection() of type \Wikimedia\Rdbms\Database\DbQuoter|\Wikimedia\Rdbms\Database\IDatabaseFlags|\Wikimedia\Rdbms\IReadableDatabase|\Wikimedia\Rdbms\Platform\ISQLPlatform|string but \Wikibase\Client\Hooks\ChangesListSpecialPageHookHandler::__construct() takes \Wikimedia\Rdbms\IDatabase defined at client/includes/Hooks/ChangesListSpecialPageHookHandler.php:45 (expected type to be the same or a subtype, but saw a supertype instead)
client/includes/Usage/Sql/EntityUsageTable.php:408 PhanTypeMismatchArgumentSuperType Argument 2 ($readConnection) is $readConnection of type \Wikimedia\Rdbms\Database\DbQuoter|\Wikimedia\Rdbms\Database\IDatabaseFlags|\Wikimedia\Rdbms\IReadableDatabase|\Wikimedia\Rdbms\Platform\ISQLPlatform|string but \Wikibase\Client\Usage\Sql\EntityUsageTable::getUsedEntityIdStringsMySql() takes \Wikimedia\Rdbms\IDatabase defined at client/includes/Usage/Sql/EntityUsageTable.php:465 (expected type to be the same or a subtype, but saw a supertype instead)
client/includes/Usage/Sql/SqlUsageTracker.php:231 PhanTypeMismatchArgumentSuperType Argument 1 ($db) is $db of type \Wikimedia\Rdbms\Database\DbQuoter|\Wikimedia\Rdbms\Database\IDatabaseFlags|\Wikimedia\Rdbms\IReadableDatabase|\Wikimedia\Rdbms\Platform\ISQLPlatform|string but \Wikibase\Client\Usage\Sql\SqlUsageTracker::newUsageTable() takes \Wikimedia\Rdbms\IDatabase defined at client/includes/Usage/Sql/SqlUsageTracker.php:79 (expected type to be the same or a subtype, but saw a supertype instead)
client/includes/Usage/Sql/SqlUsageTracker.php:252 PhanTypeMismatchArgumentSuperType Argument 1 ($db) is $db of type \Wikimedia\Rdbms\Database\DbQuoter|\Wikimedia\Rdbms\Database\IDatabaseFlags|\Wikimedia\Rdbms\IReadableDatabase|\Wikimedia\Rdbms\Platform\ISQLPlatform|string but \Wikibase\Client\Usage\Sql\SqlUsageTracker::newUsageTable() takes \Wikimedia\Rdbms\IDatabase defined at client/includes/Usage/Sql/SqlUsageTracker.php:79 (expected type to be the same or a subtype, but saw a supertype instead)
client/includes/Usage/Sql/SqlUsageTracker.php:272 PhanTypeMismatchArgumentSuperType Argument 1 ($db) is $db of type \Wikimedia\Rdbms\Database\DbQuoter|\Wikimedia\Rdbms\Database\IDatabaseFlags|\Wikimedia\Rdbms\IReadableDatabase|\Wikimedia\Rdbms\Platform\ISQLPlatform|string but \Wikibase\Client\Usage\Sql\SqlUsageTracker::newUsageTable() takes \Wikimedia\Rdbms\IDatabase defined at client/includes/Usage/Sql/SqlUsageTracker.php:79 (expected type to be the same or a subtype, but saw a supertype instead)
lib/includes/Store/Sql/EntityChangeLookup.php:57 PhanTypeMismatchArgumentSuperType Argument 1 ($db) is $dbr of type \Wikimedia\Rdbms\Database\DbQuoter|\Wikimedia\Rdbms\Database\IDatabaseFlags|\Wikimedia\Rdbms\IReadableDatabase|\Wikimedia\Rdbms\Platform\ISQLPlatform|string but \Wikibase\Lib\Store\Sql\EntityChangeLookup::newEntityChangeSelectQueryBuilder() takes \Wikimedia\Rdbms\IDatabase defined at lib/includes/Store/Sql/EntityChangeLookup.php:93 (expected type to be the same or a subtype, but saw a supertype instead)
lib/includes/Store/Sql/EntityChangeLookup.php:85 PhanTypeMismatchArgumentSuperType Argument 1 ($db) is $dbr of type \Wikimedia\Rdbms\Database\DbQuoter|\Wikimedia\Rdbms\Database\IDatabaseFlags|\Wikimedia\Rdbms\IReadableDatabase|\Wikimedia\Rdbms\Platform\ISQLPlatform|string but \Wikibase\Lib\Store\Sql\EntityChangeLookup::newEntityChangeSelectQueryBuilder() takes \Wikimedia\Rdbms\IDatabase defined at lib/includes/Store/Sql/EntityChangeLookup.php:93 (expected type to be the same or a subtype, but saw a supertype instead)
lib/includes/Store/Sql/PropertyInfoTable.php:259 PhanTypeMismatchReturnSuperType Returning $this->db->connections()->getReadConnection() of type \Wikimedia\Rdbms\IReadableDatabase but getReadConnection() is declared to return \Wikimedia\Rdbms\IDatabase (saw a supertype instead of a subtype)
lib/includes/Store/Sql/Terms/DatabaseMatchingTermsLookup.php:73 PhanTypeMismatchArgumentSuperType Argument 1 ($dbr) is $dbr of type \Wikimedia\Rdbms\Database\DbQuoter|\Wikimedia\Rdbms\Database\IDatabaseFlags|\Wikimedia\Rdbms\IReadableDatabase|\Wikimedia\Rdbms\Platform\ISQLPlatform|string but \Wikibase\Lib\Store\Sql\Terms\DatabaseMatchingTermsLookup::criteriaToQueryResults() takes \Wikimedia\Rdbms\IDatabase defined at lib/includes/Store/Sql/Terms/DatabaseMatchingTermsLookup.php:93 (expected type to be the same or a subtype, but saw a supertype instead)
lib/includes/Store/Sql/Terms/DatabaseUsageCheckingTermStoreCleaner.php:48 PhanTypeMismatchArgumentSuperType Argument 2 ($dbr) is $dbr of type \Wikimedia\Rdbms\Database\DbQuoter|\Wikimedia\Rdbms\Database\IDatabaseFlags|\Wikimedia\Rdbms\IReadableDatabase|\Wikimedia\Rdbms\Platform\ISQLPlatform|string but \Wikibase\Lib\Store\Sql\Terms\DatabaseInnerTermStoreCleaner::cleanTermInLangIds() takes \Wikimedia\Rdbms\IDatabase defined at lib/includes/Store/Sql/Terms/DatabaseInnerTermStoreCleaner.php:51 (expected type to be the same or a subtype, but saw a supertype instead)
lib/includes/Store/Sql/Terms/Util/ReplicaMasterAwareRecordIdsAcquirer.php:236 PhanTypeMismatchArgumentSuperType Argument 1 ($db) is $dbr of type \Wikimedia\Rdbms\Database\DbQuoter|\Wikimedia\Rdbms\Database\IDatabaseFlags|\Wikimedia\Rdbms\IReadableDatabase|\Wikimedia\Rdbms\Platform\ISQLPlatform|string but \Wikibase\Lib\Store\Sql\Terms\Util\ReplicaMasterAwareRecordIdsAcquirer::findExistingRecords() takes \Wikimedia\Rdbms\IDatabase defined at lib/includes/Store/Sql/Terms/Util/ReplicaMasterAwareRecordIdsAcquirer.php:268 (expected type to be the same or a subtype, but saw a supertype instead)
lib/includes/Store/Sql/Terms/Util/ReplicaMasterAwareRecordIdsAcquirer.php:246 PhanTypeMismatchArgumentSuperType Argument 1 ($db) is $dbr of type \Wikimedia\Rdbms\Database\DbQuoter|\Wikimedia\Rdbms\Database\IDatabaseFlags|\Wikimedia\Rdbms\IReadableDatabase|\Wikimedia\Rdbms\Platform\ISQLPlatform|string but \Wikibase\Lib\Store\Sql\Terms\Util\ReplicaMasterAwareRecordIdsAcquirer::findExistingRecords() takes \Wikimedia\Rdbms\IDatabase defined at lib/includes/Store/Sql/Terms/Util/ReplicaMasterAwareRecordIdsAcquirer.php:268 (expected type to be the same or a subtype, but saw a supertype instead)
lib/includes/Store/Sql/WikiPageEntityMetaDataLookup.php:91 PhanTypeMismatchArgumentSuperType Argument 2 ($db) is $dbReplicaRead of type \Wikimedia\Rdbms\Database\DbQuoter|\Wikimedia\Rdbms\Database\IDatabaseFlags|\Wikimedia\Rdbms\IReadableDatabase|\Wikimedia\Rdbms\Platform\ISQLPlatform|string but \Wikibase\Lib\Store\Sql\WikiPageEntityMetaDataLookup::selectRevisionInformationMultiple() takes \Wikimedia\Rdbms\IDatabase defined at lib/includes/Store/Sql/WikiPageEntityMetaDataLookup.php:268 (expected type to be the same or a subtype, but saw a supertype instead)
lib/includes/Store/Sql/WikiPageEntityMetaDataLookup.php:136 PhanTypeMismatchArgumentSuperType Argument 3 ($db) is $dbReplicaRead of type \Wikimedia\Rdbms\Database\DbQuoter|\Wikimedia\Rdbms\Database\IDatabaseFlags|\Wikimedia\Rdbms\IReadableDatabase|\Wikimedia\Rdbms\Platform\ISQLPlatform|string but \Wikibase\Lib\Store\Sql\WikiPageEntityMetaDataLookup::selectRevisionInformationById() takes \Wikimedia\Rdbms\IDatabase defined at lib/includes/Store/Sql/WikiPageEntityMetaDataLookup.php:246 (expected type to be the same or a subtype, but saw a supertype instead)
lib/includes/Store/Sql/WikiPageEntityMetaDataLookup.php:175 PhanTypeMismatchArgumentSuperType Argument 2 ($db) is $dbReplicaRead of type \Wikimedia\Rdbms\Database\DbQuoter|\Wikimedia\Rdbms\Database\IDatabaseFlags|\Wikimedia\Rdbms\IReadableDatabase|\Wikimedia\Rdbms\Platform\ISQLPlatform|string but \Wikibase\Lib\Store\Sql\WikiPageEntityMetaDataLookup::selectLatestRevisionIdsMultiple() takes \Wikimedia\Rdbms\IDatabase defined at lib/includes/Store/Sql/WikiPageEntityMetaDataLookup.php:290 (expected type to be the same or a subtype, but saw a supertype instead)
repo/includes/Store/Sql/DispatchStats.php:30 PhanTypeMismatchArgumentSuperType Argument 1 ($db) is $db of type \Wikimedia\Rdbms\Database\DbQuoter|\Wikimedia\Rdbms\Database\IDatabaseFlags|\Wikimedia\Rdbms\IReadableDatabase|\Wikimedia\Rdbms\Platform\ISQLPlatform|string but \Wikibase\Repo\Store\Sql\DispatchStats::loadLimitedNumberOfChanges() takes \Wikimedia\Rdbms\IDatabase defined at repo/includes/Store/Sql/DispatchStats.php:49 (expected type to be the same or a subtype, but saw a supertype instead)
repo/includes/Store/Sql/DispatchStats.php:34 PhanTypeMismatchArgumentSuperType Argument 1 ($db) is $db of type \Wikimedia\Rdbms\Database\DbQuoter|\Wikimedia\Rdbms\Database\IDatabaseFlags|\Wikimedia\Rdbms\IReadableDatabase|\Wikimedia\Rdbms\Platform\ISQLPlatform|string but \Wikibase\Repo\Store\Sql\DispatchStats::loadChangeTimes() takes \Wikimedia\Rdbms\IDatabase defined at repo/includes/Store/Sql/DispatchStats.php:74 (expected type to be the same or a subtype, but saw a supertype instead)
repo/includes/Store/Sql/DispatchStats.php:36 PhanTypeMismatchArgumentSuperType Argument 1 ($db) is $db of type \Wikimedia\Rdbms\Database\DbQuoter|\Wikimedia\Rdbms\Database\IDatabaseFlags|\Wikimedia\Rdbms\IReadableDatabase|\Wikimedia\Rdbms\Platform\ISQLPlatform|string but \Wikibase\Repo\Store\Sql\DispatchStats::getWbChangesRowEstimate() takes \Wikimedia\Rdbms\IDatabase defined at repo/includes/Store/Sql/DispatchStats.php:58 (expected type to be the same or a subtype, but saw a supertype instead)
repo/includes/Store/Sql/DispatchStats.php:44 PhanTypeMismatchArgumentSuperType Argument 1 ($db) is $db of type \Wikimedia\Rdbms\Database\DbQuoter|\Wikimedia\Rdbms\Database\IDatabaseFlags|\Wikimedia\Rdbms\IReadableDatabase|\Wikimedia\Rdbms\Platform\ISQLPlatform|string but \Wikibase\Repo\Store\Sql\DispatchStats::loadNumberOfEntities() takes \Wikimedia\Rdbms\IDatabase defined at repo/includes/Store/Sql/DispatchStats.php:66 (expected type to be the same or a subtype, but saw a supertype instead)
repo/includes/Store/Sql/Terms/DatabaseTermsCollisionDetector.php:295 PhanTypeMismatchReturnSuperType Returning $this->db->connections()->getReadConnection() of type \Wikimedia\Rdbms\IReadableDatabase but getDbr() is declared to return \Wikimedia\Rdbms\IDatabase (saw a supertype instead of a subtype)
repo/maintenance/pruneItemsPerSite.php:77 PhanTypeMismatchArgumentSuperType Argument 1 ($dbr) is $dbr of type \Wikimedia\Rdbms\Database\DbQuoter|\Wikimedia\Rdbms\Database\IDatabaseFlags|\Wikimedia\Rdbms\IReadableDatabase|\Wikimedia\Rdbms\Platform\ISQLPlatform|string but \Wikibase\Repo\Maintenance\PruneItemsPerSite::selectInRange() takes \Wikimedia\Rdbms\IDatabase defined at repo/maintenance/pruneItemsPerSite.php:91 (expected type to be the same or a subtype, but saw a supertype instead)

Details

Related Changes in Gerrit:

Subject	Repo	Branch	Lines +/-
Remove unneeded cast in EntityChangeSelectQueryBuilder	mediawiki/extensions/Wikibase	master	+0 -2
rdbms: Remove SelectQueryBuilder::lockForUpdate()	mediawiki/core	master	+2 -25
rdbms: Use IReadableDatabase in SelectQueryBuilder	mediawiki/core	master	+21 -21
rdbms: Hard-deprecate SelectQueryBuilder::lockForUpdate()	mediawiki/core	master	+3 -2
Fixes for changed ConnectionManager::getReadConnection() return type	mediawiki/extensions/Wikibase	master	+80 -67

Customize query in gerrit

Event Timeline

Lucas_Werkmeister_WMDE created this task.May 30 2023, 9:08 AM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 30 2023, 9:08 AM

Ok, some of this is just that other parameter types need to be changed from IDatabase to IReadableDatabase, no big deal.

But EntityUsageTable and SqlUsageTracker is nasty. EntityUsageTable has both an IDatabase $writeConnection and a ClientDomainDb (the latter not injected yet, ew). If you use a “write” method like removeUsages(), it uses the injected write connection. If you use a “read” method like getPagesUsing(), it gets a “read” connection from the ClientDomainDb. But you still need a connection to create an EntityUsageTable instance (it’s a required constructor argument) – and some SqlUsageTracker methods create an EntityUsageTable from a read connection! The only reason this doesn’t blow up is that those methods never call any of the “write” EntityUsageTable methods that would try to use the injected connection (which is actually a “read” connection!) like a “write” connection.

I think the only reason EntityUsageTable gets the connection injected is that SqlUsageTracker gets that connection from a SessionConsistentConnectionManager, so the “read” connection may sometimes in fact be a “write” connection. I think for now I’ll just EntityUsageTable’s injected connection nullable, and have the “write” methods throw an error if they’re called when no such connection was injected. That’s still a runtime-only error for a condition that we would ideally prevent at Phan type-checking time (split the “read” and “write” parts of EntityUsageTable?), but it’s better than mixing up “read” and “write” connections like it’s done at the moment.

Change 924441 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@master] Fixes for changed ConnectionManager::getReadConnection() return type

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

gerritbot added a project: Patch-For-Review.May 30 2023, 9:34 AM

Change 924470 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/core@master] rdbms: Use IReadableDatabase in SelectQueryBuilder

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

Lucas_Werkmeister_WMDE claimed this task.May 30 2023, 9:46 AM

Uggghhhh, but the needed MediaWiki core change fails with:

includes/libs/rdbms/querybuilder/SelectQueryBuilder.php:761 PhanUndeclaredMethod Call to undeclared method \Wikimedia\Rdbms\IReadableDatabase::lockForUpdate

SelectQueryBuilder::lockForUpdate() is actually soft-deprecated for one release already (since 1.40, and we’re in the 1.41 cycle), and as far as I can tell it’s not used anywhere – maybe we can just drop it?

Tagging rdbms since this task now involves a Gerrit change there. (Though I suppose we could temporarily work around it in Wikibase with a Phan cast.)

Restricted Application added a project: Performance-Team. · View Herald TranscriptMay 30 2023, 9:51 AM

Change 924474 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@master] Remove unneeded cast in EntityChangeSelectQueryBuilder

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

Change 924475 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/core@master] rdbms: Hard-deprecate SelectQueryBuilder::lockForUpdate()

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

Change 924476 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/core@master] rdbms: Remove SelectQueryBuilder::lockForUpdate()

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

In T337717#8887895, @Lucas_Werkmeister_WMDE wrote:

(Though I suppose we could temporarily work around it in Wikibase with a Phan cast.)

Yeah, on second thought, that seems better – let’s use the type cast to unblock Wikibase CI, and then clean up SQB in core a bit later.

So, to recap the current changes I uploaded:

Wikibase: Fixes for changed ConnectionManager::getReadConnection() return type can be merged ASAP, does not depend on anything else, and unbreaks Wikibase CI.
core: rdbms: Hard-deprecate SelectQueryBuilder::lockForUpdate() can be merged at any time (preferably soon), but, like all the following patches, isn’t immediately needed. (Only the Wikibase change above is urgent.)
core: rdbms: Remove SelectQueryBuilder::lockForUpdate() can be merged as soon as the stable interface policy allows – probably only in the 1.42 cycle, unless we’re happy to say that “it appears to have never been used within the Wikimedia maintained code and the MediaWiki ecosystem (except in the repo that defines it), and seems unlikely to be used elsewhere”, in which case we can merge it sooner. This then unblocks…
core: rdbms: Use IReadableDatabase in SelectQueryBuilder, which in turn unblocks…
Wikibase: Remove unneeded cast in EntityChangeSelectQueryBuilder, the last cleanup.