diff --git a/includes/Push/SubscriptionManager.php b/includes/Push/SubscriptionManager.php index 6e8a02240..a667a2d8f 100644 --- a/includes/Push/SubscriptionManager.php +++ b/includes/Push/SubscriptionManager.php @@ -5,7 +5,6 @@ namespace EchoPush; use EchoAbstractMapper; use IDatabase; use MediaWiki\Storage\NameTableStore; -use OverflowException; use Wikimedia\Rdbms\DBError; class SubscriptionManager extends EchoAbstractMapper { @@ -47,14 +46,6 @@ class SubscriptionManager extends EchoAbstractMapper { $this->maxSubscriptionsPerUser = $maxSubscriptionsPerUser; } - /** - * Get the configured maximum number of stored subscriptions per user. - * @return int - */ - public function getMaxSubscriptionsPerUser(): int { - return $this->maxSubscriptionsPerUser; - } - /** * Store push subscription information for a central user ID. * @param string $provider Provider name string (validated by presence in the PARAM_TYPE array) @@ -62,11 +53,20 @@ class SubscriptionManager extends EchoAbstractMapper { * @param int $centralId * @param string|null $topic APNS topic string * @return bool true if the subscription was created; false if the token already exists - * @throws OverflowException if the user already has >= the configured max subscriptions */ public function create( string $provider, string $token, int $centralId, ?string $topic = null ): bool { - if ( $this->userHasMaxAllowedSubscriptions( $centralId ) ) { - throw new OverflowException( 'Max subscriptions exceeded' ); + $subscriptions = $this->getSubscriptionsForUser( $centralId ); + if ( count( $subscriptions ) >= $this->maxSubscriptionsPerUser ) { + // If we exceed the number of subscriptions for this user, then delete the oldest subscription + // before inserting the new one, making it behave like a circular buffer. + // (Find the oldest subscription by iterating, since their order in the DB is not guaranteed.) + $oldest = $subscriptions[0]; + foreach ( $subscriptions as $subscription ) { + if ( $subscription->getUpdated() < $oldest->getUpdated() ) { + $oldest = $subscription; + } + } + $this->delete( [ $oldest->getToken() ], $centralId ); } $topicId = $topic ? $this->pushTopicStore->acquireId( $topic ) : null; $this->dbw->insert( @@ -131,27 +131,4 @@ class SubscriptionManager extends EchoAbstractMapper { return $this->dbw->affectedRows(); } - /** - * Get count of all registered subscriptions for a user (by central ID). - * @param int $centralId - * @return int - */ - private function getSubscriptionCountForUser( int $centralId ) { - return $this->dbr->selectRowCount( - 'echo_push_subscription', - 'eps_id', - [ 'eps_user' => $centralId ], - __METHOD__ - ); - } - - /** - * Returns true if the central user has >= the configured maximum push subscriptions in the DB - * @param int $centralId - * @return bool - */ - private function userHasMaxAllowedSubscriptions( int $centralId ): bool { - return $this->getSubscriptionCountForUser( $centralId ) >= $this->maxSubscriptionsPerUser; - } - } diff --git a/includes/api/Push/ApiEchoPushSubscriptionsCreate.php b/includes/api/Push/ApiEchoPushSubscriptionsCreate.php index 8eb328e27..b753703fa 100644 --- a/includes/api/Push/ApiEchoPushSubscriptionsCreate.php +++ b/includes/api/Push/ApiEchoPushSubscriptionsCreate.php @@ -7,7 +7,6 @@ use ApiMain; use EchoPush\SubscriptionManager; use EchoPush\Utils; use EchoServices; -use OverflowException; use Wikimedia\ParamValidator\ParamValidator; class ApiEchoPushSubscriptionsCreate extends ApiBase { @@ -69,14 +68,7 @@ class ApiEchoPushSubscriptionsCreate extends ApiBase { } } $userId = Utils::getPushUserId( $this->getUser() ); - try { - $success = $this->subscriptionManager->create( $provider, $token, $userId, $topic ); - } catch ( OverflowException $e ) { - $maxSubscriptionsPerUser = $this->subscriptionManager->getMaxSubscriptionsPerUser(); - $this->dieWithError( [ 'apierror-echo-push-too-many-subscriptions', - $maxSubscriptionsPerUser ] ); - } - + $success = $this->subscriptionManager->create( $provider, $token, $userId, $topic ); if ( !$success ) { $this->dieWithError( 'apierror-echo-push-token-exists' ); } diff --git a/tests/phpunit/api/Push/ApiEchoPushSubscriptionsCreateTest.php b/tests/phpunit/api/Push/ApiEchoPushSubscriptionsCreateTest.php index 3507b8357..a70c9b385 100644 --- a/tests/phpunit/api/Push/ApiEchoPushSubscriptionsCreateTest.php +++ b/tests/phpunit/api/Push/ApiEchoPushSubscriptionsCreateTest.php @@ -36,10 +36,17 @@ class ApiEchoPushSubscriptionsCreateTest extends ApiTestCase { $result = $this->doApiRequestWithToken( $params, null, $this->user ); $this->assertEquals( 'Success', $result[0]['create']['result'] ); - // After max subscriptions reached + // Make sure it's possible to register a new token even when limit is reached $params['providertoken'] = 'DEF456'; - $this->expectException( ApiUsageException::class ); - $this->doApiRequestWithToken( $params, null, $this->user ); + $result = $this->doApiRequestWithToken( $params, null, $this->user ); + $this->assertEquals( 'Success', $result[0]['create']['result'] ); + + // Explicitly verify that the oldest token was removed + $subscriptionManager = EchoServices::getInstance()->getPushSubscriptionManager(); + $subscriptions = $subscriptionManager->getSubscriptionsForUser( Utils::getPushUserId( $this->user ) ); + foreach ( $subscriptions as $subscription ) { + $this->assertNotEquals( 'XYZ789', $subscription->getToken() ); + } } public function testApiCreateSubscriptionTokenExists(): void { diff --git a/tests/phpunit/integration/Push/SubscriptionManagerTest.php b/tests/phpunit/integration/Push/SubscriptionManagerTest.php index 08909a830..530d011a3 100644 --- a/tests/phpunit/integration/Push/SubscriptionManagerTest.php +++ b/tests/phpunit/integration/Push/SubscriptionManagerTest.php @@ -26,22 +26,19 @@ class SubscriptionManagerTest extends MediaWikiIntegrationTestCase { $subscriptionManager->create( 'test', 'ABC123', $centralId ); $subscriptions = $subscriptionManager->getSubscriptionsForUser( $centralId ); $this->assertCount( 1, $subscriptions ); - $this->assertTrue( $subscriptionManager->userHasMaxAllowedSubscriptions( $centralId ) ); $subscriptionManager->delete( [ 'ABC123' ], $centralId ); $subscriptions = $subscriptionManager->getSubscriptionsForUser( $centralId ); $this->assertCount( 0, $subscriptions ); - $this->assertFalse( $subscriptionManager->userHasMaxAllowedSubscriptions( $centralId ) ); $subscriptionManager->create( 'test', 'ABC123', $centralId ); $subscriptions = $subscriptionManager->getSubscriptionsForUser( $centralId ); $this->assertCount( 1, $subscriptions ); - $this->assertTrue( $subscriptionManager->userHasMaxAllowedSubscriptions( $centralId ) ); - $subscriptionManager->delete( [ 'ABC123' ] ); + $subscriptionManager->create( 'test', 'DEF456', $centralId ); $subscriptions = $subscriptionManager->getSubscriptionsForUser( $centralId ); - $this->assertCount( 0, $subscriptions ); - $this->assertFalse( $subscriptionManager->userHasMaxAllowedSubscriptions( $centralId ) ); + $this->assertCount( 1, $subscriptions ); + $this->assertEquals( 'DEF456', $subscriptions[0]->getToken() ); } }