Page MenuHomePhabricator

Wikibase schema updaters must not modify database directly
Open, HighPublic

Description

Wikibase uses the LoadExtensionSchemaUpdates hook for database schema updates. Handlers of this hook must not modify the database directly, but must instead register updates for later execution. Wikibase does apply modifications directly in:

  • /workspace/src/extensions/Wikibase/repo/includes/Store/Sql/ChangesSubscriptionSchemaUpdater.php:48
  • /workspace/src/extensions/Wikibase/repo/includes/Store/Sql/DatabaseSchemaUpdater.php:78

This can lead to database corruption in production, see T157651.

There is a structure test up for review to prevent this kind of mistake: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/475065. However, it cannot be merged until Wikibase is fixed.

Event Timeline

daniel triaged this task as High priority.Apr 7 2020, 9:47 AM
daniel created this task.

Change 587223 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] DatabaseUpdater: don't call hook in constructor

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

No patch yet, one one above was tagged with this ticket erronously.

Change 587223 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] DatabaseUpdater: don't call hook in constructor

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

akosiaris updated the task description. (Show Details)Apr 7 2020, 12:27 PM

Change 587248 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/extensions/Wikibase@master] Use dropExtensionTable instead of dropTable in the updater

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

Change 587248 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Use dropExtensionTable instead of dropTable in the updater

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

Change 587402 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/extensions/Wikibase@master] Remove the last big of dropTable in schema updater

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

Change 587402 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Remove the last use of dropTable in schema updater

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

Looks like this is done, can the ticket be closed?

Change 507898 had a related patch set uploaded (by Daniel Kinzler; owner: Aaron Schulz):
[mediawiki/extensions/Wikibase@master] Register schema update callbacks rather than running them on DatabaseUpdater construction

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

Actually, I think that patch is wrong.

So, can we close this?

Change 507898 abandoned by Aaron Schulz:
Register schema update callbacks rather than running them on DatabaseUpdater construction

Reason:
obsolete

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

So, can we close this?

The original core test still throws errors that seem related to Wikibase, but, OTOH, it is currently 1555 commits behind master. Could you verify whether those errors are still an issue that needs resolved in Wikibase or whether they are just an artifact from the patch needing to be rebased?

See https://integration.wikimedia.org/ci/job/wmf-quibble-core-vendor-mysql-php72-docker/28710/console

