Merge "Database updates for respecting oversight within Echo"

Echo.php

@@ -95,6 +95,13 @@ $wgAutoloadClasses['EchoArrayList'] = $dir . 'includes/ContainmentSet.php';
 $wgAutoloadClasses['EchoOnWikiList'] = $dir . 'includes/ContainmentSet.php';
 $wgAutoloadClasses['EchoCachedList'] = $dir . 'includes/ContainmentSet.php';
 
+// Maintenance testing
+$wgAutoloadClasses['EchoBatchRowUpdate'] = $dir . 'includes/BatchRowUpdate.php';
+$wgAutoloadClasses['EchoBatchRowWriter'] = $dir . 'includes/BatchRowUpdate.php';
+$wgAutoloadClasses['EchoBatchRowIterator'] = $dir . 'includes/BatchRowUpdate.php';
+$wgAutoloadClasses['EchoRowUpdateGenerator'] = $dir . 'includes/BatchRowUpdate.php';
+$wgAutoloadClasses['EchoSuppressionRowUpdateGenerator'] = $dir . 'includes/schemaUpdate.php';
+
 // Housekeeping hooks
 $wgHooks['LoadExtensionSchemaUpdates'][] = 'EchoHooks::getSchemaUpdates';
 $wgHooks['GetPreferences'][] = 'EchoHooks::getPreferences';

Hooks.php

@@ -71,6 +71,7 @@ class EchoHooks {
 		$updater->dropExtensionField( 'echo_event', 'event_timestamp', "$dir/db_patches/patch-drop-echo_event-event_timestamp.sql" );
 		$updater->addExtensionField( 'echo_email_batch', 'eeb_event_hash',
 			"$dir/db_patches/patch-email_batch-new-field.sql" );
+		$updater->addExtensionField( 'echo_event', 'event_page_id', "$dir/db_patches/patch-add-echo_event-event_page_id.sql" );
 		return true;
 	}
 
@@ -572,8 +573,7 @@ class EchoHooks {
 					'title' => $title,
 					'agent' => $wgUser,
 					'extra' => array(
-						'link-from-namespace' => $linksUpdate->mTitle->getNamespace(),
-						'link-from-title' => $linksUpdate->mTitle->getDBkey(),
+						'link-from-page-id' => $linksUpdate->mTitle->getArticleId(),
 					)
 				) );
 				$max--;

db_patches/patch-add-echo_event-event_page_id.sql

@@ -0,0 +1 @@
+ALTER TABLE /*_*/echo_event ADD event_page_id int unsigned;

echo.sql

@@ -8,7 +8,8 @@ CREATE TABLE /*_*/echo_event (
 	event_agent_ip varchar(39) binary null, -- IP address who triggered it, if any
 	event_page_namespace int unsigned null,
 	event_page_title varchar(255) binary null,
-	event_extra BLOB NULL
+	event_extra BLOB NULL,
+	event_page_id int unsigned null
 ) /*$wgDBTableOptions*/;
 
 CREATE INDEX /*i*/event_type ON /*_*/echo_event (event_type);

formatters/PageLinkFormatter.php

