fix: Delete inactive sessions in one query

Signed-off-by: Julius Härtl <[email protected]>
nextcloud · Jun 21, 2023 · 163b07a · 163b07a
1 parent a673891
commit 163b07a
Show file tree

Hide file tree

Showing 6 changed files with 152 additions and 18 deletions.
diff --git a/appinfo/info.xml b/appinfo/info.xml
@@ -11,7 +11,7 @@
 - **💾 Open format:** Files are saved as [Markdown](https://en.wikipedia.org/wiki/Markdown), so you can edit them from any other text app too.
 - **✊ Strong foundation:** We use [🐈 tiptap](https://tiptap.scrumpy.io) which is based on [🦉 ProseMirror](https://prosemirror.net) – huge thanks to them!
 	]]></description>
-	<version>3.9.0</version>
+	<version>3.9.1</version>
 	<licence>agpl</licence>
 	<author mail="[email protected]">Julius Härtl</author>
 	<namespace>Text</namespace>

diff --git a/composer/composer/autoload_classmap.php b/composer/composer/autoload_classmap.php
@@ -50,6 +50,7 @@
     'OCA\\Text\\Migration\\Version030201Date20201116123153' => $baseDir . '/../lib/Migration/Version030201Date20201116123153.php',
     'OCA\\Text\\Migration\\Version030501Date20220202101853' => $baseDir . '/../lib/Migration/Version030501Date20220202101853.php',
     'OCA\\Text\\Migration\\Version030701Date20230207131313' => $baseDir . '/../lib/Migration/Version030701Date20230207131313.php',
+    'OCA\\Text\\Migration\\Version030901Date20230615085512' => $baseDir . '/../lib/Migration/Version030901Date20230615085512.php',
     'OCA\\Text\\Notification\\Notifier' => $baseDir . '/../lib/Notification/Notifier.php',
     'OCA\\Text\\Service\\ApiService' => $baseDir . '/../lib/Service/ApiService.php',
     'OCA\\Text\\Service\\AttachmentService' => $baseDir . '/../lib/Service/AttachmentService.php',

diff --git a/composer/composer/autoload_static.php b/composer/composer/autoload_static.php
@@ -65,6 +65,7 @@ class ComposerStaticInitText
         'OCA\\Text\\Migration\\Version030201Date20201116123153' => __DIR__ . '/..' . '/../lib/Migration/Version030201Date20201116123153.php',
         'OCA\\Text\\Migration\\Version030501Date20220202101853' => __DIR__ . '/..' . '/../lib/Migration/Version030501Date20220202101853.php',
         'OCA\\Text\\Migration\\Version030701Date20230207131313' => __DIR__ . '/..' . '/../lib/Migration/Version030701Date20230207131313.php',
+        'OCA\\Text\\Migration\\Version030901Date20230615085512' => __DIR__ . '/..' . '/../lib/Migration/Version030901Date20230615085512.php',
         'OCA\\Text\\Notification\\Notifier' => __DIR__ . '/..' . '/../lib/Notification/Notifier.php',
         'OCA\\Text\\Service\\ApiService' => __DIR__ . '/..' . '/../lib/Service/ApiService.php',
         'OCA\\Text\\Service\\AttachmentService' => __DIR__ . '/..' . '/../lib/Service/AttachmentService.php',

diff --git a/lib/Db/SessionMapper.php b/lib/Db/SessionMapper.php
@@ -99,30 +99,29 @@ public function findAllInactive() {
 		return $this->findEntities($qb);
 	}
 
