diff --git a/appinfo/info.xml b/appinfo/info.xml index d7ee640a300..13ec4dea494 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -1,6 +1,6 @@ - 10.0.0-dev.1 + 10.0.0-dev.2 agpl Julius Hรคrtl Text diff --git a/composer/composer/autoload_classmap.php b/composer/composer/autoload_classmap.php index 257e456ff37..38c8396e024 100644 --- a/composer/composer/autoload_classmap.php +++ b/composer/composer/autoload_classmap.php @@ -83,6 +83,7 @@ 'OCA\\Text\\Migration\\Version090000Date20260817110024' => $baseDir . '/../lib/Migration/Version090000Date20260817110024.php', 'OCA\\Text\\Migration\\Version090000Date20260819110024' => $baseDir . '/../lib/Migration/Version090000Date20260819110024.php', 'OCA\\Text\\Migration\\Version090000Date20260820132113' => $baseDir . '/../lib/Migration/Version090000Date20260820132113.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 fbcf5b3e25d..64514d97563 100644 --- a/composer/composer/autoload_static.php +++ b/composer/composer/autoload_static.php @@ -98,6 +98,7 @@ class ComposerStaticInitText 'OCA\\Text\\Migration\\Version090000Date20260817110024' => __DIR__ . '/..' . '/../lib/Migration/Version090000Date20260817110024.php', 'OCA\\Text\\Migration\\Version090000Date20260819110024' => __DIR__ . '/..' . '/../lib/Migration/Version090000Date20260819110024.php', 'OCA\\Text\\Migration\\Version090000Date20260820132113' => __DIR__ . '/..' . '/../lib/Migration/Version090000Date20260820132113.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 3aa6ee37743..9df5549d3f4 100644 --- a/lib/Cron/Cleanup.php +++ b/lib/Cron/Cleanup.php @@ -35,12 +35,14 @@ public function __construct( */ protected function run($argument): void { $this->logger->debug('Run cleanup job for text documents'); - foreach ($this->documentService->getAllWithNoActiveSession() as $document) { - if ($document->getContextType() !== 'file') { + foreach ($this->documentService->getAllPendingAttachmentCleanup() as $document) { + if ($this->documentService->hasUnsavedChanges($document)) { + $this->documentService->markAttachmentCleanup($document); continue; } $this->setupManager->tearDown(); $this->attachmentService->cleanupAttachments($document->getContextId()); + $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 2e481cf36d4..c6f59c8fd07 100644 --- a/lib/Db/Document.php +++ b/lib/Db/Document.php @@ -42,6 +42,7 @@ class Document extends SnowflakeAwareEntity 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; @@ -53,12 +54,21 @@ public function __construct() { $this->addType('currentVersion', Types::INTEGER); $this->addType('lastSavedVersion', Types::INTEGER); $this->addType('lastSavedVersionTime', Types::INTEGER); + $this->addType('lastAttachmentCleanup', Types::BIGINT); $this->addType('initialVersion', Types::INTEGER); $this->addType('checksum', Types::STRING); $this->addType('contextType', Types::STRING); $this->addType('contextId', Types::INTEGER); } + public function getLastAttachmentCleanup(): int { + return $this->lastAttachmentCleanup; + } + + public function setLastAttachmentCleanup(int $time): void { + $this->setter('lastAttachmentCleanup', [$time]); + } + public function jsonSerialize(): array { return [ 'id' => $this->id, diff --git a/lib/Db/DocumentMapper.php b/lib/Db/DocumentMapper.php index 825958df6d9..ab6f720ac37 100644 --- a/lib/Db/DocumentMapper.php +++ b/lib/Db/DocumentMapper.php @@ -72,12 +72,14 @@ 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')) + ->andWhere($qb->expr()->eq('d.context_type', $qb->createNamedParameter('file'))) ->executeQuery(); try { while ($row = $result->fetchAssociative()) { 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 d3a950d1db1..969d6bcd6f0 100644 --- a/lib/Service/DocumentService.php +++ b/lib/Service/DocumentService.php @@ -462,9 +462,7 @@ public function resetDocument(string $contextType, int $contextId, bool $force = return; } - $stepsVersion = $this->stepMapper->getLatestVersion($document->id) ?: 0; - $docVersion = $document->getLastSavedVersion(); - if (!$force && $stepsVersion !== $docVersion) { + if (!$force && $this->hasUnsavedChanges($document)) { $this->logger->debug('Did not reset document with unsaved changes for ' . $contextString); throw new DocumentHasUnsavedChangesException('Did not reset document, as it has unsaved changes'); } @@ -486,8 +484,20 @@ 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); + } + + public function hasUnsavedChanges(Document $document): bool { + if ($document->id === null) { + throw new RuntimeException('Document needs an id to check unsaved changes'); + } + return ($this->stepMapper->getLatestVersion($document->id) ?? 0) !== $document->getLastSavedVersion(); } private function ensureDocumentsFolder(): bool { diff --git a/tests/unit/CleanupTest.php b/tests/unit/CleanupTest.php new file mode 100644 index 00000000000..0c52542ac5d --- /dev/null +++ b/tests/unit/CleanupTest.php @@ -0,0 +1,118 @@ +document = Document::fromRow([ + 'id' => '123456789012345678', + 'context_type' => 'file', + 'context_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->createMock(FileService::class), + ); + $this->sessionService = $this->createMock(SessionService::class); + $this->setupManager = $this->createMock(ISetupManager::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->createMock(LoggerInterface::class), $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([]); + } +} diff --git a/tests/unit/Db/DocumentMapperTest.php b/tests/unit/Db/DocumentMapperTest.php new file mode 100644 index 00000000000..6f87ddfe2cb --- /dev/null +++ b/tests/unit/Db/DocumentMapperTest.php @@ -0,0 +1,79 @@ +documentMapper = Server::get(DocumentMapper::class); + $this->sessionMapper = Server::get(SessionMapper::class); + $this->document = $this->documentMapper->insert(Document::fromParams([ + 'contextType' => 'file', + 'contextId' => 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): string => $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 e06b2d0196e..64087365f0f 100644 --- a/tests/unit/Service/DocumentServiceTest.php +++ b/tests/unit/Service/DocumentServiceTest.php @@ -27,10 +27,12 @@ class DocumentServiceTest extends \PHPUnit\Framework\TestCase { private DocumentService $documentService; private DocumentMapper $documentMapper; + private StepMapper $stepMapper; private ICache $cache; public function setUp(): void { $this->documentMapper = $this->createMock(DocumentMapper::class); + $this->stepMapper = $this->createMock(StepMapper::class); $this->cache = $this->createMock(ICache::class); $cacheFactory = $this->createMock(ICacheFactory::class); $cacheFactory->method('createDistributed')->willReturn($this->cache); @@ -40,7 +42,7 @@ public function setUp(): void { $this->documentService = new DocumentService( $this->createMock(ContextManager::class), $this->documentMapper, - $this->createMock(StepMapper::class), + $this->stepMapper, $this->createMock(SessionMapper::class), $this->createMock(IAppData::class), 'admin', @@ -62,6 +64,64 @@ private function createDocument(string $etag, int $mtime, string $content): Docu return $document; } + public function testGetAllPendingAttachmentCleanup(): void { + $document = $this->createDocument('etag1', 1000, 'content'); + $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('etag1', 1000, 'content'); + $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 testDetectsUnsavedChangesWithStringDocumentId(): void { + $document = $this->createDocument('etag1', 1000, 'content'); + $document->setLastSavedVersion(3); + $this->stepMapper->expects(self::once()) + ->method('getLatestVersion')->with($document->id)->willReturn(4); + + self::assertTrue($this->documentService->hasUnsavedChanges($document)); + } + + public function testSavedStepsHaveNoUnsavedChanges(): void { + $document = $this->createDocument('etag1', 1000, 'content'); + $document->setLastSavedVersion(3); + $this->stepMapper->expects(self::once()) + ->method('getLatestVersion')->with($document->id)->willReturn(3); + + self::assertFalse($this->documentService->hasUnsavedChanges($document)); + } + + public function testDocumentWithoutStepsHasNoUnsavedChanges(): void { + $document = $this->createDocument('etag1', 1000, 'content'); + $this->stepMapper->expects(self::once()) + ->method('getLatestVersion')->with($document->id)->willReturn(null); + + self::assertFalse($this->documentService->hasUnsavedChanges($document)); + } + + public function testCannotCheckUnsavedChangesWithoutDocumentId(): void { + $this->stepMapper->expects(self::never())->method('getLatestVersion'); + $this->expectException(\RuntimeException::class); + $this->expectExceptionMessage('Document needs an id to check unsaved changes'); + + $this->documentService->hasUnsavedChanges(new Document()); + } + public function testNoAutosavingWhileSaveIsUnderWay(): void { $document = $this->createDocument('etag1', 1000, 'new content'); $context = $this->createMock(FileContext::class);