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); + } +}