Skip to content

Commit a4eeb24

Browse files
silverkszlomax-nextcloud
silverkszlo
authored and
max-nextcloud
committed
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 <s.szmajduch@posteo.de> Assisted-by: ClaudeCode:claude-fable-5
1 parent f81fcce commit a4eeb24

2 files changed

Lines changed: 161 additions & 1 deletion

File tree

‎lib/Service/DocumentService.php‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -307,6 +307,7 @@ public function getSteps(int $documentId, int $lastVersion): array {
307307

308308
/**
309309
* @throws DocumentSaveConflictException
310+
* @throws DoesNotExistException
310311
* @throws InvalidPathException
311312
* @throws NotFoundException
312313
*/
@@ -331,7 +332,16 @@ public function assertNoOutsideConflict(Document $document, File $file, bool $fo
331332
$fileChecksum = self::computeCheckSum($fileContent);
332333

333334
if ($storedChecksum !== $fileChecksum) {
334-
throw new DocumentSaveConflictException('File changed in the meantime from outside');
335+
// $document was loaded at the start of the request.
336+
// A save request handled in the meantime is not reflected in it
337+
// and would be mistaken for an outside change.
338+
// Reload the document to compare against the latest saved state.
339+
$document = $this->documentMapper->find($documentId);
340+
if ($document->getChecksum() !== $fileChecksum) {
341+
throw new DocumentSaveConflictException('File changed in the meantime from outside');
342+
}
343+
// The save request already stored the latest version info.
344+
return;
335345
}
336346

337347
$document->setLastSavedVersionTime($fileMtime);

‎tests/unit/Service/DocumentServiceTest.php‎

Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,150 @@
1+
<?php
2+
3+
/**
4+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
5+
* SPDX-License-Identifier: AGPL-3.0-or-later
6+
*/
7+
8+
namespace OCA\Text\Tests;
9+
10+
use OCA\Text\Db\Document;
11+
use OCA\Text\Db\DocumentMapper;
12+
use OCA\Text\Db\SessionMapper;
13+
use OCA\Text\Db\StepMapper;
14+
use OCA\Text\Exception\DocumentSaveConflictException;
15+
use OCA\Text\Service\DocumentService;
16+
use OCA\Text\Service\FileService;
17+
use OCA\Text\Service\LockService;
18+
use OCP\DirectEditing\IManager;
19+
use OCP\Files\Config\IUserMountCache;
20+
use OCP\Files\File;
21+
use OCP\Files\IAppData;
22+
use OCP\Files\IRootFolder;
23+
use OCP\ICache;
24+
use OCP\ICacheFactory;
25+
use OCP\IConfig;
26+
use OCP\IRequest;
27+
use Psr\Log\LoggerInterface;
28+
29+
class DocumentServiceTest extends \PHPUnit\Framework\TestCase {
30+
private DocumentService $documentService;
31+
32+
private DocumentMapper $documentMapper;
33+
private FileService $fileService;
34+
private ICache $cache;
35+
36+
public function setUp(): void {
37+
$this->documentMapper = $this->createMock(DocumentMapper::class);
38+
$this->fileService = $this->createMock(FileService::class);
39+
$this->cache = $this->createMock(ICache::class);
40+
$cacheFactory = $this->createMock(ICacheFactory::class);
41+
$cacheFactory->method('createDistributed')->willReturn($this->cache);
42+
$request = $this->createMock(IRequest::class);
43+
$request->method('getParam')->willReturn(null);
44+
45+
$this->fileService->method('isReadOnly')->willReturn(false);
46+
47+
$this->documentService = new DocumentService(
48+
$this->documentMapper,
49+
$this->fileService,
50+
$this->createMock(StepMapper::class),
51+
$this->createMock(SessionMapper::class),
52+
$this->createMock(IAppData::class),
53+
'admin',
54+
$this->createMock(IRootFolder::class),
55+
$cacheFactory,
56+
$this->createMock(LoggerInterface::class),
57+
$this->createMock(LockService::class),
58+
$request,
59+
$this->createMock(IManager::class),
60+
$this->createMock(IUserMountCache::class),
61+
$this->createMock(IConfig::class),
62+
);
63+
}
64+
65+
private function createDocument(string $etag, int $mtime, string $content): Document {
66+
$document = new Document();
67+
$document->setId(123);
68+
$document->setLastSavedVersionEtag($etag);
69+
$document->setLastSavedVersionTime($mtime);
70+
$document->setChecksum(DocumentService::computeCheckSum($content));
71+
return $document;
72+
}
73+
74+
private function mockFile(string $etag, int $mtime, string $content): File {
75+
$file = $this->createMock(File::class);
76+
$file->method('getEtag')->willReturn($etag);
77+
$file->method('getMtime')->willReturn($mtime);
78+
$file->method('getContent')->willReturn($content);
79+
return $file;
80+
}
81+
82+
public function testNoConflictWhenVersionInfoMatches(): void {
83+
$document = $this->createDocument('etag1', 1000, 'content');
84+
$file = $this->mockFile('etag1', 1000, 'content');
85+
86+
$this->documentMapper->expects(self::never())->method('find');
87+
$this->documentMapper->expects(self::never())->method('update');
88+
89+
$this->documentService->assertNoOutsideConflict($document, $file);
90+
}
91+
92+
public function testRefreshesVersionInfoWhenContentMatches(): void {
93+
$document = $this->createDocument('etag1', 1000, 'content');
94+
$file = $this->mockFile('etag2', 2000, 'content');
95+
96+
$this->documentMapper->expects(self::never())->method('find');
97+
$this->documentMapper->expects(self::once())
98+
->method('update')
99+
->with($document);
100+
101+
$this->documentService->assertNoOutsideConflict($document, $file);
102+
self::assertSame('etag2', $document->getLastSavedVersionEtag());
103+
self::assertSame(2000, $document->getLastSavedVersionTime());
104+
}
105+
106+
public function testNoConflictWhenOwnSaveFinishedInTheMeantime(): void {
107+
// Loaded at the start of the request - stale by now.
108+
$document = $this->createDocument('etag1', 1000, 'old content');
109+
// A save request updated the file in the meantime ...
110+
$file = $this->mockFile('etag2', 2000, 'new content');
111+
// ... and stored the new version info in the document.
112+
$freshDocument = $this->createDocument('etag2', 2000, 'new content');
113+
114+
$this->documentMapper->expects(self::once())
115+
->method('find')
116+
->with(123)
117+
->willReturn($freshDocument);
118+
$this->documentMapper->expects(self::never())->method('update');
119+
120+
$this->documentService->assertNoOutsideConflict($document, $file);
121+
}
122+
123+
public function testConflictWhenFileChangedFromOutside(): void {
124+
$document = $this->createDocument('etag1', 1000, 'old content');
125+
$file = $this->mockFile('etag2', 2000, 'outside content');
126+
// The latest saved state does not match the file either.
127+
$freshDocument = $this->createDocument('etag1', 1000, 'old content');
128+
129+
$this->documentMapper->expects(self::once())
130+
->method('find')
131+
->with(123)
132+
->willReturn($freshDocument);
133+
134+
$this->expectException(DocumentSaveConflictException::class);
135+
$this->documentService->assertNoOutsideConflict($document, $file);
136+
}
137+
138+
public function testNoConflictWhileSaveLockIsHeld(): void {
139+
$document = $this->createDocument('etag1', 1000, 'old content');
140+
$file = $this->mockFile('etag2', 2000, 'new content');
141+
142+
$this->cache->method('get')
143+
->with('document-save-lock-123')
144+
->willReturn(true);
145+
$this->documentMapper->expects(self::never())->method('find');
146+
$this->documentMapper->expects(self::never())->method('update');
147+
148+
$this->documentService->assertNoOutsideConflict($document, $file);
149+
}
150+
}

0 commit comments

Comments
 (0)