diff --git a/lib/Service/ApiService.php b/lib/Service/ApiService.php index 015bcccfd1e..c75b149bbfd 100644 --- a/lib/Service/ApiService.php +++ b/lib/Service/ApiService.php @@ -175,7 +175,6 @@ 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); } catch (NotPermittedException|NotFoundException|InvalidPathException $e) { $this->logger->info($e->getMessage(), ['exception' => $e]); return new DataResponse([ @@ -186,16 +185,9 @@ 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 - } } - return new DataResponse($result, isset($result['outsideChange']) ? Http::STATUS_CONFLICT : Http::STATUS_OK); + return new DataResponse($result, Http::STATUS_OK); } public function save(Session $session, Document $document, int $version, string $autosaveContent, string $documentState, bool $force = false, bool $manualSave = false, ?string $shareToken = null): DataResponse { diff --git a/lib/Service/DocumentService.php b/lib/Service/DocumentService.php index d4bcdf01747..91e510b08b4 100644 --- a/lib/Service/DocumentService.php +++ b/lib/Service/DocumentService.php @@ -364,6 +364,11 @@ public function autosave(Document $document, File $file, int $version, string $a $this->assertNoOutsideConflict($document, $file, $force); + // Abort autosave if already saving. + if ($this->cache->get('document-save-lock-' . $documentId) && $manualSave === false) { + return $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. @@ -402,7 +407,7 @@ public function autosave(Document $document, File $file, int $version, string $a return $document; } - $this->cache->set('document-save-lock-' . $documentId, true, 10); + $this->cache->set('document-save-lock-' . $documentId, true, 60); try { $this->lockService->runInScope($file, function () use ($file, $autoSaveDocument, $documentState): void { $this->saveFromText = true; diff --git a/tests/unit/Service/DocumentServiceTest.php b/tests/unit/Service/DocumentServiceTest.php new file mode 100644 index 00000000000..6fb5497c215 --- /dev/null +++ b/tests/unit/Service/DocumentServiceTest.php @@ -0,0 +1,137 @@ +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('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 testConflictWhenFileChangedFromOutside(): void { + $document = $this->createDocument('etag1', 1000, 'old content'); + $file = $this->mockFile('etag2', 2000, 'outside content'); + + $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); + } + + public function testNoAutosavingWhileSaveIsUnderWay(): void { + $document = $this->createDocument('etag1', 1000, 'new content'); + $file = $this->mockFile('etag1', 1000, 'old content'); + $this->cache->method('get') + ->with('document-save-lock-123') + ->willReturn(true); + $this->documentMapper->expects(self::never())->method('update'); + + $result = $this->documentService->autosave($document, $file, 1234, 'new content', 'doc state'); + self::assertSame($result, $document); + } + +}