From 6a50d1203c8d917e5d7c67ed11a6f949bd12a4a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartosz=20Dziewo=C5=84ski?= Date: Tue, 27 Apr 2021 22:21:50 +0200 Subject: [PATCH] Limit number of topic subscriptions per user Per Manuel Arostegui in T263817#7033384. The limit is 5000. (I picked it arbitrarily, there's no real rationale for it.) Also log a warning when any user reaches half of the limit, so that we might make a decision about changing this mechanism before it starts affecting users. Maybe at that time we'll have data to show that it's safe to remove the limit. Bug: T263817 Change-Id: I18a8ee0ad7383759229c5721d5253fb591457d4d --- i18n/api/en.json | 2 ++ i18n/api/qqq.json | 2 ++ includes/ApiDiscussionToolsSubscribe.php | 12 ++++++-- includes/SubscriptionStore.php | 39 ++++++++++++++++++++++++ modules/controller.js | 2 ++ 5 files changed, 54 insertions(+), 3 deletions(-) diff --git a/i18n/api/en.json b/i18n/api/en.json index 3cc8f24df..3152daaa0 100644 --- a/i18n/api/en.json +++ b/i18n/api/en.json @@ -5,6 +5,8 @@ "apierror-discussiontools-commentid-notfound": "Comment with the ID '$1' not found.", "apierror-discussiontools-commentname-ambiguous": "Multiple comments with the name '$1' found, use ID instead.", "apierror-discussiontools-commentname-notfound": "Comment with the name '$1' not found.", + "apierror-discussiontools-subscription-failed-add": "Could not subscribe to this topic.", + "apierror-discussiontools-subscription-failed-remove": "Could not unsubscribe from this topic.", "apihelp-discussiontools-param-oldid": "The revision number to use (defaults to latest revision).", "apihelp-discussiontools-paramvalue-paction-transcludedfrom": "Return titles of pages from which each comment on the given page is transcluded.", "apihelp-discussiontools-summary": "Returns metadata required to initialize the discussion tools.", diff --git a/i18n/api/qqq.json b/i18n/api/qqq.json index 9278c6777..4c30071a0 100644 --- a/i18n/api/qqq.json +++ b/i18n/api/qqq.json @@ -7,6 +7,8 @@ "apierror-discussiontools-commentid-notfound": "{{doc-apierror}}\n\nParameters:\n* $1 - Comment ID", "apierror-discussiontools-commentname-ambiguous": "{{doc-apierror}}\n\nParameters:\n* $1 - Comment name", "apierror-discussiontools-commentname-notfound": "{{doc-apierror}}\n\nParameters:\n* $1 - Comment name", + "apierror-discussiontools-subscription-failed-add": "{{doc-apierror}}", + "apierror-discussiontools-subscription-failed-remove": "{{doc-apierror}}", "apihelp-discussiontools-param-oldid": "{{doc-apihelp-param|discussiontools|oldid}}", "apihelp-discussiontools-paramvalue-paction-transcludedfrom": "{{doc-apihelp-paramvalue|discussiontools|paction|transcludedfrom}}", "apihelp-discussiontools-summary": "{{doc-apihelp-summary|discussiontools}}", diff --git a/includes/ApiDiscussionToolsSubscribe.php b/includes/ApiDiscussionToolsSubscribe.php index 255a8a81b..3bec69dcc 100644 --- a/includes/ApiDiscussionToolsSubscribe.php +++ b/includes/ApiDiscussionToolsSubscribe.php @@ -58,16 +58,22 @@ class ApiDiscussionToolsSubscribe extends ApiBase { $subscribe = $params['subscribe']; if ( $subscribe ) { - $this->subscriptionStore->addSubscriptionForUser( + $success = $this->subscriptionStore->addSubscriptionForUser( $user, $title, $commentName ); + if ( !$success ) { + $this->dieWithError( 'apierror-discussiontools-subscription-failed-add', 'subscription-failed' ); + } } else { - $this->subscriptionStore->removeSubscriptionForUser( + $success = $this->subscriptionStore->removeSubscriptionForUser( $user, $commentName ); + if ( !$success ) { + $this->dieWithError( 'apierror-discussiontools-subscription-failed-remove', 'subscription-failed' ); + } } // TODO: Subscribe should be tri-state: // * subscribe (add row) @@ -77,7 +83,7 @@ class ApiDiscussionToolsSubscribe extends ApiBase { $result = [ 'page' => $title, 'commentname' => $commentName, - 'subscribe' => $subscribe + 'subscribe' => $subscribe, ]; $this->getResult()->addValue( null, $this->getModuleName(), $result ); diff --git a/includes/SubscriptionStore.php b/includes/SubscriptionStore.php index 0afd8dc2c..2d2523944 100644 --- a/includes/SubscriptionStore.php +++ b/includes/SubscriptionStore.php @@ -3,6 +3,7 @@ namespace MediaWiki\Extension\DiscussionTools; use MediaWiki\Linker\LinkTarget; +use MediaWiki\Logger\LoggerFactory; use MediaWiki\User\UserFactory; use MediaWiki\User\UserIdentity; use ReadOnlyMode; @@ -18,6 +19,11 @@ use Wikimedia\Rdbms\IResultWrapper; // use Wikimedia\Timestamp\ConvertibleTimestamp; class SubscriptionStore { + /** + * Maximum number of subscriptions that we can store for each user. + */ + private const USER_SUBSCRIPTION_LIMIT = 5000; + /** @var ILBFactory */ private $lbFactory; @@ -196,6 +202,36 @@ class SubscriptionStore { ); } + /** + * @param UserIdentity $user + * @return bool + */ + private function userExceedsSubscriptionLimit( UserIdentity $user ) : bool { + $logger = LoggerFactory::getInstance( 'DiscussionTools' ); + // This is always queried before updating + $db = $this->getConnectionRef( DB_PRIMARY ); + + $rowCount = $db->selectRowCount( + 'discussiontools_subscription', + '*', + [ 'sub_user' => $user->getId() ], + __METHOD__, + [ 'LIMIT' => self::USER_SUBSCRIPTION_LIMIT ] + ); + + if ( $rowCount >= self::USER_SUBSCRIPTION_LIMIT / 2 ) { + $logger->warning( + "User {user} has {rowCount} subscriptions, approaching the limit", + [ + 'user' => $user->getId(), + 'rowCount' => $rowCount, + ] + ); + } + + return $rowCount >= self::USER_SUBSCRIPTION_LIMIT; + } + /** * @param UserIdentity $user * @param LinkTarget $target @@ -214,6 +250,9 @@ class SubscriptionStore { if ( !$user->isRegistered() ) { return false; } + if ( $this->userExceedsSubscriptionLimit( $user ) ) { + return false; + } $dbw = $this->getConnectionRef( DB_PRIMARY ); $dbw->upsert( 'discussiontools_subscription', diff --git a/modules/controller.js b/modules/controller.js index 0cfa74854..8ae5d7d0b 100644 --- a/modules/controller.js +++ b/modules/controller.js @@ -285,6 +285,8 @@ function initTopicSubscriptions( $container ) { ) } ); + }, function ( code, data ) { + mw.notify( api.getErrorMessage( data ), { type: 'error' } ); } ); } ); }