From 4ae63d1b4d5c2c5308f67776033c0b2fa9e6f264 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mat=C4=9Bj=20Such=C3=A1nek?= Date: Sun, 22 Sep 2024 11:42:51 +0200 Subject: [PATCH] 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 --- .../Controller/NotificationController.php | 11 ++- includes/Jobs/NotificationJob.php | 9 +- includes/Model/Event.php | 64 ++++++++++++-- includes/Model/Notification.php | 1 + includes/Notifier.php | 9 +- includes/Push/PushNotifier.php | 1 - .../Controller/NotificationControllerTest.php | 39 +++++++-- tests/phpunit/TalkPageFunctionalTest.php | 8 ++ .../integration/EventIntegrationTest.php | 87 +++++++++++++++++++ 9 files changed, 204 insertions(+), 25 deletions(-) create mode 100644 tests/phpunit/integration/EventIntegrationTest.php diff --git a/includes/Controller/NotificationController.php b/includes/Controller/NotificationController.php index c44e24089..4664542c6 100644 --- a/includes/Controller/NotificationController.php +++ b/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 ] : [] ); diff --git a/includes/Jobs/NotificationJob.php b/includes/Jobs/NotificationJob.php index e2401fba2..ca3e7d058 100644 --- a/includes/Jobs/NotificationJob.php +++ b/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; diff --git a/includes/Model/Event.php b/includes/Model/Event.php index 91c1c6ead..b823cf082 100644 --- a/includes/Model/Event.php +++ b/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" ); } /** diff --git a/includes/Model/Notification.php b/includes/Model/Notification.php index af2e4a897..1de2ab2da 100644 --- a/includes/Model/Notification.php +++ b/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' ) { diff --git a/includes/Notifier.php b/includes/Notifier.php index a955705d9..bd345533b 100644 --- a/includes/Notifier.php +++ b/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; } diff --git a/includes/Push/PushNotifier.php b/includes/Push/PushNotifier.php index 0d25502ae..4bca99fe5 100644 --- a/includes/Push/PushNotifier.php +++ b/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(); diff --git a/tests/phpunit/Controller/NotificationControllerTest.php b/tests/phpunit/Controller/NotificationControllerTest.php index cafb7bb63..2ea49dfce 100644 --- a/tests/phpunit/Controller/NotificationControllerTest.php +++ b/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 diff --git a/tests/phpunit/TalkPageFunctionalTest.php b/tests/phpunit/TalkPageFunctionalTest.php index ce1992476..ac29befd6 100644 --- a/tests/phpunit/TalkPageFunctionalTest.php +++ b/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 ); diff --git a/tests/phpunit/integration/EventIntegrationTest.php b/tests/phpunit/integration/EventIntegrationTest.php new file mode 100644 index 000000000..d5d27ea70 --- /dev/null +++ b/tests/phpunit/integration/EventIntegrationTest.php @@ -0,0 +1,87 @@ +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() ) ); + } + +}