From 72413eee47b672355102f17dc22c891743d2735c Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 10 Feb 2022 15:50:09 +0100 Subject: [PATCH 1/5] Directly delete the user status instead of getting it a second time Signed-off-by: Joas Schilling --- apps/user_status/lib/Service/StatusService.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/user_status/lib/Service/StatusService.php b/apps/user_status/lib/Service/StatusService.php index ad50a541a0022..1f72fd8a21ba2 100644 --- a/apps/user_status/lib/Service/StatusService.php +++ b/apps/user_status/lib/Service/StatusService.php @@ -474,7 +474,7 @@ public function revertUserStatus(string $userId, ?string $messageId, string $sta // Another status is set automatically, do nothing return; } - $this->removeUserStatus($userId); + $this->mapper->delete($userStatus); } catch (DoesNotExistException $ex) { // No current status => nothing to delete } From e2a86b74204e9d2562b0647a3b3ade391b686785 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 10 Feb 2022 16:51:30 +0100 Subject: [PATCH 2/5] Delete the user status without loading it first Signed-off-by: Joas Schilling --- apps/user_status/lib/Db/UserStatusMapper.php | 19 +++++++++++++++++++ .../user_status/lib/Service/StatusService.php | 15 ++++++--------- 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/apps/user_status/lib/Db/UserStatusMapper.php b/apps/user_status/lib/Db/UserStatusMapper.php index 4670c28f7f7bc..02f9a607bd869 100644 --- a/apps/user_status/lib/Db/UserStatusMapper.php +++ b/apps/user_status/lib/Db/UserStatusMapper.php @@ -166,4 +166,23 @@ public function clearMessagesOlderThan(int $timestamp): void { $qb->execute(); } + + + /** + * Deletes a user status so we can restore the backup + * + * @param string $userId + * @param string $messageId + * @param string $status + * @return bool True if an entry was deleted + */ + public function deleteCurrentStatusToRestoreBackup(string $userId, string $messageId, string $status): bool { + $qb = $this->db->getQueryBuilder(); + $qb->delete($this->tableName) + ->where($qb->expr()->eq('user_id', $qb->createNamedParameter($userId))) + ->andWhere($qb->expr()->eq('message_id', $qb->createNamedParameter($messageId))) + ->andWhere($qb->expr()->eq('status', $qb->createNamedParameter($status))) + ->andWhere($qb->expr()->eq('is_backup', $qb->createNamedParameter(false, IQueryBuilder::PARAM_BOOL))); + return $qb->executeStatement() > 0; + } } diff --git a/apps/user_status/lib/Service/StatusService.php b/apps/user_status/lib/Service/StatusService.php index 1f72fd8a21ba2..e31b1772c4d39 100644 --- a/apps/user_status/lib/Service/StatusService.php +++ b/apps/user_status/lib/Service/StatusService.php @@ -468,16 +468,13 @@ public function revertUserStatus(string $userId, ?string $messageId, string $sta // No user status to revert, do nothing return; } - try { - $userStatus = $this->mapper->findByUserId($userId); - if ($userStatus->getMessageId() !== $messageId || $userStatus->getStatus() !== $status) { - // Another status is set automatically, do nothing - return; - } - $this->mapper->delete($userStatus); - } catch (DoesNotExistException $ex) { - // No current status => nothing to delete + + $deleted = $this->mapper->deleteCurrentStatusToRestoreBackup($userId, $messageId ?? '', $status); + if (!$deleted) { + // Another status is set automatically or no status, do nothing + return; } + $backupUserStatus->setIsBackup(false); // Remove the underscore prefix added when creating the backup $backupUserStatus->setUserId(substr($backupUserStatus->getUserId(), 1)); From 7309e979071565da592819991c9d9deb3134a70d Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 11 Feb 2022 14:37:09 +0100 Subject: [PATCH 3/5] Don't include unindexed is_backup in the query, it's ensured by the user_id leading underscore already Signed-off-by: Joas Schilling --- apps/user_status/lib/Db/UserStatusMapper.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/apps/user_status/lib/Db/UserStatusMapper.php b/apps/user_status/lib/Db/UserStatusMapper.php index 02f9a607bd869..0f5dd4ef1ccf2 100644 --- a/apps/user_status/lib/Db/UserStatusMapper.php +++ b/apps/user_status/lib/Db/UserStatusMapper.php @@ -109,8 +109,7 @@ public function findByUserId(string $userId, bool $isBackup = false):UserStatus $qb ->select('*') ->from($this->tableName) - ->where($qb->expr()->eq('user_id', $qb->createNamedParameter($isBackup ? '_' . $userId : $userId, IQueryBuilder::PARAM_STR))) - ->andWhere($qb->expr()->eq('is_backup', $qb->createNamedParameter($isBackup, IQueryBuilder::PARAM_BOOL))); + ->where($qb->expr()->eq('user_id', $qb->createNamedParameter($isBackup ? '_' . $userId : $userId, IQueryBuilder::PARAM_STR))); return $this->findEntity($qb); } From 2e328fc8981e4da721a2fcdd76e3b6ae63bc248e Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 15 Feb 2022 14:55:40 +0100 Subject: [PATCH 4/5] Create the backup user status in 1 query instead of 3 Signed-off-by: Joas Schilling --- apps/user_status/lib/Db/UserStatusMapper.php | 17 +++++ .../user_status/lib/Service/StatusService.php | 27 +++----- .../tests/Unit/Service/StatusServiceTest.php | 64 ++++++++----------- 3 files changed, 50 insertions(+), 58 deletions(-) diff --git a/apps/user_status/lib/Db/UserStatusMapper.php b/apps/user_status/lib/Db/UserStatusMapper.php index 0f5dd4ef1ccf2..1ef327a21f4b8 100644 --- a/apps/user_status/lib/Db/UserStatusMapper.php +++ b/apps/user_status/lib/Db/UserStatusMapper.php @@ -184,4 +184,21 @@ public function deleteCurrentStatusToRestoreBackup(string $userId, string $messa ->andWhere($qb->expr()->eq('is_backup', $qb->createNamedParameter(false, IQueryBuilder::PARAM_BOOL))); return $qb->executeStatement() > 0; } + + /** + * @param string $userId + * @return bool + * @throws \OCP\DB\Exception + */ + public function createBackupStatus(string $userId): bool { + // Prefix user account with an underscore because user_id is marked as unique + // in the table. Starting a username with an underscore is not allowed so this + // shouldn't create any trouble. + $qb = $this->db->getQueryBuilder(); + $qb->update($this->tableName) + ->set('is_backup', $qb->createNamedParameter(true, IQueryBuilder::PARAM_BOOL)) + ->set('user_id', $qb->createNamedParameter('_' . $userId)) + ->where($qb->expr()->eq('user_id', $qb->createNamedParameter($userId))); + return $qb->executeStatement() > 0; + } } diff --git a/apps/user_status/lib/Service/StatusService.php b/apps/user_status/lib/Service/StatusService.php index e31b1772c4d39..dcabbe8e11f0d 100644 --- a/apps/user_status/lib/Service/StatusService.php +++ b/apps/user_status/lib/Service/StatusService.php @@ -35,6 +35,7 @@ use OCA\UserStatus\Exception\StatusMessageTooLongException; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Utility\ITimeFactory; +use OCP\DB\Exception; use OCP\IConfig; use OCP\UserStatus\IUserStatus; @@ -434,30 +435,18 @@ private function addDefaultMessage(UserStatus $status): void { } /** - * @return bool false iff there is already a backup. In this case abort the procedure. + * @return bool false if there is already a backup. In this case abort the procedure. */ public function backupCurrentStatus(string $userId): bool { try { - $this->mapper->findByUserId($userId, true); - return false; - } catch (DoesNotExistException $ex) { - // No backup already existing => Good - } - - try { - $userStatus = $this->mapper->findByUserId($userId); - } catch (DoesNotExistException $ex) { - // if there is no status to backup, just return + $this->mapper->createBackupStatus($userId); return true; + } catch (Exception $ex) { + if ($ex->getReason() === Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) { + return false; + } + throw $ex; } - - $userStatus->setIsBackup(true); - // Prefix user account with an underscore because user_id is marked as unique - // in the table. Starting an username with an underscore is not allowed so this - // shouldn't create any trouble. - $userStatus->setUserId('_' . $userStatus->getUserId()); - $this->mapper->update($userStatus); - return true; } public function revertUserStatus(string $userId, ?string $messageId, string $status): void { diff --git a/apps/user_status/tests/Unit/Service/StatusServiceTest.php b/apps/user_status/tests/Unit/Service/StatusServiceTest.php index a7c183eae0405..9e797271fc96e 100644 --- a/apps/user_status/tests/Unit/Service/StatusServiceTest.php +++ b/apps/user_status/tests/Unit/Service/StatusServiceTest.php @@ -26,6 +26,8 @@ */ namespace OCA\UserStatus\Tests\Service; +use Doctrine\DBAL\Exception\UniqueConstraintViolationException; +use OC\DB\Exceptions\DbalException; use OCA\UserStatus\Db\UserStatus; use OCA\UserStatus\Db\UserStatusMapper; use OCA\UserStatus\Exception\InvalidClearAtException; @@ -38,6 +40,7 @@ use OCA\UserStatus\Service\StatusService; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Utility\ITimeFactory; +use OCP\DB\Exception; use OCP\IConfig; use OCP\UserStatus\IUserStatus; use Test\TestCase; @@ -723,52 +726,35 @@ public function testCleanStatusCleanedAlready(): void { parent::invokePrivate($this->service, 'cleanStatus', [$status]); } - public function testBackupWorkingHasBackupAlready() { - $status = new UserStatus(); - $status->setStatus(IUserStatus::ONLINE); - $status->setStatusTimestamp(1337); - $status->setIsUserDefined(true); - $status->setMessageId('meeting'); - $status->setUserId('john'); - $status->setIsBackup(true); - + public function testBackupWorkingHasBackupAlready(): void { + $p = $this->createMock(UniqueConstraintViolationException::class); + $e = DbalException::wrap($p); $this->mapper->expects($this->once()) - ->method('findByUserId') - ->with('john', true) - ->willReturn($status); + ->method('createBackupStatus') + ->with('john') + ->willThrowException($e); - $this->service->backupCurrentStatus('john'); + $this->assertFalse($this->service->backupCurrentStatus('john')); } - public function testBackup() { - $currentStatus = new UserStatus(); - $currentStatus->setStatus(IUserStatus::ONLINE); - $currentStatus->setStatusTimestamp(1337); - $currentStatus->setIsUserDefined(true); - $currentStatus->setMessageId('meeting'); - $currentStatus->setUserId('john'); - - $this->mapper->expects($this->at(0)) - ->method('findByUserId') - ->with('john', true) - ->willThrowException(new DoesNotExistException('')); - $this->mapper->expects($this->at(1)) - ->method('findByUserId') - ->with('john', false) - ->willReturn($currentStatus); + public function testBackupThrowsOther(): void { + $e = new Exception('', Exception::REASON_CONNECTION_LOST); + $this->mapper->expects($this->once()) + ->method('createBackupStatus') + ->with('john') + ->willThrowException($e); - $newBackupStatus = new UserStatus(); - $newBackupStatus->setStatus(IUserStatus::ONLINE); - $newBackupStatus->setStatusTimestamp(1337); - $newBackupStatus->setIsUserDefined(true); - $newBackupStatus->setMessageId('meeting'); - $newBackupStatus->setUserId('_john'); - $newBackupStatus->setIsBackup(true); + $this->expectException(Exception::class); + $this->service->backupCurrentStatus('john'); + } + public function testBackup(): void { + $e = new Exception('', Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION); $this->mapper->expects($this->once()) - ->method('update') - ->with($newBackupStatus); + ->method('createBackupStatus') + ->with('john') + ->willReturn(true); - $this->service->backupCurrentStatus('john'); + $this->assertTrue($this->service->backupCurrentStatus('john')); } } From 9292834bebe8c1d42e3f08687f14d561c90a46bc Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 15 Feb 2022 15:28:55 +0100 Subject: [PATCH 5/5] Do status and predefined message setting in one go Signed-off-by: Joas Schilling --- .../lib/Connector/UserStatusProvider.php | 10 +-- .../user_status/lib/Service/StatusService.php | 55 +++++++++++++++++ .../tests/Unit/Db/UserStatusMapperTest.php | 61 +++++++++++++++++++ 3 files changed, 118 insertions(+), 8 deletions(-) diff --git a/apps/user_status/lib/Connector/UserStatusProvider.php b/apps/user_status/lib/Connector/UserStatusProvider.php index f3f4c5f710e01..da0a6c635ce46 100644 --- a/apps/user_status/lib/Connector/UserStatusProvider.php +++ b/apps/user_status/lib/Connector/UserStatusProvider.php @@ -57,14 +57,8 @@ public function getUserStatuses(array $userIds): array { return $userStatuses; } - public function setUserStatus(string $userId, string $messageId, string $status, bool $createBackup = false): void { - if ($createBackup) { - if ($this->service->backupCurrentStatus($userId) === false) { - return; // Already a status set automatically => abort. - } - } - $this->service->setStatus($userId, $status, null, true); - $this->service->setPredefinedMessage($userId, $messageId, null); + public function setUserStatus(string $userId, string $messageId, string $status, bool $createBackup): void { + $this->service->setUserStatus($userId, $status, $messageId, $createBackup); } public function revertUserStatus(string $userId, string $messageId, string $status): void { diff --git a/apps/user_status/lib/Service/StatusService.php b/apps/user_status/lib/Service/StatusService.php index dcabbe8e11f0d..eb48124009481 100644 --- a/apps/user_status/lib/Service/StatusService.php +++ b/apps/user_status/lib/Service/StatusService.php @@ -230,6 +230,7 @@ public function setPredefinedMessage(string $userId, $userStatus->setStatus(IUserStatus::OFFLINE); $userStatus->setStatusTimestamp(0); $userStatus->setIsUserDefined(false); + $userStatus->setIsBackup(false); } if (!$this->predefinedStatusService->isValidId($messageId)) { @@ -253,6 +254,60 @@ public function setPredefinedMessage(string $userId, return $this->mapper->update($userStatus); } + /** + * @param string $userId + * @param string $status + * @param string $messageId + * @param bool $createBackup + * @throws InvalidStatusTypeException + * @throws InvalidMessageIdException + */ + public function setUserStatus(string $userId, + string $status, + string $messageId, + bool $createBackup): void { + // Check if status-type is valid + if (!\in_array($status, self::PRIORITY_ORDERED_STATUSES, true)) { + throw new InvalidStatusTypeException('Status-type "' . $status . '" is not supported'); + } + + if (!$this->predefinedStatusService->isValidId($messageId)) { + throw new InvalidMessageIdException('Message-Id "' . $messageId . '" is not supported'); + } + + if ($createBackup) { + if ($this->backupCurrentStatus($userId) === false) { + return; // Already a status set automatically => abort. + } + + // If we just created the backup + $userStatus = new UserStatus(); + $userStatus->setUserId($userId); + } else { + try { + $userStatus = $this->mapper->findByUserId($userId); + } catch (DoesNotExistException $ex) { + $userStatus = new UserStatus(); + $userStatus->setUserId($userId); + } + } + + $userStatus->setStatus($status); + $userStatus->setStatusTimestamp($this->timeFactory->getTime()); + $userStatus->setIsUserDefined(false); + $userStatus->setIsBackup(false); + $userStatus->setMessageId($messageId); + $userStatus->setCustomIcon(null); + $userStatus->setCustomMessage(null); + $userStatus->setClearAt(null); + + if ($userStatus->getId() !== null) { + $this->mapper->update($userStatus); + return; + } + $this->mapper->insert($userStatus); + } + /** * @param string $userId * @param string|null $statusIcon diff --git a/apps/user_status/tests/Unit/Db/UserStatusMapperTest.php b/apps/user_status/tests/Unit/Db/UserStatusMapperTest.php index ddb067b862b15..4759adabb1209 100644 --- a/apps/user_status/tests/Unit/Db/UserStatusMapperTest.php +++ b/apps/user_status/tests/Unit/Db/UserStatusMapperTest.php @@ -251,4 +251,65 @@ private function insertSampleStatuses(): void { $this->mapper->insert($userStatus2); $this->mapper->insert($userStatus3); } + + public function dataCreateBackupStatus(): array { + return [ + [false, false, false], + [true, false, true], + [false, true, false], + [true, true, false], + ]; + } + + /** + * @dataProvider dataCreateBackupStatus + * @param bool $hasStatus + * @param bool $hasBackup + * @param bool $backupCreated + */ + public function testCreateBackupStatus(bool $hasStatus, bool $hasBackup, bool $backupCreated): void { + if ($hasStatus) { + $userStatus1 = new UserStatus(); + $userStatus1->setUserId('user1'); + $userStatus1->setStatus('online'); + $userStatus1->setStatusTimestamp(5000); + $userStatus1->setIsUserDefined(true); + $userStatus1->setIsBackup(false); + $userStatus1->setCustomIcon('🚀'); + $userStatus1->setCustomMessage('Current'); + $userStatus1->setClearAt(50000); + $this->mapper->insert($userStatus1); + } + + if ($hasBackup) { + $userStatus1 = new UserStatus(); + $userStatus1->setUserId('_user1'); + $userStatus1->setStatus('online'); + $userStatus1->setStatusTimestamp(5000); + $userStatus1->setIsUserDefined(true); + $userStatus1->setIsBackup(true); + $userStatus1->setCustomIcon('🚀'); + $userStatus1->setCustomMessage('Backup'); + $userStatus1->setClearAt(50000); + $this->mapper->insert($userStatus1); + } + + if ($hasStatus && $hasBackup) { + $this->expectException(Exception::class); + } + + self::assertSame($backupCreated, $this->mapper->createBackupStatus('user1')); + + if ($backupCreated) { + $user1Status = $this->mapper->findByUserId('user1', true); + $this->assertEquals('_user1', $user1Status->getUserId()); + $this->assertEquals(true, $user1Status->getIsBackup()); + $this->assertEquals('Current', $user1Status->getCustomMessage()); + } else if ($hasBackup) { + $user1Status = $this->mapper->findByUserId('user1', true); + $this->assertEquals('_user1', $user1Status->getUserId()); + $this->assertEquals(true, $user1Status->getIsBackup()); + $this->assertEquals('Backup', $user1Status->getCustomMessage()); + } + } }