Split sync of ApiService to sync and save

appinfo/routes.php

@@ -44,6 +44,8 @@
 
 		/** @see Controller\SessionController::create() */
 		['name' => 'Session#create', 'url' => '/session/create', 'verb' => 'PUT'],
+		/** @see Controller\SessionController::save() */
+		['name' => 'Session#save', 'url' => '/session/save', 'verb' => 'POST'],
 		/** @see Controller\SessionController::sync() */
 		['name' => 'Session#sync', 'url' => '/session/sync', 'verb' => 'POST'],
 		/** @see Controller\SessionController::push() */
@@ -57,6 +59,8 @@
 		['name' => 'PublicSession#create', 'url' => '/public/session/create', 'verb' => 'PUT'],
 		/** @see Controller\PublicSessionController::updateSession() */
 		['name' => 'PublicSession#updateSession', 'url' => '/public/session', 'verb' => 'POST'],
+		/** @see Controller\PublicSessionController::save() */
+		['name' => 'PublicSession#save', 'url' => '/public/session/save', 'verb' => 'POST'],
 		/** @see Controller\PublicSessionController::sync() */
 		['name' => 'PublicSession#sync', 'url' => '/public/session/sync', 'verb' => 'POST'],
 		/** @see Controller\PublicSessionController::push() */

cypress/e2e/api/SessionApi.spec.js

