Page MenuHomePhabricator

Various tests failing with Echo/UserMerge issues
Closed, ResolvedPublic

Description

18:45:47 1) MergeUserTest::testBasicMerge
18:45:47 === Logs generated by test case
18:45:47 [PHPUnitCommand] [info] End test LessFileCompilationTest::testLessFileCompilation {"private":false}
18:45:47 ===
18:45:47 Wikimedia\Rdbms\DBQueryError: A database query error has occurred. Did you forget to run your application's database schema updater after upgrading? 
18:45:47 Query: SELECT  echo_event.*  FROM `unittest_echo_notification`,`unittest_echo_event`    WHERE notification_user = '8' AND (notification_event = event_id) AND event_type = 'thank-you-edit'  ORDER BY notification_timestamp ASC 
18:45:47 Function: EchoHooks::onMergeAccountFromTo
18:45:47 Error: 1051 Unknown table 'wikidb.echo_event' (/workspace/db/quibble-mysql-7h2jwcnv/socket)
18:45:47 
18:45:47 
18:45:47 /workspace/src/includes/libs/rdbms/database/Database.php:1515
18:45:47 /workspace/src/includes/libs/rdbms/database/Database.php:1485
18:45:47 /workspace/src/includes/libs/rdbms/database/Database.php:1245
18:45:47 /workspace/src/includes/libs/rdbms/database/Database.php:1712
18:45:47 /workspace/src/extensions/Echo/includes/EchoHooks.php:1416
18:45:47 /workspace/src/includes/deferred/MWCallableUpdate.php:34
18:45:47 /workspace/src/includes/deferred/DeferredUpdates.php:270
18:45:47 /workspace/src/includes/deferred/DeferredUpdates.php:216
18:45:47 /workspace/src/includes/deferred/DeferredUpdates.php:140
18:45:47 /workspace/src/includes/deferred/DeferredUpdates.php:309
18:45:47 /workspace/src/includes/deferred/DeferredUpdates.php:104
18:45:47 /workspace/src/includes/deferred/DeferredUpdates.php:121
18:45:47 /workspace/src/extensions/Echo/includes/EchoHooks.php:1447
18:45:47 /workspace/src/includes/Hooks.php:174
18:45:47 /workspace/src/includes/Hooks.php:202
18:45:47 /workspace/src/extensions/UserMerge/includes/MergeUser.php:424
18:45:47 /workspace/src/extensions/UserMerge/includes/MergeUser.php:50
18:45:47 /workspace/src/extensions/UserMerge/tests/phpunit/MergeUserTest.php:44
18:45:47 /workspace/src/tests/phpunit/MediaWikiTestCase.php:425
18:45:47 /workspace/src/tests/phpunit/phpunit.php:90
18:45:47 /workspace/src/maintenance/doMaintenance.php:94
18:45:47 /workspace/src/tests/phpunit/phpunit.php:129
18:45:47 
18:45:47 2) MergeUserTest::testMergeOfUserGroups
18:45:47 === Logs generated by test case
18:45:47 [PHPUnitCommand] [info] End test LessFileCompilationTest::testLessFileCompilation {"private":false}
18:45:47 ===
18:45:47 Wikimedia\Rdbms\DBQueryError: A database query error has occurred. Did you forget to run your application's database schema updater after upgrading? 
18:45:47 Query: SELECT  echo_event.*  FROM `unittest_echo_notification`,`unittest_echo_event`    WHERE notification_user = '11' AND (notification_event = event_id) AND event_type = 'thank-you-edit'  ORDER BY notification_timestamp ASC 
18:45:47 Function: EchoHooks::onMergeAccountFromTo
18:45:47 Error: 1051 Unknown table 'wikidb.echo_event' (/workspace/db/quibble-mysql-7h2jwcnv/socket)
18:45:47 
18:45:47 
18:45:47 /workspace/src/includes/libs/rdbms/database/Database.php:1515
18:45:47 /workspace/src/includes/libs/rdbms/database/Database.php:1485
18:45:47 /workspace/src/includes/libs/rdbms/database/Database.php:1245
18:45:47 /workspace/src/includes/libs/rdbms/database/Database.php:1712
18:45:47 /workspace/src/extensions/Echo/includes/EchoHooks.php:1416
18:45:47 /workspace/src/includes/deferred/MWCallableUpdate.php:34
18:45:47 /workspace/src/includes/deferred/DeferredUpdates.php:270
18:45:47 /workspace/src/includes/deferred/DeferredUpdates.php:216
18:45:47 /workspace/src/includes/deferred/DeferredUpdates.php:140
18:45:47 /workspace/src/includes/deferred/DeferredUpdates.php:309
18:45:47 /workspace/src/includes/deferred/DeferredUpdates.php:104
18:45:47 /workspace/src/includes/deferred/DeferredUpdates.php:121
18:45:47 /workspace/src/extensions/Echo/includes/EchoHooks.php:1447
18:45:47 /workspace/src/includes/Hooks.php:174
18:45:47 /workspace/src/includes/Hooks.php:202
18:45:47 /workspace/src/extensions/UserMerge/includes/MergeUser.php:424
18:45:47 /workspace/src/extensions/UserMerge/includes/MergeUser.php:50
18:45:47 /workspace/src/extensions/UserMerge/tests/phpunit/MergeUserTest.php:63
18:45:47 /workspace/src/tests/phpunit/MediaWikiTestCase.php:425
18:45:47 /workspace/src/tests/phpunit/phpunit.php:90
18:45:47 /workspace/src/maintenance/doMaintenance.php:94
18:45:47 /workspace/src/tests/phpunit/phpunit.php:129
18:45:47 
18:45:47 3) MergeUserTest::testMergeEditcount
18:45:47 === Logs generated by test case
18:45:47 [PHPUnitCommand] [info] End test LessFileCompilationTest::testLessFileCompilation {"private":false}
18:45:47 ===
18:45:47 Wikimedia\Rdbms\DBQueryError: A database query error has occurred. Did you forget to run your application's database schema updater after upgrading? 
18:45:47 Query: SELECT  echo_event.*  FROM `unittest_echo_notification`,`unittest_echo_event`    WHERE notification_user = '15' AND (notification_event = event_id) AND event_type = 'thank-you-edit'  ORDER BY notification_timestamp ASC 
18:45:47 Function: EchoHooks::onMergeAccountFromTo
18:45:47 Error: 1051 Unknown table 'wikidb.echo_event' (/workspace/db/quibble-mysql-7h2jwcnv/socket)
18:45:47 
18:45:47 
18:45:47 /workspace/src/includes/libs/rdbms/database/Database.php:1515
18:45:47 /workspace/src/includes/libs/rdbms/database/Database.php:1485
18:45:47 /workspace/src/includes/libs/rdbms/database/Database.php:1245
18:45:47 /workspace/src/includes/libs/rdbms/database/Database.php:1712
18:45:47 /workspace/src/extensions/Echo/includes/EchoHooks.php:1416
18:45:47 /workspace/src/includes/deferred/MWCallableUpdate.php:34
18:45:47 /workspace/src/includes/deferred/DeferredUpdates.php:270
18:45:47 /workspace/src/includes/deferred/DeferredUpdates.php:216
18:45:47 /workspace/src/includes/deferred/DeferredUpdates.php:140
18:45:47 /workspace/src/includes/deferred/DeferredUpdates.php:309
18:45:47 /workspace/src/includes/deferred/DeferredUpdates.php:104
18:45:47 /workspace/src/includes/deferred/DeferredUpdates.php:121
18:45:47 /workspace/src/extensions/Echo/includes/EchoHooks.php:1447
18:45:47 /workspace/src/includes/Hooks.php:174
18:45:47 /workspace/src/includes/Hooks.php:202
18:45:47 /workspace/src/extensions/UserMerge/includes/MergeUser.php:424
18:45:47 /workspace/src/extensions/UserMerge/includes/MergeUser.php:50
18:45:47 /workspace/src/extensions/UserMerge/tests/phpunit/MergeUserTest.php:104
18:45:47 /workspace/src/tests/phpunit/MediaWikiTestCase.php:425
18:45:47 /workspace/src/tests/phpunit/phpunit.php:90
18:45:47 /workspace/src/maintenance/doMaintenance.php:94
18:45:47 /workspace/src/tests/phpunit/phpunit.php:129

