Page MenuHomePhabricator
Create Task
Maniphest T176041

rev_comment needs a default value in unit tests
Closed, ResolvedPublic
Actions

Description

The following error was thrown when running phpunit.php on a clean installation of MW:

[d0e08f33b510cc9f39a57c6f] [no req]   Wikimedia\Rdbms\DBQueryError from line 1149 of /var/www/html/includes/libs/rdbms/database/Database.php: A database query error has occurred. Did you forget to run your application's database schema updater after upgrading? 
Query: INSERT  INTO `unittest_revision` (rev_page,rev_text_id,rev_minor_edit,rev_user,rev_user_text,rev_timestamp,rev_deleted,rev_len,rev_parent_id,rev_sha1,rev_content_model,rev_content_format) VALUES ('1','1','0','1','UTSysop','20170916150250','0','9','0','aqhji8gcje5j0y511s7kgskw3dd6qua',NULL,NULL)
Function: Revision::insertOn
Error: 1364 Field 'rev_comment' doesn't have a default value (localhost)

Backtrace:
#0 /var/www/html/includes/libs/rdbms/database/Database.php(979): Wikimedia\Rdbms\Database->reportQueryError(string, integer, string, string, boolean)
#1 /var/www/html/includes/libs/rdbms/database/Database.php(1589): Wikimedia\Rdbms\Database->query(string, string)
#2 /var/www/html/includes/Revision.php(1505): Wikimedia\Rdbms\Database->insert(string, array, string)
#3 /var/www/html/includes/page/WikiPage.php(1873): Revision->insertOn(Wikimedia\Rdbms\DatabaseMysqli)
#4 /var/www/html/includes/page/WikiPage.php(1627): WikiPage->doCreate(WikitextContent, integer, User, string, array)
#5 /var/www/html/tests/phpunit/MediaWikiTestCase.php(1093): WikiPage->doEditContent(WikitextContent, string, integer, boolean, User)
#6 /var/www/html/tests/phpunit/MediaWikiTestCase.php(1341): MediaWikiTestCase->addCoreDBData()
#7 /var/www/html/tests/phpunit/MediaWikiTestCase.php(419): MediaWikiTestCase->resetDB(Wikimedia\Rdbms\DatabaseMysqli, array)
#8 /var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php(722): MediaWikiTestCase->run(PHPUnit_Framework_TestResult)
#9 /var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php(722): PHPUnit_Framework_TestSuite->run(PHPUnit_Framework_TestResult)
#10 /var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php(722): PHPUnit_Framework_TestSuite->run(PHPUnit_Framework_TestResult)
#11 /var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php(722): PHPUnit_Framework_TestSuite->run(PHPUnit_Framework_TestResult)
#12 /var/www/html/vendor/phpunit/phpunit/src/TextUI/TestRunner.php(440): PHPUnit_Framework_TestSuite->run(PHPUnit_Framework_TestResult)
#13 /var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php(149): PHPUnit_TextUI_TestRunner->doRun(PHPUnit_Framework_TestSuite, array)
#14 /var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php(100): PHPUnit_TextUI_Command->run(array, boolean)
#15 /var/www/html/tests/phpunit/phpunit.php(133): PHPUnit_TextUI_Command::main()
#16 /var/www/html/maintenance/doMaintenance.php(92): PHPUnitMaintClass->execute()
#17 /var/www/html/tests/phpunit/phpunit.php(163): require(string)
#18 {main}
PHP Fatal error:  Uncaught MWException: WikiPageTest::tearDown() must call parent::tearDown() in /var/www/html/tests/phpunit/MediaWikiTestCase.php:129
Stack trace:
#0 [internal function]: MediaWikiTestCase->__destruct()
#1 {main}
  thrown in /var/www/html/tests/phpunit/MediaWikiTestCase.php on line 129

Details

SubjectRepoBranchLines +/-
Don't unconditionally run patch-editsummary-length.sqlmediawiki/coremaster+24 -8
Customize query in gerrit

Related Objects
Search...

Event Timeline

Huji created this task.Sep 16 2017, 3:06 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 16 2017, 3:06 PM
Huji added a parent task: T176039: Ensure no errors or failures occur with MediaWiki core tests on a clean installation.Sep 16 2017, 3:06 PM
Huji added a subscriber: Anomie.Sep 16 2017, 3:13 PM

@Anomie can it have to do with the CommentStore?

Huji renamed this task from rev_comment needs a default value to rev_comment needs a default value in unit tests.Sep 16 2017, 3:18 PM
Huji removed a project: AbuseFilter.Sep 16 2017, 3:20 PM