12:50:45 There were 3 errors:
12:50:45 
12:50:45 1) LoadExtensionSchemaUpdatesTest::testLoadExtensionSchemaUpdates with data set #0 ('mysql', 'MysqlUpdater')
12:50:45 Wikimedia\Rdbms\DBUnexpectedError: DB connection was already closed
12:50:45 
12:50:45 /workspace/src/includes/libs/rdbms/database/Database.php:967
12:50:45 /workspace/src/includes/libs/rdbms/database/Database.php:1242
12:50:45 /workspace/src/includes/libs/rdbms/database/Database.php:1211
12:50:45 /workspace/src/includes/libs/rdbms/database/DatabaseMysqlBase.php:556
12:50:45 /workspace/src/extensions/Wikibase/repo/includes/Store/Sql/DatabaseSchemaUpdater.php:77
12:50:45 /workspace/src/extensions/Wikibase/repo/includes/Store/Sql/DatabaseSchemaUpdater.php:61
12:50:45 /workspace/src/includes/HookContainer/HookContainer.php:319
12:50:45 /workspace/src/includes/HookContainer/HookContainer.php:131
12:50:45 /workspace/src/includes/Hooks.php:136
12:50:45 /workspace/src/tests/phpunit/structure/LoadExtensionSchemaUpdatesTest.php:94
12:50:45 /workspace/src/tests/phpunit/MediaWikiIntegrationTestCase.php:437
12:50:45 /workspace/src/maintenance/doMaintenance.php:105
12:50:45 === Logs generated by test case
12:50:45 [objectcache] [debug] MainWANObjectCache using store {class} {"class":"EmptyBagOStuff"}
12:50:45 [localisation] [debug] LocalisationCache using store LCStoreNull []
12:50:45 [Wikibase] [debug] {method}: setting {settingName} was given as a closure, resolve it to {logValue} {"method":"Wikibase\\Lib\\SettingsArray::getSetting","settingName":"conceptBaseUri","logValue":"'http:\/\/127.0.0.1:9412\/entity\/'"}
12:50:45 [objectcache] [debug] MainWANObjectCache using store {class} {"class":"EmptyBagOStuff"}
12:50:45 ===
12:50:45 
12:50:45 2) LoadExtensionSchemaUpdatesTest::testLoadExtensionSchemaUpdates with data set #1 ('postgres', 'PostgresUpdater')
12:50:45 Wikimedia\Rdbms\DBUnexpectedError: DB connection was already closed
12:50:45 
12:50:45 /workspace/src/includes/libs/rdbms/database/Database.php:967
12:50:45 /workspace/src/includes/libs/rdbms/database/Database.php:1242
12:50:45 /workspace/src/includes/libs/rdbms/database/Database.php:1211
12:50:45 /workspace/src/includes/libs/rdbms/database/DatabaseMysqlBase.php:556
12:50:45 /workspace/src/includes/installer/DatabaseUpdater.php:394
12:50:45 /workspace/src/extensions/Wikibase/repo/includes/Store/Sql/ChangesSubscriptionSchemaUpdater.php:48
12:50:45 /workspace/src/extensions/Wikibase/repo/includes/Store/Sql/ChangesSubscriptionSchemaUpdater.php:37
12:50:45 /workspace/src/includes/HookContainer/HookContainer.php:319
12:50:45 /workspace/src/includes/HookContainer/HookContainer.php:131
12:50:45 /workspace/src/includes/Hooks.php:136
12:50:45 /workspace/src/tests/phpunit/structure/LoadExtensionSchemaUpdatesTest.php:94
12:50:45 /workspace/src/tests/phpunit/MediaWikiIntegrationTestCase.php:437
12:50:45 /workspace/src/maintenance/doMaintenance.php:105
12:50:45 === Logs generated by test case
12:50:45 [objectcache] [debug] MainWANObjectCache using store {class} {"class":"EmptyBagOStuff"}
12:50:45 [localisation] [debug] LocalisationCache using store LCStoreNull []
12:50:45 [Wikibase] [debug] {method}: setting {settingName} was given as a closure, resolve it to {logValue} {"method":"Wikibase\\Lib\\SettingsArray::getSetting","settingName":"conceptBaseUri","logValue":"'http:\/\/127.0.0.1:9412\/entity\/'"}
12:50:45 [objectcache] [debug] MainWANObjectCache using store {class} {"class":"EmptyBagOStuff"}
12:50:45 [warning] [info] Database type 'postgres' is not supported by the Wikibase repository. [Called from Wikibase\Repo\Store\Sql\DatabaseSchemaUpdater::doSchemaUpdate in /workspace/src/extensions/Wikibase/repo/includes/Store/Sql/DatabaseSchemaUpdater.php at line 71] {"private":false}
12:50:45 ===
12:50:45 
12:50:45 3) LoadExtensionSchemaUpdatesTest::testLoadExtensionSchemaUpdates with data set #2 ('sqlite', 'SqliteUpdater')
12:50:45 Wikimedia\Rdbms\DBUnexpectedError: DB connection was already closed
12:50:45 
12:50:45 /workspace/src/includes/libs/rdbms/database/Database.php:967
12:50:45 /workspace/src/includes/libs/rdbms/database/Database.php:1242
12:50:45 /workspace/src/includes/libs/rdbms/database/Database.php:1211
12:50:45 /workspace/src/includes/libs/rdbms/database/DatabaseMysqlBase.php:556
12:50:45 /workspace/src/extensions/Wikibase/repo/includes/Store/Sql/DatabaseSchemaUpdater.php:77
12:50:45 /workspace/src/extensions/Wikibase/repo/includes/Store/Sql/DatabaseSchemaUpdater.php:61
12:50:45 /workspace/src/includes/HookContainer/HookContainer.php:319
12:50:45 /workspace/src/includes/HookContainer/HookContainer.php:131
12:50:45 /workspace/src/includes/Hooks.php:136
12:50:45 /workspace/src/tests/phpunit/structure/LoadExtensionSchemaUpdatesTest.php:94
12:50:45 /workspace/src/tests/phpunit/MediaWikiIntegrationTestCase.php:437
12:50:45 /workspace/src/maintenance/doMaintenance.php:105
12:50:45 === Logs generated by test case
12:50:45 [objectcache] [debug] MainWANObjectCache using store {class} {"class":"EmptyBagOStuff"}
12:50:45 [localisation] [debug] LocalisationCache using store LCStoreNull []
12:50:45 [Wikibase] [debug] {method}: setting {settingName} was given as a closure, resolve it to {logValue} {"method":"Wikibase\\Lib\\SettingsArray::getSetting","settingName":"conceptBaseUri","logValue":"'http:\/\/127.0.0.1:9412\/entity\/'"}
12:50:45 [objectcache] [debug] MainWANObjectCache using store {class} {"class":"EmptyBagOStuff"}
12:50:45 ===
12:50:45 
12:50:45 ERRORS!
12:50:45 Tests: 4236, Assertions: 14220, Errors: 3, Skipped: 65.

The original core test still throws errors that seem related to Wikibase

You mean this one? https://gerrit.wikimedia.org/r/c/mediawiki/core/+/475065

As I noted there:

The "dangerous" methods on DatabaseUpdater are no longer public, see Ie73e6143bb5cbce0dae705a7451fb49cf7e3abcd.
The structure test would need to be changed to prevent access to dangerous methods on the Database object.

I'm not sure how exactly the "DB connection was already closed" issue is triggered, but the structure test as written no longer makes sense.