Event Timeline

Reedy created this task.Mar 2 2019, 6:47 PM
Restricted Application added a project: Growth-Team. · View Herald TranscriptMar 2 2019, 6:47 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Reedy triaged this task as High priority.Mar 2 2019, 7:51 PM

Change 493880 had a related patch set uploaded (by Umherirrender; owner: Umherirrender):
[mediawiki/extensions/Echo@master] Use explicit columns and avoid SELECT *

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

Reedy lowered the priority of this task from High to Normal.Mar 2 2019, 8:34 PM
Daimona added a subscriber: Daimona.Mar 2 2019, 8:59 PM

Thanks for the prompt fix! I'm sorry for the inconvenience, although IMHO it's been useful to discover a potential bug cause.

Thanks for the prompt fix! I'm sorry for the inconvenience, although IMHO it's been useful to discover a potential bug cause.

Yes, it was good to add this dependency and find the bug, but it breaks more than only Echo due to other extension using this as depenendcy

While I know the CI revert is temporary, I'd like to point out that CheckUser is currently broken because phan has been enabled for it, but now it lacks the needed directories.
I'd also like to seize the opportunity for a remark: it seems very wrong that selecting table_name.* fails because it cannot find the table. I think Database::select() and friends should detect when a table name is used this way and properly add the table prefix to it. Or at the very least, document that this syntax isn't valid, and/or ban it via a CodeSniffer check. One may argue that SELECTs involving * usually aren't the best thing to do, but that doesn't mean that we can leave them broken.
I don't guarantee that I'll get it done myself, but I believe it's very important to address this issue.