@@ -5,6 +5,32 @@
  */
 class EchoPageLinkFormatter extends EchoBasicFormatter {
 
+	/**
+	 * This is a workaround for backwards compatibility.
+	 * In https://gerrit.wikimedia.org/r/#/c/63076 we changed
+	 * the schema to save link-from-page-id instead of
+	 * link-from-namespace & link-from-title
+	 */
+	protected function extractExtra( $extra ) {
+		if ( isset( $extra['link-from-namespace'], $extra['link-from-title'] )
+			&& !isset( $extra['link-from-page-id'] )
+		) {
+			$title = Title::makeTitleSafe(
+				$extra['link-from-namespace'],
+				$extra['link-from-title']
+			);
+			if ( $title ) {
+				$extra['link-from-page-id'] = $title->getArticleId();
+				unset(
+					$extra['link-from-namespace'],
+					$extra['link-from-title']
+				);
+			}
+		}
+
+		return $extra;
+	}
+
 	/**
 	 * This method overwrite parent method and construct the bundle iterator
 	 * based on link from, it will be used in a message like this: Page A was
@@ -21,15 +47,20 @@ class EchoPageLinkFormatter extends EchoBasicFormatter {
 		if ( !$data ) {
 			return;
 		}
-		$extra = $event->getExtra();
+		$extra = self::extractExtra( $event->getExtra() );
 
 		$linkFrom = array();
 
-		if ( $this->isTitleSet( $extra ) ) {
-			$linkFrom[$this->getTitleHash( $extra )] = true;
-		} else {
-			throw new MWException( "Link from title is required for bundling notification!" );
+		if ( !$this->isTitleSet( $extra ) ) {
+			// Link from title is required for bundling notification
+			return;
 		}
+		$key = $this->getTitleHash( $extra );
+		if ( !$key ) {
+			// Page no longer exists
+			return;
+		}
+		$linkFrom[$key] = true;
 
 		$count = 1;
 		foreach ( $data as $row ) {
@@ -41,7 +72,7 @@ class EchoPageLinkFormatter extends EchoBasicFormatter {
 			if ( $this->isTitleSet( $extra ) ) {
 				$key = $this->getTitleHash( $extra );
 
-				if ( !isset( $linkFrom[$key] ) ) {
+				if ( $key && !isset( $linkFrom[$key] ) ) {
 					$linkFrom[$key] = true;
 					$count++;
 				}
@@ -63,20 +94,16 @@ class EchoPageLinkFormatter extends EchoBasicFormatter {
 	 * @return bool
 	 */
 	private function isTitleSet( $extra ) {
-		if ( isset( $extra['link-from-namespace'], $extra['link-from-title'] ) ) {
-			return true;
-		} else {
-			return false;
-		}
+		return isset( $extra['link-from-page-id'] );
 	}
 
 	/**
-	 * Internal function to generate a unique md5 of namespace and title
+	 * Internal function to return a unique identifier representing the page.
 	 * @param $extra array
-	 * @return string
+	 * @return integer Unique identifier for the linked page
 	 */
 	private function getTitleHash( $extra ) {
-		return md5( $extra['link-from-namespace'] . '-' .  $extra['link-from-title'] );
+		return $extra['link-from-page-id'];
 	}
 
 	/**
@@ -86,20 +113,21 @@ class EchoPageLinkFormatter extends EchoBasicFormatter {
 	 * @param $user User
 	 */
 	protected function processParam( $event, $param, $message, $user ) {
-		$extra = $event->getExtra();
+		$extra = self::extractExtra( $event->getExtra() );
 		switch ( $param ) {
 			// 'A' part in this message: link from page A and X others
 			case 'link-from-page':
+				$content = null;
 				if ( $this->isTitleSet( $extra ) ) {
-					$message->params(
-						Title::makeTitle(
-							$extra['link-from-namespace'],
-							$extra['link-from-title']
-						)
-					);
-				} else {
-					$message->params( '' );
+					$title = Title::newFromId( $extra['link-from-page-id'] );
+					if ( $title !== null ) {
+						$content = $this->formatTitle( $title );
 					}
+				}
+				if ( $content === null ) {
+					$content = wfMessage( 'echo-no-title' );
+				}
+				$message->params( $content );
 				break;
 
 			// example: {7} other page, {99+} other pages

includes/BatchRowUpdate.php

@@ -0,0 +1,425 @@
+<?php
+/**
+ * Provides components to update a tables rows via a batching process
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+ * http://www.gnu.org/copyleft/gpl.html
+ *
+ * @file
+ * @ingroup Maintenance
+ */
+
+/**
+ * Ties together the batch update components to provide a composable method
+ * of batch updating rows in a database. To use create a class implementing
+ * the EchoRowUpdateGenerator interface and configure the EchoBatchRowIterator and
+ * EchoBatchRowWriter for access to the correct table. The components will
+ * handle reading, writing, and waiting for slaves while the generator implementation
+ * handles generating update arrays for singular rows.
+ *
+ * Instantiate:
+ *   $updater = new EchoBatchRowUpdate(
+ *       new EchoBatchRowIterator( $dbr, 'some_table', 'primary_key_column', 500 ),
+ *       new EchoBatchRowWriter( $dbw, 'some_table', 'clusterName' ),
+ *       new MyImplementationOfEchoRowUpdateGenerator
+ *   );
+ *
+ * Run:
+ *   $updater->execute();
+ *
+ * An example maintenance script utilizing the EchoBatchRowUpdate can be located in the Echo
+ * extension file maintenance/updateSchema.php
+ *
+ * @ingroup Maintenance
+ */
+class EchoBatchRowUpdate {
+	/**
+	 * @var EchoBatchRowIterator $reader Iterator that returns an array of database rows
+	 */
+	protected $reader;
+
+	/**
+	 * @var EchoBatchRowWriter $writer Writer capable of pushing row updates to the database
+	 */
+	protected $writer;
+
+	/**
+	 * @var EchoRowUpdateGenerator $generator Generates single row updates based on the rows content
+	 */
+	protected $generator;
+
+	/**
+	 * @var callable $output Output callback
+	 */
+	protected $output;
+
+	/**
+	 * @param EchoBatchRowIterator   $reader    Iterator that returns an array of database rows
+	 * @param EchoBatchRowWriter     $writer    Writer capable of pushing row updates to the database
+	 * @param EchoRowUpdateGenerator $generator Generates single row updates based on the rows content
+	 */
+	public function __construct( EchoBatchRowIterator $reader, EchoBatchRowWriter $writer, EchoRowUpdateGenerator $generator ) {
+		$this->reader = $reader;
+		$this->writer = $writer;
+		$this->generator = $generator;
+		$this->output = function() {
+		}; // nop
+	}
+
+	/**
+	 * Runs the batch update process
+	 */
+	public function execute() {
+		foreach ( $this->reader as $rows ) {
+			$updates = array();
+			foreach ( $rows as $row ) {
+				$update = $this->generator->update( $row );
+				if ( $update ) {
+					$updates[] = array(
+						'primaryKey' => $this->reader->extractPrimaryKeys( $row ),
+						'changes' => $update,
+					);
+				}
+			}
+
+			if ( $updates ) {
+				$this->output( "Processing " . count( $updates ) . " rows\n" );
+				$this->writer->write( $updates );
+			}
+		}
+
+		$this->output( "Completed\n" );
+	}
+
+	/**
+	 * Accepts a callable which will receive a single parameter containing
+	 * string status updates
+	 *
+	 * @param callable $output A callback taking a single string parameter to output
+	 */
+	public function setOutput( $output ) {
+		if ( !is_callable( $output ) ) {
+			throw new MWException( 'Provided $output param is required to be callable.' );
+		}
+		$this->output = $output;
+	}
+
+	/**
+	 * Write out a status update
+	 *
+	 * @param string $text The value to print
+	 */
+	protected function output( $text ) {
+		call_user_func( $this->output, $text );
+	}
+}
+
+/**
+ * Interface for generating updates to single rows in the database.
+ *
+ * @ingroup Maintenance
+ */
+interface EchoRowUpdateGenerator {
+
+	/**
+	 * Given a database row, generates an array mapping column names to updated value within the database row
+	 *
+	 * Sample Response:
+	 *   return array(
+	 *       'some_col' => 'new value',
+	 *       'other_col' => 99,
+	 *   );
+	 *
+	 * @param stdClass $row A row from the database
+	 * @return array Map of column names to updated value within the database row. When no update is required
+	 *   returns an empty array.
+	 */
+	public function update( $row );
+}
+
+/**
+ * Updates database rows by primary key in batches. There are two options for writing to tables
+ * with a composite primary key.
+ *
+ * @ingroup Maintenance
+ */
+class EchoBatchRowWriter {
+	/**
+	 * @var DatabaseBase $db The database to write to
+	 */
+	protected $db;
+
+	/**
+	 * @var string $table The name of the table to update
+	 */
+	protected $table;
+
+	/**
+	 * @var string $clusterName A cluster name valid for use with LBFactory
+	 */
+	protected $clusterName;
+
+	/**
+	 * @param DatabaseBase $db          The database to write to
+	 * @param string       $table       The name of the table to update
+	 * @param string       $clusterName A cluster name valid for use with LBFactory
+	 */
+	public function __construct( DatabaseBase $db, $table, $clusterName = false ) {
+		$this->db = $db;
+		$this->table = $table;
+		$this->clusterName = $clusterName;
+	}
+
+	/**
+	 * @param array $updates Array of arrays each containing two keys, 'primaryKey' and 'changes'.
+	 *   primaryKey must contain a map of column names to values sufficient to uniquely identify the row
+	 *   changes must contain a map of column names to update values to apply to the row
+	 */
+	public function write( array $updates ) {
+		$this->db->begin();
+
+		foreach ( $updates as $id => $update ) {
+			//echo "Updating: ";var_dump( $update['primaryKey'] );
+			//echo "With values: ";var_dump( $update['changes'] );
+			$this->db->update(
+				$this->table,
+				$update['changes'],
+				$update['primaryKey'],
+				__METHOD__
+			);
+		}
+
+		$this->db->commit();
+		wfWaitForSlaves( false, false, $this->clusterName );
+	}
+}
+
+/**
+ * Fetches rows batched into groups from the database in ascending order of the primary key(s).
+ *
+ * @ingroup Maintenance
+ */
+class EchoBatchRowIterator implements Iterator {
+
+	/**
+	 * @var DatabaseBase $db The database to read from
+	 */
+	protected $db;
+
+	/**
+	 * @var string $table The name of the table to read from
+	 */
+	protected $table;
+
+	/**
+	 * @var array $primaryKey The name of the primary key(s)
+	 */
+	protected $primaryKey;
+
+	/**
+	 * @var integer $batchSize The number of rows to fetch per iteration
+	 */
+	protected $batchSize;
+
+	/**
+	 * @var array $conditions Array of strings containing SQL conditions to add to the query
+	 */
+	protected $conditions = array();
+
+	/**
+	 * @var array $fetchColumns List of column names to select from the table suitable for use with DatabaseBase::select()
+	 */
+	protected $fetchColumns = array( '*' );
+
+	/**
+	 * @var string $orderBy SQL Order by condition generated from $this->primaryKey
+	 */
+	protected $orderBy;
+
+	/**
+	 * @var array $current The current iterator value
+	 */
+	private $current = array();
+
+	/**
+	 * @var integer key 0-indexed number of pages fetched since self::reset()
+	 */
+	private $key;
+
+	/**
+	 * @param DatabaseBase $db         The database to read from
+	 * @param string       $table      The name of the table to read from
+	 * @param string|array $primaryKey The name or names of the primary key columns
+	 * @param integer      $batchSize  The number of rows to fetch per iteration
+	 */
+	public function __construct( DatabaseBase $db, $table, $primaryKey, $batchSize ) {
+		if ( $batchSize < 1 ) {
+			throw new MWException( 'Batch size must be at least 1 row.' );
+		}
+		$this->db = $db;
+		$this->table = $table;
+		$this->primaryKey = (array) $primaryKey;
+		$this->orderBy = implode( ' ASC,', $this->primaryKey ) . ' ASC';
+		$this->batchSize = $batchSize;
+	}
+
+	/**
+	 * @param string $condition Query conditions suitable for use with DatabaseBase::select
+	 */
+	public function addConditions( array $conditions ) {
+		$this->conditions = array_merge( $this->conditions, $conditions );
+	}
+
+	/**
+	 * @param array $columns List of column names to select from the table suitable for use with DatabaseBase::select()
+	 */
+	public function setFetchColumns( array $columns ) {
+		// If it's not the all column selector merge in the primary keys we need
+		if ( count( $columns ) === 1 && reset( $columns ) === '*' ) {
+			$this->fetchColumns = $columns;
+		} else {
+			$this->fetchColumns = array_unique( array_merge( $this->primaryKey, $columns ) );
+		}
+	}
+
+	/**
+	 * Extracts the primary key(s) from a database row.
+	 *
+	 * @param stdClass $row An individual database row from this iterator
+	 * @return array Map of primary key column to value within the row
+	 */
+	public function extractPrimaryKeys( $row ) {
+		$pk = array();
+		foreach ( $this->primaryKey as $column ) {
+			$pk[$column] = $row->$column;
+		}
+		return $pk;
+	}
+
+	/**
+	 * @return array The most recently fetched set of rows from the database
+	 */
+	public function current() {
+		return $this->current;
+	}
+
+	/**
+	 * @return integer 0-indexed count of the page number fetched
+	 */
+	public function key() {
+		return $this->key;
+	}
+
+	/**
+	 * Reset the iterator to the begining of the table.
+	 */
+	public function rewind() {
+		$this->key = -1; // self::next() will turn this into 0
+		$this->current = array();
+		$this->next();
+	}
+
+	/**
+	 * @return boolean True when the iterator is in a valid state
+	 */
+	public function valid() {
+		return (bool) $this->current;
+	}
+
+	/**
+	 * Fetch the next set of rows from the database.
+	 */
+	public function next() {
+		$res = $this->db->select(
+			$this->table,
+			$this->fetchColumns,
+			$this->buildConditions(),
+			__METHOD__,
+			array(
+				'LIMIT' => $this->batchSize,
+				'ORDER BY' => $this->orderBy,
+			)
+		);
+
+		// The iterator is converted to an array because in addition to returning it
+		// in self::current() we need to use the end value in self::buildConditions()
+		$this->current = iterator_to_array( $res );
+		$this->key++;
+	}
+
+	/**
+	 * Uses the primary key list and the maximal result row from the previous iteration to build
+	 * an SQL condition sufficient for selecting the next page of results.  All except the final
+	 * key use `=` conditions while the final key uses a `>` condition
+	 *
+	 * Example output:
+	 * 	  array( '( foo = 42 AND bar > 7 ) OR ( foo > 42 )' )
+	 *
+	 * @return array The SQL conditions necessary to select the next set of rows in the batched query
+	 */
+	protected function buildConditions() {
+		if ( !$this->current ) {
+			return $this->conditions;
+		}
+
+		$maxRow = end( $this->current );
+		$maximumValues = array();
+		foreach ( $this->primaryKey as $column ) {
+			$maximumValues[$column] = $this->db->addQuotes( $maxRow->$column );
+		}
+
+		$pkConditions = array();
+		// For example: If we have 3 primary keys
+		// first run through will generate
+		//   col1 = 4 AND col2 = 7 AND col3 > 1
+		// second run through will generate
+		//   col1 = 4 AND col2 > 7
+		// and the final run through will generate
+		//   col1 > 4
+		while ( $maximumValues ) {
+			$pkConditions[] = $this->buildGreaterThanCondition( $maximumValues );
+			array_pop( $maximumValues );
+		}
+
+		$conditions = $this->conditions;
+		$conditions[] = sprintf( '( %s )', implode( ' ) OR ( ', $pkConditions ) );
+
+		return $conditions;
+	}
+
+	/**
+	 * Given an array of column names and their maximum value  generate an SQL
+	 * condition where all keys except the last match $quotedMaximumValues
+	 * exactly and the last column is greater than the matching value in $quotedMaximumValues
+	 *
+	 * @param array $quotedMaximumValues The maximum values quoted with $this->db->addQuotes()
+	 * @return string An SQL condition that will select rows where all columns match the
+	 *   maximum value exactly except the last column which must be greater than the provided
+	 *   maximum value
+	 */
+	protected function buildGreaterThanCondition( array $quotedMaximumValues ) {
+		$keys = array_keys( $quotedMaximumValues );
+		$lastColumn = end( $keys );
+		$lastValue = array_pop( $quotedMaximumValues );
+		$conditions = array();
+		foreach ( $quotedMaximumValues as $column => $value ) {
+			$conditions[] = "$column = $value";
+		}
+		$conditions[] = "$lastColumn > $lastValue";
+
+		return implode( ' AND ', $conditions );
+	}
+}
+

includes/DbEchoBackend.php

@@ -107,8 +107,7 @@ class MWDbEchoBackend extends MWEchoBackend {
 					'event_agent_id',
 					'event_agent_ip',
 					'event_extra',
-					'event_page_namespace',
-					'event_page_title'
+					'event_page_id'
 				),
 				array(
 					'notification_event=event_id',
@@ -127,8 +126,7 @@ class MWDbEchoBackend extends MWEchoBackend {
 					'event_agent_id',
 					'event_agent_ip',
 					'event_extra',
-					'event_page_namespace',
-					'event_page_title'
+					'event_page_id'
 				),
 				array(
 					'eeb_event_id=event_id',

includes/schemaUpdate.php

@@ -0,0 +1,120 @@
+<?php
+
+/**
+ * Performs updates required for respecting suppression within echo:
+ *   Updates event_page_id based on event_page_title and event_page_namespace
+ *   Updates extra data for page-linked events to contain page id's
+ */
+class EchoSuppressionRowUpdateGenerator implements EchoRowUpdateGenerator
+{
+	/**
+	 * @param callable Hack to allow replacing Title::newFromText in tests
+	 */
+	protected $newTitleFromText = array( 'Title', 'newFromText' );
+
+	/**
+	 * {@inheritDoc}
+	 */
+	public function update( $row ) {
+		$update = $this->updatePageIdFromTitle( $row );
+		if ( $row->event_extra !== null && $row->event_type === 'page-linked' ) {
+			$update = $this->updatePageLinkedExtraData( $row, $update );
+		}
+
+		return $update;
+	}
+
+	/**
+	 * Hackish method of mocking Title::newFromText for tests
+	 *
+	 * @param $callable callable
+	 */
+	public function setNewTitleFromText( $callable ) {
+		$this->newTitleFromText = $callable;
+	}
+
+	/**
+	 * Hackish method of mocking Title::newFromText for tests
+	 *
+	 * @param $text string The page name to look up
+	 * @param $defaultNamespace integer The default namespace of the page to look up
+	 * @return Title|null The title located for the text + namespace, or null if invalid
+	 */
+	protected function newTitleFromText( $text, $defaultNamespace = NS_MAIN ) {
+		return call_user_func( $this->newTitleFromText, $text, $defaultNamespace );
+	}
+
+	/**
+	 * Migrates all echo events from having page title and namespace as rows in the table
+	 * to having only a page id in the table.  Any event from a page that doesn't have an
+	 * article id gets the title+namespace moved to the event extra data
+	 *
+	 * @param $row stdClass A row from the database
+	 * @return array All updates required for this row
+	 */
+	protected function updatePageIdFromTitle( $row ) {
+		$update = array();
+		$title = $this->newTitleFromText( $row->event_page_title, $row->event_page_namespace );
+		if ( $title !== null ) {
+			$pageId = $title->getArticleId();
+			if ( $pageId ) {
+				// If the title has a proper id from the database, store it
+				$update['event_page_id'] = $pageId;
+			} else {
+				// For titles that do not refer to a WikiPage stored in the database
+				// move the title/namespace into event_extra
+				$extra = $this->extra( $row );
+				$extra['page_title'] = $row->event_page_title;
+				$extra['page_namespace'] = $row->event_page_namespace;
+
+				$update['event_extra'] = serialize( $extra );
+			}
+		}
+
+		return $update;
+	}
+
+	/**
+	 * Updates the extra data for page-linked events to point to the id of the article
+	 * rather than the namespace+title combo.
+	 *
+	 * @param $row stdClass A row from the database
+	 * @return array All updates required for this row
+	 */
+	protected function updatePageLinkedExtraData( $row, array $update ) {
+		$extra = $this->extra( $row, $update );
+
+		if ( isset( $extra['link-from-title'], $extra['link-from-namespace'] ) ) {
+			$title = $this->newTitleFromText( $extra['link-from-title'], $extra['link-from-namespace'] );
+			unset( $extra['link-from-title'], $extra['link-from-namespace'] );
+			// Link from page is always from a content page, if null or no article id it was
+			// somehow invalid
+			if ( $title !== null && $title->getArticleId() ) {
+				$extra['link-from-page-id'] = $title->getArticleId();
+			}
+
+			$update['event_extra'] = serialize( $extra );
+		}
+
+		return $update;
+	}
+
+	/**
+	 * Return the extra data for a row, if an update wants to change the
+	 * extra data returns that updated data rather than the origional. If
+	 * no extra data exists returns array()
+	 *
+	 * @param $row stdClass The database row being updated
+	 * @param $update array Updates that need to be applied to the database row
+	 * @return array The event extra data
+	 */
+	protected function extra( $row, array $update = array() ) {
+		if ( isset( $update['event_extra'] ) ) {
+			return unserialize( $update['event_extra'] );
+		} elseif ( $row->event_extra ) {
+			return unserialize( $row->event_extra );
+		}
+		return array();
+	}
+
+}

maintenance/updateEchoSchemaForSuppression.php

@@ -0,0 +1,62 @@
+<?php
+/**
+ * Update event_page_id in echo_event based on event_page_title and
+ * event_page_namespace
+ *
+ * @ingroup Maintenance
+ */
+require_once ( getenv( 'MW_INSTALL_PATH' ) !== false
+	? getenv( 'MW_INSTALL_PATH' ) . '/maintenance/Maintenance.php'
+	: __DIR__ . '/../../../maintenance/Maintenance.php' );
+
+/**
+ * Maintenance script that populates the event_page_id column of echo_event
+ *
+ * @ingroup Maintenance
+ */
+class UpdateEchoSchemaForSuppression extends Maintenance {
+
+	/**
+	 * @var $table string The table to update
+	 */
+	protected $table = 'echo_event';
+
+	/**
+	 * @var $idField string The primary key column of the table to update
+	 */
+	protected $idField = 'event_id';
+
+	public function __construct() {
+		parent::__construct();
+		$this->setBatchSize( 500 );
+	}
+
+	public function execute() {
+		global $wgEchoCluster;
+
+		$reader = new EchoBatchRowIterator( MWEchoDbFactory::getDB( DB_SLAVE ), $this->table, $this->idField, $this->mBatchSize );
+		$reader->addConditions( array(
+			"event_page_title IS NOT NULL",
+			"event_page_id" => null,
+		) );
+
+		$updater = new EchoBatchRowUpdate(
+			$reader,
+			new EchoBatchRowWriter( MWEchoDbFactory::getDB( DB_MASTER ), $this->table, $wgEchoCluster ),
+			new EchoSuppressionRowUpdateGenerator
+		);
+		$updater->setOutput( array( $this, '__internalOutput' ) );
+		$updater->execute();
+	}
+
+	/**
+	 * Internal use only. parent::output() is a protected method, only way to access it from
+	 * a callback in php5.3 is to make a public function. In 5.4 can replace with a Closure.
+	 */
+	public function __internalOutput( $text ) {
+		$this->output( $text );
+	}
+}
+
+$maintClass = 'UpdateEchoSchemaForSuppression'; // Tells it to run the class
+require_once ( RUN_MAINTENANCE_IF_MAIN );

model/Event.php

@@ -145,8 +145,6 @@ class EchoEvent {
 			'event_variant' => $this->variant,
 		);
 
-		$row['event_extra'] = $this->serializeExtra();
-
 		if ( $this->agent ) {
 			if ( $this->agent->isAnon() ) {
 				$row['event_agent_ip'] = $this->agent->getName();
@@ -156,9 +154,19 @@ class EchoEvent {
 		}
 
 		if ( $this->title ) {
-			$row['event_page_namespace'] = $this->title->getNamespace();
-			$row['event_page_title'] = $this->title->getDBkey();
+			$pageId = $this->title->getArticleId();
+			if ( $pageId ) {
+				$row['event_page_id'] = $pageId;
+			} else {
+				if ( $this->extra === null ) {
+					$this->extra = array();
 				}
+				$this->extra['page_namespace'] = $this->title->getNamespace();
+				$this->extra['page_title'] = $this->title->getDBkey();
+			}
+		}
+
+		$row['event_extra'] = $this->serializeExtra();
 
 		$this->id = $wgEchoBackend->createEvent( $row );
 	}
@@ -190,11 +198,19 @@ class EchoEvent {
 			$this->agent = User::newFromName( $row->event_agent_ip, false );
 		}
 
-		if ( $row->event_page_title !== null ) {
+		if ( $row->event_page_id ) {
+			$this->title = Title::newFromId( $row->event_page_id );
+		} elseif ( isset( $row->event_page_title ) ) {
+			// BC compat with orig Echo deployment
 			$this->title = Title::makeTitleSafe(
 				$row->event_page_namespace,
 				$row->event_page_title
 			);
+		} elseif ( isset( $this->extra['page_title'] ) ) {
+			$this->title = Title::makeTitleSafe(
+				$this->extra['page_namespace'],
+				$this->extra['page_title']
+			);
 		}
 	}
 

tests/BatchRowUpdateTest.php

@@ -0,0 +1,238 @@
+<?php
+
+require_once __DIR__ . "/../includes/BatchRowUpdate.php";
+
+/**
+ * Tests for BatchRowUpdate and its components
+ */
+class BatchRowUpdateTest extends MediaWikiTestCase {
+
+	public function testWriterBasicFunctionality() {
+		$db = $this->mockDb();
+		$writer = new EchoBatchRowWriter( $db, 'echo_event' );
+
+		$updates = array(
+			self::mockUpdate( array( 'something' => 'changed' ) ),
+			self::mockUpdate( array( 'otherthing' => 'changed' ) ),
+			self::mockUpdate( array( 'and' => 'something', 'else' => 'changed' ) ),
+		);
+
+		$db->expects( $this->exactly( count( $updates ) ) )
+			->method( 'update' );
+
+		$writer->write( $updates );
+	}
+
+	static protected function mockUpdate( array $changes ) {
+		static $i = 0;
+		return array(
+			'primaryKey' => array( 'event_id' => $i++ ),
+			'changes' => $changes,
+		);
+	}
+
+	public function testReaderBasicIterate() {
+		$db = $this->mockDb();
+		$batchSize = 2;
+		$reader = new EchoBatchRowIterator( $db, 'some_table', 'id_field', $batchSize );
+
+		$response = $this->genSelectResult( $batchSize, /*numRows*/ 5, function() {
+			static $i = 0;
+			return array( 'id_field' => ++$i );
+		} );
+		$db->expects( $this->exactly( count( $response ) ) )
+			->method( 'select' )
+			->will( $this->consecutivelyReturnFromSelect( $response ) );
+
+		$pos = 0;
+		foreach ( $reader as $rows ) {
+			$this->assertEquals( $response[$pos], $rows, "Testing row in position $pos" );
+			$pos++;
+		}
+		// -1 is because the final array() marks the end and isnt included
+		$this->assertEquals( count( $response ) - 1, $pos );
+	}
+
+	static public function provider_readerGetPrimaryKey() {
+		$row = array(
+			'id_field' => 42,
+			'some_col' => 'dvorak',
+			'other_col' => 'samurai',
+		);
+		return array(
+
+			array(
+				'Must return single column pk when requested',
+				array( 'id_field' => 42 ),
+				$row
+			),
+
+			array(
+				'Must return multiple column pks when requested',
+				array( 'id_field' => 42, 'other_col' => 'samurai' ),
+				$row
+			),
+
+		);
+	}
+
+	/**
+	 * @dataProvider provider_readerGetPrimaryKey
+	 */
+	public function testReaderGetPrimaryKey( $message, array $expected, array $row ) {
+		$reader = new EchoBatchRowIterator( $this->mockDb(), 'some_table', array_keys( $expected ), 8675309 );
+		$this->assertEquals( $expected, $reader->extractPrimaryKeys( (object) $row ), $message );
+	}
+
+	static public function provider_readerSetFetchColumns() {
+		return array(
+
+			array(
+				'Must merge primary keys into select conditions',
+				// Expected column select
+				array( 'foo', 'bar' ),
+				// primary keys
+				array( 'foo' ),
+				// setFetchColumn
+				array( 'bar' )
+			),
+
+			array(
+				'Must not merge primary keys into the all columns selector',
+				// Expected column select
+				array( '*' ),
+				// primary keys
+				array( 'foo' ),
+				// setFetchColumn
+				array( '*' ),
+			),
+
+			array(
+				'Must not duplicate primary keys into column selector',
+				// Expected column select.
+				// TODO: figure out how to only assert the array_values portion and not the keys
+				array( 0 => 'foo', 1 => 'bar', 3 => 'baz' ),
+				// primary keys
+				array( 'foo', 'bar', ),
+				// setFetchColumn
+				array( 'bar', 'baz' ),
+			),
+		);
+	}
+
+	/**
+	 * @dataProvider provider_readerSetFetchColumns
+	 */
+	public function testReaderSetFetchColumns( $message, array $columns, array $primaryKeys, array $fetchColumns ) {
+		$db = $this->mockDb();
+		$db->expects( $this->once() )
+			->method( 'select' )
+			->with( 'some_table', $columns ) // only testing second parameter of DatabaseBase::select
+			->will( $this->returnValue( new ArrayIterator( array() ) ) );
+
+		$reader = new EchoBatchRowIterator( $db, 'some_table', $primaryKeys, 22 );
+		$reader->setFetchColumns( $fetchColumns );
+		// triggers first database select
+		$reader->rewind();
+	}
+
+	static public function provider_readerSelectConditions() {
+		return array(
+
+			array(
+				"With single primary key must generate id > 'value'",
+				// Expected second iteration
+				array( "( id_field > '3' )" ),
+				// Primary key(s)
+				'id_field',
+			),
+
+			array(
+				'With multiple primary keys the first conditions must use >= and the final condition must use >',
+				// Expected second iteration
+				array( "( id_field = '3' AND foo > '103' ) OR ( id_field > '3' )" ),
+				// Primary key(s)
+				array( 'id_field', 'foo' ),
+			),
+
+		);
+	}
+
+	/**
+	 * Slightly hackish to use reflection, but asserting different parameters
+	 * to consecutive calls of DatabaseBase::select in phpunit is error prone
+	 *
+	 * @dataProvider provider_readerSelectConditions
+	 */
+	public function testReaderSelectConditionsMultiplePrimaryKeys( $message, $expectedSecondIteration, $primaryKeys, $batchSize = 3 ) {
+		$results = $this->genSelectResult( $batchSize, $batchSize * 3, function() {
+			static $i = 0, $j = 100, $k = 1000;
+			return array( 'id_field' => ++$i, 'foo' => ++$j, 'bar' => ++$k );
+		} );
+		$db = $this->mockDbConsecutiveSelect( $results );
+
+		$conditions = array( 'bar' => 42, 'baz' => 'hai' );
+		$reader = new EchoBatchRowIterator( $db, 'some_table', $primaryKeys, $batchSize );
+		$reader->addConditions( $conditions );
+
+		$buildConditions = new ReflectionMethod( $reader, 'buildConditions' );
+		$buildConditions->setAccessible( true );
+
+		// On first iteration only the passed conditions must be used
+		$this->assertEquals( $conditions, $buildConditions->invoke( $reader ),
+			'First iteration must return only the conditions passed in addConditions' );
+		$reader->rewind();
+
+		// Second iteration must use the maximum primary key of last set
+		$this->assertEquals(
+			$conditions + $expectedSecondIteration,
+			$buildConditions->invoke( $reader ),
+			$message
+		);
+	}
+
+	protected function mockDbConsecutiveSelect( array $retvals ) {
+		$db = $this->mockDb();
+		$db->expects( $this->any() )
+			->method( 'select' )
+			->will( $this->consecutivelyReturnFromSelect( $retvals ) );
+		$db->expects( $this->any() )
+			->method( 'addQuotes' )
+			->will( $this->returnCallback( function( $value ) {
+				return "'$value'"; // not real quoting: doesn't matter in test
+			} ) );
+
+		return $db;
+	}
+
+	protected function consecutivelyReturnFromSelect( array $results ) {
+		$retvals = array();
+		foreach ( $results as $rows ) {
+			// The DatabaseBase::select method returns iterators, so we do too.
+			$retvals[] = $this->returnValue( new ArrayIterator( $rows ) );
+		}
+
+		return call_user_func_array( array( $this, 'onConsecutiveCalls' ), $retvals );
+	}
+
+
+	protected function genSelectResult( $batchSize, $numRows, $rowGenerator ) {
+		$res = array();
+		for ( $i = 0; $i < $numRows; $i += $batchSize ) {
+			$rows = array();
+			for ( $j = 0; $j < $batchSize && $i + $j < $numRows; $j++ ) {
+				$rows [] = (object) call_user_func( $rowGenerator );
+			}
+			$res[] = $rows;
+		}
+		$res[] = array(); // termination condition requires empty result for last row
+		return $res;
+	}
+
+	protected function mockDb() {
+		// Cant mock from DatabaseType or DatabaseBase, they dont
+		// have the full gamut of methods
+		return $this->getMock( 'DatabaseMysql' );
+	}
+}
+

tests/SupressionMaintenanceTest.php

@@ -0,0 +1,127 @@
+<?php
+
+class SuppressionMaintenanceTest extends MediaWikiTestCase {
+
+	public static function provider_updateRow() {
+		$input = array(
+			'event_id' => 2,
+			'event_type' => 'mention',
+			'event_variant' => null,
+			'event_agent_id' => 3,
+			'event_agent_ip' => null,
+			'event_page_title' => null,
+			'event_page_namespace' => null,
+			'event_page_extra' => null,
+			'event_extra' => null,
+			'event_page_id' => null,
+		);
+		return array(
+			array( 'Unrelated row must result in no update', array(), $input ),
+
+			array(
+				'Page title and namespace for non-existant page must move into event_extra',
+				array( // expected update
+					'event_extra' => serialize( array(
+						'page_title' => 'Yabba Dabba Do',
+						'page_namespace' => NS_MAIN
+					) ),
+				),
+				array( // input row
+					'event_page_title' => 'Yabba Dabba Do',
+					'event_page_namespace' => NS_MAIN,
+				) + $input,
+			),
+
+			array(
+				'Page title and namespace for existing page must be result in update to event_page_id',
+				array( // expected update
+					'event_page_id' => 42,
+				),
+				array( // input row
+					'event_page_title' => 'Mount Rushmore',
+					'event_page_namespace' => NS_MAIN,
+				) + $input,
+				self::attachTitleFor( 42, 'Mount Rushmore', NS_MAIN )
+			),
+
+			array(
+				'When updating non-existant page must keep old extra data',
+				array( // expected update
+					'event_extra' => serialize( array(
+						'foo' => 'bar',
+						'page_title' => 'Yabba Dabba Do',
+						'page_namespace' => NS_MAIN
+					) ),
+				),
+				array( // input row
+					'event_page_title' => 'Yabba Dabba Do',
+					'event_page_namespace' => NS_MAIN,
+					'event_extra' => serialize( array( 'foo' => 'bar' ) ),
+				) + $input,
+			),
+
+			array(
+				'Must update link-from-title/namespace to link-from-page-id for page-linked events',
+				array( // expected update
+					'event_extra' => serialize( array( 'link-from-page-id' => 99 ) ),
+				),
+				array( //input row
+					'event_type' => 'page-linked',
+					'event_extra' => serialize( array(
+						'link-from-title' => 'Horse',
+						'link-from-namespace' => NS_USER_TALK
+					) ),
+				) + $input,
+				self::attachTitleFor( 99, 'Horse', NS_USER_TALK )
+			),
+
+			array(
+				'Must perform both generic update and page-linked update at same time',
+				array( // expected update
+					'event_extra' => serialize( array( 'link-from-page-id' => 8675309 ) ),
+					'event_page_id' => 8675309,
+				),
+				array( //input row
+					'event_type' => 'page-linked',
+					'event_extra' => serialize( array(
+						'link-from-title' => 'Jenny',
+						'link-from-namespace' => NS_MAIN,
+					) ),
+					'event_page_title' => 'Jenny',
+					'event_page_namespace' => NS_MAIN,
+				) + $input,
+				self::attachTitleFor( 8675309, 'Jenny', NS_MAIN ),
+			),
+		);
+	}
+
+	protected static function attachTitleFor( $id, $providedText, $providedNamespace ) {
+		return function( $test, $gen ) use ( $id, $providedText, $providedNamespace ) {
+			$title = $test->getMock( 'Title' );
+			$title->expects( $test->any() )
+				->method( 'getArticleId' )
+				->will( $test->returnValue( $id ) );
+
+			$titles = array( $providedNamespace => array( $providedText => $title ) );
+
+			$gen->setNewTitleFromText( function( $text, $defaultNamespace ) use( $titles ) {
+				if ( isset( $titles[$defaultNamespace][$text] ) ) {
+					return $titles[$defaultNamespace][$text];
+				}
+				return Title::newFromText( $text, $defaultNamespace );
+			} );
+		};
+	}
+
+	/**
+	 * @dataProvider provider_updateRow
+	 */
+	public function testUpdateRow( $message, $expected, $input, $callable = null ) {
+		$gen = new EchoSuppressionRowUpdateGenerator;
+		if ( $callable ) {
+			call_user_func( $callable, $this, $gen );
+		}
+		$update = $gen->update( (object) $input );
+		$this->assertEquals( $expected, $update, $message );
+	}
+}