@@ -171,7 +171,7 @@ describe('The session Api', function() {
 			cy.pushSteps({ connection, steps: [messages.update], version })
 				.its('version')
 				.should('eql', 1)
-			cy.syncSteps(connection, { version: 1, autosaveContent: '# Heading 1', manualSave: true })
+			cy.save(connection, { version: 1, autosaveContent: '# Heading 1', manualSave: true })
 			cy.downloadFile(filePath)
 				.its('data')
 				.should('eql', '# Heading 1')
@@ -182,7 +182,7 @@ describe('The session Api', function() {
 			cy.pushSteps({ connection, steps: [messages.update], version })
 				.its('version')
 				.should('eql', 1)
-			cy.syncSteps(connection, {
+			cy.save(connection, {
 				version: 1,
 				autosaveContent: '# Heading 1',
 				documentState,
@@ -245,7 +245,7 @@ describe('The session Api', function() {
 			cy.pushSteps({ connection, steps: [messages.update], version })
 				.its('version')
 				.should('eql', 1)
-			cy.syncSteps(connection, { version: 1, autosaveContent: '# Heading 1', manualSave: true })
+			cy.save(connection, { version: 1, autosaveContent: '# Heading 1', manualSave: true })
 			cy.login(user)
 			cy.prepareSessionApi()
 			cy.downloadFile('saves.md')
@@ -258,7 +258,7 @@ describe('The session Api', function() {
 			cy.pushSteps({ connection, steps: [messages.update], version })
 				.its('version')
 				.should('eql', 1)
-			cy.syncSteps(connection, {
+			cy.save(connection, {
 				version: 1,
 				autosaveContent: '# Heading 1',
 				documentState,

cypress/e2e/sync.spec.js

@@ -33,11 +33,8 @@ describe('Sync', () => {
 		cy.login(user)
 		cy.uploadTestFile('test.md')
 		cy.visit('/apps/files')
-		cy.intercept({ method: 'POST', url: '**/apps/text/session/sync' }, (req) => {
-			if (req.body.autosaveContent) {
-				req.alias = 'save'
-			}
-		}).as('sync')
+		cy.intercept({ method: 'POST', url: '**/apps/text/session/sync' }).as('sync')
+		cy.intercept({ method: 'POST', url: '**/apps/text/session/save' }).as('save')
 		cy.openTestFile()
 		cy.getContent().find('h2').should('contain', 'Hello world')
 		cy.getContent().type('{moveToEnd}* Saving the doc saves the doc state{enter}')

cypress/support/sessions.js

@@ -55,6 +55,11 @@ Cypress.Commands.add('syncSteps', (connection, options = { version: 0 }) => {
 		.then(response => response.data)
 })
 
+Cypress.Commands.add('save', (connection, options = { version: 0 }) => {
+	return connection.save(options)
+		.then(response => response.data)
+})
+
 // Used to test for race conditions between the last push and the close request
 Cypress.Commands.add('pushAndClose', ({ connection, steps, version, awareness = '' }) => {
 	cy.log('Race between push and close')

lib/Controller/PublicSessionController.php

@@ -100,8 +100,15 @@ public function push(int $documentId, int $sessionId, string $sessionToken, int
 	#[NoAdminRequired]
 	#[PublicPage]
 	#[RequireDocumentSession]
-	public function sync(string $token, int $documentId, int $sessionId, string $sessionToken, int $version = 0, string $autosaveContent = null, string $documentState = null, bool $force = false, bool $manualSave = false): DataResponse {
-		return $this->apiService->sync($this->getSession(), $this->getDocument(), $version, $autosaveContent, $documentState, $force, $manualSave, $token);
+	public function sync(string $token, int $version = 0): DataResponse {
+		return $this->apiService->sync($this->getSession(), $this->getDocument(), $version, $token);
+	}
+
+	#[NoAdminRequired]
+	#[PublicPage]
+	#[RequireDocumentSession]
+	public function save(string $token, int $version = 0, string $autosaveContent = null, string $documentState = null, bool $force = false, bool $manualSave = false): DataResponse {
+		return $this->apiService->save($this->getSession(), $this->getDocument(), $version, $autosaveContent, $documentState, $force, $manualSave, $token);
 	}
 
 	/**

lib/Controller/SessionController.php

@@ -82,10 +82,22 @@ public function push(int $version, array $steps, string $awareness): DataRespons
 	#[NoAdminRequired]
 	#[PublicPage]
 	#[RequireDocumentSession]
-	public function sync(int $version = 0, string $autosaveContent = null, string $documentState = null, bool $force = false, bool $manualSave = false): DataResponse {
+	public function sync(int $version = 0): DataResponse {
 		try {
 			$this->loginSessionUser();
-			return $this->apiService->sync($this->getSession(), $this->getDocument(), $version, $autosaveContent, $documentState, $force, $manualSave);
+			return $this->apiService->sync($this->getSession(), $this->getDocument(), $version);
+		} finally {
+			$this->restoreSessionUser();
+		}
+	}
+
+	#[NoAdminRequired]
+	#[PublicPage]
+	#[RequireDocumentSession]
+	public function save(int $version = 0, string $autosaveContent = null, string $documentState = null, bool $force = false, bool $manualSave = false): DataResponse {
+		try {
+			$this->loginSessionUser();
+			return $this->apiService->save($this->getSession(), $this->getDocument(), $version, $autosaveContent, $documentState, $force, $manualSave);
 		} finally {
 			$this->restoreSessionUser();
 		}

lib/Service/ApiService.php

@@ -37,6 +37,7 @@
 use OCP\AppFramework\Http;
 use OCP\AppFramework\Http\DataResponse;
 use OCP\Constants;
+use OCP\Files\InvalidPathException;
 use OCP\Files\Lock\ILock;
 use OCP\Files\NotFoundException;
 use OCP\Files\NotPermittedException;
@@ -211,21 +212,44 @@ public function push(Session $session, Document $document, int $version, array $
 		return new DataResponse($result);
 	}
 
-	/**
-	 * @param null|string $autosaveContent
-	 * @param null|string $documentState
-	 * @param null|string $token
-	 */
-	public function sync(Session $session, Document $document, int $version = 0, string|null $autosaveContent = null, string|null $documentState = null, bool $force = false, bool $manualSave = false, string|null $token = null): DataResponse {
+	public function sync(Session $session, Document $document, int $version = 0, ?string $shareToken = null): DataResponse {
 		$documentId = $session->getDocumentId();
+		$result = [];
 		try {
 			$result = [
 				'steps' => $this->documentService->getSteps($documentId, $version),
 				'sessions' => $this->sessionService->getAllSessions($documentId),
 				'document' => $document,
 			];
 
-			$file = $this->documentService->getFileForSession($session, $token);
+			// ensure file is still present and accessible
+			$file = $this->documentService->getFileForSession($session, $shareToken);
+			$this->documentService->assertNoOutsideConflict($document, $file);
+		} catch (NotFoundException|InvalidPathException $e) {
+			$this->logger->info($e->getMessage(), ['exception' => $e]);
+			return new DataResponse([
+				'message' => 'File not found'
+			], 404);
+		} catch (DoesNotExistException $e) {
+			$this->logger->info($e->getMessage(), ['exception' => $e]);
+			return new DataResponse([
+				'message' => 'Document no longer exists'
+			], 404);
+		} 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']) ? 409 : 200);
+	}
+
+	public function save(Session $session, Document $document, int $version = 0, ?string $autosaveContent = null, ?string $documentState = null, bool $force = false, bool $manualSave = false, ?string $shareToken = null): DataResponse {
+		try {
+			$file = $this->documentService->getFileForSession($session, $shareToken);
 		} catch (NotFoundException $e) {
 			$this->logger->info($e->getMessage(), ['exception' => $e]);
 			return new DataResponse([
@@ -238,15 +262,16 @@ public function sync(Session $session, Document $document, int $version = 0, str
 			], 404);
 		}
 
+		$result = [];
 		try {
-			$result['document'] = $this->documentService->autosave($document, $file, $version, $autosaveContent, $documentState, $force, $manualSave, $token, $this->request->getParam('filePath'));
-		} catch (DocumentSaveConflictException $e) {
+			$result['document'] = $this->documentService->autosave($document, $file, $version, $autosaveContent, $documentState, $force, $manualSave, $shareToken);
+		} catch (DocumentSaveConflictException) {
 			try {
 				$result['outsideChange'] = $file->getContent();
-			} catch (LockedException $e) {
+			} catch (LockedException) {
 				// Ignore locked exception since it might happen due to an autosave action happening at the same time
 			}
-		} catch (NotFoundException $e) {
+		} catch (NotFoundException) {
 			return new DataResponse([], 404);
 		} catch (Exception $e) {
 			$this->logger->error($e->getMessage(), ['exception' => $e]);

lib/Service/DocumentService.php

@@ -291,6 +291,27 @@ public function getSteps(int $documentId, int $lastVersion): array {
 		return $this->stepMapper->find($documentId, $lastVersion);
 	}
 
+	/**
+	 * @throws DocumentSaveConflictException
+	 * @throws InvalidPathException
+	 * @throws NotFoundException
+	 */
+	public function assertNoOutsideConflict(Document $document, File $file, bool $force = false, ?string $shareToken = null): void {
+		$documentId = $document->getId();
+		$savedEtag = $file->getEtag();
+		$lastMTime = $document->getLastSavedVersionTime();
+
+		if ($lastMTime > 0
+			&& $force === false
+			&& !$this->isReadOnly($file, $shareToken)
+			&& $savedEtag !== $document->getLastSavedVersionEtag()
+			&& $lastMTime !== $file->getMtime()
+			&& !$this->cache->get('document-save-lock-' . $documentId)
+		) {
+			throw new DocumentSaveConflictException('File changed in the meantime from outside');
+		}
+	}
+
 	/**
 	 * @throws DocumentSaveConflictException
 	 * @throws DoesNotExistException
@@ -299,7 +320,7 @@ public function getSteps(int $documentId, int $lastVersion): array {
 	 * @throws NotPermittedException
 	 * @throws Exception
 	 */
-	public function autosave(Document $document, ?File $file, int $version, ?string $autoSaveDocument, ?string $documentState, bool $force = false, bool $manualSave = false, ?string $shareToken = null, ?string $filePath = null): Document {
+	public function autosave(Document $document, ?File $file, int $version, ?string $autoSaveDocument, ?string $documentState, bool $force = false, bool $manualSave = false, ?string $shareToken = null): Document {
 		$documentId = $document->getId();
 		if ($file === null) {
 			throw new NotFoundException();
@@ -309,17 +330,7 @@ public function autosave(Document $document, ?File $file, int $version, ?string
 			return $document;
 		}
 
-		$savedEtag = $file->getEtag();
-		$lastMTime = $document->getLastSavedVersionTime();
-
-		if ($lastMTime > 0 && $savedEtag !== $document->getLastSavedVersionEtag() && $lastMTime !== $file->getMtime() && $force === false) {
-			if (!$this->cache->get('document-save-lock-' . $documentId)) {
-				throw new DocumentSaveConflictException('File changed in the meantime from outside');
-			} else {
-				// Only return here if the document is locked, otherwise we can continue to save
-				return $document;
-			}
-		}
+		$this->assertNoOutsideConflict($document, $file, $force);
 
 		if ($autoSaveDocument === null) {
 			return $document;
@@ -335,6 +346,7 @@ public function autosave(Document $document, ?File $file, int $version, ?string
 		}
 
 		// Only save once every AUTOSAVE_MINIMUM_DELAY seconds
+		$lastMTime = $document->getLastSavedVersionTime();
 		if ($file->getMTime() === $lastMTime && $lastMTime > time() - self::AUTOSAVE_MINIMUM_DELAY && $manualSave === false) {
 			return $document;
 		}

src/services/PollingBackend.js

@@ -116,8 +116,6 @@ class PollingBackend {
 		logger.debug('[PollingBackend] Fetching steps', this.#syncService.version)
 		await this.#connection.sync({
 			version: this.#syncService.version,
-			force: false,
-			manualSave: false,
 		}).then(this._handleResponse.bind(this), this._handleError.bind(this))
 		this.#lastPoll = Date.now()
 		this.#pollActive = false

src/services/SessionApi.js

@@ -98,11 +98,19 @@ export class Connection {
 		}
 	}
 
-	sync({ version, autosaveContent, documentState, force, manualSave }) {
+	sync({ version }) {
 		return axios.post(this.#url('session/sync'), {
 			...this.#defaultParams,
 			filePath: this.#options.filePath,
 			version,
+		})
+	}
+
+	save({ version, autosaveContent, documentState, force, manualSave }) {
+		return axios.post(this.#url('session/save'), {
+			...this.#defaultParams,
+			filePath: this.#options.filePath,
+			version,
 			autosaveContent,
 			documentState,
 			force,

src/services/SyncService.js

@@ -253,7 +253,7 @@ class SyncService {
 	async save({ force = false, manualSave = true } = {}) {
 		logger.debug('[SyncService] saving', arguments[0])
 		try {
-			const response = await this.connection.sync({
+			const response = await this.connection.save({
 				version: this.version,
 				autosaveContent: this._getContent(),
 				documentState: this.getDocumentState(),