Beautify old, broken abuse_filter_history rows

And right when the throttle script seemed complete... Here is another
function! So, this change splits the logic in new functions called
sequentially, and the only actual change is adding the beautifyHistory
function. Its purpose is to search ANY row in abuse_filter_history with
empty/missing parameters and normalize it. More specifically, missing
period and count are inserted as 0, and for missing groups we add
"none", used by a newly introduced message. This way, messages shown on
Special:AbuseFilter/history will be clearer and won't have gaps.

Bug:T209565
Bug:T215787
Change-Id: I38395f4df9d83badfd26cdf584ffba743b6417a9

i18n/en.json

@@ -209,6 +209,7 @@
 	"abusefilter-throttle-editcount": "edit count",
 	"abusefilter-throttle-site": "whole site",
 	"abusefilter-throttle-page": "page",
+	"abusefilter-throttle-none": "(none)",
 	"abusefilter-throttle-details": "Allow $1 {{PLURAL:$1|action|actions}} every $2 {{PLURAL:$2|second|seconds}}, group throttle by: $3",
 	"abusefilter-edit-warn-message": "System message to use for warning:",
 	"abusefilter-edit-warn-other": "Other message",

i18n/qqq.json

@@ -243,6 +243,7 @@
 	"abusefilter-throttle-editcount": "Throttle option.",
 	"abusefilter-throttle-site": "Throttle option.",
 	"abusefilter-throttle-page": "Throttle option.",
+	"abusefilter-throttle-none": "Bogus throttle option, means that no options are enabled.",
 	"abusefilter-throttle-details": "Description for Special:AbuseFilter/history with a detailed description for throttle action. Parameters:\n* $1 is the number of allowed actions, $2 is the time in seconds to use before resetting the action count, $3 is a list of throttled groups",
 	"abusefilter-edit-warn-message": "Field label for dropdown list with system messages.",
 	"abusefilter-edit-warn-other": "Option in dropdown menu to specify no item from the list should be used.\n\nSee also:\n* {{msg-mw|abusefilter-edit-disallow-other}}",

includes/AbuseFilter.php

