Page MenuHomePhabricator

Fatal exception when editing an abuse filter: Error: 1048 Column 'afa_parameters' cannot be null
Closed, ResolvedPublic

Description

The following error happened when I tried to save an edit to this filter on ptwiki (I only replaced article_recent_contributors by page_recent_contributors, per T173889):
https://pt.wikipedia.org/wiki/Especial:Filtro_de_abusos/75

2018-09-01 19:47:59 [W4rs7wpAAD4AAAfVIW4AAABX] mw1267 ptwiki 1.32.0-wmf.19 exception ERROR: [W4rs7wpAAD4AAAfVIW4AAABX] /wiki/Especial:Filtro_de_abusos/75   Wikimedia\Rdbms\DBQueryError from line 1458 of /srv/mediawiki/php-1.32.0-wmf.19/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 `abuse_filter_action` (afa_filter,afa_consequence,afa_parameters) VALUES ('75','throttle',NULL)
Function: AbuseFilter::doSaveFilter
Error: 1048 Column 'afa_parameters' cannot be null (10.64.0.110)
 {"exception_id":"W4rs7wpAAD4AAAfVIW4AAABX","exception_url":"/wiki/Especial:Filtro_de_abusos/75","caught_by":"mwe_handler"} 
[Exception Wikimedia\Rdbms\DBQueryError] (/srv/mediawiki/php-1.32.0-wmf.19/includes/libs/rdbms/database/Database.php:1458) A database query error has occurred. Did you forget to run your application's database schema updater after upgrading? 
Query: INSERT  INTO `abuse_filter_action` (afa_filter,afa_consequence,afa_parameters) VALUES ('75','throttle',NULL)
Function: AbuseFilter::doSaveFilter
Error: 1048 Column 'afa_parameters' cannot be null (10.64.0.110)

  #0 /srv/mediawiki/php-1.32.0-wmf.19/includes/libs/rdbms/database/Database.php(1428): Wikimedia\Rdbms\Database->makeQueryException(string, integer, string, string)
  #1 /srv/mediawiki/php-1.32.0-wmf.19/includes/libs/rdbms/database/Database.php(1198): Wikimedia\Rdbms\Database->reportQueryError(string, integer, string, string, boolean)
  #2 /srv/mediawiki/php-1.32.0-wmf.19/includes/libs/rdbms/database/Database.php(2030): Wikimedia\Rdbms\Database->query(string, string)
  #3 /srv/mediawiki/php-1.32.0-wmf.19/extensions/AbuseFilter/includes/AbuseFilter.php(2447): Wikimedia\Rdbms\Database->insert(string, array, string)
  #4 /srv/mediawiki/php-1.32.0-wmf.19/extensions/AbuseFilter/includes/AbuseFilter.php(2319): AbuseFilter::doSaveFilter(array, array, string, array, boolean, AbuseFilterViewEdit)
  #5 /srv/mediawiki/php-1.32.0-wmf.19/extensions/AbuseFilter/includes/Views/AbuseFilterViewEdit.php(65): AbuseFilter::saveFilter(AbuseFilterViewEdit, string, WebRequest, stdClass, array)
  #6 /srv/mediawiki/php-1.32.0-wmf.19/extensions/AbuseFilter/includes/special/SpecialAbuseFilter.php(122): AbuseFilterViewEdit->show()
  #7 /srv/mediawiki/php-1.32.0-wmf.19/includes/specialpage/SpecialPage.php(569): SpecialAbuseFilter->execute(string)
  #8 /srv/mediawiki/php-1.32.0-wmf.19/includes/specialpage/SpecialPageFactory.php(581): SpecialPage->run(string)
  #9 /srv/mediawiki/php-1.32.0-wmf.19/includes/MediaWiki.php(288): MediaWiki\Special\SpecialPageFactory->executePath(Title, RequestContext)
  #10 /srv/mediawiki/php-1.32.0-wmf.19/includes/MediaWiki.php(868): MediaWiki->performRequest()
  #11 /srv/mediawiki/php-1.32.0-wmf.19/includes/MediaWiki.php(525): MediaWiki->main()
  #12 /srv/mediawiki/php-1.32.0-wmf.19/index.php(42): MediaWiki->run()
  #13 /srv/mediawiki/w/index.php(3): include(string)
  #14 {main}

Related Objects

Event Timeline

He7d3r created this task.Sep 1 2018, 7:54 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 1 2018, 7:54 PM
He7d3r added a comment.Sep 1 2018, 8:05 PM

Same problem happened at
https://pt.wikipedia.org/wiki/Especial:Filtro_de_abusos/91

MediaWiki internal error.
Original exception: [W4rwzQpAAEMAAIGF9IIAAABI] 2018-09-01 20:04:29: Fatal exception of type "Wikimedia\Rdbms\DBQueryError"
Exception caught inside exception handler.
Set $wgShowExceptionDetails = true; at the bottom of LocalSettings.php to show detailed debugging information.

