diff --git a/appinfo/info.xml b/appinfo/info.xml index 7a5230954ae..9145b34f741 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -1,6 +1,6 @@ $baseDir . '/../lib/Migration/Version070000Date20250925110024.php', 'OCA\\Text\\Migration\\Version080000Date20260331132113' => $baseDir . '/../lib/Migration/Version080000Date20260331132113.php', 'OCA\\Text\\Migration\\Version080000Date20261020120000' => $baseDir . '/../lib/Migration/Version080000Date20261020120000.php', + 'OCA\\Text\\Migration\\Version100000Date20261011000000' => $baseDir . '/../lib/Migration/Version100000Date20261011000000.php', 'OCA\\Text\\Notification\\Notifier' => $baseDir . '/../lib/Notification/Notifier.php', 'OCA\\Text\\Service\\AiTagService' => $baseDir . '/../lib/Service/AiTagService.php', 'OCA\\Text\\Service\\ApiService' => $baseDir . '/../lib/Service/ApiService.php', diff --git a/composer/composer/autoload_static.php b/composer/composer/autoload_static.php index 9afce9d9961..ca595772429 100644 --- a/composer/composer/autoload_static.php +++ b/composer/composer/autoload_static.php @@ -83,6 +83,7 @@ class ComposerStaticInitText 'OCA\\Text\\Migration\\Version070000Date20250925110024' => __DIR__ . '/..' . '/../lib/Migration/Version070000Date20250925110024.php', 'OCA\\Text\\Migration\\Version080000Date20260331132113' => __DIR__ . '/..' . '/../lib/Migration/Version080000Date20260331132113.php', 'OCA\\Text\\Migration\\Version080000Date20261020120000' => __DIR__ . '/..' . '/../lib/Migration/Version080000Date20261020120000.php', + 'OCA\\Text\\Migration\\Version100000Date20261011000000' => __DIR__ . '/..' . '/../lib/Migration/Version100000Date20261011000000.php', 'OCA\\Text\\Notification\\Notifier' => __DIR__ . '/..' . '/../lib/Notification/Notifier.php', 'OCA\\Text\\Service\\AiTagService' => __DIR__ . '/..' . '/../lib/Service/AiTagService.php', 'OCA\\Text\\Service\\ApiService' => __DIR__ . '/..' . '/../lib/Service/ApiService.php', diff --git a/lib/Cron/Cleanup.php b/lib/Cron/Cleanup.php index 46f19377bc3..5ed120a7773 100644 --- a/lib/Cron/Cleanup.php +++ b/lib/Cron/Cleanup.php @@ -17,6 +17,7 @@ use OCP\AppFramework\Utility\ITimeFactory; use OCP\BackgroundJob\TimedJob; use OCP\Files\ISetupManager; +use OCP\Files\NotPermittedException; use Psr\Log\LoggerInterface; class Cleanup extends TimedJob { @@ -37,9 +38,19 @@ public function __construct( */ protected function run($argument): void { $this->logger->debug('Run cleanup job for text documents'); - foreach ($this->documentService->getAllWithNoActiveSession() as $document) { + foreach ($this->documentService->getAllPendingAttachmentCleanup() as $document) { + if ($this->documentService->hasUnsavedChanges($document)) { + $this->documentService->markAttachmentCleanup($document); + continue; + } $this->setupManager->tearDown(); - $this->attachmentService->cleanupAttachments($document->getId()); + try { + $this->attachmentService->cleanupAttachments($document->getId()); + } catch (NotPermittedException $e) { + // Retrying will not help until the document is saved again, so mark it as cleaned up anyway + $this->logger->warning('Could not clean up attachments of file ' . $document->getId() . ', deleting is not permitted', ['exception' => $e]); + } + $this->documentService->markAttachmentCleanup($document); } $this->logger->debug('Run cleanup job for text sessions'); diff --git a/lib/Db/Document.php b/lib/Db/Document.php index 5fdcdffa212..778578fe6f3 100644 --- a/lib/Db/Document.php +++ b/lib/Db/Document.php @@ -33,6 +33,7 @@ class Document extends Entity implements \JsonSerializable { protected int $lastSavedVersion = 0; protected int $initialVersion = 0; protected int $lastSavedVersionTime = 0; + protected int $lastAttachmentCleanup = 0; protected string $lastSavedVersionEtag = ''; protected string $baseVersionEtag = ''; protected ?string $checksum = null; @@ -41,10 +42,19 @@ public function __construct() { $this->addType('currentVersion', 'integer'); $this->addType('lastSavedVersion', 'integer'); $this->addType('lastSavedVersionTime', 'integer'); + $this->addType('lastAttachmentCleanup', 'integer'); $this->addType('initialVersion', 'integer'); $this->addType('checksum', 'string'); } + public function getLastAttachmentCleanup(): int { + return $this->lastAttachmentCleanup; + } + + public function setLastAttachmentCleanup(int $time): void { + $this->setter('lastAttachmentCleanup', [$time]); + } + public function jsonSerialize(): array { return [ 'id' => $this->getId(), diff --git a/lib/Db/DocumentMapper.php b/lib/Db/DocumentMapper.php index efde04e80dd..caf52afe5b5 100644 --- a/lib/Db/DocumentMapper.php +++ b/lib/Db/DocumentMapper.php @@ -54,12 +54,13 @@ public function findAll(): Generator { } } - public function findAllWithNoActiveSessions(): Generator { + public function findAllPendingAttachmentCleanup(): Generator { $qb = $this->db->getQueryBuilder(); $result = $qb->select('d.*') ->from($this->getTableName(), 'd') ->leftJoin('d', 'text_sessions', 's', $qb->expr()->eq('s.document_id', 'd.id')) ->where($qb->expr()->isNull('s.id')) + ->andWhere($qb->expr()->gt('d.last_saved_version_time', 'd.last_attachment_cleanup')) ->executeQuery(); try { while ($row = $result->fetch()) { diff --git a/lib/Migration/Version100000Date20261011000000.php b/lib/Migration/Version100000Date20261011000000.php new file mode 100644 index 00000000000..792bb1a1f87 --- /dev/null +++ b/lib/Migration/Version100000Date20261011000000.php @@ -0,0 +1,36 @@ +getTable('text_documents'); + if (!$table->hasColumn('last_attachment_cleanup')) { + $table->addColumn('last_attachment_cleanup', Types::BIGINT, [ + 'notnull' => true, + 'default' => 0, + 'unsigned' => true, + ]); + return $schema; + } + + return null; + } +} diff --git a/lib/Service/DocumentService.php b/lib/Service/DocumentService.php index eae2178aebc..0532c291be3 100644 --- a/lib/Service/DocumentService.php +++ b/lib/Service/DocumentService.php @@ -487,8 +487,13 @@ public function getAll(): \Generator { return $this->documentMapper->findAll(); } - public function getAllWithNoActiveSession(): \Generator { - return $this->documentMapper->findAllWithNoActiveSessions(); + public function getAllPendingAttachmentCleanup(): \Generator { + return $this->documentMapper->findAllPendingAttachmentCleanup(); + } + + public function markAttachmentCleanup(Document $document): void { + $document->setLastAttachmentCleanup($document->getLastSavedVersionTime()); + $this->documentMapper->update($document); } /** diff --git a/tests/unit/CleanupTest.php b/tests/unit/CleanupTest.php new file mode 100644 index 00000000000..75005bd35bd --- /dev/null +++ b/tests/unit/CleanupTest.php @@ -0,0 +1,132 @@ +document = Document::fromRow([ + 'id' => 123, + 'last_saved_version_time' => 1000, + ]); + $this->documentService = $this->createMock(DocumentService::class); + $this->rootFolder = $this->createMock(IRootFolder::class); + $this->attachmentService = new AttachmentService( + $this->rootFolder, + $this->createMock(IManager::class), + $this->createMock(IPreview::class), + $this->createMock(IMimeTypeDetector::class), + $this->createMock(IURLGenerator::class), + $this->createMock(IFilenameValidator::class), + $this->createMock(IFilesMetadataManager::class), + $this->createMock(ISession::class), + ); + $this->sessionService = $this->createMock(SessionService::class); + $this->setupManager = $this->createMock(ISetupManager::class); + $this->logger = $this->createMock(LoggerInterface::class); + $this->documentService->expects(self::never())->method('resetDocument'); + $this->documentService->expects(self::once()) + ->method('getAllPendingAttachmentCleanup') + ->willReturn((function (): \Generator { + yield $this->document; + })()); + $this->cleanup = new class($this->createMock(ITimeFactory::class), $this->sessionService, $this->documentService, $this->attachmentService, $this->logger, $this->setupManager, ) extends Cleanup { + public function run($argument): void { + parent::run($argument); + } + }; + } + + public function testMarksCleanupOnlyAfterAttachmentsAreCleaned(): void { + $this->documentService->expects(self::once()) + ->method('hasUnsavedChanges')->with($this->document)->willReturn(false); + $this->setupManager->expects(self::once())->method('tearDown'); + $cleaned = false; + $this->rootFolder->expects(self::once()) + ->method('getFirstNodeById')->with(123) + ->willReturnCallback(function () use (&$cleaned): ?Node { + $cleaned = true; + return null; + }); + $this->documentService->expects(self::once()) + ->method('markAttachmentCleanup')->with($this->document) + ->willReturnCallback(function () use (&$cleaned): void { + self::assertTrue($cleaned); + }); + $this->sessionService->expects(self::once())->method('removeInactiveSessionsWithoutSteps'); + $this->sessionService->expects(self::once())->method('removeOldSessions'); + $this->sessionService->expects(self::once())->method('removeOrphanedSteps'); + $this->documentService->expects(self::once())->method('cleanupOldDocumentsFolders'); + + $this->cleanup->run([]); + } + + public function testMarksUnsavedChangesWithoutCleanup(): void { + $this->documentService->expects(self::once()) + ->method('hasUnsavedChanges')->with($this->document)->willReturn(true); + $this->setupManager->expects(self::never())->method('tearDown'); + $this->rootFolder->expects(self::never())->method('getFirstNodeById'); + $this->documentService->expects(self::once())->method('markAttachmentCleanup')->with($this->document); + + $this->cleanup->run([]); + } + + public function testFailedCleanupRemainsPending(): void { + $this->documentService->method('hasUnsavedChanges')->willReturn(false); + $this->rootFolder->expects(self::once()) + ->method('getFirstNodeById')->with(123) + ->willThrowException(new \RuntimeException('Cleanup failed')); + $this->documentService->expects(self::never())->method('markAttachmentCleanup'); + $this->expectException(\RuntimeException::class); + $this->expectExceptionMessage('Cleanup failed'); + + $this->cleanup->run([]); + } + + public function testNotPermittedCleanupIsMarkedAndJobContinues(): void { + $this->documentService->method('hasUnsavedChanges')->willReturn(false); + $this->rootFolder->expects(self::once()) + ->method('getFirstNodeById')->with(123) + ->willThrowException(new NotPermittedException()); + $this->logger->expects(self::once())->method('warning'); + $this->documentService->expects(self::once())->method('markAttachmentCleanup')->with($this->document); + $this->sessionService->expects(self::once())->method('removeInactiveSessionsWithoutSteps'); + $this->sessionService->expects(self::once())->method('removeOldSessions'); + $this->sessionService->expects(self::once())->method('removeOrphanedSteps'); + $this->documentService->expects(self::once())->method('cleanupOldDocumentsFolders'); + + $this->cleanup->run([]); + } +} diff --git a/tests/unit/Db/DocumentMapperTest.php b/tests/unit/Db/DocumentMapperTest.php new file mode 100644 index 00000000000..a18775f2ed7 --- /dev/null +++ b/tests/unit/Db/DocumentMapperTest.php @@ -0,0 +1,78 @@ +documentMapper = Server::get(DocumentMapper::class); + $this->sessionMapper = Server::get(SessionMapper::class); + $this->document = $this->documentMapper->insert(Document::fromParams([ + 'id' => 900001, + 'lastSavedVersionTime' => 1000, + 'lastSavedVersionEtag' => 'saved-etag', + 'baseVersionEtag' => 'base-etag', + ])); + } + + public function tearDown(): void { + $this->sessionMapper->deleteByDocumentId($this->document->getId()); + $this->documentMapper->delete($this->document); + parent::tearDown(); + } + + private function pendingDocumentIds(): array { + return array_map( + static fn (Document $document): int => $document->getId(), + iterator_to_array($this->documentMapper->findAllPendingAttachmentCleanup()), + ); + } + + public function testUnprocessedDocumentsArePending(): void { + self::assertSame(0, $this->documentMapper->find($this->document->getId())->getLastAttachmentCleanup()); + self::assertContains($this->document->getId(), $this->pendingDocumentIds()); + } + + public function testDocumentsWithSessionsAreExcluded(): void { + $session = $this->sessionMapper->insert(Session::fromParams([ + 'documentId' => $this->document->getId(), + 'userId' => 'admin', + 'token' => uniqid(), + 'lastContact' => time(), + ])); + self::assertNotContains($this->document->getId(), $this->pendingDocumentIds()); + $this->sessionMapper->delete($session); + self::assertContains($this->document->getId(), $this->pendingDocumentIds()); + } + + public function testCleanedDocumentsAreExcludedUntilSavedAgain(): void { + $this->document->setLastAttachmentCleanup(1000); + $this->documentMapper->update($this->document); + self::assertNotContains($this->document->getId(), $this->pendingDocumentIds()); + + $this->document->setLastSavedVersionTime(1001); + $this->documentMapper->update($this->document); + self::assertContains($this->document->getId(), $this->pendingDocumentIds()); + } + + public function testOlderSavedTimestampsAreExcluded(): void { + $this->document->setLastAttachmentCleanup(1001); + $this->documentMapper->update($this->document); + + self::assertNotContains($this->document->getId(), $this->pendingDocumentIds()); + } +} diff --git a/tests/unit/Service/DocumentServiceTest.php b/tests/unit/Service/DocumentServiceTest.php index dea1ddab808..0a317b0d3f8 100644 --- a/tests/unit/Service/DocumentServiceTest.php +++ b/tests/unit/Service/DocumentServiceTest.php @@ -2,6 +2,7 @@ namespace OCA\Text\Tests; +use OCA\Text\Db\Document; use OCA\Text\Db\DocumentMapper; use OCA\Text\Db\Session; use OCA\Text\Db\SessionMapper; @@ -73,6 +74,63 @@ public function setUp(): void { ); } + private function createDocument(): Document { + return Document::fromRow([ + 'id' => 1, + 'last_saved_version' => 3, + 'last_saved_version_time' => 1000, + ]); + } + + public function testGetAllPendingAttachmentCleanup(): void { + $document = $this->createDocument(); + $documents = (static function () use ($document): \Generator { + yield $document; + })(); + $this->documentMapper->expects(self::once()) + ->method('findAllPendingAttachmentCleanup')->willReturn($documents); + + self::assertSame($documents, $this->documentService->getAllPendingAttachmentCleanup()); + } + + public function testMarksOnlyTheProcessedSavedTimestamp(): void { + $document = $this->createDocument(); + $document->resetUpdatedFields(); + $this->documentMapper->expects(self::once()) + ->method('update')->with($document)->willReturn($document); + + $this->documentService->markAttachmentCleanup($document); + + self::assertSame(1000, $document->getLastAttachmentCleanup()); + self::assertSame(1000, $document->getLastSavedVersionTime()); + self::assertSame(['lastAttachmentCleanup' => true], $document->getUpdatedFields()); + } + + public function testDetectsUnsavedChanges(): void { + $document = $this->createDocument(); + $this->setpMapper->expects(self::once()) + ->method('getLatestVersion')->with(1)->willReturn(4); + + self::assertTrue($this->documentService->hasUnsavedChanges($document)); + } + + public function testSavedStepsHaveNoUnsavedChanges(): void { + $document = $this->createDocument(); + $this->setpMapper->expects(self::once()) + ->method('getLatestVersion')->with(1)->willReturn(3); + + self::assertFalse($this->documentService->hasUnsavedChanges($document)); + } + + public function testDocumentWithoutStepsHasNoUnsavedChanges(): void { + $document = $this->createDocument(); + $document->setLastSavedVersion(0); + $this->setpMapper->expects(self::once()) + ->method('getLatestVersion')->with(1)->willReturn(null); + + self::assertFalse($this->documentService->hasUnsavedChanges($document)); + } + public function testGetFileById() { $userFolder = $this->createMock(Folder::class); $this->rootFolder->method('getUserFolder')->willReturn($userFolder);