In case of table_name.* the table_name could be the table alias and not the name of the table.
So it is possible to use it this way, but than it needs an explizit alias on the Database::select to avoid the problem on table prefix

I have not tested, but this could work, because table prefix is first added and than checked if table name and alias are equal and optimized:

				$dbw->select(
					[ 'echo_notification', 'echo_event' => 'echo_event' ],
					'echo_event.*',
					[
						'notification_user' => $newUser->getId(),
						'notification_event = event_id',
						'event_type' => 'thank-you-edit',
					],
					$method,
					[ 'ORDER BY' => 'notification_timestamp ASC' ]
				)

With alias

				$dbw->select(
					[ 'echo_notification', 'event' => 'echo_event' ],
					'event.*',
					[
						'notification_user' => $newUser->getId(),
						'notification_event = event_id',
						'event_type' => 'thank-you-edit',
					],
					$method,
					[ 'ORDER BY' => 'notification_timestamp ASC' ]
				)

The best is to avoid SELECT * and only select the fields which are needed.

This seems to complex for phpcs, because it has also to check for return of getQueryInfo or use of variable. The security plugin also has some limitation there.

@Umherirrender Your patch for echo is definitely good, and fine for now. I'm trying to see if I can implement the feature in the Database class, although it may be tricky... Yes, avoiding * is the best choice, but it's not always applicable, and as I was saying above we should keep it working anyway.
Agreed on PHPCS, definitely too complicated.

Change 494040 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/core@master] [WIP] Add table prefix when using 'table_name.*' in SELECT

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

Change 493880 merged by jenkins-bot:
[mediawiki/extensions/Echo@master] Fix UserMerge integration for use with table prefix

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

Change 494304 had a related patch set uploaded (by Reedy; owner: Umherirrender):
[mediawiki/extensions/Echo@wmf/1.33.0-wmf.19] Fix UserMerge integration for use with table prefix

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

Change 494304 merged by jenkins-bot:
[mediawiki/extensions/Echo@wmf/1.33.0-wmf.19] Fix UserMerge integration for use with table prefix

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

Mentioned in SAL (#wikimedia-operations) [2019-03-04T20:44:39Z] <reedy@deploy1001> Synchronized php-1.33.0-wmf.19/extensions/Echo/: T217487 (duration: 00m 53s)

Reedy renamed this task from OAuth tests failing with Echo/UserMerge issues to Various tests failing with Echo/UserMerge issues.Mar 4 2019, 8:45 PM
Reedy removed a project: MediaWiki-extensions-OAuth.
JTannerWMF closed this task as Resolved.Mar 5 2019, 7:39 PM
JTannerWMF claimed this task.

The patch for changing Database::select was still pending, but it's better to move it to a separate task.