-	public function deleteInactiveWithoutSteps(?int $documentId = null) {
-		$qb = $this->db->getQueryBuilder();
-		$qb->select('session_id')
-			->from('text_steps');
+	public function deleteInactiveWithoutSteps(?int $documentId = null): int {
+		$selectSubQuery = $this->db->getQueryBuilder();
+		$selectSubQuery->select('s.id')
+			->from('text_sessions', 's')
+			->leftJoin('s', 'text_steps', 'st', $selectSubQuery->expr()->eq('st.session_id', 's.id'))
+			->where($selectSubQuery->expr()->lt('last_contact', $selectSubQuery->createParameter('lastContact')))
+			->andWhere($selectSubQuery->expr()->isNull('st.id'));
 		if ($documentId !== null) {
-			$qb->where($qb->expr()->eq('document_id', $qb->createNamedParameter($documentId)));
+			$selectSubQuery->andWhere($selectSubQuery->expr()->eq('s.document_id', $selectSubQuery->createParameter('documentId')));
 		}
-		$result = $qb
-			->groupBy('session_id')
-			->executeQuery();
-		$activeSessions = $result->fetchAll(\PDO::FETCH_COLUMN);
-		$result->closeCursor();
 
 		$qb = $this->db->getQueryBuilder();
-		$qb->delete($this->getTableName());
-		$qb->where($qb->expr()->lt('last_contact', $qb->createNamedParameter(time() - SessionService::SESSION_VALID_TIME)));
-		if ($documentId !== null) {
-			$qb->andWhere($qb->expr()->eq('document_id', $qb->createNamedParameter($documentId)));
-		}
-		$qb->andWhere($qb->expr()->notIn('id', $qb->createNamedParameter($activeSessions, IQueryBuilder::PARAM_INT_ARRAY)));
+		$qb->delete($this->getTableName())
+			->where($qb->expr()->in('id', $qb->createFunction($selectSubQuery->getSQL())));
+		$qb->setParameters([
+			'lastContact' => time() - SessionService::SESSION_VALID_TIME,
+			'documentId' => $documentId,
+		]);
+
 		return $qb->executeStatement();
 	}
 
-	public function deleteByDocumentId($documentId) {
+	public function deleteByDocumentId($documentId): int {
 		$qb = $this->db->getQueryBuilder();
 		$qb->delete($this->getTableName())
 			->where($qb->expr()->eq('document_id', $qb->createNamedParameter($documentId)));

diff --git a/lib/Migration/Version030901Date20230615085512.php b/lib/Migration/Version030901Date20230615085512.php
@@ -0,0 +1,32 @@
+<?php
+
+declare(strict_types=1);
+
+namespace OCA\Text\Migration;
+
+use Closure;
+use OCP\DB\ISchemaWrapper;
+use OCP\Migration\IOutput;
+use OCP\Migration\SimpleMigrationStep;
+
+class Version030901Date20230615085512 extends SimpleMigrationStep {
+
+	/**
+	 * @param IOutput $output
+	 * @param Closure $schemaClosure The `\Closure` returns a `ISchemaWrapper`
+	 * @param array $options
+	 * @return null|ISchemaWrapper
+	 */
+	public function changeSchema(IOutput $output, Closure $schemaClosure, array $options) {
+		/** @var ISchemaWrapper $schema */
+		$schema = $schemaClosure();
+
+		$table = $schema->getTable('text_steps');
+		if (!$table->hasIndex('ts_session')) {
+			$table->addIndex(['session_id'], 'ts_session');
+			return $schema;
+		}
+
+		return null;
+	}
+}
diff --git a/tests/unit/Db/SessionMapperTest.php b/tests/unit/Db/SessionMapperTest.php
@@ -0,0 +1,101 @@
+<?php
+
+namespace OCA\Text\Db;
+
+/**
+ * @group DB
+ */
+class SessionMapperTest extends \Test\TestCase {
+
+	private SessionMapper $sessionMapper;
+	private StepMapper $stepMapper;
+
+	public function setUp(): void {
+		parent::setUp();
+		$this->sessionMapper = \OCP\Server::get(SessionMapper::class);
+		$this->stepMapper = \OCP\Server::get(StepMapper::class);
+
+	}
+
+	public function testDeleteInactiveWithoutSteps() {
+		$this->sessionMapper->deleteByDocumentId(1);
+		$this->sessionMapper->deleteByDocumentId(2);
+		$this->sessionMapper->insert(Session::fromParams([
+			'userId' => 'admin',
+			'documentId' => 1,
+			'lastContact' => 1337,
+			'token' => uniqid(),
+			'color' => '00ff00',
+		]));
+		$this->sessionMapper->insert(Session::fromParams([
+			'userId' => 'admin',
+			'documentId' => 2,
+			'lastContact' => 1337,
+			'token' => uniqid(),
+			'color' => '00ff00',
+		]));
+		$this->sessionMapper->deleteInactiveWithoutSteps(1);
+		self::assertCount(0, $this->sessionMapper->findAll(1));
+		self::assertCount(1, $this->sessionMapper->findAll(2));
+		$this->sessionMapper->deleteInactiveWithoutSteps();
+		self::assertCount(0, $this->sessionMapper->findAll(2));
+	}
+
+	public function testDeleteInactiveWithoutStepsKeep() {
+		$this->stepMapper->deleteAll(1);
+		$this->sessionMapper->deleteByDocumentId(1);
+		$this->sessionMapper->deleteByDocumentId(2);
+
+		$s1 = $this->sessionMapper->insert(Session::fromParams([
+			'userId' => 'admin',
+			'documentId' => 1,
+			'lastContact' => 1337,
+			'token' => uniqid(),
+			'color' => '00ff00',
+		]));
+		$s2 = $this->sessionMapper->insert(Session::fromParams([
+			'userId' => 'admin',
+			'documentId' => 2,
+			'lastContact' => 1337,
+			'token' => uniqid(),
+			'color' => '00ff00',
+		]));
+		$this->stepMapper->insert(Step::fromParams([
+			'sessionId' => $s1->getId(),
+			'documentId' => 1,
+			'data' => 'YJSDATA',
+			'version' => 1,
+		]));
+		self::assertCount(1, $this->sessionMapper->findAll(1));
+
+		$this->sessionMapper->deleteInactiveWithoutSteps(1);
+		self::assertCount(1, $this->sessionMapper->findAll(1));
+		self::assertCount(1, $this->sessionMapper->findAll(2));
+
+		$this->sessionMapper->deleteInactiveWithoutSteps();
+		self::assertCount(1, $this->sessionMapper->findAll(1));
+		self::assertCount(0, $this->sessionMapper->findAll(2));
+	}
+
+	public function testDeleteInactiveWithoutStepsMultiple() {
+		$this->sessionMapper->deleteByDocumentId(1);
+
+		$count = 1010;
+		for ($i = 0;$i < $count;$i++) {
+			$this->sessionMapper->insert(Session::fromParams([
+				'userId' => 'admin',
+				'documentId' => 1,
+				'lastContact' => 1337,
+				'token' => uniqid(),
+				'color' => '00ff00',
+			]));
+		}
+
+		self::assertCount($count, $this->sessionMapper->findAll(1));
+
+		$deleted = $this->sessionMapper->deleteInactiveWithoutSteps();
+		self::assertEquals($count, $deleted);
+
+		self::assertCount(0, $this->sessionMapper->findAll(1));
+	}
+}