From 14d201ea9e218cea5ee4349c36f80e5b9747d7ca Mon Sep 17 00:00:00 2001 From: Max Date: Tue, 18 Aug 2026 17:11:26 +0200 Subject: [PATCH 01/30] chore(cleanup): error handling * `checkSharePermissions` has not been throwing `NotPermittedException` for a while. See https://github.com/nextcloud/text/pull/3765 . * `InvalidArgumentException` was not being handled. * throw `NotFoundException` if file cannot be found and there is no share token. Signed-off-by: Max --- lib/Controller/PublicSessionController.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/Controller/PublicSessionController.php b/lib/Controller/PublicSessionController.php index b12fa1e5f42..8bb40f5e4f3 100644 --- a/lib/Controller/PublicSessionController.php +++ b/lib/Controller/PublicSessionController.php @@ -77,7 +77,8 @@ public function create(string $token, ?string $filePath = null, ?string $baseVer * If not then well 404 it is. */ try { - $this->fileService->checkSharePermissions($token); + $context = $this->fileContextFactory->buildForShareWithPath($token, $filePath); + return $this->apiService->create($context, $baseVersionEtag, $guestName); } catch (NotFoundException|\InvalidArgumentException) { return new DataResponse([], Http::STATUS_NOT_FOUND); } From d20af4fef3ea137930525ca51e759e8af953c077 Mon Sep 17 00:00:00 2001 From: Max Date: Wed, 12 Aug 2026 23:24:34 +0200 Subject: [PATCH 02/30] chore(refactor): Introduce IContext and FileContext Use it for creating the session for now. Signed-off-by: Max --- lib/Context/FileContext.php | 98 ++++++++++++++++++++++ lib/Context/FileContextFactory.php | 89 ++++++++++++++++++++ lib/Context/IContext.php | 26 ++++++ lib/Controller/PublicSessionController.php | 12 +-- lib/Controller/SessionController.php | 9 +- lib/DirectEditing/TextDirectEditor.php | 5 +- lib/Service/ApiService.php | 52 +++++------- lib/Service/DocumentService.php | 19 ++--- lib/Service/FileService.php | 17 ++++ 9 files changed, 270 insertions(+), 57 deletions(-) create mode 100644 lib/Context/FileContext.php create mode 100644 lib/Context/FileContextFactory.php create mode 100644 lib/Context/IContext.php diff --git a/lib/Context/FileContext.php b/lib/Context/FileContext.php new file mode 100644 index 00000000000..5e4bdd85fce --- /dev/null +++ b/lib/Context/FileContext.php @@ -0,0 +1,98 @@ +fileService->isDownloadDisabled($this->file)) { + return $this->l10n->t('This file cannot be displayed as download is disabled by the share'); + } + return null; + } + + public function checkDocument(Document $document): ?string { + if ($this->baseVersionEtag !== null && $this->baseVersionEtag !== $document->getBaseVersionEtag()) { + return $this->l10n->t('Editing session has expired. Please reload the page.'); + } + return null; + } + + public function isReadOnly(): bool { + return $this->fileService->isReadOnly($this->file, $this->token); + } + + public function getId(): int { + return $this->file->getId(); + } + + public function getType(): string { + return 'file'; + } + + public function toString(): string { + return $this->getType() . ' (' . $this->getId() . ')'; + } + + public function loadContent(): ?string { + return $this->fileService->loadContent($this->file); + } + + public function getLockInfo(): ?ILock { + return $this->lockService->getLockByOthers($this->file); + } + + public function getOwner(): ?IUser { + return $this->file->getOwner(); + } + + public function lock(): bool { + // Disable file locking for Readme.md files, because in the + // current setup, this makes it almost impossible to delete these files. + if (strcasecmp($this->file->getName(), 'Readme.md') !== 0) { + return $this->lockService->lock($this->file); + } + return true; + } + + public function createDocument(): Document { + $document = new Document(); + $document->setId($this->getId()); + $document->setLastSavedVersion(0); + $document->setLastSavedVersionTime($this->file->getMTime()); + $document->setLastSavedVersionEtag($this->file->getEtag()); + $document->setChecksum($this->computeChecksum()); + // This is a new document - so it needs a fresh base version etag. + $document->setBaseVersionEtag(uniqid()); + return $document; + } + + public function computeCheckSum(): string { + return hash('crc32', $this->file->getContent()); + } + +} diff --git a/lib/Context/FileContextFactory.php b/lib/Context/FileContextFactory.php new file mode 100644 index 00000000000..fae357c4613 --- /dev/null +++ b/lib/Context/FileContextFactory.php @@ -0,0 +1,89 @@ +userSession->getUser()?->getUID(); + if ($userId === null) { + throw new NotPermittedException(); + } + $file = $this->fileService->getFileById($id, $userId); + return new FileContext( + $this->fileService, + $this->l10n, + $this->lockService, + $file, + $baseVersionEtag, + ); + } + + /** + * @throws NotFoundException if the file cannot be found + * @throws \InvalidArgumentException if the share token is for a folder and path is missing + */ + public function buildForShareWithPath( + string $token, + ?string $filePath, + ?string $baseVersionEtag, + ): FileContext { + $file = $this->fileService->getFileByShareToken($token, $filePath); + /* + * Check if we have proper read access (files drop) + * If not then well 404 it is. + */ + $this->fileService->checkSharePermissions($token); + return new FileContext( + $this->fileService, + $this->l10n, + $this->lockService, + $file, + $baseVersionEtag, + $token, + ); + } + + /** + * @throws NotFoundException if the file cannot be found + */ + public function buildForDirectEditing(IToken $token): FileContext { + $file = $token->getFile(); + return new FileContext( + $this->fileService, + $this->l10n, + $this->lockService, + $file, + null, + ); + } + +} diff --git a/lib/Context/IContext.php b/lib/Context/IContext.php new file mode 100644 index 00000000000..7d953f7606c --- /dev/null +++ b/lib/Context/IContext.php @@ -0,0 +1,26 @@ +fileService->getFileByShareToken($token, $filePath); - /* - * Check if we have proper read access (files drop) - * If not then well 404 it is. - */ try { - $context = $this->fileContextFactory->buildForShareWithPath($token, $filePath); - return $this->apiService->create($context, $baseVersionEtag, $guestName); + $context = $this->fileContextFactory->buildForShareWithPath($token, $filePath, $baseVersionEtag); + return $this->apiService->create($context, $guestName); } catch (NotFoundException|\InvalidArgumentException) { return new DataResponse([], Http::STATUS_NOT_FOUND); } - return $this->apiService->create($file, $baseVersionEtag, $token, $guestName); } #[NoAdminRequired] diff --git a/lib/Controller/SessionController.php b/lib/Controller/SessionController.php index e58c20f6b0e..65625e63a24 100644 --- a/lib/Controller/SessionController.php +++ b/lib/Controller/SessionController.php @@ -8,6 +8,7 @@ namespace OCA\Text\Controller; +use OCA\Text\Context\FileContextFactory; use OCA\Text\Exception\InvalidSessionException; use OCA\Text\Middleware\Attribute\RequireDocumentBaseVersionEtag; use OCA\Text\Middleware\Attribute\RequireDocumentSession; @@ -40,6 +41,7 @@ public function __construct( string $appName, IRequest $request, private ApiService $apiService, + private FileContextFactory $fileContextFactory, private FileService $fileService, private SessionService $sessionService, private NotificationService $notificationService, @@ -53,13 +55,12 @@ public function __construct( #[NoAdminRequired] public function create(?int $fileId = null, ?string $baseVersionEtag = null): DataResponse { - $userId = $this->userSession->getUser()?->getUID(); - if ($fileId === null || $userId === null) { + if ($fileId === null) { return new DataResponse(['error' => 'No valid file argument provided'], Http::STATUS_PRECONDITION_FAILED); } try { - $file = $this->fileService->getFileById($fileId, $userId); + $context = $this->fileContextFactory->buildForId($fileId, $baseVersionEtag); } catch (NotFoundException|NotPermittedException $e) { $this->logger->error('No permission to access this file', [ 'exception' => $e ]); return new DataResponse([ @@ -67,7 +68,7 @@ public function create(?int $fileId = null, ?string $baseVersionEtag = null): Da ], Http::STATUS_NOT_FOUND); } - return $this->apiService->create($file, $baseVersionEtag); + return $this->apiService->create($context); } #[NoAdminRequired] diff --git a/lib/DirectEditing/TextDirectEditor.php b/lib/DirectEditing/TextDirectEditor.php index 9725755cc0b..63a213d9ed6 100644 --- a/lib/DirectEditing/TextDirectEditor.php +++ b/lib/DirectEditing/TextDirectEditor.php @@ -8,6 +8,7 @@ namespace OCA\Text\DirectEditing; use OCA\Text\AppInfo\Application; +use OCA\Text\Context\FileContextFactory; use OCA\Text\Service\ApiService; use OCA\Text\Service\InitialStateProvider; use OCP\AppFramework\Http\NotFoundResponse; @@ -29,6 +30,7 @@ public function __construct( private readonly InitialStateProvider $initialStateProvider, private readonly ApiService $apiService, private readonly IAppConfig $appConfig, + private readonly FileContextFactory $fileContextFactory, ) { } @@ -131,7 +133,8 @@ public function isSecure(): bool { public function open(IToken $token): Response { $token->useTokenScope(); try { - $session = $this->apiService->create($token->getFile()); + $context = $this->fileContextFactory->buildForDirectEditing($token); + $session = $this->apiService->create($context); $this->initialStateProvider->provideFile([ 'fileId' => $token->getFile()->getId(), 'mimetype' => $token->getFile()->getMimeType(), diff --git a/lib/Service/ApiService.php b/lib/Service/ApiService.php index 015bcccfd1e..6e42c8c75a7 100644 --- a/lib/Service/ApiService.php +++ b/lib/Service/ApiService.php @@ -12,6 +12,7 @@ use Exception; use InvalidArgumentException; use OCA\NotifyPush\Queue\IQueue; +use OCA\Text\Context\IContext; use OCA\Text\Db\Document; use OCA\Text\Db\Session; use OCA\Text\Exception\DocumentSaveConflictException; @@ -32,7 +33,6 @@ public function __construct( private readonly SessionService $sessionService, private readonly DocumentService $documentService, private readonly FileService $fileService, - private readonly EncodingService $encodingService, private readonly LoggerInterface $logger, private readonly LockService $lockService, private readonly IL10N $l10n, @@ -40,19 +40,20 @@ public function __construct( ) { } - public function create(File $file, ?string $baseVersionEtag = null, ?string $token = null, ?string $guestName = null): DataResponse { + public function create(IContext $context, ?string $guestName = null): DataResponse { try { - // Block using text for disabled download internal shares - if ($this->fileService->isDownloadDisabled($file)) { - return new DataResponse(['error' => $this->l10n->t('This file cannot be displayed as download is disabled by the share')], Http::STATUS_FORBIDDEN); + $error = $context->check(); + if ($error !== null) { + return new DataResponse(['error' => $error], Http::STATUS_FORBIDDEN); } - $readOnly = $this->fileService->isReadOnly($file, $token); + $readOnly = $context->isReadOnly(); - $this->sessionService->removeInactiveSessionsWithoutSteps($file->getId()); - $document = $this->documentService->getOrCreateDocument($file); - if ($baseVersionEtag !== null && $baseVersionEtag !== $document->getBaseVersionEtag()) { - return new DataResponse(['error' => $this->l10n->t('Editing session has expired. Please reload the page.')], Http::STATUS_PRECONDITION_FAILED); + $document = $this->documentService->getOrCreateDocument($context); + $this->sessionService->removeInactiveSessionsWithoutSteps($document->getId()); + $error = $context->checkDocument($document); + if ($error !== null) { + return new DataResponse(['error' => $error], Http::STATUS_PRECONDITION_FAILED); } } catch (Exception $e) { @@ -67,30 +68,30 @@ public function create(File $file, ?string $baseVersionEtag = null, ?string $tok $documentState = null; $content = null; if ($document->getLastSavedVersion() === 0) { - $this->logger->debug('Sending content for unsaved file ' . $file->getId()); - $content = $this->loadContent($file); + $this->logger->debug('Sending content for unsaved ' . $context->toString()); + $content = $context->loadContent(); } else { - $this->logger->debug('Loading saved document state for ' . $file->getId()); + $this->logger->debug('Loading saved document state for ' . $context->toString()); try { $stateFile = $this->documentService->getStateFile($document->getId()); $documentState = $stateFile->getContent(); } catch (NotFoundException) { - $this->logger->warning('State file not found for saved document' . $file->getId()); + $this->logger->warning('State file not found for saved document' . $context->toString()); // If we have no state file we need to load the content from the file // On the client side we use this to initialize a idempotent initial y.js document - $content = $this->loadContent($file); + $content = $context->loadContent(); } } - $lockInfo = $this->lockService->getLockByOthers($file); + $lockInfo = $context->getLockInfo(); - $hasOwner = $file->getOwner() !== null; + $hasOwner = $context->getOwner() !== null; // Disable file locking for Readme.md files, because in the // current setup, this makes it almost impossible to delete these files. - if (!$readOnly && strcasecmp($file->getName(), 'Readme.md') !== 0) { - $isLocked = $this->lockService->lock($file); + if (!$readOnly) { + $isLocked = $context->lock(); if (!$isLocked) { $readOnly = true; } @@ -242,17 +243,4 @@ public function updateSession(Session $session, string $guestName): DataResponse return new DataResponse($this->sessionService->updateSession($session, $guestName)); } - private function loadContent(\OCP\Files\File $file): ?string { - try { - $content = $file->getContent(); - $content = $this->encodingService->encodeToUtf8($content); - if ($content === null) { - $this->logger->warning('Failed to encode file to UTF8. File ID: ' . $file->getId()); - } - } catch (NotFoundException $e) { - $this->logger->warning($e->getMessage(), ['exception' => $e]); - $content = null; - } - return $content; - } } diff --git a/lib/Service/DocumentService.php b/lib/Service/DocumentService.php index d4bcdf01747..c0f3f82d6c1 100644 --- a/lib/Service/DocumentService.php +++ b/lib/Service/DocumentService.php @@ -10,6 +10,7 @@ namespace OCA\Text\Service; use InvalidArgumentException; +use OCA\Text\Context\IContext; use OCA\Text\Db\Document; use OCA\Text\Db\DocumentMapper; use OCA\Text\Db\Session; @@ -96,10 +97,10 @@ public function isSaveFromText(): bool { * @throws NotPermittedException * @throws Exception */ - public function getOrCreateDocument(File $file): Document { - $document = $this->getDocument($file->getId()); + public function getOrCreateDocument(IContext $context): Document { + $document = $this->getDocument($context->getId()); if ($document !== null) { - $this->logger->info('Keep previous document of ' . $file->getId()); + $this->logger->info('Keep previous document of ' . $context->toString()); return $document; } @@ -107,14 +108,8 @@ public function getOrCreateDocument(File $file): Document { throw new NotFoundException('No app data folder present for text documents'); } - $this->logger->info('Create new document of ' . $file->getId()); - $document = new Document(); - $document->setId($file->getId()); - $document->setLastSavedVersion(0); - $document->setLastSavedVersionTime($file->getMTime()); - $document->setLastSavedVersionEtag($file->getEtag()); - $document->setBaseVersionEtag(uniqid()); - $document->setChecksum(self::computeCheckSum($file->getContent())); + $this->logger->info('Create new document of ' . $context->toString()); + $document = $context->createDocument(); try { /** @var Document $document */ $document = $this->documentMapper->insert($document); @@ -124,7 +119,7 @@ public function getOrCreateDocument(File $file): Document { throw $e; } // Document might have been created in the meantime - $document = $this->getDocument($file->getId()); + $document = $this->getDocument($context->getId()); if ($document === null) { throw $e; } diff --git a/lib/Service/FileService.php b/lib/Service/FileService.php index 471a6770783..e05a8643a3a 100644 --- a/lib/Service/FileService.php +++ b/lib/Service/FileService.php @@ -21,13 +21,16 @@ use OCP\Share\Exceptions\ShareNotFound; use OCP\Share\IManager as ShareManager; use OCP\Share\IShare; +use Psr\Log\LoggerInterface; class FileService { public function __construct( + private readonly EncodingService $encodingService, private readonly ISession $session, private readonly IRootFolder $rootFolder, private readonly LockService $lockService, + private readonly LoggerInterface $logger, private readonly ShareManager $shareManager, ) { } @@ -237,4 +240,18 @@ public function getDocumentIdForUser(int $fileId, string $userId): int { throw new InvalidSessionException(); } + public function loadContent(File $file): ?string { + try { + $content = $file->getContent(); + $content = $this->encodingService->encodeToUtf8($content); + if ($content === null) { + $this->logger->warning('Failed to encode file to UTF8. File ID: ' . $file->getId()); + } + } catch (NotFoundException $e) { + $this->logger->warning($e->getMessage(), ['exception' => $e]); + $content = null; + } + return $content; + } + } From 405dbcdc44cc122b38085b5c55cc30233685dd09 Mon Sep 17 00:00:00 2001 From: Max Date: Sat, 15 Aug 2026 13:50:48 +0200 Subject: [PATCH 03/30] chore(refactor): simplify IContext API with helper classes For some reason `$document->getId` is suspected to return void by vscode. `$document->id` is public. So use that instead. Signed-off-by: Max --- composer/composer/autoload_classmap.php | 3 + composer/composer/autoload_static.php | 3 + lib/Context/FileContext.php | 96 ++++++++++++++++--------- lib/Context/FileContextFactory.php | 44 ++++++------ lib/Context/IContext.php | 69 +++++++++++++++--- lib/Db/Document.php | 7 ++ lib/Db/DocumentMapper.php | 9 +++ lib/Service/ApiService.php | 80 +++++++-------------- lib/Service/DocumentService.php | 41 ++++++++--- tests/unit/Service/ApiServiceTest.php | 60 +++++++++------- tests/unit/Service/FileServiceTest.php | 4 ++ 11 files changed, 259 insertions(+), 157 deletions(-) diff --git a/composer/composer/autoload_classmap.php b/composer/composer/autoload_classmap.php index 29e480fbcd4..ad130420ebc 100644 --- a/composer/composer/autoload_classmap.php +++ b/composer/composer/autoload_classmap.php @@ -10,6 +10,9 @@ 'OCA\\Text\\AppInfo\\Application' => $baseDir . '/../lib/AppInfo/Application.php', 'OCA\\Text\\Command\\ResetDocument' => $baseDir . '/../lib/Command/ResetDocument.php', 'OCA\\Text\\ConfigLexicon' => $baseDir . '/../lib/ConfigLexicon.php', + 'OCA\\Text\\Context\\FileContext' => $baseDir . '/../lib/Context/FileContext.php', + 'OCA\\Text\\Context\\FileContextFactory' => $baseDir . '/../lib/Context/FileContextFactory.php', + 'OCA\\Text\\Context\\IContext' => $baseDir . '/../lib/Context/IContext.php', 'OCA\\Text\\Controller\\AiController' => $baseDir . '/../lib/Controller/AiController.php', 'OCA\\Text\\Controller\\AttachmentController' => $baseDir . '/../lib/Controller/AttachmentController.php', 'OCA\\Text\\Controller\\ISessionAwareController' => $baseDir . '/../lib/Controller/ISessionAwareController.php', diff --git a/composer/composer/autoload_static.php b/composer/composer/autoload_static.php index a06a64546ec..2800c11727e 100644 --- a/composer/composer/autoload_static.php +++ b/composer/composer/autoload_static.php @@ -25,6 +25,9 @@ class ComposerStaticInitText 'OCA\\Text\\AppInfo\\Application' => __DIR__ . '/..' . '/../lib/AppInfo/Application.php', 'OCA\\Text\\Command\\ResetDocument' => __DIR__ . '/..' . '/../lib/Command/ResetDocument.php', 'OCA\\Text\\ConfigLexicon' => __DIR__ . '/..' . '/../lib/ConfigLexicon.php', + 'OCA\\Text\\Context\\FileContext' => __DIR__ . '/..' . '/../lib/Context/FileContext.php', + 'OCA\\Text\\Context\\FileContextFactory' => __DIR__ . '/..' . '/../lib/Context/FileContextFactory.php', + 'OCA\\Text\\Context\\IContext' => __DIR__ . '/..' . '/../lib/Context/IContext.php', 'OCA\\Text\\Controller\\AiController' => __DIR__ . '/..' . '/../lib/Controller/AiController.php', 'OCA\\Text\\Controller\\AttachmentController' => __DIR__ . '/..' . '/../lib/Controller/AttachmentController.php', 'OCA\\Text\\Controller\\ISessionAwareController' => __DIR__ . '/..' . '/../lib/Controller/ISessionAwareController.php', diff --git a/lib/Context/FileContext.php b/lib/Context/FileContext.php index 5e4bdd85fce..7dad3a0c347 100644 --- a/lib/Context/FileContext.php +++ b/lib/Context/FileContext.php @@ -14,6 +14,8 @@ use OCP\Files\Lock\ILock; use OCP\IL10N; use OCP\IUser; +use Override; +use Psr\Log\LoggerInterface; class FileContext implements IContext { @@ -21,56 +23,98 @@ public function __construct( private readonly FileService $fileService, private readonly IL10N $l10n, private readonly LockService $lockService, + private readonly LoggerInterface $logger, private readonly File $file, private readonly ?string $baseVersionEtag, private readonly ?string $token = null, ) { } - public function check(): ?string { + #[Override] + public function getId(): int { + return $this->file->getId(); + } + + #[Override] + public function getType(): string { + return 'file'; + } + + #[Override] + public function toString(): string { + return $this->getType() . ' (' . $this->getId() . ')'; + } + + #[Override] + public function buildDocument(): Document|string { // Block using text for disabled download internal shares if ($this->fileService->isDownloadDisabled($this->file)) { return $this->l10n->t('This file cannot be displayed as download is disabled by the share'); } - return null; + $document = new Document(); + $document->setId($this->getId()); + $document->setLastSavedVersion(0); + $document->setLastSavedVersionTime($this->file->getMTime()); + $document->setLastSavedVersionEtag($this->file->getEtag()); + $document->setChecksum($this->computeChecksum()); + // This is a new document - so it needs a fresh base version etag. + $document->setBaseVersionEtag(uniqid()); + return $document; } - public function checkDocument(Document $document): ?string { + #[Override] + public function prepareSession(DocumentData $documentData): SessionInfo|string { + $document = $documentData->document; + $documentState = $documentData->documentState; + if ($this->baseVersionEtag !== null && $this->baseVersionEtag !== $document->getBaseVersionEtag()) { return $this->l10n->t('Editing session has expired. Please reload the page.'); } - return null; - } - public function isReadOnly(): bool { - return $this->fileService->isReadOnly($this->file, $this->token); - } + $content = null; + if ($documentState === null) { + $this->logger->debug('Sending content for ' . $document->toString()); + $content = $this->loadContent(); + } - public function getId(): int { - return $this->file->getId(); + $readOnly = $this->isReadOnly(); + $lockInfo = $this->getLockInfo(); + if (!$readOnly) { + $isLocked = $this->lock(); + if (!$isLocked) { + $readOnly = true; + } + } + + return new SessionInfo( + content: $content, + readOnly: $readOnly, + lock: $lockInfo, + hasOwner: $this->getOwner() !== null, + ); } - public function getType(): string { - return 'file'; + private function computeCheckSum(): string { + return hash('crc32', $this->file->getContent()); } - public function toString(): string { - return $this->getType() . ' (' . $this->getId() . ')'; + private function isReadOnly(): bool { + return $this->fileService->isReadOnly($this->file, $this->token); } - public function loadContent(): ?string { + private function loadContent(): ?string { return $this->fileService->loadContent($this->file); } - public function getLockInfo(): ?ILock { + private function getLockInfo(): ?ILock { return $this->lockService->getLockByOthers($this->file); } - public function getOwner(): ?IUser { + private function getOwner(): ?IUser { return $this->file->getOwner(); } - public function lock(): bool { + private function lock(): bool { // Disable file locking for Readme.md files, because in the // current setup, this makes it almost impossible to delete these files. if (strcasecmp($this->file->getName(), 'Readme.md') !== 0) { @@ -79,20 +123,4 @@ public function lock(): bool { return true; } - public function createDocument(): Document { - $document = new Document(); - $document->setId($this->getId()); - $document->setLastSavedVersion(0); - $document->setLastSavedVersionTime($this->file->getMTime()); - $document->setLastSavedVersionEtag($this->file->getEtag()); - $document->setChecksum($this->computeChecksum()); - // This is a new document - so it needs a fresh base version etag. - $document->setBaseVersionEtag(uniqid()); - return $document; - } - - public function computeCheckSum(): string { - return hash('crc32', $this->file->getContent()); - } - } diff --git a/lib/Context/FileContextFactory.php b/lib/Context/FileContextFactory.php index fae357c4613..ddbd236bcb4 100644 --- a/lib/Context/FileContextFactory.php +++ b/lib/Context/FileContextFactory.php @@ -10,10 +10,12 @@ use OCA\Text\Service\FileService; use OCA\Text\Service\LockService; use OCP\DirectEditing\IToken; +use OCP\Files\File; use OCP\Files\NotFoundException; use OCP\Files\NotPermittedException; use OCP\IL10N; use OCP\IUserSession; +use Psr\Log\LoggerInterface; class FileContextFactory { @@ -21,10 +23,27 @@ public function __construct( private readonly FileService $fileService, private readonly IL10N $l10n, private readonly LockService $lockService, + private readonly LoggerInterface $logger, private readonly IUserSession $userSession, ) { } + private function build( + File $file, + ?string $baseVersionEtag, + ?string $token = null, + ): FileContext { + return new FileContext( + $this->fileService, + $this->l10n, + $this->lockService, + $this->logger, + $file, + $baseVersionEtag, + $token, + ); + } + /** * @throws NotPermittedException if not logged in * @throws NotFoundException if the file cannot be found @@ -38,13 +57,7 @@ public function buildForId( throw new NotPermittedException(); } $file = $this->fileService->getFileById($id, $userId); - return new FileContext( - $this->fileService, - $this->l10n, - $this->lockService, - $file, - $baseVersionEtag, - ); + return $this->build($file, $baseVersionEtag); } /** @@ -62,14 +75,7 @@ public function buildForShareWithPath( * If not then well 404 it is. */ $this->fileService->checkSharePermissions($token); - return new FileContext( - $this->fileService, - $this->l10n, - $this->lockService, - $file, - $baseVersionEtag, - $token, - ); + return $this->build($file, $baseVersionEtag, $token); } /** @@ -77,13 +83,7 @@ public function buildForShareWithPath( */ public function buildForDirectEditing(IToken $token): FileContext { $file = $token->getFile(); - return new FileContext( - $this->fileService, - $this->l10n, - $this->lockService, - $file, - null, - ); + return $this->build($file, null); } } diff --git a/lib/Context/IContext.php b/lib/Context/IContext.php index 7d953f7606c..72884a1d0a6 100644 --- a/lib/Context/IContext.php +++ b/lib/Context/IContext.php @@ -8,19 +8,70 @@ namespace OCA\Text\Context; use OCA\Text\Db\Document; +use OCA\Text\Db\Session; use OCP\Files\Lock\ILock; -use OCP\IUser; interface IContext { - public function check(): ?string; - public function checkDocument(Document $document): ?string; - public function isReadOnly(): bool; public function getId(): int; public function getType(): string; public function toString(): string; - public function loadContent(): ?string; - public function getLockInfo(): ?ILock; - public function getOwner(): ?IUser; - public function lock(): bool; - public function createDocument(): Document; + public function buildDocument(): Document|string; + public function prepareSession(DocumentData $documentData): SessionInfo|string; +} + +readonly class DocumentData { + public function __construct( + public Document $document, + public ?string $documentState, + ) { + } + + public function jsonSerialize(): array { + return [ + 'document' => $this->document, + 'documentState' => $this->documentState, + ]; + } +} + +readonly class SessionInfo { + public function __construct( + public ?string $content, + public bool $readOnly, + public ?ILock $lock, + public bool $hasOwner, + ) { + } + + public function jsonSerialize(): array { + return [ + 'content' => $this->content, + 'readOnly' => $this->readOnly, + 'lock' => $this->lock, + 'hasOwner' => $this->hasOwner, + ]; + } +} + +readonly class NewSessionData { + public function __construct( + public DocumentData $documentData, + public SessionInfo $sessionInfo, + public Session $session, + public ?string $displayName, + ) { + } + + public function jsonSerialize(): array { + return array_merge( + $this->documentData->jsonSerialize(), + $this->sessionInfo->jsonSerialize(), + [ + 'session' => array_merge( + $this->session->jsonSerialize(), + ['displayName' => $this->displayName], + ), + ], + ); + } } diff --git a/lib/Db/Document.php b/lib/Db/Document.php index 5fdcdffa212..60e4e392f8b 100644 --- a/lib/Db/Document.php +++ b/lib/Db/Document.php @@ -55,4 +55,11 @@ public function jsonSerialize(): array { 'checksum' => $this->checksum ]; } + + /** + * Short identifier - mostly for logging + */ + public function toString(): string { + return 'file' . ' (' . $this->id . ')'; + } } diff --git a/lib/Db/DocumentMapper.php b/lib/Db/DocumentMapper.php index aedf04ad5d0..66f95ba6ff4 100644 --- a/lib/Db/DocumentMapper.php +++ b/lib/Db/DocumentMapper.php @@ -8,6 +8,7 @@ namespace OCA\Text\Db; use Generator; +use OCA\Text\Context\IContext; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Db\QBMapper; use OCP\DB\QueryBuilder\IQueryBuilder; @@ -40,6 +41,14 @@ public function find(int $documentId): Document { return Document::fromRow($data); } + /** + * @throws DoesNotExistException + */ + public function load(IContext $context): Document { + $id = $context->getId(); + return $this->find($id); + } + public function findAll(): Generator { $qb = $this->db->getQueryBuilder(); $result = $qb->select('*') diff --git a/lib/Service/ApiService.php b/lib/Service/ApiService.php index 6e42c8c75a7..6c21a6bd397 100644 --- a/lib/Service/ApiService.php +++ b/lib/Service/ApiService.php @@ -13,6 +13,8 @@ use InvalidArgumentException; use OCA\NotifyPush\Queue\IQueue; use OCA\Text\Context\IContext; +use OCA\Text\Context\NewSessionData; +use OCA\Text\Context\SessionInfo; use OCA\Text\Db\Document; use OCA\Text\Db\Session; use OCA\Text\Exception\DocumentSaveConflictException; @@ -41,71 +43,37 @@ public function __construct( } public function create(IContext $context, ?string $guestName = null): DataResponse { - try { - $error = $context->check(); - if ($error !== null) { - return new DataResponse(['error' => $error], Http::STATUS_FORBIDDEN); - } - - $readOnly = $context->isReadOnly(); - - $document = $this->documentService->getOrCreateDocument($context); - $this->sessionService->removeInactiveSessionsWithoutSteps($document->getId()); - $error = $context->checkDocument($document); - if ($error !== null) { - return new DataResponse(['error' => $error], Http::STATUS_PRECONDITION_FAILED); - } + $document = $context->buildDocument(); + if (!$document instanceof Document) { + return new DataResponse(['error' => $document], Http::STATUS_FORBIDDEN); + } + try { + $document = $this->documentService->getOrCreateDocument($document, $context); } catch (Exception $e) { $this->logger->error($e->getMessage(), ['exception' => $e]); return new DataResponse(['error' => 'Failed to create the document session'], Http::STATUS_INTERNAL_SERVER_ERROR); } + $documentData = $this->documentService->getDocumentData($document); - /** @var Document $document */ - - $session = $this->sessionService->initSession($document->getId(), $guestName); - - $documentState = null; - $content = null; - if ($document->getLastSavedVersion() === 0) { - $this->logger->debug('Sending content for unsaved ' . $context->toString()); - $content = $context->loadContent(); - } else { - $this->logger->debug('Loading saved document state for ' . $context->toString()); - try { - $stateFile = $this->documentService->getStateFile($document->getId()); - $documentState = $stateFile->getContent(); - } catch (NotFoundException) { - $this->logger->warning('State file not found for saved document' . $context->toString()); - - // If we have no state file we need to load the content from the file - // On the client side we use this to initialize a idempotent initial y.js document - $content = $context->loadContent(); - } + $sessionInfo = $context->prepareSession($documentData); + if (!$sessionInfo instanceof SessionInfo) { + return new DataResponse(['error' => $sessionInfo], Http::STATUS_PRECONDITION_FAILED); } - $lockInfo = $context->getLockInfo(); + $session = $this->sessionService->initSession($document->id, $guestName); + $displayName = $this->sessionService->getNameForSession($session); - $hasOwner = $context->getOwner() !== null; - - // Disable file locking for Readme.md files, because in the - // current setup, this makes it almost impossible to delete these files. - if (!$readOnly) { - $isLocked = $context->lock(); - if (!$isLocked) { - $readOnly = true; - } - } + $newSession = new NewSessionData( + documentData: $documentData, + sessionInfo: $sessionInfo, + session: $session, + displayName: $displayName, + ); - return new DataResponse([ - 'document' => $document, - 'session' => array_merge($session->jsonSerialize(), ['displayName' => $this->sessionService->getNameForSession($session)]), - 'readOnly' => $readOnly, - 'content' => $content, - 'documentState' => $documentState, - 'lock' => $lockInfo, - 'hasOwner' => $hasOwner, - ]); + return new DataResponse( + $newSession->jsonSerialize() + ); } public function close(int $documentId, int $sessionId, string $sessionToken, File $file): DataResponse { @@ -147,7 +115,7 @@ private function addToPushQueue(Document $document, array $steps): void { return; } - $sessions = $this->sessionService->getActiveSessions($document->getId()); + $sessions = $this->sessionService->getActiveSessions($document->id); $userIds = array_values(array_filter(array_unique( array_map(fn ($session): ?string => $session['userId'], $sessions) ))); diff --git a/lib/Service/DocumentService.php b/lib/Service/DocumentService.php index c0f3f82d6c1..ff45887c7b2 100644 --- a/lib/Service/DocumentService.php +++ b/lib/Service/DocumentService.php @@ -10,6 +10,7 @@ namespace OCA\Text\Service; use InvalidArgumentException; +use OCA\Text\Context\DocumentData; use OCA\Text\Context\IContext; use OCA\Text\Db\Document; use OCA\Text\Db\DocumentMapper; @@ -97,29 +98,29 @@ public function isSaveFromText(): bool { * @throws NotPermittedException * @throws Exception */ - public function getOrCreateDocument(IContext $context): Document { - $document = $this->getDocument($context->getId()); - if ($document !== null) { - $this->logger->info('Keep previous document of ' . $context->toString()); - return $document; + public function getOrCreateDocument(Document $document, IContext $context): Document { + // TODO: drop $context once $document contains contextId and contextType + $loaded = $this->getDocument($context->getId()); + if ($loaded !== null) { + $this->logger->info('Keep previous document of ' . $document->toString()); + return $loaded; } if (!$this->ensureDocumentsFolder()) { throw new NotFoundException('No app data folder present for text documents'); } - $this->logger->info('Create new document of ' . $context->toString()); - $document = $context->createDocument(); + $this->logger->info('Create new document of ' . $document->toString()); try { /** @var Document $document */ $document = $this->documentMapper->insert($document); - $this->cache->set('document-version-' . $document->getId(), 0); + $this->cache->set('document-version-' . $document->id, 0); } catch (Exception $e) { if ($e->getReason() !== Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) { throw $e; } // Document might have been created in the meantime - $document = $this->getDocument($context->getId()); + $document = $this->getDocument($document->id); if ($document === null) { throw $e; } @@ -127,6 +128,28 @@ public function getOrCreateDocument(IContext $context): Document { return $document; } + public function getDocumentData(Document $document): DocumentData { + $documentState = null; + if ($document->getLastSavedVersion() > 0) { + $this->logger->debug('Loading saved document state for ' . $document->toString()); + try { + $stateFile = $this->getStateFile($document->id); + $documentState = $stateFile->getContent(); + } catch (NotFoundException) { + // If we have no state file we need to load the content from the file + // On the client side we use this to initialize a idempotent initial y.js document + $this->logger->warning('State file not found for saved document' . $document->toString()); + } + } + + $documentData = new DocumentData( + document: $document, + documentState: $documentState, + ); + + return $documentData; + } + /** * @param int $documentId * @return ISimpleFile diff --git a/tests/unit/Service/ApiServiceTest.php b/tests/unit/Service/ApiServiceTest.php index 19e303c3e19..d36aa8d8302 100644 --- a/tests/unit/Service/ApiServiceTest.php +++ b/tests/unit/Service/ApiServiceTest.php @@ -2,12 +2,14 @@ namespace OCA\Text\Tests; +use OCA\Text\Context\DocumentData; +use OCA\Text\Context\IContext; +use OCA\Text\Context\SessionInfo; use OCA\Text\Db\Document; use OCA\Text\Db\Session; use OCA\Text\Service\ApiService; use OCA\Text\Service\ConfigService; use OCA\Text\Service\DocumentService; -use OCA\Text\Service\EncodingService; use OCA\Text\Service\FileService; use OCA\Text\Service\LockService; use OCA\Text\Service\SessionService; @@ -21,33 +23,24 @@ class ApiServiceTest extends \PHPUnit\Framework\TestCase { private SessionService $sessionService; private DocumentService $documentService; private FileService $fileService; - private EncodingService $encodingService; private LoggerInterface $loggerInterface; private LockService $lockService; private IL10N $l10n; public function setUp(): void { - $this->configService = $this->createMock(ConfigService::class); - $this->sessionService = $this->createMock(SessionService::class); - $this->documentService = $this->createMock(DocumentService::class); - $this->fileService = $this->createMock(FileService::class); - $this->encodingService = $this->createMock(EncodingService::class); - $this->loggerInterface = $this->createMock(LoggerInterface::class); - $this->lockService = $this->createMock(LockService::class); - $this->l10n = $this->createMock(IL10N::class); - - $document = new Document(); - $document->setId(123); - $this->documentService->method('getOrCreateDocument')->willReturn($document); - $this->fileService->method('isReadOnly')->willReturn(false); - $this->encodingService->method('encodeToUtf8')->willReturnCallback(fn ($str) => $str); + $this->configService = $this->createStub(ConfigService::class); + $this->sessionService = $this->createStub(SessionService::class); + $this->documentService = $this->createStub(DocumentService::class); + $this->fileService = $this->createStub(FileService::class); + $this->loggerInterface = $this->createStub(LoggerInterface::class); + $this->lockService = $this->createStub(LockService::class); + $this->l10n = $this->createStub(IL10N::class); $this->apiService = new ApiService( $this->configService, $this->sessionService, $this->documentService, $this->fileService, - $this->encodingService, $this->loggerInterface, $this->lockService, $this->l10n, @@ -56,16 +49,29 @@ public function setUp(): void { } public function testCreateNewSession() { - $file = $this->mockFile(1234, 'admin'); - $actual = $this->apiService->create($file); - self::assertTrue($actual->getData()['hasOwner']); - self::assertEquals('file content', $actual->getData()['content']); - } - - public function testCreateNewSessionWithoutOwner() { - $file = $this->mockFile(1234, null); - $actual = $this->apiService->create($file); - self::assertFalse($actual->getData()['hasOwner']); + $document = new Document(); + $document->setId(123); + $context = $this->createMock(IContext::class); + $documentData = new DocumentData(document: $document, documentState: 'documentState'); + $sessionInfo = new SessionInfo(content: 'content', readOnly: false, lock: null, hasOwner: true); + $context + ->expects($this->once()) + ->method('buildDocument') + ->willReturn($document); + $this->documentService->method('getOrCreateDocument')->willReturn($document); + $this->documentService->method('getDocumentData')->willReturn($documentData); + $context + ->expects($this->once()) + ->method('prepareSession') + ->with($documentData) + ->willReturn($sessionInfo); + $actual = $this->apiService->create($context); + foreach ($documentData as $key => $value) { + self::assertEquals($value, $actual->getData()[$key]); + } + foreach ($sessionInfo as $key => $value) { + self::assertEquals($value, $actual->getData()[$key]); + } } public function testSaveWithNotPermittedException() { diff --git a/tests/unit/Service/FileServiceTest.php b/tests/unit/Service/FileServiceTest.php index ad577e3a499..ba5cffbafb2 100644 --- a/tests/unit/Service/FileServiceTest.php +++ b/tests/unit/Service/FileServiceTest.php @@ -3,6 +3,7 @@ namespace OCA\Text\Tests; use OCA\Text\Exception\InvalidSessionException; +use OCA\Text\Service\EncodingService; use OCA\Text\Service\FileService; use OCA\Text\Service\LockService; use OCP\Constants; @@ -14,6 +15,7 @@ use OCP\Share\Exceptions\ShareNotFound; use OCP\Share\IManager; use OCP\Share\IShare; +use Psr\Log\LoggerInterface; class FileServiceTest extends \PHPUnit\Framework\TestCase { private FileService $fileService; @@ -28,9 +30,11 @@ public function setUp(): void { $this->shareManager = $this->createMock(IManager::class); $this->fileService = new FileService( + $this->createStub(EncodingService::class), $this->session, $this->rootFolder, $this->createMock(LockService::class), + $this->createStub(LoggerInterface::class), $this->shareManager, ); } From 63490fca08803e196349f414cbd3b8afbff1dc52 Mon Sep 17 00:00:00 2001 From: Max Date: Sat, 15 Aug 2026 21:43:50 +0200 Subject: [PATCH 04/30] chore(refactor): very basic registry for context factories Signed-off-by: Max --- lib/Controller/SessionController.php | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/lib/Controller/SessionController.php b/lib/Controller/SessionController.php index 65625e63a24..1905e297764 100644 --- a/lib/Controller/SessionController.php +++ b/lib/Controller/SessionController.php @@ -9,6 +9,7 @@ namespace OCA\Text\Controller; use OCA\Text\Context\FileContextFactory; +use OCA\Text\Context\IContext; use OCA\Text\Exception\InvalidSessionException; use OCA\Text\Middleware\Attribute\RequireDocumentBaseVersionEtag; use OCA\Text\Middleware\Attribute\RequireDocumentSession; @@ -55,12 +56,17 @@ public function __construct( #[NoAdminRequired] public function create(?int $fileId = null, ?string $baseVersionEtag = null): DataResponse { - if ($fileId === null) { + $type = 'file'; + $id = $fileId; + $builders = [ + 'file' => fn (int $id, string $_type, ?string $baseVersionEtag): IContext => $this->fileContextFactory->buildForId($id, $baseVersionEtag), + ]; + if ($id === null) { return new DataResponse(['error' => 'No valid file argument provided'], Http::STATUS_PRECONDITION_FAILED); } try { - $context = $this->fileContextFactory->buildForId($fileId, $baseVersionEtag); + $context = $builders[$type]($id, $type, $baseVersionEtag); } catch (NotFoundException|NotPermittedException $e) { $this->logger->error('No permission to access this file', [ 'exception' => $e ]); return new DataResponse([ From 251a62d3f9d8ff55459d7319dd6215b1b04bdddc Mon Sep 17 00:00:00 2001 From: Max Date: Sat, 15 Aug 2026 21:57:01 +0200 Subject: [PATCH 05/30] chore(refactor): handle baseVersionEtag outside IContext Signed-off-by: Max --- lib/Context/FileContext.php | 7 +------ lib/Context/FileContextFactory.php | 8 ++------ lib/Context/IContext.php | 2 +- lib/Controller/PublicSessionController.php | 4 ++-- lib/Controller/SessionController.php | 6 +++--- lib/DirectEditing/TextDirectEditor.php | 2 +- lib/Service/ApiService.php | 10 +++++----- tests/unit/Service/ApiServiceTest.php | 2 +- 8 files changed, 16 insertions(+), 25 deletions(-) diff --git a/lib/Context/FileContext.php b/lib/Context/FileContext.php index 7dad3a0c347..0a9a1a3d8d7 100644 --- a/lib/Context/FileContext.php +++ b/lib/Context/FileContext.php @@ -25,7 +25,6 @@ public function __construct( private readonly LockService $lockService, private readonly LoggerInterface $logger, private readonly File $file, - private readonly ?string $baseVersionEtag, private readonly ?string $token = null, ) { } @@ -63,14 +62,10 @@ public function buildDocument(): Document|string { } #[Override] - public function prepareSession(DocumentData $documentData): SessionInfo|string { + public function prepareSession(DocumentData $documentData): SessionInfo { $document = $documentData->document; $documentState = $documentData->documentState; - if ($this->baseVersionEtag !== null && $this->baseVersionEtag !== $document->getBaseVersionEtag()) { - return $this->l10n->t('Editing session has expired. Please reload the page.'); - } - $content = null; if ($documentState === null) { $this->logger->debug('Sending content for ' . $document->toString()); diff --git a/lib/Context/FileContextFactory.php b/lib/Context/FileContextFactory.php index ddbd236bcb4..8916e5d7b94 100644 --- a/lib/Context/FileContextFactory.php +++ b/lib/Context/FileContextFactory.php @@ -30,7 +30,6 @@ public function __construct( private function build( File $file, - ?string $baseVersionEtag, ?string $token = null, ): FileContext { return new FileContext( @@ -39,7 +38,6 @@ private function build( $this->lockService, $this->logger, $file, - $baseVersionEtag, $token, ); } @@ -50,14 +48,13 @@ private function build( */ public function buildForId( int $id, - ?string $baseVersionEtag, ): FileContext { $userId = $this->userSession->getUser()?->getUID(); if ($userId === null) { throw new NotPermittedException(); } $file = $this->fileService->getFileById($id, $userId); - return $this->build($file, $baseVersionEtag); + return $this->build($file); } /** @@ -67,7 +64,6 @@ public function buildForId( public function buildForShareWithPath( string $token, ?string $filePath, - ?string $baseVersionEtag, ): FileContext { $file = $this->fileService->getFileByShareToken($token, $filePath); /* @@ -75,7 +71,7 @@ public function buildForShareWithPath( * If not then well 404 it is. */ $this->fileService->checkSharePermissions($token); - return $this->build($file, $baseVersionEtag, $token); + return $this->build($file, $token); } /** diff --git a/lib/Context/IContext.php b/lib/Context/IContext.php index 72884a1d0a6..3be91ef838a 100644 --- a/lib/Context/IContext.php +++ b/lib/Context/IContext.php @@ -16,7 +16,7 @@ public function getId(): int; public function getType(): string; public function toString(): string; public function buildDocument(): Document|string; - public function prepareSession(DocumentData $documentData): SessionInfo|string; + public function prepareSession(DocumentData $documentData): SessionInfo; } readonly class DocumentData { diff --git a/lib/Controller/PublicSessionController.php b/lib/Controller/PublicSessionController.php index 72e6c324cf8..c934ccd0875 100644 --- a/lib/Controller/PublicSessionController.php +++ b/lib/Controller/PublicSessionController.php @@ -74,8 +74,8 @@ protected function isPasswordProtected(): bool { #[PublicPage] public function create(string $token, ?string $filePath = null, ?string $baseVersionEtag = null, ?string $guestName = null): DataResponse { try { - $context = $this->fileContextFactory->buildForShareWithPath($token, $filePath, $baseVersionEtag); - return $this->apiService->create($context, $guestName); + $context = $this->fileContextFactory->buildForShareWithPath($token, $filePath); + return $this->apiService->create($context, $baseVersionEtag, $guestName); } catch (NotFoundException|\InvalidArgumentException) { return new DataResponse([], Http::STATUS_NOT_FOUND); } diff --git a/lib/Controller/SessionController.php b/lib/Controller/SessionController.php index 1905e297764..8b5f83bada7 100644 --- a/lib/Controller/SessionController.php +++ b/lib/Controller/SessionController.php @@ -59,14 +59,14 @@ public function create(?int $fileId = null, ?string $baseVersionEtag = null): Da $type = 'file'; $id = $fileId; $builders = [ - 'file' => fn (int $id, string $_type, ?string $baseVersionEtag): IContext => $this->fileContextFactory->buildForId($id, $baseVersionEtag), + 'file' => fn (int $id, string $_type): IContext => $this->fileContextFactory->buildForId($id), ]; if ($id === null) { return new DataResponse(['error' => 'No valid file argument provided'], Http::STATUS_PRECONDITION_FAILED); } try { - $context = $builders[$type]($id, $type, $baseVersionEtag); + $context = $builders[$type]($id, $type); } catch (NotFoundException|NotPermittedException $e) { $this->logger->error('No permission to access this file', [ 'exception' => $e ]); return new DataResponse([ @@ -74,7 +74,7 @@ public function create(?int $fileId = null, ?string $baseVersionEtag = null): Da ], Http::STATUS_NOT_FOUND); } - return $this->apiService->create($context); + return $this->apiService->create($context, $baseVersionEtag); } #[NoAdminRequired] diff --git a/lib/DirectEditing/TextDirectEditor.php b/lib/DirectEditing/TextDirectEditor.php index 63a213d9ed6..0d086640ad8 100644 --- a/lib/DirectEditing/TextDirectEditor.php +++ b/lib/DirectEditing/TextDirectEditor.php @@ -134,7 +134,7 @@ public function open(IToken $token): Response { $token->useTokenScope(); try { $context = $this->fileContextFactory->buildForDirectEditing($token); - $session = $this->apiService->create($context); + $session = $this->apiService->create($context, null); $this->initialStateProvider->provideFile([ 'fileId' => $token->getFile()->getId(), 'mimetype' => $token->getFile()->getMimeType(), diff --git a/lib/Service/ApiService.php b/lib/Service/ApiService.php index 6c21a6bd397..ab8d16754fe 100644 --- a/lib/Service/ApiService.php +++ b/lib/Service/ApiService.php @@ -14,7 +14,6 @@ use OCA\NotifyPush\Queue\IQueue; use OCA\Text\Context\IContext; use OCA\Text\Context\NewSessionData; -use OCA\Text\Context\SessionInfo; use OCA\Text\Db\Document; use OCA\Text\Db\Session; use OCA\Text\Exception\DocumentSaveConflictException; @@ -42,7 +41,7 @@ public function __construct( ) { } - public function create(IContext $context, ?string $guestName = null): DataResponse { + public function create(IContext $context, ?string $baseVersionEtag, ?string $guestName = null): DataResponse { $document = $context->buildDocument(); if (!$document instanceof Document) { return new DataResponse(['error' => $document], Http::STATUS_FORBIDDEN); @@ -56,11 +55,12 @@ public function create(IContext $context, ?string $guestName = null): DataRespon } $documentData = $this->documentService->getDocumentData($document); - $sessionInfo = $context->prepareSession($documentData); - if (!$sessionInfo instanceof SessionInfo) { - return new DataResponse(['error' => $sessionInfo], Http::STATUS_PRECONDITION_FAILED); + if ($baseVersionEtag !== null && $baseVersionEtag !== $document->getBaseVersionEtag()) { + $error = $this->l10n->t('Editing session has expired. Please reload the page.'); + return new DataResponse(['error' => $error], Http::STATUS_PRECONDITION_FAILED); } + $sessionInfo = $context->prepareSession($documentData); $session = $this->sessionService->initSession($document->id, $guestName); $displayName = $this->sessionService->getNameForSession($session); diff --git a/tests/unit/Service/ApiServiceTest.php b/tests/unit/Service/ApiServiceTest.php index d36aa8d8302..b0720cc9835 100644 --- a/tests/unit/Service/ApiServiceTest.php +++ b/tests/unit/Service/ApiServiceTest.php @@ -65,7 +65,7 @@ public function testCreateNewSession() { ->method('prepareSession') ->with($documentData) ->willReturn($sessionInfo); - $actual = $this->apiService->create($context); + $actual = $this->apiService->create($context, null); foreach ($documentData as $key => $value) { self::assertEquals($value, $actual->getData()[$key]); } From 73632612defd57fdecd77b48361fb7bf0e98ed45 Mon Sep 17 00:00:00 2001 From: Max Date: Sun, 16 Aug 2026 14:06:27 +0200 Subject: [PATCH 06/30] chore(refactor): introduce RegisterContextEvent Signed-off-by: Max --- composer/composer/autoload_classmap.php | 3 + composer/composer/autoload_static.php | 3 + lib/AppInfo/Application.php | 4 ++ lib/Context/ContextManager.php | 56 +++++++++++++++++++ lib/Controller/SessionController.php | 12 ++-- lib/Event/RegisterContextEvent.php | 33 +++++++++++ .../RegisterContextEventListener.php | 36 ++++++++++++ 7 files changed, 139 insertions(+), 8 deletions(-) create mode 100644 lib/Context/ContextManager.php create mode 100644 lib/Event/RegisterContextEvent.php create mode 100644 lib/Listeners/RegisterContextEventListener.php diff --git a/composer/composer/autoload_classmap.php b/composer/composer/autoload_classmap.php index ad130420ebc..71ecafa45e9 100644 --- a/composer/composer/autoload_classmap.php +++ b/composer/composer/autoload_classmap.php @@ -10,6 +10,7 @@ 'OCA\\Text\\AppInfo\\Application' => $baseDir . '/../lib/AppInfo/Application.php', 'OCA\\Text\\Command\\ResetDocument' => $baseDir . '/../lib/Command/ResetDocument.php', 'OCA\\Text\\ConfigLexicon' => $baseDir . '/../lib/ConfigLexicon.php', + 'OCA\\Text\\Context\\ContextManager' => $baseDir . '/../lib/Context/ContextManager.php', 'OCA\\Text\\Context\\FileContext' => $baseDir . '/../lib/Context/FileContext.php', 'OCA\\Text\\Context\\FileContextFactory' => $baseDir . '/../lib/Context/FileContextFactory.php', 'OCA\\Text\\Context\\IContext' => $baseDir . '/../lib/Context/IContext.php', @@ -35,6 +36,7 @@ 'OCA\\Text\\DirectEditing\\TextDocumentCreator' => $baseDir . '/../lib/DirectEditing/TextDocumentCreator.php', 'OCA\\Text\\Event\\LoadEditor' => $baseDir . '/../lib/Event/LoadEditor.php', 'OCA\\Text\\Event\\MentionEvent' => $baseDir . '/../lib/Event/MentionEvent.php', + 'OCA\\Text\\Event\\RegisterContextEvent' => $baseDir . '/../lib/Event/RegisterContextEvent.php', 'OCA\\Text\\Exception\\AccountDisabledException' => $baseDir . '/../lib/Exception/AccountDisabledException.php', 'OCA\\Text\\Exception\\DocumentHasUnsavedChangesException' => $baseDir . '/../lib/Exception/DocumentHasUnsavedChangesException.php', 'OCA\\Text\\Exception\\DocumentSaveConflictException' => $baseDir . '/../lib/Exception/DocumentSaveConflictException.php', @@ -52,6 +54,7 @@ 'OCA\\Text\\Listeners\\LoadViewerListener' => $baseDir . '/../lib/Listeners/LoadViewerListener.php', 'OCA\\Text\\Listeners\\NodeCopiedListener' => $baseDir . '/../lib/Listeners/NodeCopiedListener.php', 'OCA\\Text\\Listeners\\NodeWrittenResetDocumentListener' => $baseDir . '/../lib/Listeners/NodeWrittenResetDocumentListener.php', + 'OCA\\Text\\Listeners\\RegisterContextEventListener' => $baseDir . '/../lib/Listeners/RegisterContextEventListener.php', 'OCA\\Text\\Listeners\\RegisterDirectEditorEventListener' => $baseDir . '/../lib/Listeners/RegisterDirectEditorEventListener.php', 'OCA\\Text\\Listeners\\RegisterTemplateCreatorListener' => $baseDir . '/../lib/Listeners/RegisterTemplateCreatorListener.php', 'OCA\\Text\\Listeners\\VersionRestoredListener' => $baseDir . '/../lib/Listeners/VersionRestoredListener.php', diff --git a/composer/composer/autoload_static.php b/composer/composer/autoload_static.php index 2800c11727e..c235f061bde 100644 --- a/composer/composer/autoload_static.php +++ b/composer/composer/autoload_static.php @@ -25,6 +25,7 @@ class ComposerStaticInitText 'OCA\\Text\\AppInfo\\Application' => __DIR__ . '/..' . '/../lib/AppInfo/Application.php', 'OCA\\Text\\Command\\ResetDocument' => __DIR__ . '/..' . '/../lib/Command/ResetDocument.php', 'OCA\\Text\\ConfigLexicon' => __DIR__ . '/..' . '/../lib/ConfigLexicon.php', + 'OCA\\Text\\Context\\ContextManager' => __DIR__ . '/..' . '/../lib/Context/ContextManager.php', 'OCA\\Text\\Context\\FileContext' => __DIR__ . '/..' . '/../lib/Context/FileContext.php', 'OCA\\Text\\Context\\FileContextFactory' => __DIR__ . '/..' . '/../lib/Context/FileContextFactory.php', 'OCA\\Text\\Context\\IContext' => __DIR__ . '/..' . '/../lib/Context/IContext.php', @@ -50,6 +51,7 @@ class ComposerStaticInitText 'OCA\\Text\\DirectEditing\\TextDocumentCreator' => __DIR__ . '/..' . '/../lib/DirectEditing/TextDocumentCreator.php', 'OCA\\Text\\Event\\LoadEditor' => __DIR__ . '/..' . '/../lib/Event/LoadEditor.php', 'OCA\\Text\\Event\\MentionEvent' => __DIR__ . '/..' . '/../lib/Event/MentionEvent.php', + 'OCA\\Text\\Event\\RegisterContextEvent' => __DIR__ . '/..' . '/../lib/Event/RegisterContextEvent.php', 'OCA\\Text\\Exception\\AccountDisabledException' => __DIR__ . '/..' . '/../lib/Exception/AccountDisabledException.php', 'OCA\\Text\\Exception\\DocumentHasUnsavedChangesException' => __DIR__ . '/..' . '/../lib/Exception/DocumentHasUnsavedChangesException.php', 'OCA\\Text\\Exception\\DocumentSaveConflictException' => __DIR__ . '/..' . '/../lib/Exception/DocumentSaveConflictException.php', @@ -67,6 +69,7 @@ class ComposerStaticInitText 'OCA\\Text\\Listeners\\LoadViewerListener' => __DIR__ . '/..' . '/../lib/Listeners/LoadViewerListener.php', 'OCA\\Text\\Listeners\\NodeCopiedListener' => __DIR__ . '/..' . '/../lib/Listeners/NodeCopiedListener.php', 'OCA\\Text\\Listeners\\NodeWrittenResetDocumentListener' => __DIR__ . '/..' . '/../lib/Listeners/NodeWrittenResetDocumentListener.php', + 'OCA\\Text\\Listeners\\RegisterContextEventListener' => __DIR__ . '/..' . '/../lib/Listeners/RegisterContextEventListener.php', 'OCA\\Text\\Listeners\\RegisterDirectEditorEventListener' => __DIR__ . '/..' . '/../lib/Listeners/RegisterDirectEditorEventListener.php', 'OCA\\Text\\Listeners\\RegisterTemplateCreatorListener' => __DIR__ . '/..' . '/../lib/Listeners/RegisterTemplateCreatorListener.php', 'OCA\\Text\\Listeners\\VersionRestoredListener' => __DIR__ . '/..' . '/../lib/Listeners/VersionRestoredListener.php', diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index cd1c8f2c703..8447130b01b 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -12,6 +12,7 @@ use OCA\Files_Sharing\Event\BeforeTemplateRenderedEvent; use OCA\Files_Versions\Events\VersionRestoredEvent; use OCA\Text\Event\LoadEditor; +use OCA\Text\Event\RegisterContextEvent; use OCA\Text\Listeners\AddMissingIndicesListener; use OCA\Text\Listeners\BeforeAssistantNotificationListener; use OCA\Text\Listeners\BeforeNodeDeletedListener; @@ -22,6 +23,7 @@ use OCA\Text\Listeners\LoadViewerListener; use OCA\Text\Listeners\NodeCopiedListener; use OCA\Text\Listeners\NodeWrittenResetDocumentListener; +use OCA\Text\Listeners\RegisterContextEventListener; use OCA\Text\Listeners\RegisterDirectEditorEventListener; use OCA\Text\Listeners\RegisterTemplateCreatorListener; use OCA\Text\Listeners\VersionRestoredListener; @@ -69,6 +71,8 @@ public function register(IRegistrationContext $context): void { $context->registerEventListener(VersionRestoredEvent::class, VersionRestoredListener::class); + $context->registerEventListener(RegisterContextEvent::class, RegisterContextEventListener::class); + $context->registerNotifierService(Notifier::class); $context->registerMiddleware(SessionMiddleware::class); } diff --git a/lib/Context/ContextManager.php b/lib/Context/ContextManager.php new file mode 100644 index 00000000000..aa0e47911bd --- /dev/null +++ b/lib/Context/ContextManager.php @@ -0,0 +1,56 @@ + */ + private array $contexts = []; + public function __construct( + private readonly IEventDispatcher $eventDispatcher, + private readonly LoggerInterface $logger, + ) { + } + + private function getContexts(): array { + $contexts = $this->contexts; + if (!empty($contexts)) { + return $contexts; + } + $this->eventDispatcher->dispatchTyped(new RegisterContextEvent($this)); + if (empty($this->contexts)) { + $this->logger->warning('Failed to register contexts.'); + } + return $this->contexts; + } + + public function registerContext(string $type, callable $createContext): void { + $this->logger->debug('Registering context for type "' . $type . '".'); + if (array_key_exists($type, $this->contexts)) { + $this->logger->warning('Context of type "' . $type . '" was already registered!'); + return; + } + $this->contexts[$type] = $createContext; + } + + public function getContext(int $id, string $type): IContext { + $createContext = $this->getContexts()[$type]; + if (!is_callable($createContext)) { + throw new NotFoundException('Context of type "' . $type . '" was not registered!'); + } + $context = $createContext($id, $type); + if (!$context instanceof IContext) { + throw new NotFoundException('Failed to create context of type ' . $type . '!'); + } + return $context; + } +} diff --git a/lib/Controller/SessionController.php b/lib/Controller/SessionController.php index 8b5f83bada7..0c4225fec87 100644 --- a/lib/Controller/SessionController.php +++ b/lib/Controller/SessionController.php @@ -8,8 +8,7 @@ namespace OCA\Text\Controller; -use OCA\Text\Context\FileContextFactory; -use OCA\Text\Context\IContext; +use OCA\Text\Context\ContextManager; use OCA\Text\Exception\InvalidSessionException; use OCA\Text\Middleware\Attribute\RequireDocumentBaseVersionEtag; use OCA\Text\Middleware\Attribute\RequireDocumentSession; @@ -42,7 +41,7 @@ public function __construct( string $appName, IRequest $request, private ApiService $apiService, - private FileContextFactory $fileContextFactory, + private ContextManager $contextManager, private FileService $fileService, private SessionService $sessionService, private NotificationService $notificationService, @@ -58,17 +57,14 @@ public function __construct( public function create(?int $fileId = null, ?string $baseVersionEtag = null): DataResponse { $type = 'file'; $id = $fileId; - $builders = [ - 'file' => fn (int $id, string $_type): IContext => $this->fileContextFactory->buildForId($id), - ]; if ($id === null) { return new DataResponse(['error' => 'No valid file argument provided'], Http::STATUS_PRECONDITION_FAILED); } try { - $context = $builders[$type]($id, $type); + $context = $this->contextManager->getContext($id, $type); } catch (NotFoundException|NotPermittedException $e) { - $this->logger->error('No permission to access this file', [ 'exception' => $e ]); + $this->logger->error('No permission to access this context', [ 'exception' => $e ]); return new DataResponse([ 'error' => $this->l10n->t('File not found') ], Http::STATUS_NOT_FOUND); diff --git a/lib/Event/RegisterContextEvent.php b/lib/Event/RegisterContextEvent.php new file mode 100644 index 00000000000..8cdab36d5db --- /dev/null +++ b/lib/Event/RegisterContextEvent.php @@ -0,0 +1,33 @@ +contextManager; + } +} diff --git a/lib/Listeners/RegisterContextEventListener.php b/lib/Listeners/RegisterContextEventListener.php new file mode 100644 index 00000000000..a1f539e7c39 --- /dev/null +++ b/lib/Listeners/RegisterContextEventListener.php @@ -0,0 +1,36 @@ + */ +class RegisterContextEventListener implements IEventListener { + + public function __construct( + private readonly FileContextFactory $fileContextFactory, + ) { + } + + #[Override] + public function handle(Event $event): void { + if (!$event instanceof RegisterContextEvent) { + return; + } + + $event->getContextManager()->registerContext( + 'file', + fn (int $id) => $this->fileContextFactory->buildForId($id) + ); + } +} From d2237ad598cb7e6bff7b2dce4f6df7683874d2d2 Mon Sep 17 00:00:00 2001 From: Max Date: Sun, 16 Aug 2026 22:04:08 +0200 Subject: [PATCH 07/30] chore(refactor): split open api functions Two api endpoints deserve two different functions in particular as they also expect different params. This also allows narrowing down the types a little more. Signed-off-by: Max --- cypress/e2e/api/SessionApi.spec.js | 24 +++++----- cypress/e2e/api/UsersApi.spec.js | 2 +- cypress/support/sessions.js | 7 +-- src/apis/connect.ts | 36 ++++++++++++--- src/composables/useConnection.ts | 63 ++++++++++++++++++++++---- src/tests/services/SyncService.spec.ts | 2 +- 6 files changed, 102 insertions(+), 32 deletions(-) diff --git a/cypress/e2e/api/SessionApi.spec.js b/cypress/e2e/api/SessionApi.spec.js index 50d932b3249..521ab791899 100644 --- a/cypress/e2e/api/SessionApi.spec.js +++ b/cypress/e2e/api/SessionApi.spec.js @@ -36,14 +36,14 @@ describe('The session Api', function() { }) it('returns connection', function() { - cy.openConnection({ fileId }).then(({ connection }) => { + cy.openFileConnection({ fileId }).then(({ connection }) => { cy.wrap(connection).its('documentId').should('equal', fileId) cy.closeConnection(connection) }) }) it('provides initial content', function() { - cy.openConnection({ fileId, filePath }).then(({ connection, data }) => { + cy.openFileConnection({ fileId, filePath }).then(({ connection, data }) => { cy.wrap(data).its('content').should('eql', '## Hello world\n') cy.closeConnection(connection) }) @@ -63,7 +63,7 @@ describe('The session Api', function() { beforeEach(function() { cy.uploadTestFile() - .then((fileId) => cy.openConnection({ fileId })) + .then((fileId) => cy.openFileConnection({ fileId })) .then(({ connection: con }) => { connection = con }) @@ -116,7 +116,7 @@ describe('The session Api', function() { cy.uploadTestFile() .then((id) => { fileId = id - return cy.openConnection({ fileId, filePath }) + return cy.openFileConnection({ fileId, filePath }) }) .then(({ connection: con }) => { connection = con @@ -151,7 +151,7 @@ describe('The session Api', function() { documentState, manualSave: true, }) - cy.openConnection({ fileId, filePath }) + cy.openFileConnection({ fileId, filePath }) .as('joining') .its('data.documentState') .should('eql', documentState) @@ -183,7 +183,7 @@ describe('The session Api', function() { .then(() => cy.clearCookies()) .then(() => { return cy - .openConnection({ filePath: '', token: shareToken }) + .openShareConnection({ filePath: '', token: shareToken }) .then(({ connection: con }) => { connection = con }) @@ -223,7 +223,7 @@ describe('The session Api', function() { documentState, manualSave: true, }) - cy.openConnection({ filePath: '', token: shareToken }) + cy.openShareConnection({ filePath: '', token: shareToken }) .as('joining') .its('data.documentState') .should('eql', documentState) @@ -247,7 +247,7 @@ describe('The session Api', function() { cy.log(token) shareToken = token cy.clearCookies() - cy.openConnection({ filePath: '', token: shareToken }).then(({ connection: con }) => { + cy.openShareConnection({ filePath: '', token: shareToken }).then(({ connection: con }) => { connection = con }) }) @@ -255,7 +255,7 @@ describe('The session Api', function() { it('does not send initial content if other session is alive but did not push any steps', function() { let joining - cy.openConnection({ filePath: '', token: shareToken }) + cy.openShareConnection({ filePath: '', token: shareToken }) .then(({ connection: con, data }) => { joining = con return data @@ -271,7 +271,7 @@ describe('The session Api', function() { cy.pushSteps({ connection, steps: [messages.update], version }) .its('version') .should('eql', 0) - cy.openConnection({ filePath: '', token: shareToken }) + cy.openShareConnection({ filePath: '', token: shareToken }) .then(({ connection: con, data }) => { joining = con return data @@ -312,7 +312,7 @@ describe('The session Api', function() { .its('version') .should('eql', 0) cy.log('Other user creates session') - cy.openConnection({ filePath: '', token: shareToken }).then(({ connection: con }) => { + cy.openShareConnection({ filePath: '', token: shareToken }).then(({ connection: con }) => { joining = con }) cy.log('Initial user closes session') @@ -330,7 +330,7 @@ describe('The session Api', function() { // Skipped for now since the behaviour chanced by not cleaning up the state on close/create it.skip('ignores steps stored after close cleaned up', function() { cy.pushAndClose({ connection, steps: [messages.update], version }) - cy.openConnection({ filePath: '', token: shareToken }) + cy.openShareConnection({ filePath: '', token: shareToken }) .then(({ connection: con, data }) => { connection = con return data diff --git a/cypress/e2e/api/UsersApi.spec.js b/cypress/e2e/api/UsersApi.spec.js index 90319eea5e5..3f21d11200e 100644 --- a/cypress/e2e/api/UsersApi.spec.js +++ b/cypress/e2e/api/UsersApi.spec.js @@ -16,7 +16,7 @@ describe('The user mention API', function() { cy.login(user) cy.uploadTestFile('test.md') .as('fileId') - .then((fileId) => cy.openConnection({ fileId })) + .then((fileId) => cy.openFileConnection({ fileId })) .its('connection') .as('connection') }) diff --git a/cypress/support/sessions.js b/cypress/support/sessions.js index 4795df43825..2edc5bdd4da 100644 --- a/cypress/support/sessions.js +++ b/cypress/support/sessions.js @@ -4,20 +4,21 @@ */ import axios from '@nextcloud/axios' -import { close, open } from '../../src/apis/connect.ts' +import { close, openFile, openShare } from '../../src/apis/connect.ts' import { save } from '../../src/apis/save.ts' import { push, sync } from '../../src/apis/sync.ts' const url = Cypress.config('baseUrl').replace(/\/index.php\/?$/g, '') -Cypress.Commands.add('openConnection', open) +Cypress.Commands.add('openFileConnection', openFile) +Cypress.Commands.add('openShareConnection', openShare) Cypress.Commands.add('closeConnection', close) Cypress.Commands.add( 'failToCreateTextSession', (fileId, baseVersionEtag = null, options = {}) => { - return open({ fileId, ...options, baseVersionEtag }).then( + return openFile({ fileId, ...options, baseVersionEtag }).then( () => { throw new Error('Expected request to fail - but it succeeded!') }, diff --git a/src/apis/connect.ts b/src/apis/connect.ts index 33e8ebdbb00..042f56ef2e2 100644 --- a/src/apis/connect.ts +++ b/src/apis/connect.ts @@ -10,10 +10,16 @@ import axios from '@nextcloud/axios' import { generateUrl } from '@nextcloud/router' export interface OpenParams { - fileId?: number + fileId: number + filePath: string // not send to the api but included in the connection baseVersionEtag?: string +} + +export interface OpenShareParams { + token: string + fileId: number filePath: string - token?: string + baseVersionEtag?: string guestName?: string } @@ -27,16 +33,32 @@ export interface OpenData { hasOwner: boolean } +/** + * Open editing connection to a file when logged in + * + * @param params Parameters identifying the document + */ +export async function openFile(params: OpenParams): Promise<{ connection: Connection, data: OpenData }> { + const url = generateUrl(`/apps/text/session/${params.fileId}/create`) + const response = await axios.put(url, params) + const { document, session } = response.data + const connection = { + documentId: document.id, + sessionId: session.id, + sessionToken: session.token, + baseVersionEtag: document.baseVersionEtag, + filePath: params.filePath, + } + return { connection, data: response.data } +} + /** * Open editing connection to the document * * @param params Parameters identifying the document */ -export async function open(params: OpenParams): Promise<{ connection: Connection, data: OpenData }> { - const _baseUrl = params.token - ? generateUrl('/apps/text/public') - : generateUrl('/apps/text') - const url = `${_baseUrl}/session/${params.fileId}/create` +export async function openShare(params: OpenShareParams): Promise<{ connection: Connection, data: OpenData }> { + const url = generateUrl('/apps/text/public/session/123/create') const response = await axios.put(url, params) const { document, session } = response.data const connection = { diff --git a/src/composables/useConnection.ts b/src/composables/useConnection.ts index 7fdcc217bb0..2d853124a63 100644 --- a/src/composables/useConnection.ts +++ b/src/composables/useConnection.ts @@ -8,7 +8,7 @@ import type { OpenData } from '../apis/connect.ts' import type { Document, Session } from '../services/SyncService.ts' import { inject, provide, shallowRef } from 'vue' -import { open } from '../apis/connect.ts' +import * as api from '../apis/connect.ts' export interface Connection { documentId: number @@ -65,13 +65,8 @@ export function provideConnection( const guestName = localStorage.getItem('nick') ?? '' const { connection: opened, data } = openInitialSession(props, baseVersionEtag) - || (await open({ - fileId: props.fileId, - guestName, - token: props.shareToken, - filePath: props.relativePath, - baseVersionEtag, - })) + || await openShare(props, baseVersionEtag, guestName) + || await openFile(props, baseVersionEtag) await setBaseVersionEtag(data.document.baseVersionEtag) connection.value = opened openData.value = data @@ -134,3 +129,55 @@ function openInitialSession( return { connection, data: props.initialSession } } } + +/** + * Get the connection and additional data from the initial session if available. + * + * @param props Props of the editor component + * @param props.relativePath Relative path to the file. + * @param props.shareToken Share token of the file. + * @param props.fileId id of the file + * @param baseVersionEtag Etag from the last editing session. + * @param guestName to be shown to other participants. + */ +async function openShare( + props: { + fileId: number + relativePath: string + shareToken?: string + }, + baseVersionEtag: string | undefined, + guestName: string | undefined, +) { + if (props.shareToken) { + return api.openShare({ + guestName, + token: props.shareToken, + filePath: props.relativePath, + fileId: props.fileId, + baseVersionEtag, + }) + } +} + +/** + * Get the connection and additional data from the initial session if available. + * + * @param props Props of the editor component + * @param props.fileId id of the file + * @param props.relativePath Relative path to the file. + * @param baseVersionEtag Etag from the last editing session. + */ +async function openFile( + props: { + fileId: number + relativePath: string + }, + baseVersionEtag: string | undefined, +) { + return api.openFile({ + fileId: props.fileId, + filePath: props.relativePath, + baseVersionEtag, + }) +} diff --git a/src/tests/services/SyncService.spec.ts b/src/tests/services/SyncService.spec.ts index 2707f1fd0f5..2f0915e26f4 100644 --- a/src/tests/services/SyncService.spec.ts +++ b/src/tests/services/SyncService.spec.ts @@ -54,7 +54,7 @@ describe('Sync service', () => { setBaseVersionEtag, ) vi.mock('../../apis/connect') - vi.mocked(connect.open).mockResolvedValue(openResult) + vi.mocked(connect.openFile).mockResolvedValue(openResult) const openHandler = vi.fn() const service = new SyncService({ connection, openConnection }) service.bus.on('opened', openHandler) From c92618e61ee1d25c886f12d89cc704a8ffdda496 Mon Sep 17 00:00:00 2001 From: Max Date: Mon, 17 Aug 2026 14:49:36 +0200 Subject: [PATCH 08/30] enh(api): allow opening contexts of different types Still missing the actual database migration on the backend and the props etc. on the frontend. Signed-off-by: Max --- appinfo/routes.php | 2 +- cypress/e2e/api/SessionApi.spec.js | 2 +- cypress/e2e/sync.spec.js | 4 ++-- cypress/support/sessions.js | 12 +++++++++--- lib/Controller/SessionController.php | 10 ++-------- src/apis/connect.ts | 12 +++++++----- src/composables/useConnection.ts | 5 +++-- src/tests/services/SyncService.spec.ts | 2 +- 8 files changed, 26 insertions(+), 23 deletions(-) diff --git a/appinfo/routes.php b/appinfo/routes.php index 0eb6c066aad..7495440142d 100644 --- a/appinfo/routes.php +++ b/appinfo/routes.php @@ -28,7 +28,7 @@ ['name' => 'Attachment#getMediaFilePreview', 'url' => '/mediaPreview', 'verb' => 'GET'], /** @see Controller\SessionController::create() */ - ['name' => 'Session#create', 'url' => '/session/{documentId}/create', 'verb' => 'PUT'], + ['name' => 'Session#create', 'url' => '/session/{type}/{id}/create', 'verb' => 'PUT'], /** @see Controller\SessionController::save() */ ['name' => 'Session#save', 'url' => '/session/{documentId}/save', 'verb' => 'POST'], /** @see Controller\SessionController::sync() */ diff --git a/cypress/e2e/api/SessionApi.spec.js b/cypress/e2e/api/SessionApi.spec.js index 521ab791899..51291027a31 100644 --- a/cypress/e2e/api/SessionApi.spec.js +++ b/cypress/e2e/api/SessionApi.spec.js @@ -37,7 +37,7 @@ describe('The session Api', function() { it('returns connection', function() { cy.openFileConnection({ fileId }).then(({ connection }) => { - cy.wrap(connection).its('documentId').should('equal', fileId) + cy.wrap(connection).its('documentId').should('be.greaterThan', 0) cy.closeConnection(connection) }) }) diff --git a/cypress/e2e/sync.spec.js b/cypress/e2e/sync.spec.js index b76414a6fe6..f850a785a16 100644 --- a/cypress/e2e/sync.spec.js +++ b/cypress/e2e/sync.spec.js @@ -102,7 +102,7 @@ describe('Sync', () => { 'contain', 'The document could not be loaded.', ) - cy.intercept('**/apps/text/session/*/create').as('create') + cy.intercept('**/apps/text/session/*/*/create').as('create') cy.get('#editor-container .document-status').find('button').click() // let first attempt fail cy.wait('@create', { timeout: 10000 }) @@ -180,7 +180,7 @@ describe('Sync', () => { it('passes the doc content from one session to the next', () => { cy.closeFile() - cy.intercept({ method: 'PUT', url: '**/apps/text/session/*/create' }).as('create') + cy.intercept({ method: 'PUT', url: '**/apps/text/session/*/*/create' }).as('create') cy.openTestFile() cy.wait('@create', { timeout: 10000 }) .its('response.body') diff --git a/cypress/support/sessions.js b/cypress/support/sessions.js index 2edc5bdd4da..bb3d8e25cc1 100644 --- a/cypress/support/sessions.js +++ b/cypress/support/sessions.js @@ -4,13 +4,19 @@ */ import axios from '@nextcloud/axios' -import { close, openFile, openShare } from '../../src/apis/connect.ts' +import { close, openContext, openShare } from '../../src/apis/connect.ts' import { save } from '../../src/apis/save.ts' import { push, sync } from '../../src/apis/sync.ts' const url = Cypress.config('baseUrl').replace(/\/index.php\/?$/g, '') -Cypress.Commands.add('openFileConnection', openFile) +Cypress.Commands.add( + 'openFileConnection', + ({ fileId, filePath }) => { + return openContext({ type: 'file', id: fileId, filePath }) + }, +) + Cypress.Commands.add('openShareConnection', openShare) Cypress.Commands.add('closeConnection', close) @@ -18,7 +24,7 @@ Cypress.Commands.add('closeConnection', close) Cypress.Commands.add( 'failToCreateTextSession', (fileId, baseVersionEtag = null, options = {}) => { - return openFile({ fileId, ...options, baseVersionEtag }).then( + return openContext({ type: 'file', id: fileId, ...options, baseVersionEtag }).then( () => { throw new Error('Expected request to fail - but it succeeded!') }, diff --git a/lib/Controller/SessionController.php b/lib/Controller/SessionController.php index 0c4225fec87..3f383db799f 100644 --- a/lib/Controller/SessionController.php +++ b/lib/Controller/SessionController.php @@ -54,17 +54,11 @@ public function __construct( } #[NoAdminRequired] - public function create(?int $fileId = null, ?string $baseVersionEtag = null): DataResponse { - $type = 'file'; - $id = $fileId; - if ($id === null) { - return new DataResponse(['error' => 'No valid file argument provided'], Http::STATUS_PRECONDITION_FAILED); - } - + public function create(string $type, int $id, ?string $baseVersionEtag = null): DataResponse { try { $context = $this->contextManager->getContext($id, $type); } catch (NotFoundException|NotPermittedException $e) { - $this->logger->error('No permission to access this context', [ 'exception' => $e ]); + $this->logger->error('No context for ' . $type . ' (' . $id . ') ', [ 'exception' => $e ]); return new DataResponse([ 'error' => $this->l10n->t('File not found') ], Http::STATUS_NOT_FOUND); diff --git a/src/apis/connect.ts b/src/apis/connect.ts index 042f56ef2e2..e22e41c0669 100644 --- a/src/apis/connect.ts +++ b/src/apis/connect.ts @@ -9,8 +9,9 @@ import type { Document, GuestSession, Session } from '../services/SyncService.ts import axios from '@nextcloud/axios' import { generateUrl } from '@nextcloud/router' -export interface OpenParams { - fileId: number +export interface OpenContextParams { + type: string + id: number filePath: string // not send to the api but included in the connection baseVersionEtag?: string } @@ -38,9 +39,10 @@ export interface OpenData { * * @param params Parameters identifying the document */ -export async function openFile(params: OpenParams): Promise<{ connection: Connection, data: OpenData }> { - const url = generateUrl(`/apps/text/session/${params.fileId}/create`) - const response = await axios.put(url, params) +export async function openContext(params: OpenContextParams): Promise<{ connection: Connection, data: OpenData }> { + const { type, id, baseVersionEtag } = params + const url = generateUrl(`/apps/text/session/${type}/${id}/create`) + const response = await axios.put(url, { baseVersionEtag }) const { document, session } = response.data const connection = { documentId: document.id, diff --git a/src/composables/useConnection.ts b/src/composables/useConnection.ts index 2d853124a63..582c13e97ef 100644 --- a/src/composables/useConnection.ts +++ b/src/composables/useConnection.ts @@ -175,8 +175,9 @@ async function openFile( }, baseVersionEtag: string | undefined, ) { - return api.openFile({ - fileId: props.fileId, + return api.openContext({ + type: 'file', + id: props.fileId, filePath: props.relativePath, baseVersionEtag, }) diff --git a/src/tests/services/SyncService.spec.ts b/src/tests/services/SyncService.spec.ts index 2f0915e26f4..66744ca974f 100644 --- a/src/tests/services/SyncService.spec.ts +++ b/src/tests/services/SyncService.spec.ts @@ -54,7 +54,7 @@ describe('Sync service', () => { setBaseVersionEtag, ) vi.mock('../../apis/connect') - vi.mocked(connect.openFile).mockResolvedValue(openResult) + vi.mocked(connect.openContext).mockResolvedValue(openResult) const openHandler = vi.fn() const service = new SyncService({ connection, openConnection }) service.bus.on('opened', openHandler) From d0e8a86d9ff6388d26d06d163c74a6d8361f25af Mon Sep 17 00:00:00 2001 From: Max Date: Tue, 18 Aug 2026 09:23:04 +0200 Subject: [PATCH 09/30] enh(db): add context to document Signed-off-by: Max --- composer/composer/autoload_classmap.php | 1 + composer/composer/autoload_static.php | 1 + lib/Context/FileContext.php | 3 +- lib/Db/Document.php | 14 ++++- lib/Db/DocumentMapper.php | 18 ++++++- .../Version090000Date20260817110024.php | 52 +++++++++++++++++++ tests/unit/Db/SessionMapperTest.php | 3 +- 7 files changed, 87 insertions(+), 5 deletions(-) create mode 100644 lib/Migration/Version090000Date20260817110024.php diff --git a/composer/composer/autoload_classmap.php b/composer/composer/autoload_classmap.php index 71ecafa45e9..03d612e6175 100644 --- a/composer/composer/autoload_classmap.php +++ b/composer/composer/autoload_classmap.php @@ -73,6 +73,7 @@ 'OCA\\Text\\Migration\\Version040100Date20240611165300' => $baseDir . '/../lib/Migration/Version040100Date20240611165300.php', 'OCA\\Text\\Migration\\Version070000Date20250925110024' => $baseDir . '/../lib/Migration/Version070000Date20250925110024.php', 'OCA\\Text\\Migration\\Version080000Date20260331132113' => $baseDir . '/../lib/Migration/Version080000Date20260331132113.php', + 'OCA\\Text\\Migration\\Version090000Date20260817110024' => $baseDir . '/../lib/Migration/Version090000Date20260817110024.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 c235f061bde..041995902f6 100644 --- a/composer/composer/autoload_static.php +++ b/composer/composer/autoload_static.php @@ -88,6 +88,7 @@ class ComposerStaticInitText 'OCA\\Text\\Migration\\Version040100Date20240611165300' => __DIR__ . '/..' . '/../lib/Migration/Version040100Date20240611165300.php', 'OCA\\Text\\Migration\\Version070000Date20250925110024' => __DIR__ . '/..' . '/../lib/Migration/Version070000Date20250925110024.php', 'OCA\\Text\\Migration\\Version080000Date20260331132113' => __DIR__ . '/..' . '/../lib/Migration/Version080000Date20260331132113.php', + 'OCA\\Text\\Migration\\Version090000Date20260817110024' => __DIR__ . '/..' . '/../lib/Migration/Version090000Date20260817110024.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/Context/FileContext.php b/lib/Context/FileContext.php index 0a9a1a3d8d7..a0fc6fbcc80 100644 --- a/lib/Context/FileContext.php +++ b/lib/Context/FileContext.php @@ -51,7 +51,8 @@ public function buildDocument(): Document|string { return $this->l10n->t('This file cannot be displayed as download is disabled by the share'); } $document = new Document(); - $document->setId($this->getId()); + $document->setContextType('file'); + $document->setContextId($this->getId()); $document->setLastSavedVersion(0); $document->setLastSavedVersionTime($this->file->getMTime()); $document->setLastSavedVersionEtag($this->file->getEtag()); diff --git a/lib/Db/Document.php b/lib/Db/Document.php index 60e4e392f8b..58c0f227265 100644 --- a/lib/Db/Document.php +++ b/lib/Db/Document.php @@ -25,6 +25,10 @@ * @method setBaseVersionEtag(string $etag): void * @method getChecksum(): ?string * @method setChecksum(?string $checksum): void + * @method getContextType(): string + * @method setContextType(string $contextType): void + * @method getContextId(): int + * @method setContextId(int $contextId): void */ class Document extends Entity implements \JsonSerializable { public $id = null; @@ -36,6 +40,8 @@ class Document extends Entity implements \JsonSerializable { protected string $lastSavedVersionEtag = ''; protected string $baseVersionEtag = ''; protected ?string $checksum = null; + protected string $contextType = ''; + protected int $contextId = 0; public function __construct() { $this->addType('currentVersion', 'integer'); @@ -43,6 +49,8 @@ public function __construct() { $this->addType('lastSavedVersionTime', 'integer'); $this->addType('initialVersion', 'integer'); $this->addType('checksum', 'string'); + $this->addType('contextType', 'string'); + $this->addType('contextId', 'integer'); } public function jsonSerialize(): array { @@ -52,7 +60,9 @@ public function jsonSerialize(): array { 'lastSavedVersionTime' => $this->lastSavedVersionTime, 'baseVersionEtag' => $this->baseVersionEtag, 'initialVersion' => $this->initialVersion, - 'checksum' => $this->checksum + 'checksum' => $this->checksum, + 'contextType' => $this->contextType, + 'contextId' => $this->contextId, ]; } @@ -60,6 +70,6 @@ public function jsonSerialize(): array { * Short identifier - mostly for logging */ public function toString(): string { - return 'file' . ' (' . $this->id . ')'; + return $this->contextType . ' (' . $this->contextId . ')'; } } diff --git a/lib/Db/DocumentMapper.php b/lib/Db/DocumentMapper.php index 66f95ba6ff4..96d76dc30f9 100644 --- a/lib/Db/DocumentMapper.php +++ b/lib/Db/DocumentMapper.php @@ -26,6 +26,7 @@ public function __construct(IDBConnection $db) { * @throws DoesNotExistException */ public function find(int $documentId): Document { + /* @var $qb IQueryBuilder */ $qb = $this->db->getQueryBuilder(); $result = $qb->select('*') @@ -45,8 +46,23 @@ public function find(int $documentId): Document { * @throws DoesNotExistException */ public function load(IContext $context): Document { + $type = $context->getType(); $id = $context->getId(); - return $this->find($id); + + /* @var $qb IQueryBuilder */ + $qb = $this->db->getQueryBuilder(); + $result = $qb->select('*') + ->from($this->getTableName()) + ->where($qb->expr()->eq('context_type', $qb->createNamedParameter($type))) + ->where($qb->expr()->eq('context_id', $qb->createNamedParameter($id))) + ->executeQuery(); + + $data = $result->fetchAssociative(); + $result->closeCursor(); + if ($data === false) { + throw new DoesNotExistException('Document doesn\'t exist'); + } + return Document::fromRow($data); } public function findAll(): Generator { diff --git a/lib/Migration/Version090000Date20260817110024.php b/lib/Migration/Version090000Date20260817110024.php new file mode 100644 index 00000000000..5d17dd55a73 --- /dev/null +++ b/lib/Migration/Version090000Date20260817110024.php @@ -0,0 +1,52 @@ +getTable('text_documents'); + + if (!$table->hasColumn('context_type')) { + $table->addColumn('context_type', Types::STRING, [ + 'notnull' => true, + 'length' => 64, + ]); + } + + if (!$table->hasColumn('context_id')) { + $table->addColumn('context_id', Types::BIGINT, [ + 'notnull' => true, + 'unsigned' => true, + ]); + } + + $column = $table->getColumn('id'); + if (!$column->getAutoincrement()) { + $table->modifyColumn('id', [ + 'autoincrement' => true, + ]); + return $schema; + } + return null; + } +} diff --git a/tests/unit/Db/SessionMapperTest.php b/tests/unit/Db/SessionMapperTest.php index eda047c2e52..b0da154b7a1 100644 --- a/tests/unit/Db/SessionMapperTest.php +++ b/tests/unit/Db/SessionMapperTest.php @@ -146,7 +146,8 @@ public function testDeleteOrphanedSteps() { // Create document $document = $this->documentMapper->insert(Document::fromParams([ - 'id' => 1, + 'contextId' => 123, + 'contextType' => 'file', 'currentVersion' => 0, 'lastSavedVersion' => 100, 'lastSavedVersionTime' => time() From afc80ee85a4b3ea51cc81145b1cd15271e2f3eff Mon Sep 17 00:00:00 2001 From: Max Date: Tue, 18 Aug 2026 09:23:34 +0200 Subject: [PATCH 10/30] chore(version): bump to run migration Signed-off-by: Max --- appinfo/info.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/appinfo/info.xml b/appinfo/info.xml index 6dc7cdd8b44..c8335c42950 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -15,7 +15,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! ]]> - 9.0.0-dev.0 + 9.0.0-dev.1 agpl Julius Härtl Text From e8a8abb75b0bcc3d725090f9258e3aba002038ef Mon Sep 17 00:00:00 2001 From: Max Date: Tue, 18 Aug 2026 09:26:49 +0200 Subject: [PATCH 11/30] chore(refactor): store outside change in the DocumentSaveConflictException This saves one file read and makes handling the exception easier Signed-off-by: Max --- .../DocumentSaveConflictException.php | 15 +++++++++++++++ lib/Service/ApiService.php | 18 ++++-------------- lib/Service/DocumentService.php | 2 +- 3 files changed, 20 insertions(+), 15 deletions(-) diff --git a/lib/Exception/DocumentSaveConflictException.php b/lib/Exception/DocumentSaveConflictException.php index 6f7a73139dd..a6556124c33 100644 --- a/lib/Exception/DocumentSaveConflictException.php +++ b/lib/Exception/DocumentSaveConflictException.php @@ -9,5 +9,20 @@ namespace OCA\Text\Exception; +use Throwable; + class DocumentSaveConflictException extends \Exception { + + public function __construct( + private readonly string $content, + int $code = 0, + ?Throwable $previous = null, + ) { + $message = 'File changed in the meantime from outside'; + parent::__construct($message, $code, $previous); + } + + public function getContent(): string { + return $this->content; + } } diff --git a/lib/Service/ApiService.php b/lib/Service/ApiService.php index ab8d16754fe..8f44fede9d1 100644 --- a/lib/Service/ApiService.php +++ b/lib/Service/ApiService.php @@ -25,7 +25,6 @@ use OCP\Files\NotFoundException; use OCP\Files\NotPermittedException; use OCP\IL10N; -use OCP\Lock\LockedException; use Psr\Log\LoggerInterface; class ApiService { @@ -155,13 +154,8 @@ public function sync(Session $session, Document $document, int $version = 0, ?st return new DataResponse([ 'message' => 'Document no longer exists' ], Http::STATUS_NOT_FOUND); - } catch (DocumentSaveConflictException) { - try { - /** @psalm-suppress PossiblyUndefinedVariable */ - $result['outsideChange'] = $file->getContent(); - } catch (LockedException) { - // Ignore locked exception since it might happen due to an autosave action happening at the same time - } + } catch (DocumentSaveConflictException $e) { + $result['outsideChange'] = $e->getContent(); } return new DataResponse($result, isset($result['outsideChange']) ? Http::STATUS_CONFLICT : Http::STATUS_OK); @@ -185,12 +179,8 @@ public function save(Session $session, Document $document, int $version, string $result = []; try { $result['document'] = $this->documentService->autosave($document, $file, $version, $autosaveContent, $documentState, $force, $manualSave, $shareToken); - } catch (DocumentSaveConflictException) { - try { - $result['outsideChange'] = $file->getContent(); - } catch (LockedException) { - // Ignore locked exception since it might happen due to an autosave action happening at the same time - } + } catch (DocumentSaveConflictException $e) { + $result['outsideChange'] = $e->getContent(); } catch (NotPermittedException) { return new DataResponse([ 'error' => $this->l10n->t('Read-only permission cannot save document changes. Please reload the page.') diff --git a/lib/Service/DocumentService.php b/lib/Service/DocumentService.php index ff45887c7b2..3955d8c9e5b 100644 --- a/lib/Service/DocumentService.php +++ b/lib/Service/DocumentService.php @@ -349,7 +349,7 @@ public function assertNoOutsideConflict(Document $document, File $file, bool $fo $fileChecksum = self::computeCheckSum($fileContent); if ($storedChecksum !== $fileChecksum) { - throw new DocumentSaveConflictException('File changed in the meantime from outside'); + throw new DocumentSaveConflictException($fileContent); } $document->setLastSavedVersionTime($fileMtime); From e8222960507ad86d2ddec5e5b247880891f6b65d Mon Sep 17 00:00:00 2001 From: Max Date: Tue, 18 Aug 2026 09:28:44 +0200 Subject: [PATCH 12/30] enh(api): handle sync requests with context Signed-off-by: Max --- lib/Context/FileContext.php | 50 ++++++++++++++++++++++++--- lib/Context/IContext.php | 2 ++ lib/Service/ApiService.php | 11 ++++-- lib/Service/DocumentService.php | 8 +++++ tests/unit/Service/ApiServiceTest.php | 4 +++ 5 files changed, 68 insertions(+), 7 deletions(-) diff --git a/lib/Context/FileContext.php b/lib/Context/FileContext.php index a0fc6fbcc80..00c50081bb4 100644 --- a/lib/Context/FileContext.php +++ b/lib/Context/FileContext.php @@ -8,12 +8,16 @@ namespace OCA\Text\Context; use OCA\Text\Db\Document; +use OCA\Text\Exception\DocumentSaveConflictException; use OCA\Text\Service\FileService; use OCA\Text\Service\LockService; use OCP\Files\File; +use OCP\Files\GenericFileException; use OCP\Files\Lock\ILock; +use OCP\Files\NotPermittedException; use OCP\IL10N; use OCP\IUser; +use OCP\Lock\LockedException; use Override; use Psr\Log\LoggerInterface; @@ -90,12 +94,50 @@ public function prepareSession(DocumentData $documentData): SessionInfo { ); } - private function computeCheckSum(): string { - return hash('crc32', $this->file->getContent()); + public function isReadOnly(): bool { + return $this->fileService->isReadOnly($this->file, $this->token); } - private function isReadOnly(): bool { - return $this->fileService->isReadOnly($this->file, $this->token); + /** + * @throws DocumentSaveConflictException + * @throws GenericFileException if the file changed and reading the content fails. + * @throws LockedException if the file changed and a lock prevents reading the content. + * @throws NotPermittedException if the file changed and reading is not allowed. + * @return Document|null Updated document if there was an update + */ + public function updateDocument(Document $document): ?Document { + $lastMTime = $document->getLastSavedVersionTime(); + $lastEtag = $document->getLastSavedVersionEtag(); + + if ($lastMTime <= 0 || $this->isReadOnly()) { + return null; + } + + $fileMtime = $this->file->getMtime(); + $fileEtag = $this->file->getEtag(); + + if ($lastEtag === $fileEtag && $lastMTime === $fileMtime) { + return null; + } + + $storedChecksum = $document->getChecksum(); + $fileContent = $this->file->getContent(); + $fileChecksum = self::computeCheckSum($fileContent); + + if ($storedChecksum !== $fileChecksum) { + throw new DocumentSaveConflictException($fileContent); + } + + $document->setLastSavedVersionTime($fileMtime); + $document->setLastSavedVersionEtag($fileEtag); + return $document; + } + + private function computeCheckSum(?string $content = null): string { + if ($content === null) { + $content = $this->file->getContent(); + } + return hash('crc32', $content); } private function loadContent(): ?string { diff --git a/lib/Context/IContext.php b/lib/Context/IContext.php index 3be91ef838a..b8347d595ea 100644 --- a/lib/Context/IContext.php +++ b/lib/Context/IContext.php @@ -17,6 +17,8 @@ public function getType(): string; public function toString(): string; public function buildDocument(): Document|string; public function prepareSession(DocumentData $documentData): SessionInfo; + public function isReadOnly(): bool; + public function updateDocument(Document $document): ?Document; } readonly class DocumentData { diff --git a/lib/Service/ApiService.php b/lib/Service/ApiService.php index 8f44fede9d1..e8abbaf449c 100644 --- a/lib/Service/ApiService.php +++ b/lib/Service/ApiService.php @@ -12,6 +12,7 @@ use Exception; use InvalidArgumentException; use OCA\NotifyPush\Queue\IQueue; +use OCA\Text\Context\ContextManager; use OCA\Text\Context\IContext; use OCA\Text\Context\NewSessionData; use OCA\Text\Db\Document; @@ -30,6 +31,7 @@ class ApiService { public function __construct( private readonly ConfigService $configService, + private readonly ContextManager $contextManager, private readonly SessionService $sessionService, private readonly DocumentService $documentService, private readonly FileService $fileService, @@ -141,9 +143,12 @@ public function sync(Session $session, Document $document, int $version = 0, ?st ]; // ensure file is still present and accessible - $file = $this->fileService->getFileForSession($session, $shareToken); - $result['readOnly'] = $this->fileService->isReadOnly($file, $shareToken); - $this->documentService->assertNoOutsideConflict($document, $file); + $context = $this->contextManager->getContext($document->getContextId(), $document->getContextType()); + $result['readOnly'] = $context->isReadOnly(); + $document = $context->updateDocument($document); + if ($document) { + $this->documentService->updateDocument($document); + } } catch (NotPermittedException|NotFoundException|InvalidPathException $e) { $this->logger->info($e->getMessage(), ['exception' => $e]); return new DataResponse([ diff --git a/lib/Service/DocumentService.php b/lib/Service/DocumentService.php index 3955d8c9e5b..f45e529fb69 100644 --- a/lib/Service/DocumentService.php +++ b/lib/Service/DocumentService.php @@ -182,6 +182,14 @@ public function updateDocumentVersionInfo(File $file): void { $this->documentMapper->update($document); } + /** + * @throws Exception + * @throws InvalidPathException + */ + public function updateDocument(Document $document): void { + $this->documentMapper->update($document); + } + /** * @param int $documentId * diff --git a/tests/unit/Service/ApiServiceTest.php b/tests/unit/Service/ApiServiceTest.php index b0720cc9835..fa912b9e133 100644 --- a/tests/unit/Service/ApiServiceTest.php +++ b/tests/unit/Service/ApiServiceTest.php @@ -2,6 +2,7 @@ namespace OCA\Text\Tests; +use OCA\Text\Context\ContextManager; use OCA\Text\Context\DocumentData; use OCA\Text\Context\IContext; use OCA\Text\Context\SessionInfo; @@ -20,6 +21,7 @@ class ApiServiceTest extends \PHPUnit\Framework\TestCase { private ApiService $apiService; private ConfigService $configService; + private ContextManager $contextManager; private SessionService $sessionService; private DocumentService $documentService; private FileService $fileService; @@ -29,6 +31,7 @@ class ApiServiceTest extends \PHPUnit\Framework\TestCase { public function setUp(): void { $this->configService = $this->createStub(ConfigService::class); + $this->contextManager = $this->createStub(ContextManager::class); $this->sessionService = $this->createStub(SessionService::class); $this->documentService = $this->createStub(DocumentService::class); $this->fileService = $this->createStub(FileService::class); @@ -38,6 +41,7 @@ public function setUp(): void { $this->apiService = new ApiService( $this->configService, + $this->contextManager, $this->sessionService, $this->documentService, $this->fileService, From 2d019809c82084e6c984ef6092505528d8d4d560 Mon Sep 17 00:00:00 2001 From: Max Date: Tue, 18 Aug 2026 12:51:39 +0200 Subject: [PATCH 13/30] fix(attachments): get file via context Non-file contexts will need to return null. Signed-off-by: Max --- lib/Context/FileContext.php | 5 +++++ lib/Context/IContext.php | 2 ++ lib/Service/AttachmentService.php | 19 ++++++++++++++++--- 3 files changed, 23 insertions(+), 3 deletions(-) diff --git a/lib/Context/FileContext.php b/lib/Context/FileContext.php index 00c50081bb4..f362d9615df 100644 --- a/lib/Context/FileContext.php +++ b/lib/Context/FileContext.php @@ -48,6 +48,11 @@ public function toString(): string { return $this->getType() . ' (' . $this->getId() . ')'; } + #[Override] + public function getFile(): ?File { + return $this->file; + } + #[Override] public function buildDocument(): Document|string { // Block using text for disabled download internal shares diff --git a/lib/Context/IContext.php b/lib/Context/IContext.php index b8347d595ea..e35118fec79 100644 --- a/lib/Context/IContext.php +++ b/lib/Context/IContext.php @@ -9,6 +9,7 @@ use OCA\Text\Db\Document; use OCA\Text\Db\Session; +use OCP\Files\File; use OCP\Files\Lock\ILock; interface IContext { @@ -19,6 +20,7 @@ public function buildDocument(): Document|string; public function prepareSession(DocumentData $documentData): SessionInfo; public function isReadOnly(): bool; public function updateDocument(Document $document): ?Document; + public function getFile(): ?File; } readonly class DocumentData { diff --git a/lib/Service/AttachmentService.php b/lib/Service/AttachmentService.php index 7147630afc7..01f7683e93a 100755 --- a/lib/Service/AttachmentService.php +++ b/lib/Service/AttachmentService.php @@ -12,7 +12,9 @@ use OC\User\NoUserException; use OCA\DAV\Connector\Sabre\PublicAuth; use OCA\Files_Sharing\SharedStorage; +use OCA\Text\Context\ContextManager; use OCA\Text\Controller\AttachmentController; +use OCA\Text\Db\DocumentMapper; use OCA\Text\Db\Session; use OCP\Constants; use OCP\Files\File; @@ -44,6 +46,8 @@ public function __construct( private IFilenameValidator $filenameValidator, private IFilesMetadataManager $filesMetadataManager, private ISession $session, + private DocumentMapper $documentMapper, + private ContextManager $contextManager, ) { } @@ -515,12 +519,21 @@ private function isDownloadDisabled(File $file): bool { * @throws NotPermittedException */ private function getTextFile(int $documentId, string $userId): File { - $userFolder = $this->rootFolder->getUserFolder($userId); - $file = $userFolder->getFirstNodeById($documentId); + $document = $this->documentMapper->find($documentId); + $type = $document->getContextType(); + $id = $document->getContextId(); + $context = $this->contextManager->getContext($id, $type); + $file = $context->getFile(); if ($file instanceof File && !$this->isDownloadDisabled($file)) { return $file; } - throw new NotFoundException('Text file with id=' . $documentId . ' was not found in storage of ' . $userId); + throw new NotFoundException('Text file for document' + . $documentId + . ' (' + . $context->toString() + . ') was not found in storage of ' + . $userId + ); } /** From 9ee754ddd9151c454592bab0c0271b851634c301 Mon Sep 17 00:00:00 2001 From: Max Date: Wed, 19 Aug 2026 07:31:12 +0200 Subject: [PATCH 14/30] fix(share): consider share token in ContextManager Signed-off-by: Max --- lib/Context/ContextManager.php | 4 ++-- lib/Context/FileContextFactory.php | 16 ++++++++++++++++ lib/Controller/SessionController.php | 2 +- lib/Listeners/RegisterContextEventListener.php | 8 +++++++- lib/Service/ApiService.php | 4 +++- lib/Service/AttachmentService.php | 2 +- 6 files changed, 30 insertions(+), 6 deletions(-) diff --git a/lib/Context/ContextManager.php b/lib/Context/ContextManager.php index aa0e47911bd..893556fd145 100644 --- a/lib/Context/ContextManager.php +++ b/lib/Context/ContextManager.php @@ -42,12 +42,12 @@ public function registerContext(string $type, callable $createContext): void { $this->contexts[$type] = $createContext; } - public function getContext(int $id, string $type): IContext { + public function getContext(string $type, int $id, ?string $shareToken): IContext { $createContext = $this->getContexts()[$type]; if (!is_callable($createContext)) { throw new NotFoundException('Context of type "' . $type . '" was not registered!'); } - $context = $createContext($id, $type); + $context = $createContext($id, $type, $shareToken); if (!$context instanceof IContext) { throw new NotFoundException('Failed to create context of type ' . $type . '!'); } diff --git a/lib/Context/FileContextFactory.php b/lib/Context/FileContextFactory.php index 8916e5d7b94..b1b42ca19a6 100644 --- a/lib/Context/FileContextFactory.php +++ b/lib/Context/FileContextFactory.php @@ -57,6 +57,22 @@ public function buildForId( return $this->build($file); } + /** + * @throws NotFoundException if the file cannot be found + */ + public function buildForShareWithId( + string $token, + int $id, + ): FileContext { + $file = $this->fileService->getFileByIdFromShare($id, $token); + /* + * Check if we have proper read access (files drop) + * If not then well 404 it is. + */ + $this->fileService->checkSharePermissions($token); + return $this->build($file, $token); + } + /** * @throws NotFoundException if the file cannot be found * @throws \InvalidArgumentException if the share token is for a folder and path is missing diff --git a/lib/Controller/SessionController.php b/lib/Controller/SessionController.php index 3f383db799f..a62d05a7c1c 100644 --- a/lib/Controller/SessionController.php +++ b/lib/Controller/SessionController.php @@ -56,7 +56,7 @@ public function __construct( #[NoAdminRequired] public function create(string $type, int $id, ?string $baseVersionEtag = null): DataResponse { try { - $context = $this->contextManager->getContext($id, $type); + $context = $this->contextManager->getContext($type, $id, null); } catch (NotFoundException|NotPermittedException $e) { $this->logger->error('No context for ' . $type . ' (' . $id . ') ', [ 'exception' => $e ]); return new DataResponse([ diff --git a/lib/Listeners/RegisterContextEventListener.php b/lib/Listeners/RegisterContextEventListener.php index a1f539e7c39..82e53532817 100644 --- a/lib/Listeners/RegisterContextEventListener.php +++ b/lib/Listeners/RegisterContextEventListener.php @@ -30,7 +30,13 @@ public function handle(Event $event): void { $event->getContextManager()->registerContext( 'file', - fn (int $id) => $this->fileContextFactory->buildForId($id) + function (int $id, string $type, ?string $shareToken) { + if ($shareToken === null) { + return $this->fileContextFactory->buildForId($id); + } else { + return $this->fileContextFactory->buildForShareWithId($shareToken, $id); + } + } ); } } diff --git a/lib/Service/ApiService.php b/lib/Service/ApiService.php index e8abbaf449c..20dded4d6ff 100644 --- a/lib/Service/ApiService.php +++ b/lib/Service/ApiService.php @@ -143,7 +143,9 @@ public function sync(Session $session, Document $document, int $version = 0, ?st ]; // ensure file is still present and accessible - $context = $this->contextManager->getContext($document->getContextId(), $document->getContextType()); + $type = $document->getContextType(); + $id = $document->getContextId(); + $context = $this->contextManager->getContext($type, $id, $shareToken); $result['readOnly'] = $context->isReadOnly(); $document = $context->updateDocument($document); if ($document) { diff --git a/lib/Service/AttachmentService.php b/lib/Service/AttachmentService.php index 01f7683e93a..55c1f2ded31 100755 --- a/lib/Service/AttachmentService.php +++ b/lib/Service/AttachmentService.php @@ -522,7 +522,7 @@ private function getTextFile(int $documentId, string $userId): File { $document = $this->documentMapper->find($documentId); $type = $document->getContextType(); $id = $document->getContextId(); - $context = $this->contextManager->getContext($id, $type); + $context = $this->contextManager->getContext($type, $id, null); $file = $context->getFile(); if ($file instanceof File && !$this->isDownloadDisabled($file)) { return $file; From 1c826102d0eba3114720fa07be37dc02e39e7bcf Mon Sep 17 00:00:00 2001 From: Max Date: Wed, 19 Aug 2026 07:32:03 +0200 Subject: [PATCH 15/30] fix(push): use context to check if file is readonly Signed-off-by: Max --- lib/Service/DocumentService.php | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/Service/DocumentService.php b/lib/Service/DocumentService.php index f45e529fb69..89127e92142 100644 --- a/lib/Service/DocumentService.php +++ b/lib/Service/DocumentService.php @@ -10,6 +10,7 @@ namespace OCA\Text\Service; use InvalidArgumentException; +use OCA\Text\Context\ContextManager; use OCA\Text\Context\DocumentData; use OCA\Text\Context\IContext; use OCA\Text\Db\Document; @@ -52,6 +53,7 @@ class DocumentService { private readonly ICache $cache; public function __construct( + private readonly ContextManager $contextManager, private readonly DocumentMapper $documentMapper, private readonly FileService $fileService, private readonly StepMapper $stepMapper, @@ -242,8 +244,10 @@ public function addStep(Document $document, Session $session, array $steps, int } } if (count($stepsToInsert) > 0) { - $file = $this->fileService->getFileForSession($session, $shareToken); - if (!$this->fileService->isReadOnly($file, $shareToken)) { + $type = $document->getContextType(); + $id = $document->getContextId(); + $context = $this->contextManager->getContext($type, $id, $shareToken); + if (!$context->isReadOnly()) { $this->insertSteps($document, $session, $stepsToInsert); } } From 1031a582f41ac418b448a33913b309d64a186048 Mon Sep 17 00:00:00 2001 From: Max Date: Wed, 19 Aug 2026 10:46:35 +0200 Subject: [PATCH 16/30] enh(save): use context for saving Signed-off-by: Max --- lib/Context/FileContext.php | 17 ++++-- lib/Context/IContext.php | 2 + lib/Service/ApiService.php | 14 ++--- lib/Service/DocumentService.php | 81 ++++++++------------------- lib/Service/FileService.php | 23 -------- lib/Service/LockService.php | 3 + tests/unit/Service/ApiServiceTest.php | 5 -- 7 files changed, 47 insertions(+), 98 deletions(-) diff --git a/lib/Context/FileContext.php b/lib/Context/FileContext.php index f362d9615df..ae8e6e9b156 100644 --- a/lib/Context/FileContext.php +++ b/lib/Context/FileContext.php @@ -104,6 +104,8 @@ public function isReadOnly(): bool { } /** + * Update the document last saved version metadata to be in line with the data saved in the context. + * * @throws DocumentSaveConflictException * @throws GenericFileException if the file changed and reading the content fails. * @throws LockedException if the file changed and a lock prevents reading the content. @@ -138,6 +140,17 @@ public function updateDocument(Document $document): ?Document { return $document; } + public function loadContent(): ?string { + return $this->fileService->loadContent($this->file); + } + + public function saveWithLock(string $content, callable $doWhileLocked): void { + $this->lockService->runInScope($this->file, function () use ($content, $doWhileLocked): void { + $this->file->putContent($content); + $doWhileLocked(); + }); + } + private function computeCheckSum(?string $content = null): string { if ($content === null) { $content = $this->file->getContent(); @@ -145,10 +158,6 @@ private function computeCheckSum(?string $content = null): string { return hash('crc32', $content); } - private function loadContent(): ?string { - return $this->fileService->loadContent($this->file); - } - private function getLockInfo(): ?ILock { return $this->lockService->getLockByOthers($this->file); } diff --git a/lib/Context/IContext.php b/lib/Context/IContext.php index e35118fec79..2ce52ee1004 100644 --- a/lib/Context/IContext.php +++ b/lib/Context/IContext.php @@ -21,6 +21,8 @@ public function prepareSession(DocumentData $documentData): SessionInfo; public function isReadOnly(): bool; public function updateDocument(Document $document): ?Document; public function getFile(): ?File; + public function loadContent(): ?string; + public function saveWithLock(string $content, callable $doWhileLocked): void; } readonly class DocumentData { diff --git a/lib/Service/ApiService.php b/lib/Service/ApiService.php index 20dded4d6ff..883a753ed9b 100644 --- a/lib/Service/ApiService.php +++ b/lib/Service/ApiService.php @@ -34,7 +34,6 @@ public function __construct( private readonly ContextManager $contextManager, private readonly SessionService $sessionService, private readonly DocumentService $documentService, - private readonly FileService $fileService, private readonly LoggerInterface $logger, private readonly LockService $lockService, private readonly IL10N $l10n, @@ -170,22 +169,19 @@ public function sync(Session $session, Document $document, int $version = 0, ?st public function save(Session $session, Document $document, int $version, string $autosaveContent, string $documentState, bool $force = false, bool $manualSave = false, ?string $shareToken = null): DataResponse { try { - $file = $this->fileService->getFileForSession($session, $shareToken); - } catch (NotPermittedException|NotFoundException $e) { + $type = $document->getContextType(); + $id = $document->getContextId(); + $context = $this->contextManager->getContext($type, $id, $shareToken); + } catch (NotFoundException $e) { $this->logger->info($e->getMessage(), ['exception' => $e]); return new DataResponse([ 'message' => 'File not found' ], Http::STATUS_NOT_FOUND); - } catch (DoesNotExistException $e) { - $this->logger->info($e->getMessage(), ['exception' => $e]); - return new DataResponse([ - 'message' => 'Document no longer exists' - ], Http::STATUS_NOT_FOUND); } $result = []; try { - $result['document'] = $this->documentService->autosave($document, $file, $version, $autosaveContent, $documentState, $force, $manualSave, $shareToken); + $result['document'] = $this->documentService->autosave($document, $context, $version, $autosaveContent, $documentState, $force, $manualSave, $shareToken); } catch (DocumentSaveConflictException $e) { $result['outsideChange'] = $e->getContent(); } catch (NotPermittedException) { diff --git a/lib/Service/DocumentService.php b/lib/Service/DocumentService.php index 89127e92142..a236a9017d3 100644 --- a/lib/Service/DocumentService.php +++ b/lib/Service/DocumentService.php @@ -335,40 +335,6 @@ public function getSteps(int $documentId, int $lastVersion): array { return $this->stepMapper->find($documentId, $lastVersion); } - /** - * @throws DocumentSaveConflictException - * @throws InvalidPathException - * @throws NotFoundException - */ - public function assertNoOutsideConflict(Document $document, File $file, bool $force = false, ?string $shareToken = null): void { - $documentId = $document->getId(); - $lastMTime = $document->getLastSavedVersionTime(); - $lastEtag = $document->getLastSavedVersionEtag(); - - if ($lastMTime <= 0 || $force || $this->fileService->isReadOnly($file, $shareToken) || $this->cache->get('document-save-lock-' . $documentId)) { - return; - } - - $fileMtime = $file->getMtime(); - $fileEtag = $file->getEtag(); - - if ($lastEtag === $fileEtag && $lastMTime === $fileMtime) { - return; - } - - $storedChecksum = $document->getChecksum(); - $fileContent = $file->getContent(); - $fileChecksum = self::computeCheckSum($fileContent); - - if ($storedChecksum !== $fileChecksum) { - throw new DocumentSaveConflictException($fileContent); - } - - $document->setLastSavedVersionTime($fileMtime); - $document->setLastSavedVersionEtag($fileEtag); - $this->documentMapper->update($document); - } - /** * @param string $content * @return string @@ -385,19 +351,25 @@ public static function computeCheckSum(string $content): string { * @throws NotPermittedException * @throws Exception */ - public function autosave(Document $document, File $file, int $version, string $autoSaveDocument, string $documentState, bool $force = false, bool $manualSave = false, ?string $shareToken = null): Document { - $documentId = $document->getId(); - - if ($this->fileService->isReadOnly($file, $shareToken)) { + public function autosave(Document $document, IContext $context, int $version, string $autoSaveDocument, string $documentState, bool $force = false, bool $manualSave = false, ?string $shareToken = null): Document { + if ($context->isReadOnly()) { throw new NotPermittedException('Read-only permission cannot save document changes. Please reload the page.'); } - $this->assertNoOutsideConflict($document, $file, $force); + $lastMTime = $document->getLastSavedVersionTime(); + if ($lastMTime > 0 && !$force && !$this->cache->get('document-save-lock-' . $document->id)) { + $updatedDocument = $context->updateDocument($document); + if ($updatedDocument !== null) { + $document = $updatedDocument; + $lastMTime = $document->getLastSavedVersionTime(); + $this->documentMapper->update($document); + } + } // Do not save if newer version already saved // Note that $version is the version of the steps the client has fetched. // It may have added steps on top of that - so if the versions match we still save. - $stepsVersion = $this->stepMapper->getLatestVersion($documentId) ?? 0; + $stepsVersion = $this->stepMapper->getLatestVersion($document->id) ?? 0; $savedVersion = $document->getLastSavedVersion(); $outdated = $savedVersion > 0 && $savedVersion > $version; if (!$force && ($outdated || $version > (string)$stepsVersion)) { @@ -405,50 +377,45 @@ public function autosave(Document $document, File $file, int $version, string $a } // Only save once every AUTOSAVE_MINIMUM_DELAY seconds - $lastMTime = $document->getLastSavedVersionTime(); - if ($file->getMTime() === $lastMTime && $lastMTime > time() - self::AUTOSAVE_MINIMUM_DELAY && $manualSave === false) { + if ($lastMTime > time() - self::AUTOSAVE_MINIMUM_DELAY && $manualSave === false) { return $document; } if (empty($autoSaveDocument)) { + $file = $context->getFile(); $this->logger->warning('Saving empty document', [ 'requestVersion' => $version, 'requestAutosaveDocument' => $autoSaveDocument, 'requestDocumentState' => $documentState, 'document' => $document->jsonSerialize(), - 'fileSizeBeforeSave' => $file->getSize(), - 'steps' => array_map(static fn (Step $step) => $step->jsonSerialize(), $this->stepMapper->find($documentId, 0)), - 'sessions' => array_map(static fn (Session $session) => $session->jsonSerialize(), $this->sessionMapper->findAll($documentId)) + 'fileSizeBeforeSave' => $file ? $file->getSize() : $context->getType() . ' is not stored in a file', + 'steps' => array_map(static fn (Step $step) => $step->jsonSerialize(), $this->stepMapper->find($document->id, 0)), + 'sessions' => array_map(static fn (Session $session) => $session->jsonSerialize(), $this->sessionMapper->findAll($document->id)) ]); } // Version changed but the content remains the same - if ($autoSaveDocument === $file->getContent()) { - $this->writeDocumentState($file->getId(), $documentState); + if ($autoSaveDocument === $context->loadContent()) { + $this->writeDocumentState($document->id, $documentState); $document->setLastSavedVersion($version); - $document->setLastSavedVersionTime($file->getMTime()); - $document->setLastSavedVersionEtag($file->getEtag()); $this->documentMapper->update($document); return $document; } - $this->cache->set('document-save-lock-' . $documentId, true, 10); + $this->cache->set('document-save-lock-' . $document->id, true, 10); + $this->saveFromText = true; try { - $this->lockService->runInScope($file, function () use ($file, $autoSaveDocument, $documentState): void { - $this->saveFromText = true; - $file->putContent($autoSaveDocument); - $this->writeDocumentState($file->getId(), $documentState); + $context->saveWithLock($autoSaveDocument, function () use ($document, $documentState): void { + $this->writeDocumentState($document->id, $documentState); }); $document->setLastSavedVersion($version); - $document->setLastSavedVersionTime($file->getMTime()); - $document->setLastSavedVersionEtag($file->getEtag()); $document->setChecksum(self::computeCheckSum($autoSaveDocument)); $this->documentMapper->update($document); } catch (LockedException) { // Ignore lock since it might occur when multiple people save at the same time return $document; } finally { - $this->cache->remove('document-save-lock-' . $documentId); + $this->cache->remove('document-save-lock-' . $document->id); } return $document; } diff --git a/lib/Service/FileService.php b/lib/Service/FileService.php index e05a8643a3a..1449e4d922d 100644 --- a/lib/Service/FileService.php +++ b/lib/Service/FileService.php @@ -8,7 +8,6 @@ namespace OCA\Text\Service; use OCA\Files_Sharing\SharedStorage; -use OCA\Text\Db\Session; use OCA\Text\Exception\InvalidSessionException; use OCP\Constants; use OCP\Files\File; @@ -35,28 +34,6 @@ public function __construct( ) { } - /** - * @throws NotPermittedException - * @throws NotFoundException - */ - public function getFileForSession(Session $session, ?string $shareToken = null): File { - if (!$session->isGuest()) { - try { - return $this->getFileById($session->getDocumentId(), $session->getUserId()); - } catch (NotFoundException $e) { - if ($shareToken === null) { - throw $e; - } - // We may still have a user session but on a public share link so move on - } - } - - if ($shareToken === null) { - throw new \InvalidArgumentException('No proper share data'); - } - return $this->getFileByIdFromShare($session->getDocumentId(), $shareToken); - } - /** * @throws NotFoundException */ diff --git a/lib/Service/LockService.php b/lib/Service/LockService.php index 9bce9254824..1f717d57674 100644 --- a/lib/Service/LockService.php +++ b/lib/Service/LockService.php @@ -23,6 +23,9 @@ public function __construct( ) { } + /** + * @throws PreConditionNotMetException if another lock scope is already active + */ public function runInScope(File $file, callable $callback): void { $this->lockManager->runInScope( new LockContext( diff --git a/tests/unit/Service/ApiServiceTest.php b/tests/unit/Service/ApiServiceTest.php index fa912b9e133..1309696819d 100644 --- a/tests/unit/Service/ApiServiceTest.php +++ b/tests/unit/Service/ApiServiceTest.php @@ -11,7 +11,6 @@ use OCA\Text\Service\ApiService; use OCA\Text\Service\ConfigService; use OCA\Text\Service\DocumentService; -use OCA\Text\Service\FileService; use OCA\Text\Service\LockService; use OCA\Text\Service\SessionService; use OCP\IL10N; @@ -24,7 +23,6 @@ class ApiServiceTest extends \PHPUnit\Framework\TestCase { private ContextManager $contextManager; private SessionService $sessionService; private DocumentService $documentService; - private FileService $fileService; private LoggerInterface $loggerInterface; private LockService $lockService; private IL10N $l10n; @@ -34,7 +32,6 @@ public function setUp(): void { $this->contextManager = $this->createStub(ContextManager::class); $this->sessionService = $this->createStub(SessionService::class); $this->documentService = $this->createStub(DocumentService::class); - $this->fileService = $this->createStub(FileService::class); $this->loggerInterface = $this->createStub(LoggerInterface::class); $this->lockService = $this->createStub(LockService::class); $this->l10n = $this->createStub(IL10N::class); @@ -44,7 +41,6 @@ public function setUp(): void { $this->contextManager, $this->sessionService, $this->documentService, - $this->fileService, $this->loggerInterface, $this->lockService, $this->l10n, @@ -86,7 +82,6 @@ public function testSaveWithNotPermittedException() { $file = $this->mockFile(123, 'admin'); - $this->fileService->method('getFileForSession')->willReturn($file); $this->documentService->method('autosave')->willThrowException(new \OCP\Files\NotPermittedException()); $this->l10n->method('t') From 84cb573ae54391ff2acb86a5c9769ca9bb344c6b Mon Sep 17 00:00:00 2001 From: Max Date: Wed, 19 Aug 2026 11:29:37 +0200 Subject: [PATCH 17/30] chore(cleanup): redundant check for lock provider We handle the NoLockProviderException gracefully anyway. Signed-off-by: Max --- lib/Service/LockService.php | 8 -------- 1 file changed, 8 deletions(-) diff --git a/lib/Service/LockService.php b/lib/Service/LockService.php index 1f717d57674..a64fd133b60 100644 --- a/lib/Service/LockService.php +++ b/lib/Service/LockService.php @@ -52,10 +52,6 @@ public function getLockByOthers(File $file): ?ILock { } public function lock(File $file): bool { - if (!$this->lockManager->isLockProviderAvailable()) { - return true; - } - try { $this->lockManager->lock(new LockContext( $file, @@ -70,10 +66,6 @@ public function lock(File $file): bool { } public function unlock(File $file): void { - if (!$this->lockManager->isLockProviderAvailable()) { - return; - } - try { $this->lockManager->unlock(new LockContext( $file, From f5a43cec64fab4cdb01ade39c44a46dae8abd8a7 Mon Sep 17 00:00:00 2001 From: Max Date: Wed, 19 Aug 2026 11:55:41 +0200 Subject: [PATCH 18/30] chore(tweak): return first editable file early Addresses https://github.com/nextcloud/text/pull/9028#discussion_r3811480603 . Signed-off-by: Max --- lib/Service/FileService.php | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/lib/Service/FileService.php b/lib/Service/FileService.php index 1449e4d922d..97e1a169e9a 100644 --- a/lib/Service/FileService.php +++ b/lib/Service/FileService.php @@ -74,27 +74,29 @@ public function getFileById(int $fileId, string $userId): File { return $file; } - // Ideally we'd optimize this part in the future by storing the path and getting the acutal target directly - $files = $userFolder->getById($fileId); + // Ideally we'd optimize this part in the future by storing the path and getting the actual target directly + $files = array_filter($userFolder->getById($fileId), fn (Node $f) => $f instanceof File); if (count($files) === 0) { throw new NotFoundException(); } // Workaround to always open files with edit permissions if multiple occurrences of // the same file id are in the user home, ideally we should also track the path of the file when opening - usort($files, static fn (Node $a, Node $b) => ($b->getPermissions() & Constants::PERMISSION_UPDATE) <=> ($a->getPermissions() & Constants::PERMISSION_UPDATE)); - - $file = array_shift($files); - - if (!$file instanceof File) { - throw new NotFoundException(); + $readableFile = null; + foreach ($files as $file) { + $permissions = $file->getPermissions(); + if ($permissions & Constants::PERMISSION_READ && $permissions & Constants::PERMISSION_UPDATE) { + return $file; + } + if ($permissions & Constants::PERMISSION_READ) { + $readableFile = $file; + } } - - if (($file->getPermissions() & Constants::PERMISSION_READ) !== Constants::PERMISSION_READ) { - throw new NotPermittedException(); + if ($readableFile !== null) { + return $readableFile; } - return $file; + throw new NotPermittedException(); } /** From 7995a5ad56640852b6478a06d9dfb965a1f2872a Mon Sep 17 00:00:00 2001 From: Max Date: Wed, 19 Aug 2026 15:42:38 +0200 Subject: [PATCH 19/30] fix(session): also consider `shareToken` parameter as used by AttachmentsController Most controllers use the `token` parameter for shares. The AttachmentsController however uses the `shareToken` parameter. This prevented `assertDocumentSession` from passing, as the middleware always tried to set a userId which was not present. Signed-off-by: Max --- lib/Middleware/SessionMiddleware.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/Middleware/SessionMiddleware.php b/lib/Middleware/SessionMiddleware.php index aa6a0644504..042a11f2c5b 100644 --- a/lib/Middleware/SessionMiddleware.php +++ b/lib/Middleware/SessionMiddleware.php @@ -94,7 +94,8 @@ private function assertDocumentSession(ISessionAwareController $controller): voi $documentId = (int)$this->request->getParam('documentId'); $sessionId = (int)$this->request->getParam('sessionId'); $token = (string)$this->request->getParam('sessionToken'); - $shareToken = (string)$this->request->getParam('token'); + $shareToken = (string)$this->request->getParam('token') + || (string)$this->request->getParam('shareToken'); $session = $this->sessionService->getValidSession($documentId, $sessionId, $token); if (!$session) { From d6fa3051e04afaa881219419f3e5fee98e0423d5 Mon Sep 17 00:00:00 2001 From: Max Date: Wed, 19 Aug 2026 15:52:04 +0200 Subject: [PATCH 20/30] fix(middleware): get fileId from context rather than documentId Signed-off-by: Max --- lib/Middleware/SessionMiddleware.php | 12 +++++++--- lib/Service/FileService.php | 11 ++++----- .../unit/Middleware/SessionMiddlewareTest.php | 22 +++++++++++------- tests/unit/Service/FileServiceTest.php | 23 ++++++++----------- 4 files changed, 37 insertions(+), 31 deletions(-) diff --git a/lib/Middleware/SessionMiddleware.php b/lib/Middleware/SessionMiddleware.php index 042a11f2c5b..c7a08847f17 100644 --- a/lib/Middleware/SessionMiddleware.php +++ b/lib/Middleware/SessionMiddleware.php @@ -128,18 +128,24 @@ private function assertDocumentSession(ISessionAwareController $controller): voi * @throws InvalidSessionException */ private function assertUserOrShareToken(ISessionAwareController $controller): void { - $fileId = (int)$this->request->getParam('documentId'); + $documentId = (int)$this->request->getParam('documentId'); $shareToken = (string)$this->request->getParam('shareToken'); $userId = $this->userSession->getUser()?->getUID(); + $document = $this->documentService->getDocument($documentId); + if (!$document || $document->getContextType() !== 'file') { + throw new InvalidSessionException(); + } + $fileId = $document->getContextId(); + if ($shareToken !== '') { - $documentId = $this->fileService->getDocumentIdFromShare($fileId, $shareToken); + $this->fileService->checkFileAccessFromShare($fileId, $shareToken); $controller->setDocumentId($documentId); return; } if ($userId !== null) { - $documentId = $this->fileService->getDocumentIdForUser($fileId, $userId); + $this->fileService->checkFileAccessForUser($fileId, $userId); $controller->setUserId($userId); $controller->setDocumentId($documentId); return; diff --git a/lib/Service/FileService.php b/lib/Service/FileService.php index 97e1a169e9a..1af9270dcd5 100644 --- a/lib/Service/FileService.php +++ b/lib/Service/FileService.php @@ -168,7 +168,7 @@ public function checkSharePermissions(string $shareToken, int $permission = Cons } } - public function getDocumentIdFromShare(int $fileId, string $shareToken): int { + public function checkFileAccessFromShare(int $fileId, string $shareToken): void { try { $share = $this->shareManager->getShareByToken($shareToken); } catch (ShareNotFound) { @@ -208,15 +208,12 @@ public function getDocumentIdFromShare(int $fileId, string $shareToken): int { if ($attributes !== null && $attributes->getAttribute('permissions', 'download') === false) { throw new InvalidSessionException(); } - - return $fileId; } - public function getDocumentIdForUser(int $fileId, string $userId): int { - if ($this->rootFolder->getUserFolder($userId)->getFirstNodeById($fileId) !== null) { - return $fileId; + public function checkFileAccessForUser(int $fileId, string $userId): void { + if ($this->rootFolder->getUserFolder($userId)->getFirstNodeById($fileId) === null) { + throw new InvalidSessionException(); } - throw new InvalidSessionException(); } public function loadContent(File $file): ?string { diff --git a/tests/unit/Middleware/SessionMiddlewareTest.php b/tests/unit/Middleware/SessionMiddlewareTest.php index c48167842ef..a4cccdde4df 100644 --- a/tests/unit/Middleware/SessionMiddlewareTest.php +++ b/tests/unit/Middleware/SessionMiddlewareTest.php @@ -39,6 +39,12 @@ protected function setUp(): void { $this->userManager = $this->createMock(IUserManager::class); $this->fileService = $this->createMock(FileService::class); + $document = new Document(); + $document->setId(111); + $document->setContextId(999); + $document->setContextType('file'); + $this->documentService->method('getDocument')->with(111)->willReturn($document); + $this->middleware = new SessionMiddleware( $this->request, $this->sessionService, @@ -53,20 +59,20 @@ protected function setUp(): void { public function testUnauthenticatedAccessBlocked(): void { $this->expectException(InvalidSessionException::class); - $this->fileService->method('getDocumentIdFromShare')->with(999, 'token')->willThrowException(new InvalidSessionException()); + $this->fileService->expects($this->once())->method('checkFileAccessFromShare')->with(999, 'token')->willThrowException(new InvalidSessionException()); $this->invokeMiddleware('token'); } public function testAuthenticatedSingleIdAllowed(): void { - $this->fileService->method('getDocumentIdFromShare')->with(999, 'token')->willReturn(999); + $this->fileService->expects($this->once())->method('checkFileAccessFromShare')->with(999, 'token'); $this->invokeMiddleware('token'); $this->assertTrue(true); } public function testLoggedInUserWithValidToken(): void { - $this->fileService->method('getDocumentIdFromShare')->with(999, 'token')->willReturn(999); + $this->fileService->expects($this->once())->method('checkFileAccessFromShare')->with(999, 'token'); $controller = $this->createMock(ISessionAwareController::class); $controller->expects($this->never())->method('setUserId'); @@ -79,7 +85,7 @@ public function testLoggedInUserWithOwnFile(): void { $user = $this->createMock(IUser::class); $user->method('getUID')->willReturn('user1'); - $this->fileService->method('getDocumentIdForUser')->with(999, 'user1')->willReturn(999); + $this->fileService->expects($this->once())->method('checkFileAccessForUser')->with(999, 'user1'); $controller = $this->createMock(ISessionAwareController::class); $controller->expects($this->once())->method('setUserId'); @@ -94,7 +100,7 @@ public function testLoggedInUserMissingFile(): void { $user = $this->createMock(IUser::class); $user->method('getUID')->willReturn('user1'); - $this->fileService->method('getDocumentIdForUser')->with(999, 'user1')->willThrowException(new InvalidSessionException()); + $this->fileService->expects($this->once())->method('checkFileAccessForUser')->with(999, 'user1')->willThrowException(new InvalidSessionException()); $this->invokeMiddleware(null, 'user1'); } @@ -105,7 +111,7 @@ public function testLoggedInUserWithValidTokenMissingPassword(): void { $user = $this->createMock(IUser::class); $user->method('getUID')->willReturn('user1'); - $this->fileService->method('getDocumentIdFromShare')->with(999, 'token')->willThrowException(new InvalidSessionException()); + $this->fileService->expects($this->once())->method('checkFileAccessFromShare')->with(999, 'token')->willThrowException(new InvalidSessionException()); $this->invokeMiddleware('token', 'user1'); } @@ -185,7 +191,7 @@ public function testAfterExceptionMapsAccountDisabledToForbidden(): void { private function invokeAssertDocumentSession(ISessionAwareController $controller, ?string $shareToken = null): void { $this->request->method('getParam')->willReturnMap([ - ['documentId', null, 999], + ['documentId', null, 111], ['sessionId', null, 1], ['sessionToken', null, 'sessionToken'], ['token', null, $shareToken], @@ -196,7 +202,7 @@ private function invokeAssertDocumentSession(ISessionAwareController $controller private function invokeMiddleware(?string $token, ?string $userName = null, ?ISessionAwareController $controller = null): void { $this->request->method('getParam')->willReturnMap([ - ['documentId', null, 999], + ['documentId', null, 111], ['shareToken', null, $token], ]); diff --git a/tests/unit/Service/FileServiceTest.php b/tests/unit/Service/FileServiceTest.php index ba5cffbafb2..2a32f3b335b 100644 --- a/tests/unit/Service/FileServiceTest.php +++ b/tests/unit/Service/FileServiceTest.php @@ -79,14 +79,13 @@ public function testInvalidToken(): void { $this->shareManager->method('getShareByToken')->with('invalid')->willThrowException(new ShareNotFound()); - $this->fileService->getDocumentIdFromShare(123, 'invalid'); + $this->fileService->checkFileAccessFromShare(123, 'invalid'); } public function testValidTokenWithoutPassword(): void { $share = $this->createShare('plain-share'); - $result = $this->invokeGetDocumentIdFromShare(123, $share); - self::assertEquals(123, $result); + $this->invokeCheckFileAccessFromShare(123, $share); } public function testValidTokenMissingPassword(): void { @@ -95,23 +94,21 @@ public function testValidTokenMissingPassword(): void { $share = $this->createShare('protected-share', 'password'); $this->session->method('get')->with('public_link_authenticated')->willReturn(null); - $this->invokeGetDocumentIdFromShare(123, $share); + $this->invokeCheckFileAccessFromShare(123, $share); } public function testValidTokenWithPasswordArray(): void { $share = $this->createShare('42', 'password'); $this->session->method('get')->with('public_link_authenticated')->willReturn(['1', '42']); - $result = $this->invokeGetDocumentIdFromShare(123, $share); - self::assertEquals(123, $result); + $this->invokeCheckFileAccessFromShare(123, $share); } public function testValidTokenWithSinglePassword(): void { $share = $this->createShare('42', 'password'); $this->session->method('get')->with('public_link_authenticated')->willReturn('42'); - $result = $this->invokeGetDocumentIdFromShare(123, $share); - self::assertEquals(123, $result); + $this->invokeCheckFileAccessFromShare(123, $share); } public function testValidTokenWithOtherPassword(): void { @@ -120,7 +117,7 @@ public function testValidTokenWithOtherPassword(): void { $share = $this->createShare('42', 'password'); $this->session->method('get')->with('public_link_authenticated')->willReturn('10'); - $this->invokeGetDocumentIdFromShare(123, $share); + $this->invokeCheckFileAccessFromShare(123, $share); } public function testValidTokenWithOtherPasswords(): void { @@ -129,17 +126,17 @@ public function testValidTokenWithOtherPasswords(): void { $share = $this->createShare('42', 'password'); $this->session->method('get')->with('public_link_authenticated')->willReturn(['10', '20', '30']); - $this->invokeGetDocumentIdFromShare(123, $share); + $this->invokeCheckFileAccessFromShare(123, $share); } - private function invokeGetDocumentIdFromShare(int $fileId, IShare $share): int { - $this->shareManager->method('getShareByToken')->willReturn($share); + private function invokeCheckFileAccessFromShare(int $fileId, IShare $share): void { + $this->shareManager->expects($this->once())->method('getShareByToken')->willReturn($share); $folder = $this->createMock(Folder::class); $folder->method('getFirstNodeById')->willReturn($this->createMock(File::class)); $this->rootFolder->method('getUserFolder')->with('owner')->willReturn($folder); - return $this->fileService->getDocumentIdFromShare($fileId, 'token'); + $this->fileService->checkFileAccessFromShare($fileId, 'token'); } private function createShare(string $id, ?string $password = null): IShare { From 946871fdd105a23d88d6067a76615b9f28d085c5 Mon Sep 17 00:00:00 2001 From: Max Date: Wed, 19 Aug 2026 15:53:51 +0200 Subject: [PATCH 21/30] fix(share): get file from context for attachments Signed-off-by: Max --- lib/Service/AttachmentService.php | 39 ++++++++++++------------------- lib/Service/FileService.php | 2 +- 2 files changed, 16 insertions(+), 25 deletions(-) diff --git a/lib/Service/AttachmentService.php b/lib/Service/AttachmentService.php index 55c1f2ded31..194ed405750 100755 --- a/lib/Service/AttachmentService.php +++ b/lib/Service/AttachmentService.php @@ -309,7 +309,7 @@ public function uploadAttachment(int $documentId, string $newFileName, $newFileR * @throws InvalidPathException * @throws NoUserException */ - public function uploadAttachmentPublic(?int $documentId, string $newFileName, $newFileResource, string $shareToken): array { + public function uploadAttachmentPublic(int $documentId, string $newFileName, $newFileResource, string $shareToken): array { try { $share = $this->shareManager->getShareByToken($shareToken); } catch (ShareNotFound) { @@ -541,32 +541,23 @@ private function getTextFile(int $documentId, string $userId): File { * * @throws NotFoundException */ - private function getTextFilePublic(?int $documentId, string $shareToken): File { - // is the file shared with this token? + private function getTextFilePublic(int $documentId, string $shareToken): File { + // TODO: Lazy load the context and enable these additional checks inside the context try { $share = $this->shareManager->getShareByToken($shareToken); - if (in_array($share->getShareType(), [IShare::TYPE_LINK, IShare::TYPE_EMAIL])) { - // shared file or folder? - if ($share->getNodeType() === 'file') { - $textFile = $share->getNode(); - if ($textFile instanceof File - && !$this->isDownloadDisabled($textFile) - && $textFile->getId() === $documentId - ) { - return $textFile; - } - } elseif ($documentId !== null && $share->getNodeType() === 'folder') { - $folder = $share->getNode(); - if ($folder instanceof Folder) { - $textFile = $folder->getFirstNodeById($documentId); - if ($textFile instanceof File && !$this->isDownloadDisabled($textFile)) { - return $textFile; - } - } - } - } } catch (ShareNotFound) { - // same as below + throw new NotFoundException(); + } + if (!in_array($share->getShareType(), [IShare::TYPE_LINK, IShare::TYPE_EMAIL])) { + throw new NotFoundException(); + } + $document = $this->documentMapper->find($documentId); + $type = $document->getContextType(); + $id = $document->getContextId(); + $context = $this->contextManager->getContext($type, $id, $shareToken); + $file = $context->getFile(); + if ($file instanceof File && !$this->isDownloadDisabled($file)) { + return $file; } throw new NotFoundException('Text file with id=' . (string)$documentId . ' and shareToken ' . $shareToken . ' was not found.'); } diff --git a/lib/Service/FileService.php b/lib/Service/FileService.php index 1af9270dcd5..b83a30114be 100644 --- a/lib/Service/FileService.php +++ b/lib/Service/FileService.php @@ -48,7 +48,7 @@ public function getFileByIdFromShare(int $fileId, string $shareToken): File { if ($node instanceof Folder) { $node = $node->getFirstNodeById($fileId); } - if ($node instanceof File) { + if ($node instanceof File && $node->getId() === $fileId) { return $node; } throw new NotFoundException(); From 75bb07bfba2227b9d32ba206260d406931db3e27 Mon Sep 17 00:00:00 2001 From: Max Date: Wed, 19 Aug 2026 17:47:26 +0200 Subject: [PATCH 22/30] fix(migration): migrate old document rows to new format * Start without `notnull`. * Migrate data so `context_type` and `context_id` are filled. * Set `notnull` on those columns. Signed-off-by: Max --- composer/composer/autoload_classmap.php | 1 + composer/composer/autoload_static.php | 1 + .../Version090000Date20260817110024.php | 4 +- .../Version090000Date20260819110024.php | 70 +++++++++++++++++++ 4 files changed, 74 insertions(+), 2 deletions(-) create mode 100644 lib/Migration/Version090000Date20260819110024.php diff --git a/composer/composer/autoload_classmap.php b/composer/composer/autoload_classmap.php index 03d612e6175..8ec50071c32 100644 --- a/composer/composer/autoload_classmap.php +++ b/composer/composer/autoload_classmap.php @@ -74,6 +74,7 @@ 'OCA\\Text\\Migration\\Version070000Date20250925110024' => $baseDir . '/../lib/Migration/Version070000Date20250925110024.php', 'OCA\\Text\\Migration\\Version080000Date20260331132113' => $baseDir . '/../lib/Migration/Version080000Date20260331132113.php', 'OCA\\Text\\Migration\\Version090000Date20260817110024' => $baseDir . '/../lib/Migration/Version090000Date20260817110024.php', + 'OCA\\Text\\Migration\\Version090000Date20260819110024' => $baseDir . '/../lib/Migration/Version090000Date20260819110024.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 041995902f6..2bed7d7d3aa 100644 --- a/composer/composer/autoload_static.php +++ b/composer/composer/autoload_static.php @@ -89,6 +89,7 @@ class ComposerStaticInitText 'OCA\\Text\\Migration\\Version070000Date20250925110024' => __DIR__ . '/..' . '/../lib/Migration/Version070000Date20250925110024.php', 'OCA\\Text\\Migration\\Version080000Date20260331132113' => __DIR__ . '/..' . '/../lib/Migration/Version080000Date20260331132113.php', 'OCA\\Text\\Migration\\Version090000Date20260817110024' => __DIR__ . '/..' . '/../lib/Migration/Version090000Date20260817110024.php', + 'OCA\\Text\\Migration\\Version090000Date20260819110024' => __DIR__ . '/..' . '/../lib/Migration/Version090000Date20260819110024.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/Migration/Version090000Date20260817110024.php b/lib/Migration/Version090000Date20260817110024.php index 5d17dd55a73..0a911de25a7 100644 --- a/lib/Migration/Version090000Date20260817110024.php +++ b/lib/Migration/Version090000Date20260817110024.php @@ -28,14 +28,14 @@ public function changeSchema(IOutput $output, Closure $schemaClosure, array $opt if (!$table->hasColumn('context_type')) { $table->addColumn('context_type', Types::STRING, [ - 'notnull' => true, + 'notnull' => false, 'length' => 64, ]); } if (!$table->hasColumn('context_id')) { $table->addColumn('context_id', Types::BIGINT, [ - 'notnull' => true, + 'notnull' => false, 'unsigned' => true, ]); } diff --git a/lib/Migration/Version090000Date20260819110024.php b/lib/Migration/Version090000Date20260819110024.php new file mode 100644 index 00000000000..6ba16154c17 --- /dev/null +++ b/lib/Migration/Version090000Date20260819110024.php @@ -0,0 +1,70 @@ +connection->getQueryBuilder(); + $qb->update('text_documents', 'd') + ->set('d.context_type', $qb->createNamedParameter('file')) + ->where($qb->expr()->isNull('context_type')) + ->executeStatement(); + $qb->update('text_documents', 'd') + ->set('d.context_id', 'd.id') + ->where($qb->expr()->isNull('context_id')) + ->executeStatement(); + } + + public function changeSchema(IOutput $output, Closure $schemaClosure, array $options) { + /** @var ISchemaWrapper $schema */ + $schema = $schemaClosure(); + + $table = $schema->getTable('text_documents'); + + $column = $table->getColumn('context_type'); + if (!$column->getNotnull()) { + $table->modifyColumn('context_type', [ + 'notnull' => true, + ]); + } + + $column = $table->getColumn('context_id'); + if (!$column->getNotnull()) { + $table->modifyColumn('context_id', [ + 'notnull' => true, + ]); + return $schema; + } + return null; + } +} From cb849186bf836659c528fd73aba3755b97754ff1 Mon Sep 17 00:00:00 2001 From: Max Date: Thu, 20 Aug 2026 06:43:05 +0200 Subject: [PATCH 23/30] fix(db): migrations adding context type and id * Return the schema - even if the migrations do nothing. * Use the plain column name of the column to set. Fixes the migration on postgres DB. Signed-off-by: Max --- lib/Migration/Version090000Date20260817110024.php | 3 +-- lib/Migration/Version090000Date20260819110024.php | 7 +++---- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/lib/Migration/Version090000Date20260817110024.php b/lib/Migration/Version090000Date20260817110024.php index 0a911de25a7..627d673b141 100644 --- a/lib/Migration/Version090000Date20260817110024.php +++ b/lib/Migration/Version090000Date20260817110024.php @@ -45,8 +45,7 @@ public function changeSchema(IOutput $output, Closure $schemaClosure, array $opt $table->modifyColumn('id', [ 'autoincrement' => true, ]); - return $schema; } - return null; + return $schema; } } diff --git a/lib/Migration/Version090000Date20260819110024.php b/lib/Migration/Version090000Date20260819110024.php index 6ba16154c17..9c65cbcd5ee 100644 --- a/lib/Migration/Version090000Date20260819110024.php +++ b/lib/Migration/Version090000Date20260819110024.php @@ -36,11 +36,11 @@ public function __construct( public function preSchemaChange(IOutput $output, Closure $schemaClosure, array $options) { $qb = $this->connection->getQueryBuilder(); $qb->update('text_documents', 'd') - ->set('d.context_type', $qb->createNamedParameter('file')) + ->set('context_type', $qb->createNamedParameter('file')) ->where($qb->expr()->isNull('context_type')) ->executeStatement(); $qb->update('text_documents', 'd') - ->set('d.context_id', 'd.id') + ->set('context_id', 'd.id') ->where($qb->expr()->isNull('context_id')) ->executeStatement(); } @@ -63,8 +63,7 @@ public function changeSchema(IOutput $output, Closure $schemaClosure, array $opt $table->modifyColumn('context_id', [ 'notnull' => true, ]); - return $schema; } - return null; + return $schema; } } From 00402bd5a3bc554c2d0f8d41142a45e3ce81cda5 Mon Sep 17 00:00:00 2001 From: Max Date: Thu, 20 Aug 2026 07:20:43 +0200 Subject: [PATCH 24/30] fix(api): Unlock the file via the context Signed-off-by: Max --- lib/Context/FileContext.php | 13 +++++++++++++ lib/Context/IContext.php | 4 ++++ lib/Controller/PublicSessionController.php | 3 +-- lib/Controller/SessionController.php | 3 +-- lib/Service/ApiService.php | 10 ++++++++-- 5 files changed, 27 insertions(+), 6 deletions(-) diff --git a/lib/Context/FileContext.php b/lib/Context/FileContext.php index ae8e6e9b156..aab0dea3a11 100644 --- a/lib/Context/FileContext.php +++ b/lib/Context/FileContext.php @@ -151,6 +151,11 @@ public function saveWithLock(string $content, callable $doWhileLocked): void { }); } + #[Override] + public function cleanup(): void { + $this->unlock(); + } + private function computeCheckSum(?string $content = null): string { if ($content === null) { $content = $this->file->getContent(); @@ -175,4 +180,12 @@ private function lock(): bool { return true; } + private function unlock(): void { + // Disable file locking for Readme.md files, because in the + // current setup, this makes it almost impossible to delete these files. + if (strcasecmp($this->file->getName(), 'Readme.md') !== 0) { + $this->lockService->unlock($this->file); + } + } + } diff --git a/lib/Context/IContext.php b/lib/Context/IContext.php index 2ce52ee1004..cc4f9c83d58 100644 --- a/lib/Context/IContext.php +++ b/lib/Context/IContext.php @@ -23,6 +23,10 @@ public function updateDocument(Document $document): ?Document; public function getFile(): ?File; public function loadContent(): ?string; public function saveWithLock(string $content, callable $doWhileLocked): void; + /** + * This will be called when the last active editing session ends. + */ + public function cleanup(): void; } readonly class DocumentData { diff --git a/lib/Controller/PublicSessionController.php b/lib/Controller/PublicSessionController.php index c934ccd0875..d3d44f87f79 100644 --- a/lib/Controller/PublicSessionController.php +++ b/lib/Controller/PublicSessionController.php @@ -85,8 +85,7 @@ public function create(string $token, ?string $filePath = null, ?string $baseVer #[NoAdminRequired] #[PublicPage] public function close(int $documentId, int $sessionId, string $sessionToken, string $token): DataResponse { - $file = $this->fileService->getFileByIdFromShare($documentId, $token); - return $this->apiService->close($documentId, $sessionId, $sessionToken, $file); + return $this->apiService->close($documentId, $sessionId, $sessionToken, $token); } #[NoAdminRequired] diff --git a/lib/Controller/SessionController.php b/lib/Controller/SessionController.php index a62d05a7c1c..c875c7055af 100644 --- a/lib/Controller/SessionController.php +++ b/lib/Controller/SessionController.php @@ -74,8 +74,7 @@ public function close(int $documentId, int $sessionId, string $sessionToken): Da if ($userId === null) { throw new InvalidSessionException(); } - $file = $this->fileService->getFileById($documentId, $userId); - return $this->apiService->close($documentId, $sessionId, $sessionToken, $file); + return $this->apiService->close($documentId, $sessionId, $sessionToken, null); } #[NoAdminRequired] diff --git a/lib/Service/ApiService.php b/lib/Service/ApiService.php index 883a753ed9b..28f926832c2 100644 --- a/lib/Service/ApiService.php +++ b/lib/Service/ApiService.php @@ -76,12 +76,18 @@ public function create(IContext $context, ?string $baseVersionEtag, ?string $gue ); } - public function close(int $documentId, int $sessionId, string $sessionToken, File $file): DataResponse { + public function close(int $documentId, int $sessionId, string $sessionToken, ?string $shareToken): DataResponse { $this->sessionService->closeSession($documentId, $sessionId, $sessionToken); $this->sessionService->removeInactiveSessionsWithoutSteps($documentId); $activeSessions = $this->sessionService->getActiveSessions($documentId); if (count($activeSessions) === 0) { - $this->lockService->unlock($file); + $document = $this->documentService->getDocument($documentId); + if ($document !== null) { + $type = $document->getContextType(); + $id = $document->getContextId(); + $context = $this->contextManager->getContext($type, $id, $shareToken); + $context->cleanup(); + } } return new DataResponse([]); } From fe14a80dca80c778ae2328a3ec7cec5f3d5b45fb Mon Sep 17 00:00:00 2001 From: Max Date: Thu, 20 Aug 2026 08:27:58 +0200 Subject: [PATCH 25/30] fix(api): use context to load document Signed-off-by: Max --- lib/Db/DocumentMapper.php | 13 +++---------- lib/Service/ApiService.php | 2 +- lib/Service/DocumentService.php | 5 ++--- 3 files changed, 6 insertions(+), 14 deletions(-) diff --git a/lib/Db/DocumentMapper.php b/lib/Db/DocumentMapper.php index 96d76dc30f9..f94c0564faf 100644 --- a/lib/Db/DocumentMapper.php +++ b/lib/Db/DocumentMapper.php @@ -8,7 +8,6 @@ namespace OCA\Text\Db; use Generator; -use OCA\Text\Context\IContext; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Db\QBMapper; use OCP\DB\QueryBuilder\IQueryBuilder; @@ -42,25 +41,19 @@ public function find(int $documentId): Document { return Document::fromRow($data); } - /** - * @throws DoesNotExistException - */ - public function load(IContext $context): Document { - $type = $context->getType(); - $id = $context->getId(); - + public function load(string $type, int $id): ?Document { /* @var $qb IQueryBuilder */ $qb = $this->db->getQueryBuilder(); $result = $qb->select('*') ->from($this->getTableName()) ->where($qb->expr()->eq('context_type', $qb->createNamedParameter($type))) - ->where($qb->expr()->eq('context_id', $qb->createNamedParameter($id))) + ->andWhere($qb->expr()->eq('context_id', $qb->createNamedParameter($id))) ->executeQuery(); $data = $result->fetchAssociative(); $result->closeCursor(); if ($data === false) { - throw new DoesNotExistException('Document doesn\'t exist'); + return null; } return Document::fromRow($data); } diff --git a/lib/Service/ApiService.php b/lib/Service/ApiService.php index 28f926832c2..32aaa71f51e 100644 --- a/lib/Service/ApiService.php +++ b/lib/Service/ApiService.php @@ -48,7 +48,7 @@ public function create(IContext $context, ?string $baseVersionEtag, ?string $gue } try { - $document = $this->documentService->getOrCreateDocument($document, $context); + $document = $this->documentService->getOrCreateDocument($document); } catch (Exception $e) { $this->logger->error($e->getMessage(), ['exception' => $e]); return new DataResponse(['error' => 'Failed to create the document session'], Http::STATUS_INTERNAL_SERVER_ERROR); diff --git a/lib/Service/DocumentService.php b/lib/Service/DocumentService.php index a236a9017d3..fe0e8087979 100644 --- a/lib/Service/DocumentService.php +++ b/lib/Service/DocumentService.php @@ -100,9 +100,8 @@ public function isSaveFromText(): bool { * @throws NotPermittedException * @throws Exception */ - public function getOrCreateDocument(Document $document, IContext $context): Document { - // TODO: drop $context once $document contains contextId and contextType - $loaded = $this->getDocument($context->getId()); + public function getOrCreateDocument(Document $document): Document { + $loaded = $this->documentMapper->load($document->getContextType(), $document->getContextId()); if ($loaded !== null) { $this->logger->info('Keep previous document of ' . $document->toString()); return $loaded; From 41ea7a5a1909ddf0f69aa176ee1d2ab829484f40 Mon Sep 17 00:00:00 2001 From: Max Date: Thu, 20 Aug 2026 09:03:40 +0200 Subject: [PATCH 26/30] chore(perf): add documents index for context When creating a new session we load the document by context. Signed-off-by: Max --- composer/composer/autoload_classmap.php | 1 + composer/composer/autoload_static.php | 1 + .../Version090000Date20260820132113.php | 40 +++++++++++++++++++ 3 files changed, 42 insertions(+) create mode 100644 lib/Migration/Version090000Date20260820132113.php diff --git a/composer/composer/autoload_classmap.php b/composer/composer/autoload_classmap.php index 8ec50071c32..71ef7775e67 100644 --- a/composer/composer/autoload_classmap.php +++ b/composer/composer/autoload_classmap.php @@ -75,6 +75,7 @@ 'OCA\\Text\\Migration\\Version080000Date20260331132113' => $baseDir . '/../lib/Migration/Version080000Date20260331132113.php', '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\\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 2bed7d7d3aa..1173f0425da 100644 --- a/composer/composer/autoload_static.php +++ b/composer/composer/autoload_static.php @@ -90,6 +90,7 @@ class ComposerStaticInitText 'OCA\\Text\\Migration\\Version080000Date20260331132113' => __DIR__ . '/..' . '/../lib/Migration/Version080000Date20260331132113.php', '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\\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/Migration/Version090000Date20260820132113.php b/lib/Migration/Version090000Date20260820132113.php new file mode 100644 index 00000000000..e19ba349ad4 --- /dev/null +++ b/lib/Migration/Version090000Date20260820132113.php @@ -0,0 +1,40 @@ +getTable('text_documents'); + if (!$table->hasIndex('text_documents_context_index')) { + $table->addIndex(['context_type', 'context_id'], 'text_documents_context_index'); + } + + return $schema; + } +} From 907236aa99f09594993ecd54382fd50cb6e8ab2c Mon Sep 17 00:00:00 2001 From: Max Date: Thu, 20 Aug 2026 11:18:15 +0200 Subject: [PATCH 27/30] chore(refactor): migrate PublicFilesEditor to script setup and ts Signed-off-by: Max --- src/components/PublicFilesEditor.vue | 60 +++++++--------------------- 1 file changed, 15 insertions(+), 45 deletions(-) diff --git a/src/components/PublicFilesEditor.vue b/src/components/PublicFilesEditor.vue index b255c9378f1..3dc8965fd22 100644 --- a/src/components/PublicFilesEditor.vue +++ b/src/components/PublicFilesEditor.vue @@ -15,56 +15,26 @@ - From f295bd1246dcb5639b8d75ecd76e6e991659511c Mon Sep 17 00:00:00 2001 From: Max Date: Thu, 20 Aug 2026 12:25:02 +0200 Subject: [PATCH 28/30] chore(refactor): ViewerComponent to script setup and ts Signed-off-by: Max --- src/components/ViewerComponent.vue | 159 ++++++++++------------------- 1 file changed, 54 insertions(+), 105 deletions(-) diff --git a/src/components/ViewerComponent.vue b/src/components/ViewerComponent.vue index 5d53d81f68f..889f37cc97e 100644 --- a/src/components/ViewerComponent.vue +++ b/src/components/ViewerComponent.vue @@ -21,120 +21,69 @@ :mime :source v-bind="$attrs" - @loaded="onLoaded" + @loaded="onLoadedHandler" @edit="toggleEdit" /> -