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 0f8ca17cf02..91e510b08b4 100644 --- a/lib/Service/DocumentService.php +++ b/lib/Service/DocumentService.php @@ -307,7 +307,6 @@ public function getSteps(int $documentId, int $lastVersion): array { /** * @throws DocumentSaveConflictException - * @throws DoesNotExistException * @throws InvalidPathException * @throws NotFoundException */ @@ -332,16 +331,7 @@ public function assertNoOutsideConflict(Document $document, File $file, bool $fo $fileChecksum = self::computeCheckSum($fileContent); if ($storedChecksum !== $fileChecksum) { - // $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; + throw new DocumentSaveConflictException('File changed in the meantime from outside'); } $document->setLastSavedVersionTime($fileMtime); @@ -374,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. @@ -412,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 index bf22c06aa68..6fb5497c215 100644 --- a/tests/unit/Service/DocumentServiceTest.php +++ b/tests/unit/Service/DocumentServiceTest.php @@ -83,9 +83,7 @@ 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); } @@ -103,33 +101,9 @@ public function testRefreshesVersionInfoWhenContentMatches(): void { 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); @@ -147,4 +121,17 @@ public function testNoConflictWhileSaveLockIsHeld(): void { $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); + } + }