@@ -3143,37 +3143,29 @@ class AbuseFilter {
 				array_shift( $parameters );
 				list( $actions, $time ) = explode( ',', array_shift( $parameters ) );
 
-				if ( $parameters === [ '' ] ) {
-					// Having empty groups won't happen for new filters due to validation upon saving,
-					// but old entries may have it. We'd better not show a broken message. Also,
-					// the array has an empty string inside because we haven't been passing an empty array
-					// as the default when retrieving wpFilterThrottleGroups with getArray (when it was
-					// a CheckboxMultiselect).
-					$groups = '';
-				} else {
-					// Join comma-separated groups in a commaList with a final "and", and convert to messages.
-					// Messages used here: abusefilter-throttle-ip, abusefilter-throttle-user,
-					// abusefilter-throttle-site, abusefilter-throttle-creationdate, abusefilter-throttle-editcount
-					// abusefilter-throttle-range, abusefilter-throttle-page
-					foreach ( $parameters as &$val ) {
-						if ( strpos( $val, ',' ) !== false ) {
-							$subGroups = explode( ',', $val );
-							foreach ( $subGroups as &$group ) {
-								$msg = wfMessage( "abusefilter-throttle-$group" );
-								// We previously accepted literally everything in this field, so old entries
-								// may have weird stuff.
-								$group = $msg->exists() ? $msg->text() : $group;
-							}
-							unset( $group );
-							$val = $lang->listToText( $subGroups );
-						} else {
-							$msg = wfMessage( "abusefilter-throttle-$val" );
-							$val = $msg->exists() ? $msg->text() : $val;
+				// Join comma-separated groups in a commaList with a final "and", and convert to messages.
+				// Messages used here: abusefilter-throttle-ip, abusefilter-throttle-user,
+				// abusefilter-throttle-site, abusefilter-throttle-creationdate, abusefilter-throttle-editcount
+				// abusefilter-throttle-range, abusefilter-throttle-page, abusefilter-throttle-none
+				foreach ( $parameters as &$val ) {
+					if ( strpos( $val, ',' ) !== false ) {
+						$subGroups = explode( ',', $val );
+						foreach ( $subGroups as &$group ) {
+							$msg = wfMessage( "abusefilter-throttle-$group" );
+							// We previously accepted literally everything in this field, so old entries
+							// may have weird stuff.
+							$group = $msg->exists() ? $msg->text() : $group;
 						}
+						unset( $group );
+						$val = $lang->listToText( $subGroups );
+					} else {
+						$msg = wfMessage( "abusefilter-throttle-$val" );
+						$val = $msg->exists() ? $msg->text() : $val;
 					}
-					unset( $val );
-					$groups = $lang->semicolonList( $parameters );
 				}
+				unset( $val );
+				$groups = $lang->semicolonList( $parameters );
+
 				$displayAction = self::getActionDisplay( $action ) .
 				wfMessage( 'colon-separator' )->escaped() .
 				wfMessage( 'abusefilter-throttle-details' )->params( $actions, $time, $groups )->escaped();

maintenance/normalizeThrottleParameters.php

@@ -11,6 +11,10 @@
  *   highly unlikely to happen anyway. (T203585)
  * - If throttle groups are empty (or only contain unknown keywords), ask users to fix every case
  *   by hand. (T203584)
+ * - Change some edge cases of throttle parameters saved in abuse_filter_history (T215787):
+ *     - parameters = null ==> parameters = [ filterID, "0,0", 'none' ]
+ *     - at least a number missing from parameters[1] ==> insert 0 in place of the missing param
+ *     - empty groups ==> 'none' (special case, uses the message abusefilter-throttle-none)
  *
  * @ingroup Maintenance
  */
@@ -41,7 +45,7 @@ class NormalizeThrottleParameters extends LoggedUpdateMaintenance {
 		return __CLASS__;
 	}
 
-	/** @var \Wikimedia\Rdbms\IDatabase $db The master database */
+	/** @var \Wikimedia\Rdbms\Database $dbw The master database */
 	private $dbw;
 
 	/**
@@ -133,12 +137,12 @@ class NormalizeThrottleParameters extends LoggedUpdateMaintenance {
 	}
 
 	/**
-	 * @see Maintenance::doDBUpdates
-	 * @return bool
+	 * Main logic of parameters normalization
+	 *
+	 * @return int Amount of normalized rows
 	 */
-	public function doDBUpdates() {
+	protected function normalizeParameters() {
 		$user = AbuseFilter::getFilterUser();
-		$this->dbw = wfGetDB( DB_MASTER );
 		$dryRun = $this->hasOption( 'dry-run' );
 
 		// IDs of filters with invalid rate (count or period)
@@ -152,8 +156,6 @@ class NormalizeThrottleParameters extends LoggedUpdateMaintenance {
 		// holds an empty string and (unserialize(afh_actions))['throttle'] is null.
 		$totallyEmpty = [];
 
-		$this->beginTransaction( $this->dbw, __METHOD__ );
-
 		// Only select throttle actions
 		$actionRows = $this->dbw->select(
 			'abuse_filter_action',
@@ -320,8 +322,7 @@ class NormalizeThrottleParameters extends LoggedUpdateMaintenance {
 		$touchedIDs = array_merge( $changeActionIDs, $deleteActionIDs );
 		if ( count( $touchedIDs ) === 0 ) {
 			$this->output( "No throttle parameters to normalize.\n" );
-			$this->commitTransaction( $this->dbw, __METHOD__ );
-			return !$dryRun;
+			return 0;
 		}
 
 		// Create new history rows for every changed filter
@@ -396,13 +397,97 @@ class NormalizeThrottleParameters extends LoggedUpdateMaintenance {
 				);
 			}
 		}
+		return $affectedActionRows + $historyCount;
+	}
+
+	/**
+	 * Beautify empty/missing/corrupted parameters in abuse_filter_history
+	 *
+	 * @return int Amount of beautified rows
+	 */
+	protected function beautifyHistory() {
+		$dryRun = $this->hasOption( 'dry-run' );
+
+		// We need any row containing throttle, but there's no
+		// need to lock as these rows aren't changed by the actual code.
+		$likeClause = $this->dbw->buildLike(
+			$this->dbw->anyString(),
+			'throttle',
+			$this->dbw->anyString()
+		);
+		$histRows = $this->dbw->select(
+			'abuse_filter_history',
+			[ 'afh_id', 'afh_actions', 'afh_filter' ],
+			[ 'afh_actions ' . $likeClause ],
+			__METHOD__
+		);
+
+		$beautyIDs = [];
+		foreach ( $histRows as $row ) {
+			$acts = unserialize( $row->afh_actions );
+			if ( !array_key_exists( 'throttle', $acts ) ) {
+				// The LIKE clause is very raw, so this could happen
+				continue;
+			}
+
+			if ( $acts['throttle'] === null ) {
+				// Corrupted row, rebuild it (T215787)
+				$acts['throttle'] = [ $row->afh_filter, '0,0', 'none' ];
+			} elseif ( $this->checkThrottleRate( $acts['throttle'][1] ) !== null ) {
+				// Missing count, make it explicitly 0
+				$acts['throttle'][1] = preg_replace( '/^,/', '0,', $acts['throttle'][1] );
+				// Missing period, make it explicitly 0
+				$acts['throttle'][1] = preg_replace( '/,$/', ',0', $acts['throttle'][1] );
+			} elseif ( count( $acts['throttle'] ) === 2 ) {
+				// Missing groups, make them explicitly "none" (special group)
+				$acts['throttle'][] = 'none';
+			} else {
+				// Everything's fine!
+				continue;
+			}
+
+			$beautyIDs[] = $row->afh_id;
+			if ( !$dryRun ) {
+				$this->dbw->update(
+					'abuse_filter_history',
+					[ 'afh_actions' => serialize( $acts ) ],
+					[ 'afh_id' => $row->afh_id ],
+					__METHOD__
+				);
+			}
+		}
+
+		$changed = count( $beautyIDs );
+		if ( $changed ) {
+			$verb = $dryRun ? 'would beautify' : 'beautified';
+			$this->output(
+				"normalizeThrottleParameter $verb $changed rows in abuse_filter_history" .
+				" for the following history IDs: " . implode( ', ', $beautyIDs ) . "\n"
+			);
+		}
+		return $changed;
+	}
+
+	/**
+	 * @inheritDoc
+	 */
+	public function doDBUpdates() {
+		$dryRun = $this->hasOption( 'dry-run' );
+		$this->dbw = wfGetDB( DB_MASTER );
+		$this->beginTransaction( $this->dbw, __METHOD__ );
+
+		$normalized = $this->normalizeParameters();
+		$beautified = $this->beautifyHistory();
 
 		$this->commitTransaction( $this->dbw, __METHOD__ );
-		$changed = $affectedActionRows + $historyCount;
+
+		$changed = $normalized + $beautified;
+
 		$resultMsg = $dryRun ?
 			"Throttle parameter normalization would change a total of $changed rows.\n" :
 			"Throttle parameters successfully normalized. Changed $changed rows.\n";
 		$this->output( $resultMsg );
+
 		return !$dryRun;
 	}
 }