Aklapper renamed this task from Fatal exception of type when editing an abuse filter to Fatal exception when editing an abuse filter: Error: 1048 Column 'afa_parameters' cannot be null (10.64.0.110).Sep 1 2018, 8:54 PM
Reedy renamed this task from Fatal exception when editing an abuse filter: Error: 1048 Column 'afa_parameters' cannot be null (10.64.0.110) to Fatal exception when editing an abuse filter: Error: 1048 Column 'afa_parameters' cannot be null.Sep 1 2018, 9:04 PM
Reedy updated the task description. (Show Details)
Daimona triaged this task as High priority.Sep 2 2018, 8:04 AM
Daimona added a subscriber: Daimona.

@He7d3r many thanks for reporting. At a glance, I think I know what is causing this. Recently, the AF interface has been switched to OOUI (https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/AbuseFilter/+/421487/). This change mostly influenced users from a visual POV, aside for a couple of changes. One of these is changing the "throttle" option from a plain textarea to a checkbox multiselect; also, when "throttle" is selected, at least one checkbox must be selected as well. The problem here has three concauses:

  1. The checkbox are not shown as mandatory (i.e. the field is not required), so users may leave them blank without noticing (known problem, see T196984. I'm not aware of an elegant and short solution)
  2. Such checkboxes are not validated serverside, nor throttle parameters have ever been. I'll shortly send a patch for this.
  3. The filter in question seems to have all throttle fields unselected. However, the filter's history tells a different story, so I suspect something got lost while transitioning to OOUI. I'll also investigate it now.
Daimona claimed this task.Sep 2 2018, 9:05 AM

Change 457086 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Fix throttle validation and avoid DB query error

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

The patch above will solve the problem once merged and deployed. However, there are two notes about that:

  1. After this patch, it will be possible to effectively save a filter with no real change. This only happens on the first time that a filter is edited after the OOUI switch. Specifically, throttle parameters won't be treated as equal in two cases: first, if in the old textarea they were split by commas; the edit diff will show a space added after the comma, e.g. user,page => user, page. Second, if they were specified in a different order than the checkboxes' one, parameters will be moved; for instance, user, page, site => user, site, page. I'm not sure that we want to fix it, as it would be a temporary special case for AbuseFilter::compareVersions; alternatively, we could write a maintenance script to normalise parameters directly in the DB.
  2. As specified in the patch commit message, we still need a follow-up to enforce validation on other fields, both client- and server-side. Currently, this includes:
    1. Warning (server-side and client-side): select "Other message" and leave the second field blank. This also produces a PHP notice for undefined offset 0 in ViewEdit (look for $warnMsg = $parameters[0];)
    2. Throttle (client-side): the same as we do server-side in the patch above
    3. Tag (client-side): mostly needs to clearly show that at least one tag is needed. Also, when saving a filter with tag enabled but without touching the tag input field correctly returns an error but an empty tag is added in the field topbar; this shouldn't probably happen, but it's a minor problem.

Another minor problem I just noticed: throttle count and period fields are HTML input elements of type number, generated via OOUI\TextInputWidget with type number. OOUI lets entering all digits, comma, dot, and (at least) 'e' in the field (I guess this is related to the e constant), but on-browser validation doesn't accept the e. The main problem is that if the user enters e in a throttle field, then deselects throttle and tries to save the filter, the form won't be submitted and the following error is silently logged in the console:

An invalid form control with name='wpFilterThrottleCount' is not focusable.

This happens because the browser tries to display the validation warning, but doesn't succeed since the field is hidden. I guess this should be fixed in OOUI anyway.

Another minor problem I just noticed: throttle count and period fields are HTML input elements of type number, generated via OOUI\TextInputWidget with type number. OOUI lets entering all digits, comma, dot, and (at least) 'e' in the field (I guess this is related to the e constant), but on-browser validation doesn't accept the e.

Well, actually OOUI has nothing to do with that, it's just HTML and 'e' is to be used with exponential notation. The problem still remains, though, and should probably be addressed together with T196984. I'm also filing a new task for the two points above.

As for point 1, I think a maintenance script is the best thing. I'm already adding one to the above patch, which should be run shortly for WMF wikis.

The maintenance script is ready and, as well as the whole patch, needs to be reviewed and merged. Probably, given the long time before this error has been noticed, we won't need a backport. However, the script should be executed as soon as possible after the train. Another follow-up is T203359.

Daimona added a comment.EditedSep 7 2018, 4:15 PM

While the maintenance script included in the patch above needs careful review, I'd like to discuss about the way it edits the DB. (Copying from gerrit) As I wrote it, the changes from the script will be shown in AF history as performed by the last editor of each filter. If we decide that this is not correct, we need to change the afh_user(text) to be a system user, thus adding a new history entry.

Also, I added an option to perform a dry run, which should be executed before update.php.

EDIT: We also need to decide whether we want to handle empty groups inside this script before going on. See T203584#4567290.

Daimona added a comment.EditedSep 8 2018, 9:14 AM

I changed my mind again. First, an overview. I envisioned three different ways to edit history rows:

  1. Only change the last version for each filter (current approach). Pros: less rows to change, avoids inserting new rows. Cons: it'll be shown as if the last filter editor made the changes from the script as well; if the last edit was to only reorder throttle groups, or to use commas instead of linebreaks (i.e. change the parameters to be in an undesired form), the diff will show no differences between the revisions.
  2. Change every row in abuse_filter_history to reflect the correct order and format. Pros: avoids inserting new rows; it'll be like if the throttle parameters have always had the same format. Cons: could change lots of rows; there may be the same issue as above which could generate empty diffs.
  3. Insert a new row, performed by a system user (approach that I'll like to use). Pros: will only change filters with wrong parameters; the edit will be clearly attributed to a system user; no risk to generate empty diffs; same amount of changed rows as in 1. Cons: being new rows, they could clog filter history.

The reason I'd like to go with 3 is that the script has been expanded to perform several tasks, which I'd rather keep well visible in a separate edit. Moreover, some changes could be partly controversial (e.g. the one for empty groups), and we'd better not make such changes under the name of another user.
With this new method in mind, I'm also implementing a change for empty throttle groups. If groups are empty, we get to this line with an empty array, which causes the following foreach to be skipped. This means that "throttle" is completely ignored, just as if it was disabled, and therefore I'd just go ahead and remove "throttle" for the filter. Just a note: I'd rather write a couple of lines in Tech/News to warn users about the changes, asking them to check if everything is fine.

Daimona added a comment.EditedSep 8 2018, 4:38 PM

The patch is ready. As I wrote in the commit message, review by two people for the maintenance script would be great. Also, the script should be executed in dry-run before running update.php, to see if any problem arise. I have to confess that I don't really know how the process work in this cases, just as I don't know how long should we keep the exceptions asking to run update.php after this patch will be merged.

Note: the script will be added to update.php in a subsequent patch, so that we'll have plenty of time to execute a dry run.

Change 459245 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Add normalizeThrottleParameters script to update.php

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

Paladox added a subscriber: Paladox.Sep 8 2018, 5:42 PM

Change 459368 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Change throttle selector to restore lost functionality

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

Change 457086 abandoned by Daimona Eaytoy:
Fix throttle validation to avoid DB query error

Reason:
Superseded by I7831dbb0bab55807392ac1f7915d6cb0cb713593. The few things of the maintenance script that we still need will be included inside the one in Iee56f4726337a42a04e7912e1ee67738fac0b488

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

The patch is ready. As I wrote in the commit message, review by two people for the maintenance script would be great. Also, the script should be executed in dry-run before running update.php, to see if any problem arise. I have to confess that I don't really know how the process work in this cases, just as I don't know how long should we keep the exceptions asking to run update.php after this patch will be merged.
Note: the script will be added to update.php in a subsequent patch, so that we'll have plenty of time to execute a dry run.

I submitted a brand new patch, since that one was wrong (T203587#4569698). However, my last comment is still valid: two reviewers would be nice, and we still need a dry-run before adding the script to update.php.

Daimona raised the priority of this task from High to Unbreak Now!.Oct 17 2018, 1:47 PM

Since this isn't still fixed in REL1_32, people updating their wikis to 1.32 will possibly break their filters as well. This really cannot wait.

Restricted Application added subscribers: Liuxinyu970226, TerraCodes. · View Herald TranscriptOct 17 2018, 1:47 PM

Change 468007 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@REL1_32] Change throttle selector to restore old functionality, overall improvement

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

Change 459368 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Change throttle selector to restore old functionality, overall improvement

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

Change 468007 merged by Daimona Eaytoy:
[mediawiki/extensions/AbuseFilter@REL1_32] Change throttle selector to restore old functionality, overall improvement

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

greg added a subscriber: greg.Nov 27 2018, 6:37 PM

Status/next steps for this UBN! with no updates for almost a week?

Change 476244 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] [WIP] Fix big problems with normalizeThrottleParameters

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

Change 476251 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@REL1_32] Fix big problems with normalizeThrottleParameters

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

Change 476244 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Fix big problems with normalizeThrottleParameters

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

Change 479998 had a related patch set uploaded (by Tim Starling; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@wmf/1.33.0-wmf.8] [1.33.0-wmf.8] Fix big problems with normalizeThrottleParameters

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

Change 479998 merged by Tim Starling:
[mediawiki/extensions/AbuseFilter@wmf/1.33.0-wmf.8] [1.33.0-wmf.8] Fix big problems with normalizeThrottleParameters

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

Change 476251 merged by Daimona Eaytoy:
[mediawiki/extensions/AbuseFilter@REL1_32] Fix big problems with normalizeThrottleParameters

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

(Is this done now?)

Daimona closed this task as Resolved.Dec 19 2018, 6:59 PM
Daimona removed a project: Patch-For-Review.

@matmarex Yes. The exception isn't thrown anymore thanks to the first merged patch. There are still other problems, but they're tracked in other tasks, specifically, T209565. We'll discuss there what to do to improve the script, perform other dry runs if needed, and finally add it to update.php once ready (and once faulty filters on WMF which need a manual fix will have been fixed).

Change 459245 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Add normalizeThrottleParameters script to update.php

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

Change 459245 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Add normalizeThrottleParameters script to update.php

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