Skip to content

Commit 7c00374

Browse files
mejo-mejo-
authored
Merge pull request #9157 from nextcloud/backport/9149/stable35
[stable35] fix/no conflicts on sync
2 parents 0a0b686 + a79b84d commit 7c00374

3 files changed

Lines changed: 144 additions & 10 deletions

File tree

‎lib/Service/ApiService.php‎

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -175,7 +175,6 @@ public function sync(Session $session, Document $document, int $version = 0, ?st
175175
// ensure file is still present and accessible
176176
$file = $this->fileService->getFileForSession($session, $shareToken);
177177
$result['readOnly'] = $this->fileService->isReadOnly($file, $shareToken);
178-
$this->documentService->assertNoOutsideConflict($document, $file);
179178
} catch (NotPermittedException|NotFoundException|InvalidPathException $e) {
180179
$this->logger->info($e->getMessage(), ['exception' => $e]);
181180
return new DataResponse([
@@ -186,16 +185,9 @@ public function sync(Session $session, Document $document, int $version = 0, ?st
186185
return new DataResponse([
187186
'message' => 'Document no longer exists'
188187
], Http::STATUS_NOT_FOUND);
189-
} catch (DocumentSaveConflictException) {
190-
try {
191-
/** @psalm-suppress PossiblyUndefinedVariable */
192-
$result['outsideChange'] = $file->getContent();
193-
} catch (LockedException) {
194-
// Ignore locked exception since it might happen due to an autosave action happening at the same time
195-
}
196188
}
197189

198-
return new DataResponse($result, isset($result['outsideChange']) ? Http::STATUS_CONFLICT : Http::STATUS_OK);
190+
return new DataResponse($result, Http::STATUS_OK);
199191
}
200192

201193
public function save(Session $session, Document $document, int $version, string $autosaveContent, string $documentState, bool $force = false, bool $manualSave = false, ?string $shareToken = null): DataResponse {

‎lib/Service/DocumentService.php‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -364,6 +364,11 @@ public function autosave(Document $document, File $file, int $version, string $a
364364

365365
$this->assertNoOutsideConflict($document, $file, $force);
366366

367+
// Abort autosave if already saving.
368+
if ($this->cache->get('document-save-lock-' . $documentId) && $manualSave === false) {
369+
return $document;
370+
}
371+
367372
// Do not save if newer version already saved
368373
// Note that $version is the version of the steps the client has fetched.
369374
// It may have added steps on top of that - so if the versions match we still save.
@@ -402,7 +407,7 @@ public function autosave(Document $document, File $file, int $version, string $a
402407
return $document;
403408
}
404409

405-
$this->cache->set('document-save-lock-' . $documentId, true, 10);
410+
$this->cache->set('document-save-lock-' . $documentId, true, 60);
406411
try {
407412
$this->lockService->runInScope($file, function () use ($file, $autoSaveDocument, $documentState): void {
408413
$this->saveFromText = true;

‎tests/unit/Service/DocumentServiceTest.php‎

Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,137 @@
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('update');
87+
$this->documentService->assertNoOutsideConflict($document, $file);
88+
}
89+
90+
public function testRefreshesVersionInfoWhenContentMatches(): void {
91+
$document = $this->createDocument('etag1', 1000, 'content');
92+
$file = $this->mockFile('etag2', 2000, 'content');
93+
94+
$this->documentMapper->expects(self::never())->method('find');
95+
$this->documentMapper->expects(self::once())
96+
->method('update')
97+
->with($document);
98+
99+
$this->documentService->assertNoOutsideConflict($document, $file);
100+
self::assertSame('etag2', $document->getLastSavedVersionEtag());
101+
self::assertSame(2000, $document->getLastSavedVersionTime());
102+
}
103+
104+
public function testConflictWhenFileChangedFromOutside(): void {
105+
$document = $this->createDocument('etag1', 1000, 'old content');
106+
$file = $this->mockFile('etag2', 2000, 'outside content');
107+
108+
$this->expectException(DocumentSaveConflictException::class);
109+
$this->documentService->assertNoOutsideConflict($document, $file);
110+
}
111+
112+
public function testNoConflictWhileSaveLockIsHeld(): void {
113+
$document = $this->createDocument('etag1', 1000, 'old content');
114+
$file = $this->mockFile('etag2', 2000, 'new content');
115+
116+
$this->cache->method('get')
117+
->with('document-save-lock-123')
118+
->willReturn(true);
119+
$this->documentMapper->expects(self::never())->method('find');
120+
$this->documentMapper->expects(self::never())->method('update');
121+
122+
$this->documentService->assertNoOutsideConflict($document, $file);
123+
}
124+
125+
public function testNoAutosavingWhileSaveIsUnderWay(): void {
126+
$document = $this->createDocument('etag1', 1000, 'new content');
127+
$file = $this->mockFile('etag1', 1000, 'old content');
128+
$this->cache->method('get')
129+
->with('document-save-lock-123')
130+
->willReturn(true);
131+
$this->documentMapper->expects(self::never())->method('update');
132+
133+
$result = $this->documentService->autosave($document, $file, 1234, 'new content', 'doc state');
134+
self::assertSame($result, $document);
135+
}
136+
137+
}

0 commit comments

Comments
 (0)