diff --git a/.github/workflows/playwright.yml b/.github/workflows/playwright.yml index 551325b62a5..7bd5beb4241 100644 --- a/.github/workflows/playwright.yml +++ b/.github/workflows/playwright.yml @@ -15,6 +15,7 @@ jobs: timeout-minutes: 60 runs-on: ubuntu-latest strategy: + fail-fast: false matrix: shardIndex: [1, 2, 3] shardTotal: [3] diff --git a/lib/Service/DocumentService.php b/lib/Service/DocumentService.php index 0532c291be3..43533caafb1 100644 --- a/lib/Service/DocumentService.php +++ b/lib/Service/DocumentService.php @@ -116,10 +116,20 @@ public function isSaveFromText(): bool { * @throws Exception */ public function getOrCreateDocument(File $file): Document { + $checksum = self::computeCheckSum($file->getContent()); $document = $this->getDocument($file->getId()); if ($document !== null) { - $this->logger->info('Keep previous document of ' . $file->getId()); - return $document; + if ($document->getChecksum() === $checksum || $this->hasUnsavedChanges($document)) { + $this->logger->info('Keep previous document of ' . $file->getId()); + return $document; + } + // The file was changed without passing through Nextcloud, e.g. directly on external storage. + $this->logger->info('Reset outdated document of ' . $file->getId()); + try { + $this->resetDocument($file->getId()); + } catch (DocumentHasUnsavedChangesException) { + return $document; + } } if (!$this->ensureDocumentsFolder()) { @@ -133,7 +143,7 @@ public function getOrCreateDocument(File $file): Document { $document->setLastSavedVersionTime($file->getMTime()); $document->setLastSavedVersionEtag($file->getEtag()); $document->setBaseVersionEtag(uniqid()); - $document->setChecksum(self::computeCheckSum($file->getContent())); + $document->setChecksum($checksum); try { /** @var Document $document */ $document = $this->documentMapper->insert($document); @@ -340,24 +350,18 @@ public function assertNoOutsideConflict(Document $document, File $file, bool $fo return; } - $fileMtime = $file->getMtime(); - $fileEtag = $file->getEtag(); - - if ($lastEtag === $fileEtag && $lastMTime === $fileMtime) { - return; - } - - $storedChecksum = $document->getChecksum(); - $fileContent = $file->getContent(); - $fileChecksum = self::computeCheckSum($fileContent); - - if ($storedChecksum !== $fileChecksum) { + $fileChecksum = self::computeCheckSum($file->getContent()); + if ($document->getChecksum() !== $fileChecksum) { throw new DocumentSaveConflictException('File changed in the meantime from outside'); } - $document->setLastSavedVersionTime($fileMtime); - $document->setLastSavedVersionEtag($fileEtag); - $this->documentMapper->update($document); + $fileMtime = $file->getMtime(); + $fileEtag = $file->getEtag(); + if ($lastEtag !== $fileEtag || $lastMTime !== $fileMtime) { + $document->setLastSavedVersionTime($fileMtime); + $document->setLastSavedVersionEtag($fileEtag); + $this->documentMapper->update($document); + } } /** diff --git a/playwright/e2e/outside-change.spec.ts b/playwright/e2e/outside-change.spec.ts new file mode 100644 index 00000000000..2e316b42259 --- /dev/null +++ b/playwright/e2e/outside-change.spec.ts @@ -0,0 +1,96 @@ +/** + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +import { expect, mergeTests } from '@playwright/test' +import { test as editorTest } from '../support/fixtures/editor' +import { test as randomUserTest } from '../support/fixtures/random-user' +import { test as uploadFileTest } from '../support/fixtures/upload-file' + +const test = mergeTests(editorTest, randomUserTest, uploadFileTest) + +test.beforeEach(async ({ open }) => { + await open() +}) + +const resolutionVariants = [ + { + source: 'local', + buttonName: /Keep my local changes and overwrite the remote version/, + headingName: 'Hello world', + }, + { + source: 'server', + buttonName: /Discard my local changes and use the remote version/, + headingName: 'Good bye', + }, +] + +resolutionVariants.forEach(({ source, buttonName, headingName }) => { + test(`[conflict] saving after the file was changed outside of Nextcloud - resolve with ${source} version`, async ({ + container, + editor, + file, + page, + reader, + user, + }) => { + await expect(editor.el).toBeVisible() + const pushPromise = page.waitForRequest(/push/) + await editor.typeHeading('Hello world') + await pushPromise + await user.writeFileOutsideNextcloud({ + name: file.name, + content: '## Good bye', + }) + // Clicking the save indicator would force the save and skip the conflict check. + await page.keyboard.press('ControlOrMeta+s') + + // Verify both versions are shown + await expect(reader.getHeading({ name: 'Good bye' })).toBeVisible({ + timeout: 10_000, + }) + await expect(editor.getHeading({ name: 'Hello world' })).toBeVisible() + + // Resolve conflict + await container.getButton({ name: buttonName }).click() + await expect(reader.el).not.toBeVisible() + await expect(editor.getHeading({ name: headingName })).toBeVisible() + await expect(async () => { + expect(await file.getContent()).toContain(headingName) + }).toPass() + }) +}) + +test('reopening after the file was changed outside of Nextcloud shows the new content', async ({ + close, + container, + editor, + file, + page, + reader, + user, +}) => { + await expect(editor.el).toBeVisible() + const pushPromise = page.waitForRequest(/push/) + await editor.typeHeading('Hello world') + await pushPromise + await editor.saveIndicator.click() + await expect(editor.saveIndicator).not.toHaveAccessibleName(/Unsaved changes/) + await close() + + await user.writeFileOutsideNextcloud({ name: file.name, content: '## Good bye' }) + await file.open() + + await expect(editor.getHeading({ name: 'Good bye' })).toBeVisible({ + timeout: 10_000, + }) + await expect(editor.getHeading({ name: 'Hello world' })).not.toBeVisible() + await expect(reader.el).not.toBeVisible() + await expect( + container.getButton({ + name: /Keep my local changes and overwrite the remote version/, + }), + ).not.toBeVisible() +}) diff --git a/playwright/support/fixtures/User.ts b/playwright/support/fixtures/User.ts index f12fa2e504e..6b5fd1643b3 100644 --- a/playwright/support/fixtures/User.ts +++ b/playwright/support/fixtures/User.ts @@ -3,8 +3,10 @@ * SPDX-License-Identifier: AGPL-3.0-or-later */ -import { type Page } from '@playwright/test' import type { User as Account } from '@nextcloud/e2e-test-server' +import type { Page } from '@playwright/test' + +import { runExec } from '@nextcloud/e2e-test-server/docker' import { uploadFile } from './Node' export class User { @@ -23,4 +25,18 @@ export class User { return uploadFile({ ...upload, owner: this }) } + /** + * Write a file directly to the user's storage on the server, bypassing Nextcloud + * like a change on external storage would. + * + * @param options options for the write + * @param options.name Name of the file + * @param options.content File content + */ + async writeFileOutsideNextcloud({ name, content }: { name: string, content: string }) { + await runExec( + ['sh', '-c', 'printf "%s" "$CONTENT" > "data/$OWNER/files/$NAME"'], + { env: [`CONTENT=${content}`, `OWNER=${this.account.userId}`, `NAME=${name}`] }, + ) + } } diff --git a/playwright/support/fixtures/offline.ts b/playwright/support/fixtures/offline.ts index 0163aa9673b..276570bf6db 100644 --- a/playwright/support/fixtures/offline.ts +++ b/playwright/support/fixtures/offline.ts @@ -33,14 +33,16 @@ const setClientOffline = async (client: CDPSession): Promise => { /** * setOffline will turn the network off for the rest of the test and then on again. */ -export const test = base.extend({ - setOffline: async ({ context, page }, use) => { +export const test = base.extend({ + _cdpClient: async ({ context, page }, use) => { const client = await context.newCDPSession(page) - await use (() => setClientOffline(client)) - await setClientOnline(client) + await use(client) }, - setOnline: async ({ context, page }, use) => { - const client = await context.newCDPSession(page) - await use (() => setClientOnline(client)) + setOffline: async ({ _cdpClient }, use) => { + await use(() => setClientOffline(_cdpClient)) + await setClientOnline(_cdpClient) + }, + setOnline: async ({ _cdpClient }, use) => { + await use(() => setClientOnline(_cdpClient)) }, }) diff --git a/src/services/SaveService.ts b/src/services/SaveService.ts index 14c6430a73f..4786e1a75e6 100644 --- a/src/services/SaveService.ts +++ b/src/services/SaveService.ts @@ -93,12 +93,24 @@ class SaveService { } catch (e) { logger.error('Failed to save document.', { error: e }) const response = ( - e as { response?: { status?: number; data?: { error?: string } } } + e as { + response?: { + status?: number + data?: { error?: string; outsideChange?: string } + } + } ).response if (response?.status === 403) { // Document is now read-only; permissionChange from sync will update the UI return } + if (response?.status === 409) { + this.bus.emit('error', { + type: ERROR_TYPE.SAVE_COLLISION, + data: { outsideChange: response.data?.outsideChange }, + }) + return + } if (response?.status === 412) { this.bus.emit('error', { type: ERROR_TYPE.LOAD_ERROR, diff --git a/src/tests/services/SaveService.spec.ts b/src/tests/services/SaveService.spec.ts new file mode 100644 index 00000000000..30260057c4b --- /dev/null +++ b/src/tests/services/SaveService.spec.ts @@ -0,0 +1,53 @@ +/** + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +import type { Connection } from '../../composables/useConnection.ts' + +import { describe, expect, it, vi } from 'vitest' +import { ref, shallowRef } from 'vue' +import * as saveApi from '../../apis/save.ts' +import { SaveService } from '../../services/SaveService.ts' +import { ERROR_TYPE } from '../../services/SyncService.ts' + +vi.mock('../../apis/save.ts') + +const connection = shallowRef({ + documentId: 123, + sessionId: 345, + sessionToken: 'sessionToken', + filePath: './', + baseVersionEtag: 'etag', +}) +/** + */ +function getSaveData() { + return { + version: 1, + autosaveContent: 'content', + documentState: 'state', + } +} + +describe('SaveService', () => { + it('emits a save collision when the file was changed outside', async () => { + vi.mocked(saveApi.save).mockRejectedValue({ + response: { status: 409, data: { outsideChange: 'outside content' } }, + }) + const saveService = new SaveService({ + connection, + document: ref(undefined), + getSaveData, + }) + const onError = vi.fn() + saveService.bus.on('error', onError) + + await saveService.save() + + expect(onError).toHaveBeenCalledWith({ + type: ERROR_TYPE.SAVE_COLLISION, + data: { outsideChange: 'outside content' }, + }) + }) +}) diff --git a/tests/unit/Service/DocumentServiceTest.php b/tests/unit/Service/DocumentServiceTest.php index 0a317b0d3f8..2e0adc402f1 100644 --- a/tests/unit/Service/DocumentServiceTest.php +++ b/tests/unit/Service/DocumentServiceTest.php @@ -191,4 +191,52 @@ public function testGetFileForSessionFromShareOfOtherFile() { $this->expectException(\InvalidArgumentException::class); $this->documentService->getFileForSession($session, 'shareToken'); } + + private function createDocumentWithContent(string $content): Document { + $document = new Document(); + $document->setId(42); + $document->setLastSavedVersion(3); + $document->setChecksum(DocumentService::computeCheckSum($content)); + return $document; + } + + private function createFile(string $content): File { + $file = $this->createMock(File::class); + $file->method('getId')->willReturn(42); + $file->method('getContent')->willReturn($content); + $file->method('getMTime')->willReturn(1000); + $file->method('getEtag')->willReturn('etag'); + return $file; + } + + public function testKeepsDocumentWhenFileIsUnchanged(): void { + $loaded = $this->createDocumentWithContent('content'); + $this->documentMapper->method('find')->willReturn($loaded); + $this->documentMapper->expects(self::never())->method('delete'); + $this->documentMapper->expects(self::never())->method('insert'); + + self::assertSame($loaded, $this->documentService->getOrCreateDocument($this->createFile('content'))); + } + + public function testKeepsDocumentWithUnsavedChangesWhenFileChangedOutside(): void { + $loaded = $this->createDocumentWithContent('content'); + $this->documentMapper->method('find')->willReturn($loaded); + $this->setpMapper->method('getLatestVersion')->willReturn(4); + $this->documentMapper->expects(self::never())->method('delete'); + $this->documentMapper->expects(self::never())->method('insert'); + + self::assertSame($loaded, $this->documentService->getOrCreateDocument($this->createFile('changed outside'))); + } + + public function testResetsDocumentWhenFileChangedOutside(): void { + $loaded = $this->createDocumentWithContent('content'); + $this->documentMapper->method('find')->willReturn($loaded); + $this->setpMapper->method('getLatestVersion')->willReturn(3); + $this->documentMapper->expects(self::once())->method('delete')->with($loaded); + $this->documentMapper->expects(self::once())->method('insert')->willReturnArgument(0); + + $document = $this->documentService->getOrCreateDocument($this->createFile('changed outside')); + self::assertNotSame($loaded, $document); + self::assertSame(DocumentService::computeCheckSum('changed outside'), $document->getChecksum()); + } }