From 57662dff439d33ca902597f43b8ff28477784bde Mon Sep 17 00:00:00 2001 From: Max Date: Wed, 17 Jan 2024 20:28:29 +0100 Subject: [PATCH 1/6] fix(sync): push local changes on reconnect When an editing session is interrupted steps that were already pushed to the server may be cleared alongside the sync service session. Make sure to push all local state that is not part of the document state when (re-)connecting. Tests Yjs relies on a browser environment. Therefore we test it in a cypress test. Move these tests into component tests to separate them from 'normal' e2e tests. Signed-off-by: Max --- cypress.config.js | 15 ++-- cypress/component/helpers/yjs.cy.js | 77 +++++++++++++++++++ .../upstream/Yjs.cy.js} | 0 cypress/support/component-index.html | 12 +++ cypress/support/component.js | 27 +++++++ package-lock.json | 26 ++++--- package.json | 1 + src/components/Editor.vue | 7 +- src/helpers/yjs.js | 73 ++++++++++++++++-- 9 files changed, 214 insertions(+), 24 deletions(-) create mode 100644 cypress/component/helpers/yjs.cy.js rename cypress/{e2e/api/Yjs.spec.js => component/upstream/Yjs.cy.js} (100%) create mode 100644 cypress/support/component-index.html create mode 100644 cypress/support/component.js diff --git a/cypress.config.js b/cypress.config.js index 8076b4830d0..b2edb1c3dff 100644 --- a/cypress.config.js +++ b/cypress.config.js @@ -2,15 +2,6 @@ const { defineConfig } = require('cypress') const cypressSplit = require('cypress-split') const getCompareSnapshotsPlugin = require('cypress-visual-regression/dist/plugin.js') -module.exports = defineConfig({ - e2e: { - setupNodeEvents(on, config) { - // IMPORTANT: return the config object - return config - }, - }, -}) - module.exports = defineConfig({ projectId: 'hx9gqy', viewportWidth: 1280, @@ -43,6 +34,12 @@ module.exports = defineConfig({ baseUrl: 'http://localhost:8081/index.php/', specPattern: 'cypress/e2e/**/*.{js,jsx,ts,tsx}', }, + component: { + devServer: { + framework: "vue", + bundler: "webpack", + }, + }, retries: { runMode: 2, // do not retry in `cypress open` diff --git a/cypress/component/helpers/yjs.cy.js b/cypress/component/helpers/yjs.cy.js new file mode 100644 index 00000000000..6f4d261383f --- /dev/null +++ b/cypress/component/helpers/yjs.cy.js @@ -0,0 +1,77 @@ +/* + * @copyright Copyright (c) 2023 Jonas + * + * @author Jonas + * + * @license AGPL-3.0-or-later + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +import * as Y from 'yjs' +import { getDocumentState, getUpdateMessage, applyUpdateMessage } from '../../../src/helpers/yjs.js' + +describe('Yjs base64 wrapped with our helpers', function() { + it('applies step in wrong order', function() { + const source = new Y.Doc() + const target = new Y.Doc() + const sourceMap = source.getMap() + const targetMap = target.getMap() + + target.on('afterTransaction', (tr, doc) => { + // console.log('afterTransaction', tr) + }) + + const state0 = getDocumentState(source) + + // Add keyA to source and apply to target + sourceMap.set('keyA', 'valueA') + + const stateA = getDocumentState(source) + const update0A = getUpdateMessage(source, state0) + applyUpdateMessage(target, update0A) + expect(targetMap.get('keyA')).to.be.eq('valueA') + + // Add keyB to source, don't apply to target yet + sourceMap.set('keyB', 'valueB') + const stateB = getDocumentState(source) + const updateAB = getUpdateMessage(source, stateA) + + // Add keyC to source, apply to target + sourceMap.set('keyC', 'valueC') + const updateBC = getUpdateMessage(source, stateB) + applyUpdateMessage(target, updateBC) + expect(targetMap.get('keyB')).to.be.eq(undefined) + expect(targetMap.get('keyC')).to.be.eq(undefined) + + // Apply keyB to target + applyUpdateMessage(target, updateAB) + expect(targetMap.get('keyB')).to.be.eq('valueB') + expect(targetMap.get('keyC')).to.be.eq('valueC') + }) + + it('update message is empty if no additional state exists', function() { + const source = new Y.Doc() + const sourceMap = source.getMap() + const state0 = getDocumentState(source) + sourceMap.set('keyA', 'valueA') + const stateA = getDocumentState(source) + const update0A = getUpdateMessage(source, state0) + const updateAA = getUpdateMessage(source, stateA) + expect(update0A.length).to.be.eq(40) + expect(updateAA).to.be.eq(undefined) + }) + +}) diff --git a/cypress/e2e/api/Yjs.spec.js b/cypress/component/upstream/Yjs.cy.js similarity index 100% rename from cypress/e2e/api/Yjs.spec.js rename to cypress/component/upstream/Yjs.cy.js diff --git a/cypress/support/component-index.html b/cypress/support/component-index.html new file mode 100644 index 00000000000..ac6e79fd83d --- /dev/null +++ b/cypress/support/component-index.html @@ -0,0 +1,12 @@ + + + + + + + Components App + + +
+ + \ No newline at end of file diff --git a/cypress/support/component.js b/cypress/support/component.js new file mode 100644 index 00000000000..c037b9ffb3b --- /dev/null +++ b/cypress/support/component.js @@ -0,0 +1,27 @@ +// *********************************************************** +// This example support/component.js is processed and +// loaded automatically before your test files. +// +// This is a great place to put global configuration and +// behavior that modifies Cypress. +// +// You can change the location of this file or turn off +// automatically serving support files with the +// 'supportFile' configuration option. +// +// You can read more here: +// https://on.cypress.io/configuration +// *********************************************************** + +// Import commands.js using ES2015 syntax: +import './commands.js' + +// Alternatively you can use CommonJS syntax: +// require('./commands') + +import { mount } from 'cypress/vue2' + +Cypress.Commands.add('mount', mount) + +// Example use: +// cy.mount(MyComponent) diff --git a/package-lock.json b/package-lock.json index 8818375490f..92a8b5132f3 100644 --- a/package-lock.json +++ b/package-lock.json @@ -82,6 +82,7 @@ "vue-click-outside": "^1.1.0", "vue-material-design-icons": "^5.2.0", "vuex": "^3.6.2", + "y-protocols": "^1.0.6", "y-websocket": "^1.5.1", "yjs": "^13.6.10" }, @@ -115,7 +116,7 @@ }, "engines": { "node": "^20.0.0", - "npm": "^9.0.0" + "npm": "^10.0.0" } }, "node_modules/@aashutoshrathi/word-wrap": { @@ -26037,15 +26038,22 @@ } }, "node_modules/y-protocols": { - "version": "1.0.5", - "resolved": "https://registry.npmjs.org/y-protocols/-/y-protocols-1.0.5.tgz", - "integrity": "sha512-Wil92b7cGk712lRHDqS4T90IczF6RkcvCwAD0A2OPg+adKmOe+nOiT/N2hvpQIWS3zfjmtL4CPaH5sIW1Hkm/A==", + "version": "1.0.6", + "resolved": "https://registry.npmjs.org/y-protocols/-/y-protocols-1.0.6.tgz", + "integrity": "sha512-vHRF2L6iT3rwj1jub/K5tYcTT/mEYDUppgNPXwp8fmLpui9f7Yeq3OEtTLVF012j39QnV+KEQpNqoN7CWU7Y9Q==", "dependencies": { - "lib0": "^0.2.42" + "lib0": "^0.2.85" + }, + "engines": { + "node": ">=16.0.0", + "npm": ">=8.0.0" }, "funding": { "type": "GitHub Sponsors ❤", "url": "https://github.com/sponsors/dmonad" + }, + "peerDependencies": { + "yjs": "^13.0.0" } }, "node_modules/y-websocket": { @@ -45265,11 +45273,11 @@ } }, "y-protocols": { - "version": "1.0.5", - "resolved": "https://registry.npmjs.org/y-protocols/-/y-protocols-1.0.5.tgz", - "integrity": "sha512-Wil92b7cGk712lRHDqS4T90IczF6RkcvCwAD0A2OPg+adKmOe+nOiT/N2hvpQIWS3zfjmtL4CPaH5sIW1Hkm/A==", + "version": "1.0.6", + "resolved": "https://registry.npmjs.org/y-protocols/-/y-protocols-1.0.6.tgz", + "integrity": "sha512-vHRF2L6iT3rwj1jub/K5tYcTT/mEYDUppgNPXwp8fmLpui9f7Yeq3OEtTLVF012j39QnV+KEQpNqoN7CWU7Y9Q==", "requires": { - "lib0": "^0.2.42" + "lib0": "^0.2.85" } }, "y-websocket": { diff --git a/package.json b/package.json index 2580e9521df..1360309fc6c 100644 --- a/package.json +++ b/package.json @@ -108,6 +108,7 @@ "vue-click-outside": "^1.1.0", "vue-material-design-icons": "^5.2.0", "vuex": "^3.6.2", + "y-protocols": "^1.0.6", "y-websocket": "^1.5.1", "yjs": "^13.6.10" }, diff --git a/src/components/Editor.vue b/src/components/Editor.vue index 40b9690f161..b60eb8757ac 100644 --- a/src/components/Editor.vue +++ b/src/components/Editor.vue @@ -103,7 +103,7 @@ import { import ReadonlyBar from './Menu/ReadonlyBar.vue' import { logger } from '../helpers/logger.js' -import { getDocumentState, applyDocumentState } from '../helpers/yjs.js' +import { getDocumentState, applyDocumentState, getUpdateMessage } from '../helpers/yjs.js' import { SyncService, ERROR_TYPE, IDLE_TIMEOUT } from './../services/SyncService.js' import createSyncServiceProvider from './../services/SyncServiceProvider.js' import AttachmentResolver from './../services/AttachmentResolver.js' @@ -487,6 +487,11 @@ export default { onLoaded({ documentSource, documentState }) { if (documentState) { applyDocumentState(this.$ydoc, documentState, this.$providers[0]) + // distribute additional state that may exist locally + const updateMessage = getUpdateMessage(this.$ydoc, documentState) + if (updateMessage) { + this.$queue.push(updateMessage) + } } this.hasConnectionIssue = false diff --git a/src/helpers/yjs.js b/src/helpers/yjs.js index bbc4ee24e13..3b44714d31a 100644 --- a/src/helpers/yjs.js +++ b/src/helpers/yjs.js @@ -21,27 +21,80 @@ */ import { encodeArrayBuffer, decodeArrayBuffer } from '../helpers/base64.js' -import { Doc, encodeStateAsUpdate, applyUpdate } from 'yjs' +import * as Y from 'yjs' import * as decoding from 'lib0/decoding.js' +import * as encoding from 'lib0/encoding.js' +import * as syncProtocol from 'y-protocols/sync' +import { messageSync } from 'y-websocket' /** + * Get Document state encode as base64. * - * @param {Doc} ydoc - encode state of this doc + * Used to store yjs state on the server. + * @param {Y.Doc} ydoc - encode state of this doc */ export function getDocumentState(ydoc) { - const update = encodeStateAsUpdate(ydoc) + const update = Y.encodeStateAsUpdate(ydoc) return encodeArrayBuffer(update) } /** * - * @param {Doc} ydoc - apply state to this doc + * @param {Y.Doc} ydoc - apply state to this doc * @param {string} documentState - base64 encoded doc state * @param {object} origin - initiator object e.g. WebsocketProvider */ export function applyDocumentState(ydoc, documentState, origin) { const update = decodeArrayBuffer(documentState) - applyUpdate(ydoc, update, origin) + Y.applyUpdate(ydoc, update, origin) +} + +/** + * Update message for everything in ydoc that is not in encodedBaseUpdate + * + * @param {Y.Doc} ydoc - encode state of this doc + * @param {string} encodedBaseUpdate - base64 encoded doc update to build upon + */ +export function getUpdateMessage(ydoc, encodedBaseUpdate) { + const baseUpdate = decodeArrayBuffer(encodedBaseUpdate) + const baseStateVector = Y.encodeStateVectorFromUpdate(baseUpdate) + const docStateVector = Y.encodeStateVector(ydoc) + if (sameState(baseStateVector, docStateVector)) { + // no additional state in the ydoc - early return + return + } + const encoder = encoding.createEncoder() + encoding.writeVarUint(encoder, messageSync) + const update = Y.encodeStateAsUpdate(ydoc, baseStateVector) + syncProtocol.writeUpdate(encoder, update) + const buf = encoding.toUint8Array(encoder) + return encodeArrayBuffer(buf) +} + +/** + * Apply an updated message to the ydoc. + * + * Only used in tests right now. + * @param {Y.Doc} ydoc - encode state of this doc + * @param {string} updateMessage - base64 encoded y-websocket sync message with update + * @param {object} origin - initiator object e.g. WebsocketProvider + */ +export function applyUpdateMessage(ydoc, updateMessage, origin = 'origin') { + const updateBuffer = decodeArrayBuffer(updateMessage) + const decoder = decoding.createDecoder(updateBuffer) + const messageType = decoding.readVarUint(decoder) + if (messageType !== messageSync) { + console.error('y.js update message with invalid type', messageType) + return + } + // There are no responses to updates - so this is a dummy. + const encoder = encoding.createEncoder() + syncProtocol.readSyncMessage( + decoder, + encoder, + ydoc, + origin, + ) } /** @@ -71,3 +124,13 @@ export function logStep(step) { break } } + +/** + * Helper function to check if two state vectors have the same state + * @param {Array} arr - state vector to compare + * @param {Array} other - state vector to compare against + */ +function sameState(arr, other) { + return arr.length === other.length + && arr.every((value, index) => other[index] === value) +} From fa145e85ad9a4df84c892ff5912cef56431b403e Mon Sep 17 00:00:00 2001 From: Max Date: Thu, 18 Jan 2024 07:42:13 +0100 Subject: [PATCH 2/6] fix(sync): keep yjs file during sync session cleanup Even if all sessions have expired and been removed there may still be disconnected clients that hold state from the last editing session. When they reconnect they will send their yjs updates based on the old state they had. Preserve the yjs state accross editing sessions so updates send after a reconnect can still be processed. Signed-off-by: Max --- lib/Service/DocumentService.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/Service/DocumentService.php b/lib/Service/DocumentService.php index 496200f2afc..fa3c34e5b8d 100644 --- a/lib/Service/DocumentService.php +++ b/lib/Service/DocumentService.php @@ -420,7 +420,9 @@ public function resetDocument(int $documentId, bool $force = false): void { $this->sessionMapper->deleteByDocumentId($documentId); $this->documentMapper->delete($document); - $this->getStateFile($documentId)->delete(); + if ($force) { + $this->getStateFile($documentId)->delete(); + } $this->logger->debug('document reset for ' . $documentId); } catch (DoesNotExistException|NotFoundException $e) { // Ignore if document not found or state file not found From a643da06a6437fb4031804b699385db7c33b32da Mon Sep 17 00:00:00 2001 From: Max Date: Thu, 18 Jan 2024 09:01:29 +0100 Subject: [PATCH 3/6] test(cy): components in CI Signed-off-by: Max --- .github/workflows/cypress-component.yml | 56 +++++++++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 .github/workflows/cypress-component.yml diff --git a/.github/workflows/cypress-component.yml b/.github/workflows/cypress-component.yml new file mode 100644 index 00000000000..9d530d8dfda --- /dev/null +++ b/.github/workflows/cypress-component.yml @@ -0,0 +1,56 @@ +name: Cypress Component Tests + +on: pull_request + +env: + # Adjust APP_NAME if your repository name is different + APP_NAME: ${{ github.event.repository.name }} + + # This represents the server branch to checkout. + # Usually it's the base branch of the PR, but for pushes it's the branch itself. + # e.g. 'main', 'stable27' or 'feature/my-feature + # n.b. server will use head_ref, as we want to test the PR branch. + BRANCH: ${{ github.base_ref || github.ref_name }} + +jobs: + cypress-component: + runs-on: ubuntu-latest + + steps: + - name: Checkout app + uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1 + + - name: Read package.json node and npm engines version + uses: skjnldsv/read-package-engines-version-actions@8205673bab74a63eb9b8093402fd9e0e018663a1 # v2.2 + id: versions + with: + fallbackNode: "^20" + fallbackNpm: "^9" + + - name: Set up node ${{ steps.versions.outputs.nodeVersion }} + uses: actions/setup-node@8f152de45cc393bb48ce5d89d36b731f54556e65 # v4.0.0 + with: + node-version: ${{ steps.versions.outputs.nodeVersion }} + + - name: Set up npm ${{ steps.versions.outputs.npmVersion }} + run: npm i -g npm@"${{ steps.versions.outputs.npmVersion }}" + + - name: Install node dependencies + run: | + npm ci + + - name: Cypress component tests + uses: cypress-io/github-action@ebe8b24c4428922d0f793a5c4c96853a633180e3 # v6.6.0 + with: + component: true + env: + # Needs to be prefixed with CYPRESS_ + CYPRESS_BRANCH: ${{ env.BRANCH }} + # https://github.com/cypress-io/github-action/issues/124 + COMMIT_INFO_MESSAGE: ${{ github.event.pull_request.title }} + # Needed for some specific code workarounds + TESTING: true + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + CYPRESS_BUILD_ID: ${{ github.sha }}-${{ github.run_number }} + CYPRESS_GROUP: Run component + npm_package_name: ${{ env.APP_NAME }} From 31d0592423d7c5e5fd2e1224dba5fe8ab6d832ea Mon Sep 17 00:00:00 2001 From: Max Date: Thu, 18 Jan 2024 10:34:24 +0100 Subject: [PATCH 4/6] enh(log): debug log when sending updateMessage Signed-off-by: Max --- src/components/Editor.vue | 1 + 1 file changed, 1 insertion(+) diff --git a/src/components/Editor.vue b/src/components/Editor.vue index b60eb8757ac..30e891fe873 100644 --- a/src/components/Editor.vue +++ b/src/components/Editor.vue @@ -490,6 +490,7 @@ export default { // distribute additional state that may exist locally const updateMessage = getUpdateMessage(this.$ydoc, documentState) if (updateMessage) { + logger.debug('onLoaded: Pushing local changes to server') this.$queue.push(updateMessage) } } From a7b61752474406292e55f6517dc72afc4c18a518 Mon Sep 17 00:00:00 2001 From: Max Date: Thu, 18 Jan 2024 10:34:55 +0100 Subject: [PATCH 5/6] docs(js): add return type annotations Signed-off-by: Max --- src/helpers/yjs.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/helpers/yjs.js b/src/helpers/yjs.js index 3b44714d31a..490ae4cdfd2 100644 --- a/src/helpers/yjs.js +++ b/src/helpers/yjs.js @@ -32,6 +32,7 @@ import { messageSync } from 'y-websocket' * * Used to store yjs state on the server. * @param {Y.Doc} ydoc - encode state of this doc + * @return {string} */ export function getDocumentState(ydoc) { const update = Y.encodeStateAsUpdate(ydoc) @@ -54,6 +55,7 @@ export function applyDocumentState(ydoc, documentState, origin) { * * @param {Y.Doc} ydoc - encode state of this doc * @param {string} encodedBaseUpdate - base64 encoded doc update to build upon + * @return {string|undefined} */ export function getUpdateMessage(ydoc, encodedBaseUpdate) { const baseUpdate = decodeArrayBuffer(encodedBaseUpdate) From 89493af804323eba987f4562e7d0c456060a87cb Mon Sep 17 00:00:00 2001 From: Max Date: Thu, 18 Jan 2024 12:14:40 +0100 Subject: [PATCH 6/6] fix(sync): yjs messages are Uint8Arrays in the queue Remove duplicate encoding for updateMessage Signed-off-by: Max --- cypress/component/helpers/yjs.cy.js | 2 +- src/helpers/yjs.js | 10 ++++------ 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/cypress/component/helpers/yjs.cy.js b/cypress/component/helpers/yjs.cy.js index 6f4d261383f..7472e28adc8 100644 --- a/cypress/component/helpers/yjs.cy.js +++ b/cypress/component/helpers/yjs.cy.js @@ -70,7 +70,7 @@ describe('Yjs base64 wrapped with our helpers', function() { const stateA = getDocumentState(source) const update0A = getUpdateMessage(source, state0) const updateAA = getUpdateMessage(source, stateA) - expect(update0A.length).to.be.eq(40) + expect(update0A.length).to.be.eq(29) expect(updateAA).to.be.eq(undefined) }) diff --git a/src/helpers/yjs.js b/src/helpers/yjs.js index 490ae4cdfd2..a34331d2ebb 100644 --- a/src/helpers/yjs.js +++ b/src/helpers/yjs.js @@ -55,7 +55,7 @@ export function applyDocumentState(ydoc, documentState, origin) { * * @param {Y.Doc} ydoc - encode state of this doc * @param {string} encodedBaseUpdate - base64 encoded doc update to build upon - * @return {string|undefined} + * @return {Uint8Array|undefined} */ export function getUpdateMessage(ydoc, encodedBaseUpdate) { const baseUpdate = decodeArrayBuffer(encodedBaseUpdate) @@ -69,8 +69,7 @@ export function getUpdateMessage(ydoc, encodedBaseUpdate) { encoding.writeVarUint(encoder, messageSync) const update = Y.encodeStateAsUpdate(ydoc, baseStateVector) syncProtocol.writeUpdate(encoder, update) - const buf = encoding.toUint8Array(encoder) - return encodeArrayBuffer(buf) + return encoding.toUint8Array(encoder) } /** @@ -78,12 +77,11 @@ export function getUpdateMessage(ydoc, encodedBaseUpdate) { * * Only used in tests right now. * @param {Y.Doc} ydoc - encode state of this doc - * @param {string} updateMessage - base64 encoded y-websocket sync message with update + * @param {Uint8Array} updateMessage - y-websocket sync message with update * @param {object} origin - initiator object e.g. WebsocketProvider */ export function applyUpdateMessage(ydoc, updateMessage, origin = 'origin') { - const updateBuffer = decodeArrayBuffer(updateMessage) - const decoder = decoding.createDecoder(updateBuffer) + const decoder = decoding.createDecoder(updateMessage) const messageType = decoding.readVarUint(decoder) if (messageType !== messageSync) { console.error('y.js update message with invalid type', messageType)