Avoid event insertion if possible

Why:
* On wikis with lots of bot activity like Wikidata, there is a large
  volume of edits which can potentially create an article-linked
  notification. These notifications are now actually rarely sent
  because they are disabled for bots (T318523). However, the event
  record is always inserted into the database, with no reference to
  it, bloating the database.

What:
* Do not unconditionally insert an event into the database when
  Event::create is called. Pass it to downstream calls and have
  it inserted when it's clear it will actually be needed (i.e.,
  a notification is definitely going to be created).
* Pass the event's payload to the job queue instead of requiring
  its ID. Introduce Event::newFromArray, which unlike ::loadFromRow
  handles ::toDbArray values that haven't been inserted into
  the database yet.
* Introduce Event::acquireId which ensures the event has been
  inserted prior to returning its ID as well as it does not get
  re-inserted.

Bug: T221258
Change-Id: I8b9a99a197d6af2845d85d9e35c6703640f70b91

includes/Controller/NotificationController.php

@@ -150,7 +150,7 @@ class NotificationController {
 			}
 			$hookRunner->onEchoGetNotificationTypes( $user, $event, $userNotifyTypes );
 
-			// types such as web, email, etc
+			// types such as web, email, etc.
 			foreach ( $userNotifyTypes as $type ) {
 				self::doNotification( $event, $user, $type );
 			}
