Merge pull request #9149 from nextcloud/fix/no-conflicts-on-sync

fix/no conflicts on sync

Author: max-nextcloud

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 {

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;

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