Page MenuHomePhabricator

Rewrite throttle parameters
Open, Needs TriagePublic

Description

Currently, throttle parameters (found in abuse_filter_action->afa_parameters and abuse_filter_history->afh_actions) are in the form

[ filterID, "count,period", ...groups ]

This is extremely awkward, for two good reasons:

  1. It stores the filter ID, which is already stored in the row (afa_filter for abuse_filter_consequences); it'll be used when checking if a user must be throttled to build the throttle key, but it's enough to retrieve it from afa_filter at runtime.
  2. Count and period are stored together. I don't really know the reason for it, but it's a highly risky way of storing two separate NUMERIC values. What if the user adds a comma to one of the two? When we split the string we end up with completely wrong numbers. And no, there's currently no validation on the fields, so commas CAN be added. This patch will help with validation, although I guess commas will have to be treated separately (see below).

My proposal is to first merge the patch linked above, run the script and think about T203359 (i.e. deprecate old stuff). Then, what should we do with values with a comma inside? Before adding validation (or right after) we'd need to identify them and emit well visible errors, so that they can be fixed (which is something we'd better not do automatically). Then, we should merge the patch which I'll send in a minute in order to rewrite throttle parameters. The proposed form is:

[ count, period, ...groups ]

which should make much more sense. The patch is backward compatible and adds another maintenance script to rewrite DB tables. Then again, we should add deprecation for old-type parameters and eventually remove all the old code.
The whole operation must be well coordinated, and as I envisioned it, it'll take at least a major MW version or two to be fully completed.

Event Timeline

Change 458164 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Rewrite throttle parameters

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

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

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

Change 459378 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Throw exceptions if update.php hasn't been executed

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

Change 459379 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Remove legacy code for throttle parameters

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

Will file a separate task once the patch will be merged.

Change 458164 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Rewrite throttle parameters

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

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

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

Change 459378 had a related patch set uploaded (by Huji; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Throw exceptions if update.php hasn't been executed

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

Change 459378 abandoned by Huji:
Throw exceptions if update.php hasn't been executed

Reason:
Dependency was abandoned

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

Removing task assignee due to inactivity, as this open task has been assigned to the same person for more than two years (see the emails sent to the task assignee on Oct27 and Nov23). Please assign this task to yourself again if you still realistically [plan to] work on this task - it would be welcome.
(See https://www.mediawiki.org/wiki/Bug_management/Assignee_cleanup for tips how to best manage your individual work in Phabricator.)