@@ -191,9 +191,9 @@ class NotificationController {
 		if ( !$rev ) {
 			$logger = LoggerFactory::getInstance( 'Echo' );
 			$logger->debug(
-				'Notifying for event {eventId}. Revision \'{revId}\' not found.',
+				'Notifying for event type \'{eventType}\'. Revision \'{revId}\' not found.',
 				[
-					'eventId' => $event->getId(),
+					'eventType' => $event->getType(),
 					'revId' => $revId,
 				]
 			);
@@ -253,7 +253,10 @@ class NotificationController {
 		$rootJobSignature = $event->getExtraParam( 'rootJobSignature' );
 		$rootJobTimestamp = $event->getExtraParam( 'rootJobTimestamp' );
 
-		return [ 'eventId' => $event->getId() ]
+		$params = $event->getId()
+			? [ 'eventId' => $event->getId() ]
+			: [ 'eventData' => $event->toDbArray() ];
+		return $params
 			+ ( $delay ? [ 'jobReleaseTimestamp' => (int)wfTimestamp() + $delay ] : [] )
 			+ ( $rootJobSignature ? [ 'rootJobSignature' => $rootJobSignature ] : [] )
 			+ ( $rootJobTimestamp ? [ 'rootJobTimestamp' => $rootJobTimestamp ] : [] );

includes/Jobs/NotificationJob.php

@@ -5,6 +5,7 @@ namespace MediaWiki\Extension\Notifications\Jobs;
 use Job;
 use MediaWiki\Extension\Notifications\Controller\NotificationController;
 use MediaWiki\Extension\Notifications\Mapper\EventMapper;
+use MediaWiki\Extension\Notifications\Model\Event;
 use MediaWiki\Title\Title;
 
 class NotificationJob extends Job {
@@ -15,8 +16,12 @@ class NotificationJob extends Job {
 	}
 
 	public function run() {
-		$eventMapper = new EventMapper();
-		$event = $eventMapper->fetchById( $this->params['eventId'], true );
+		if ( isset( $this->params['eventId'] ) ) {
+			$eventMapper = new EventMapper();
+			$event = $eventMapper->fetchById( $this->params['eventId'], true );
+		} else {
+			$event = Event::newFromArray( $this->params['eventData'] );
+		}
 		NotificationController::notify( $event, false );
 
 		return true;

includes/Model/Event.php

@@ -84,6 +84,9 @@ class Event extends AbstractEntity implements Bundleable {
 	 */
 	protected $deleted = 0;
 
+	/** For use in tests */
+	public static bool $alwaysInsert = false;
+
 	/**
 	 * You should not call the constructor.
 	 * Instead, use one of the factory functions:
@@ -198,19 +201,14 @@ class Event extends AbstractEntity implements Bundleable {
 		}
 
 		// @Todo - Database insert logic should not be inside the model
-		$obj->insert();
-
-		$hookRunner->onEventInsertComplete( $obj );
+		if ( self::$alwaysInsert ) {
+			$obj->insert();
+		}
 
 		global $wgEchoUseJobQueue;
 
 		NotificationController::notify( $obj, $wgEchoUseJobQueue );
 
-		$stats = $services->getStatsdDataFactory();
-		$type = $info['type'];
-		$stats->increment( 'echo.event.all' );
-		$stats->increment( "echo.event.$type" );
-
 		return $obj;
 	}
 
@@ -250,6 +248,34 @@ class Event extends AbstractEntity implements Bundleable {
 		return $data;
 	}
 
+	/**
+	 * Creates an Event from an array. The array should be output from ::toDbArray().
+	 *
+	 * @param array $data
+	 * @return self
+	 */
+	public static function newFromArray( $data ) {
+		$obj = new self();
+		if ( isset( $data['event_id'] ) ) {
+			$obj->id = $data['event_id'];
+		}
+		$obj->type = $data['event_type'];
+		$obj->variant = $data['event_variant'];
+		$obj->extra = $data['event_extra'] ? unserialize( $data['event_extra'] ) : [];
+		if ( isset( $data['event_page_id'] ) ) {
+			$obj->pageId = $data['event_page_id'];
+		}
+		$obj->deleted = $data['event_deleted'];
+
+		if ( $data['event_agent_id'] ?? 0 ) {
+			$obj->agent = User::newFromId( $data['event_agent_id'] );
+		} elseif ( isset( $data['event_agent_ip'] ) ) {
+			$obj->agent = User::newFromName( $data['event_agent_ip'], false );
+		}
+
+		return $obj;
+	}
+
 	/**
 	 * Check whether the echo event is an enabled event
 	 * @return bool
@@ -259,6 +285,19 @@ class Event extends AbstractEntity implements Bundleable {
 		return isset( $wgEchoNotifications[$this->getType()] );
 	}
 
+	/**
+	 * Insert this event into the database if it hasn't been yet, and return its id.
+	 * Subsequent calls on this instance will not cause repeated insertion
+	 * and will always return the same id.
+	 * @return int
+	 */
+	public function acquireId() {
+		if ( !$this->id ) {
+			$this->insert();
+		}
+		return $this->id;
+	}
+
 	/**
 	 * Inserts the object into the database.
 	 */
@@ -276,6 +315,15 @@ class Event extends AbstractEntity implements Bundleable {
 				}
 			}
 		}
+
+		$services = MediaWikiServices::getInstance();
+		$hookRunner = new HookRunner( $services->getHookContainer() );
+		$hookRunner->onEventInsertComplete( $this );
+
+		$stats = $services->getStatsdDataFactory();
+		$type = $this->getType();
+		$stats->increment( 'echo.event.all' );
+		$stats->increment( "echo.event.$type" );
 	}
 
 	/**

includes/Model/Notification.php

@@ -129,6 +129,7 @@ class Notification extends AbstractEntity implements Bundleable {
 			}
 		);
 
+		$this->event->acquireId();
 		$notifMapper->insert( $this );
 
 		if ( $this->event->getCategory() === 'edit-user-talk' ) {

includes/Notifier.php

@@ -81,7 +81,7 @@ class Notifier {
 				}
 			}
 		} elseif ( $event->getExtraParam( 'noemail' ) ) {
-			// Could be set for API triggered notifications were email is not
+			// Could be set for API triggered notifications where email is not
 			// requested in API request params
 			return false;
 		}
@@ -114,14 +114,15 @@ class Notifier {
 				$bundleHash = md5( $bundleString );
 			}
 
-			// email digest notification ( weekly or daily )
+			// email digest notification (weekly or daily)
 			if ( $wgEchoEnableEmailBatch && $userOptionsLookup->getOption( $user, 'echo-email-frequency' ) > 0 ) {
 				// always create a unique event hash for those events don't support bundling
 				// this is mainly for group by
+				$eventId = $event->acquireId();
 				if ( !$bundleHash ) {
-					$bundleHash = md5( $event->getType() . '-' . $event->getId() );
+					$bundleHash = md5( $event->getType() . '-' . $eventId );
 				}
-				EmailBatch::addToQueue( $user->getId(), $event->getId(), $priority, $bundleHash );
+				EmailBatch::addToQueue( $user->getId(), $eventId, $priority, $bundleHash );
 
 				return true;
 			}

includes/Push/PushNotifier.php

@@ -33,7 +33,6 @@ class PushNotifier {
 		$params = [ 'centralId' => $centralId ];
 		// below params are only needed for debug logging (T255068)
 		if ( $event !== null ) {
-			$params['eventId'] = $event->getId();
 			$params['eventType'] = $event->getType();
 			if ( $event->getAgent() !== null ) {
 				$params['agent'] = $event->getAgent()->getId();

tests/phpunit/Controller/NotificationControllerTest.php

@@ -266,8 +266,13 @@ class NotificationControllerTest extends MediaWikiIntegrationTestCase {
 			->method( 'getTitle' )
 			->willReturn( Title::newFromText( 'test-title' ) );
 		$event->expects( $this->once() )
-			->method( 'getId' )
-			->willReturn( 42 );
+			->method( 'toDbArray' )
+			->willReturn( [
+				'event_type' => 'test',
+				'event_extra' => [
+					'extra-key' => 'extra'
+				],
+			] );
 		NotificationController::enqueueEvent( $event );
 		$jobQueueGroup = $this->getServiceContainer()->getJobQueueGroup();
 		$queues = $jobQueueGroup->getQueuesWithJobs();
@@ -275,7 +280,15 @@ class NotificationControllerTest extends MediaWikiIntegrationTestCase {
 		$this->assertEquals( 'EchoNotificationJob', $queues[0] );
 		$job = $jobQueueGroup->pop( 'EchoNotificationJob' );
 		$this->assertEquals( 'Test-title', $job->params[ 'title' ] );
-		$this->assertEquals( 42, $job->params[ 'eventId' ] );
+		$this->assertEquals(
+			[
+				'event_type' => 'test',
+				'event_extra' => [
+					'extra-key' => 'extra'
+				],
+			],
+			$job->params[ 'eventData' ]
+		);
 	}
 
 	public function testNotSupportedDelay() {
@@ -311,12 +324,26 @@ class NotificationControllerTest extends MediaWikiIntegrationTestCase {
 				[ 'rootJobTimestamp', null, $rootJobTimestamp ]
 			] );
 		$event->expects( $this->once() )
-			->method( 'getId' )
-			->willReturn( 42 );
+			->method( 'toDbArray' )
+			->willReturn( [
+				'event_type' => 'test',
+				'event_extra' => [
+					'delay' => 10,
+					'rootJobSignature' => 'test-signature',
+					'rootJobTimestamp' => $rootJobTimestamp,
+				],
+			] );
 
 		$params = NotificationController::getEventParams( $event );
 		$expectedParams = [
-			'eventId' => 42,
+			'eventData' => [
+				'event_type' => 'test',
+				'event_extra' => [
+					'delay' => 10,
+					'rootJobSignature' => 'test-signature',
+					'rootJobTimestamp' => $rootJobTimestamp,
+				],
+			],
 			'rootJobSignature' => 'test-signature',
 			'rootJobTimestamp' => $rootJobTimestamp,
 			'jobReleaseTimestamp' => 10

tests/phpunit/TalkPageFunctionalTest.php

@@ -9,12 +9,20 @@ use MediaWiki\Tests\Api\ApiTestCase;
  * @group medium
  */
 class TalkPageFunctionalTest extends ApiTestCase {
+
+	protected function tearDown(): void {
+		Event::$alwaysInsert = false;
+		parent::tearDown();
+	}
+
 	/**
 	 * Creates and updates a user talk page a few times to ensure proper events are
 	 * created.
 	 * @covers \MediaWiki\Extension\Notifications\DiscussionParser
 	 */
 	public function testAddCommentsToTalkPage() {
+		Event::$alwaysInsert = true;
+
 		$editor = $this->getTestSysop()->getUser();
 		$talkTitle = $this->getTestSysop()->getUser()->getTalkPage();
 		$talkPage = $this->getServiceContainer()->getWikiPageFactory()->newFromTitle( $talkTitle );

tests/phpunit/integration/EventIntegrationTest.php

@@ -0,0 +1,87 @@
+<?php
+
+use MediaWiki\Extension\Notifications\Mapper\EventMapper;
+use MediaWiki\Extension\Notifications\Model\Event;
+use MediaWiki\User\UserIdentityValue;
+
+/**
+ * @covers \MediaWiki\Extension\Notifications\Model\Event
+ * @covers \MediaWiki\Extension\Notifications\Controller\NotificationController
+ * @covers \MediaWiki\Extension\Notifications\Jobs\NotificationJob
+ * @group Database
+ */
+class EventIntegrationTest extends MediaWikiIntegrationTestCase {
+
+	public function testEventInsertionImmediate() {
+		$this->clearHook( 'BeforeEchoEventInsert' );
+		$this->overrideConfigValue( 'EchoUseJobQueue', false );
+
+		$user = $this->getTestUser()->getUser();
+		$event = Event::create( [
+			'type' => 'welcome',
+			'agent' => $user,
+			'extra' => [ 'key' => 'value' ]
+		] );
+		$eventId = $event->getId();
+		$this->assertNotFalse( $eventId );
+		$this->assertSame( $eventId, $event->acquireId() );
+		$this->assertSelect(
+			'echo_event',
+			[ 'count' => 'COUNT(*)' ],
+			[ 'event_type' => 'welcome' ],
+			[ [ '1' ] ]
+		);
+	}
+
+	public function testEventNotInserted() {
+		$this->clearHook( 'BeforeEchoEventInsert' );
+		$this->overrideConfigValue( 'EchoUseJobQueue', false );
+
+		$event = Event::create( [
+			'type' => 'welcome',
+			// anons cannot be notified
+			'agent' => UserIdentityValue::newAnonymous( '1.2.3.4' ),
+			'extra' => [ 'key' => 'value' ]
+		] );
+		$this->assertFalse( $event->getId() );
+		$this->assertSelect(
+			'echo_event',
+			[ 'count' => 'COUNT(*)' ],
+			[ 'event_type' => 'welcome' ],
+			[ [ '0' ] ]
+		);
+	}
+
+	public function testEventInsertionDeferred() {
+		$this->clearHook( 'BeforeEchoEventInsert' );
+		$this->clearHook( 'PageSaveComplete' );
+		$this->overrideConfigValue( 'EchoUseJobQueue', true );
+
+		$user = $this->getTestUser()->getUser();
+		$title = $this->getExistingTestPage()->getTitle();
+		$this->runJobs();
+
+		$event = Event::create( [
+			'type' => 'welcome',
+			'agent' => $user,
+			'title' => $title,
+			'extra' => [ 'key' => 'value' ]
+		] );
+		$this->assertFalse( $event->getId() );
+		unset( $event );
+
+		$jobQueueGroup = $this->getServiceContainer()->getJobQueueGroup();
+		$queues = $jobQueueGroup->getQueuesWithJobs();
+		$this->assertSame( [ 'EchoNotificationJob' ], $queues );
+		$job = $jobQueueGroup->pop( 'EchoNotificationJob' );
+		$job->run();
+
+		$eventMapper = new EventMapper();
+		$events = $eventMapper->fetchByPage( $title->getArticleID() );
+		$this->assertCount( 1, $events );
+		[ $event ] = $events;
+		$this->assertSame( 'welcome', $event->getType() );
+		$this->assertTrue( $user->equals( $event->getAgent() ) );
+	}
+
+}