I reverted my LocalSettings to a state where no mention of AbuseFilter exists and I still get the above fatal error.

Huji updated the task description. (Show Details)Sep 16 2017, 3:22 PM
Huji triaged this task as Low priority.Sep 16 2017, 8:54 PM
Huji moved this task from Inbox to PHPUnit on the MediaWiki-Core-Tests board.

The fatal error happens while running WikiPageTest::testCommentMigrationOnDeletion

Steps to reproduce:

  1. Install MediaWiki using the head revision (currently at ecefa8f)
  2. Running phpunit, you will *not* get a fatal error
  3. Now run php maintenance/update.php
  4. Re-run phpunit and you will get the fatal error

PS: In the "real" revision table (not the unittest_ one), rev_comment does have a default value (it is NULL).

Huji added a comment.Sep 16 2017, 9:11 PM

Further details:

When I run php tests/phpunit/phpunit.php --tap tests/phpunit/includes/page/WikiPageTest.php I get the fatal. When I comment out line 90 I do not get the fatal. That line does, however, provide a summary string to WikiPage::doEditContent so I am not sure where the DB error is coming from

Huji added subscribers: matmarex, Legoktm.Sep 17 2017, 1:12 AM

I am notifying @matmarex and @Legoktm who have recently contributed to WikiPageTest.php

Huji raised the priority of this task from Low to High.EditedSep 17 2017, 10:17 PM
Huji added projects: DBA, MediaWiki-Maintenance-system.

I was wrong above. And I think I have uncovered a bigger issue.

I installed MediaWiki again, and ran mysqldump immediately thereafter. Here is the pertinent part:

CREATE TABLE `revision` (
  `rev_id` int(10) unsigned NOT NULL AUTO_INCREMENT,
  `rev_page` int(10) unsigned NOT NULL,
  `rev_text_id` int(10) unsigned NOT NULL,
  `rev_comment` varbinary(767) NOT NULL DEFAULT '',
  `rev_user` int(10) unsigned NOT NULL DEFAULT '0',
  `rev_user_text` varbinary(255) NOT NULL DEFAULT '',
  `rev_timestamp` binary(14) NOT NULL DEFAULT '\0\0\0\0\0\0\0\0\0\0\0\0\0\0',
  `rev_minor_edit` tinyint(3) unsigned NOT NULL DEFAULT '0',
  `rev_deleted` tinyint(3) unsigned NOT NULL DEFAULT '0',
  `rev_len` int(10) unsigned DEFAULT NULL,
  `rev_parent_id` int(10) unsigned DEFAULT NULL,
  `rev_sha1` varbinary(32) NOT NULL DEFAULT '',
  `rev_content_model` varbinary(32) DEFAULT NULL,
  `rev_content_format` varbinary(64) DEFAULT NULL,
  PRIMARY KEY (`rev_id`),
  KEY `rev_page_id` (`rev_page`,`rev_id`),
  KEY `rev_timestamp` (`rev_timestamp`),
  KEY `page_timestamp` (`rev_page`,`rev_timestamp`),
  KEY `user_timestamp` (`rev_user`,`rev_timestamp`),
  KEY `usertext_timestamp` (`rev_user_text`,`rev_timestamp`),
  KEY `page_user_timestamp` (`rev_page`,`rev_user`,`rev_timestamp`)
) ENGINE=InnoDB AUTO_INCREMENT=2 DEFAULT CHARSET=binary MAX_ROWS=10000000 AVG_ROW_LENGTH=1024;
/*!40101 SET character_set_client = @saved_cs_client */;

Then I ran php maintenance/update.php and ran mysqldump again. Here is the pertinent part again:

CREATE TABLE `revision` (
  `rev_id` int(10) unsigned NOT NULL AUTO_INCREMENT,
  `rev_page` int(10) unsigned NOT NULL,
  `rev_text_id` int(10) unsigned NOT NULL,
  `rev_comment` varbinary(767) NOT NULL,
  `rev_user` int(10) unsigned NOT NULL DEFAULT '0',
  `rev_user_text` varbinary(255) NOT NULL DEFAULT '',
  `rev_timestamp` binary(14) NOT NULL DEFAULT '\0\0\0\0\0\0\0\0\0\0\0\0\0\0',
  `rev_minor_edit` tinyint(3) unsigned NOT NULL DEFAULT '0',
  `rev_deleted` tinyint(3) unsigned NOT NULL DEFAULT '0',
  `rev_len` int(10) unsigned DEFAULT NULL,
  `rev_parent_id` int(10) unsigned DEFAULT NULL,
  `rev_sha1` varbinary(32) NOT NULL DEFAULT '',
  `rev_content_model` varbinary(32) DEFAULT NULL,
  `rev_content_format` varbinary(64) DEFAULT NULL,
  PRIMARY KEY (`rev_id`),
  KEY `rev_page_id` (`rev_page`,`rev_id`),
  KEY `rev_timestamp` (`rev_timestamp`),
  KEY `page_timestamp` (`rev_page`,`rev_timestamp`),
  KEY `user_timestamp` (`rev_user`,`rev_timestamp`),
  KEY `usertext_timestamp` (`rev_user_text`,`rev_timestamp`),
  KEY `page_user_timestamp` (`rev_page`,`rev_user`,`rev_timestamp`)
) ENGINE=InnoDB AUTO_INCREMENT=2 DEFAULT CHARSET=binary MAX_ROWS=10000000 AVG_ROW_LENGTH=1024;
/*!40101 SET character_set_client = @saved_cs_client */;

