From 85ef18d3fa0939fb3e863ee920a227989bf3a4f1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Julius=20H=C3=A4rtl?= <jus@bitgrid.net>
Date: Thu, 29 Jun 2023 08:46:58 +0200
Subject: [PATCH] fix: Delete inactive sessions in one query
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Julius Härtl <jus@bitgrid.net>
---
 lib/Db/SessionMapper.php            |  33 +++++----
 tests/unit/Db/SessionMapperTest.php | 101 ++++++++++++++++++++++++++++
 2 files changed, 117 insertions(+), 17 deletions(-)
 create mode 100644 tests/unit/Db/SessionMapperTest.php

diff --git a/lib/Db/SessionMapper.php b/lib/Db/SessionMapper.php
index 8b8eec553a..957e21d576 100644
--- 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/tests/unit/Db/SessionMapperTest.php b/tests/unit/Db/SessionMapperTest.php
new file mode 100644
index 0000000000..310b61e957
--- /dev/null
+++ 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));
+	}
+}