From c4349a2a71c5c3c1316c724892fa81f0775e963a Mon Sep 17 00:00:00 2001 From: silver Date: Tue, 25 Aug 2026 13:56:20 +0200 Subject: [PATCH] fix(sync): do not mistake own save requests for outside changes The document is loaded at the start of each request. A sync request compares its etag and checksum against the file only after several other queries. If a save request finishes in between, the file has changed but the loaded document does not reflect that yet, so the sync request then returned 409 and the client showed the conflict view, even though nobody else edited the file. Reload the document before raising the conflict to compare against the latest saved state. Genuine outside changes still mismatch after the reload and keep raising the conflict. Signed-off-by: silver Assisted-by: ClaudeCode:claude-fable-5 --- lib/Service/DocumentService.php | 12 +- tests/unit/Service/DocumentServiceTest.php | 150 +++++++++++++++++++++ 2 files changed, 161 insertions(+), 1 deletion(-) create mode 100644 tests/unit/Service/DocumentServiceTest.php diff --git a/lib/Service/DocumentService.php b/lib/Service/DocumentService.php index d4bcdf01747..0f8ca17cf02 100644 --- a/lib/Service/DocumentService.php +++ b/lib/Service/DocumentService.php @@ -307,6 +307,7 @@ public function getSteps(int $documentId, int $lastVersion): array { /** * @throws DocumentSaveConflictException + * @throws DoesNotExistException * @throws InvalidPathException * @throws NotFoundException */ @@ -331,7 +332,16 @@ 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'); + // $document was loaded at the start of the request. + // A save request handled in the meantime is not reflected in it + // and would be mistaken for an outside change. + // Reload the document to compare against the latest saved state. + $document = $this->documentMapper->find($documentId); + if ($document->getChecksum() !== $fileChecksum) { + throw new DocumentSaveConflictException('File changed in the meantime from outside'); + } + // The save request already stored the latest version info. + return; } $document->setLastSavedVersionTime($fileMtime); diff --git a/tests/unit/Service/DocumentServiceTest.php b/tests/unit/Service/DocumentServiceTest.php new file mode 100644 index 00000000000..bf22c06aa68 --- /dev/null +++ b/tests/unit/Service/DocumentServiceTest.php @@ -0,0 +1,150 @@ +documentMapper = $this->createMock(DocumentMapper::class); + $this->fileService = $this->createMock(FileService::class); + $this->cache = $this->createMock(ICache::class); + $cacheFactory = $this->createMock(ICacheFactory::class); + $cacheFactory->method('createDistributed')->willReturn($this->cache); + $request = $this->createMock(IRequest::class); + $request->method('getParam')->willReturn(null); + + $this->fileService->method('isReadOnly')->willReturn(false); + + $this->documentService = new DocumentService( + $this->documentMapper, + $this->fileService, + $this->createMock(StepMapper::class), + $this->createMock(SessionMapper::class), + $this->createMock(IAppData::class), + 'admin', + $this->createMock(IRootFolder::class), + $cacheFactory, + $this->createMock(LoggerInterface::class), + $this->createMock(LockService::class), + $request, + $this->createMock(IManager::class), + $this->createMock(IUserMountCache::class), + $this->createMock(IConfig::class), + ); + } + + private function createDocument(string $etag, int $mtime, string $content): Document { + $document = new Document(); + $document->setId(123); + $document->setLastSavedVersionEtag($etag); + $document->setLastSavedVersionTime($mtime); + $document->setChecksum(DocumentService::computeCheckSum($content)); + return $document; + } + + private function mockFile(string $etag, int $mtime, string $content): File { + $file = $this->createMock(File::class); + $file->method('getEtag')->willReturn($etag); + $file->method('getMtime')->willReturn($mtime); + $file->method('getContent')->willReturn($content); + return $file; + } + + public function testNoConflictWhenVersionInfoMatches(): void { + $document = $this->createDocument('etag1', 1000, 'content'); + $file = $this->mockFile('etag1', 1000, 'content'); + + $this->documentMapper->expects(self::never())->method('find'); + $this->documentMapper->expects(self::never())->method('update'); + + $this->documentService->assertNoOutsideConflict($document, $file); + } + + public function testRefreshesVersionInfoWhenContentMatches(): void { + $document = $this->createDocument('etag1', 1000, 'content'); + $file = $this->mockFile('etag2', 2000, 'content'); + + $this->documentMapper->expects(self::never())->method('find'); + $this->documentMapper->expects(self::once()) + ->method('update') + ->with($document); + + $this->documentService->assertNoOutsideConflict($document, $file); + self::assertSame('etag2', $document->getLastSavedVersionEtag()); + self::assertSame(2000, $document->getLastSavedVersionTime()); + } + + public function testNoConflictWhenOwnSaveFinishedInTheMeantime(): void { + // Loaded at the start of the request - stale by now. + $document = $this->createDocument('etag1', 1000, 'old content'); + // A save request updated the file in the meantime ... + $file = $this->mockFile('etag2', 2000, 'new content'); + // ... and stored the new version info in the document. + $freshDocument = $this->createDocument('etag2', 2000, 'new content'); + + $this->documentMapper->expects(self::once()) + ->method('find') + ->with(123) + ->willReturn($freshDocument); + $this->documentMapper->expects(self::never())->method('update'); + + $this->documentService->assertNoOutsideConflict($document, $file); + } + + public function testConflictWhenFileChangedFromOutside(): void { + $document = $this->createDocument('etag1', 1000, 'old content'); + $file = $this->mockFile('etag2', 2000, 'outside content'); + // The latest saved state does not match the file either. + $freshDocument = $this->createDocument('etag1', 1000, 'old content'); + + $this->documentMapper->expects(self::once()) + ->method('find') + ->with(123) + ->willReturn($freshDocument); + + $this->expectException(DocumentSaveConflictException::class); + $this->documentService->assertNoOutsideConflict($document, $file); + } + + public function testNoConflictWhileSaveLockIsHeld(): void { + $document = $this->createDocument('etag1', 1000, 'old content'); + $file = $this->mockFile('etag2', 2000, 'new content'); + + $this->cache->method('get') + ->with('document-save-lock-123') + ->willReturn(true); + $this->documentMapper->expects(self::never())->method('find'); + $this->documentMapper->expects(self::never())->method('update'); + + $this->documentService->assertNoOutsideConflict($document, $file); + } +}