You can notice that running the update script has changed the schema of the brand new table, such that rev_comment now does not have a default value anymore.

And even worse, this is *not* an isolated case. P6012 shows the result of running diff on the output of mysqldump before and after running the update script. As you see there, several other tables lose their default value.

PS: The output of the update script is also stored in P6013 for future reference.
PPS: We already have this line in patch-comment-table.sql which should add the default value if one does not exist. Running it manually fixes this problem. I wonder why the update script skips that line?

Reedy subscribed.Sep 17 2017, 10:45 PM
Modifying rc_comment field of table recentchanges ...done.

That's the offending updater chnge

It's being run... on every run. So they're gonna fight, and break the world

https://github.com/wikimedia/mediawiki/blob/master/maintenance/archives/patch-editsummary-length.sql


ALTER TABLE /*_*/revision MODIFY rev_comment varbinary(767) NOT NULL;
ALTER TABLE /*_*/archive MODIFY ar_comment varbinary(767) NOT NULL;
ALTER TABLE /*_*/image MODIFY img_description varbinary(767) NOT NULL;
ALTER TABLE /*_*/oldimage MODIFY oi_description varbinary(767) NOT NULL;
ALTER TABLE /*_*/filearchive MODIFY fa_description varbinary(767);
ALTER TABLE /*_*/filearchive MODIFY fa_deleted_reason varbinary(767) default '';
ALTER TABLE /*_*/recentchanges MODIFY rc_comment varbinary(767) NOT NULL default '';
ALTER TABLE /*_*/logging MODIFY log_comment varbinary(767) NOT NULL default '';
ALTER TABLE /*_*/ipblocks MODIFY ipb_reason varbinary(767) NOT NULL;
ALTER TABLE /*_*/protected_titles MODIFY pt_reason varbinary(767);

We should probably fix the defaults in that at a minimum... And make it more smart about modifying the field; possibly making it into an updater function, and split the changes into a file per table, and check the length/options?

Reedy added a comment.Sep 17 2017, 10:47 PM
This comment was removed by Reedy.
gerritbot subscribed.Sep 17 2017, 10:57 PM

Change 378638 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/core@master] Don't unconditionally run patch-editsummary-length.sql

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

gerritbot added a project: Patch-For-Review.Sep 17 2017, 10:57 PM
Reedy added a comment.Sep 17 2017, 11:17 PM

Kinda hacky... But if one field has been affected on a dev wiki by this bug, all of them would be; granted, it should only be any "new" wikis created after the comment patch was actually merged

Swapped the check from recentchanges.rc_comment to revision.rev_comment as that's the field that keeps getting changed, and is one that needs changing back (because default '' wasn't in that patch for that column, as well as others)

Reedy removed a project: DBA.Sep 17 2017, 11:28 PM
Reedy lowered the priority of this task from High to Medium.Sep 17 2017, 11:46 PM

Moving back to normal, as it only affects new wikis, that hadn't run update.php before 6th June / commit 11cf01dd9a8512ad4d9bded43cf22ebd38af8818 but did after - so in the 1.30 dev cycle.

gerritbot added a comment.Sep 18 2017, 3:51 PM

Change 378638 merged by jenkins-bot:
[mediawiki/core@master] Don't unconditionally run patch-editsummary-length.sql

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

Reedy closed this task as Resolved.Sep 18 2017, 3:53 PM
Reedy claimed this task.
Reedy removed a project: Patch-For-Review.
ReleaseTaggerBot added a project: MW-1.30-release-notes (WMF-deploy-2017-09-19 (1.30.0-wmf.19)).Sep 18 2017, 4:00 PM
Aklapper removed a subscriber: Anomie.Oct 16 2020, 5:42 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits