From fb1b4e20df12373e3e2919f267054f3a8b4dbdb2 Mon Sep 17 00:00:00 2001 From: David Bosschaert Date: Fri, 5 Sep 2025 14:11:57 +0100 Subject: [PATCH 1/3] fix: use document GUID as prosemirror reference to avoid updating deleted documents Previously the prosemirror document would be stored in the 'prosemirror' attribute on the YDoc. This changes that to be prosemirror-${guid}. This is to avoid accidentally left open editors from accidentally re-creating a deleted document. --- src/collab.js | 18 ++-- src/edge.js | 4 +- src/shareddoc.js | 115 +++++++++++++++++++--- test/collab.test.js | 98 +++++++++---------- test/collab2.test.js | 4 +- test/shareddoc.test.js | 216 +++++++++++++++++++++++++++++++++++++---- 6 files changed, 362 insertions(+), 93 deletions(-) diff --git a/src/collab.js b/src/collab.js index dfbfc66..265877e 100644 --- a/src/collab.js +++ b/src/collab.js @@ -10,9 +10,9 @@ * governing permissions and limitations under the License. */ import { - prosemirrorToYXmlFragment, yDocToProsemirror, + prosemirrorToYXmlFragment, yDocToProsemirrorJSON, } from 'y-prosemirror'; -import { DOMParser, DOMSerializer } from 'prosemirror-model'; +import { DOMParser, DOMSerializer, Node } from 'prosemirror-model'; import { fromHtml } from 'hast-util-from-html'; import { matches } from 'hast-util-select'; import { getSchema } from './schema.js'; @@ -142,7 +142,7 @@ function removeComments(node) { export const EMPTY_DOC = '
'; -export function aem2doc(html, ydoc) { +export function aem2doc(html, ydoc, guid) { if (!html) { // eslint-disable-next-line no-param-reassign html = EMPTY_DOC; @@ -280,7 +280,7 @@ export function aem2doc(html, ydoc) { }; const json = DOMParser.fromSchema(getSchema()).parse(new Proxy(main || tree, handler2)); - prosemirrorToYXmlFragment(json, ydoc.getXmlFragment('prosemirror')); + prosemirrorToYXmlFragment(json, ydoc.getXmlFragment(`prosemirror-${guid}`)); } const getAttrString = (attributes) => Object.entries(attributes).map(([key, value]) => ` ${key}="${value}"`).join(''); @@ -366,9 +366,15 @@ export function tableToBlock(child, fragment) { }); } -export function doc2aem(ydoc) { +export function doc2aem(ydoc, guid) { + if (!guid) { + // this is a brand new document + return EMPTY_DOC; + } + const schema = getSchema(); - const json = yDocToProsemirror(schema, ydoc); + const state = yDocToProsemirrorJSON(ydoc, `prosemirror-${guid}`); + const json = Node.fromJSON(schema, state); const fragment = { type: 'div', children: [], attributes: {} }; const handler3 = { diff --git a/src/edge.js b/src/edge.js index fe9c1fb..e716172 100644 --- a/src/edge.js +++ b/src/edge.js @@ -9,7 +9,7 @@ * OF ANY KIND, either express or implied. See the License for the specific language * governing permissions and limitations under the License. */ -import { invalidateFromAdmin, setupWSConnection } from './shareddoc.js'; +import { deleteFromAdmin, invalidateFromAdmin, setupWSConnection } from './shareddoc.js'; // This is the Edge Worker, built using Durable Objects! @@ -243,7 +243,7 @@ export class DocRoom { const api = url.searchParams.get('api'); switch (api) { case 'deleteAdmin': - if (await invalidateFromAdmin(baseURL)) { + if (await deleteFromAdmin(baseURL)) { return new Response(null, { status: 204 }); } else { return new Response('Not Found', { status: 404 }); diff --git a/src/shareddoc.js b/src/shareddoc.js index b7c67d2..862f7d2 100644 --- a/src/shareddoc.js +++ b/src/shareddoc.js @@ -189,7 +189,7 @@ export const persistence = { * @param {string} docName - The document name * @param {string} auth - The authorization header * @param {object} daadmin - The da-admin worker service binding - * @returns {Promise} - The content of the document + * @returns {object} - text: The content of the document and guid: the guid of the document. */ get: async (docName, auth, daadmin) => { const initalOpts = {}; @@ -198,7 +198,7 @@ export const persistence = { } const initialReq = await daadmin.fetch(docName, initalOpts); if (initialReq.ok) { - return initialReq.text(); + return { text: await initialReq.text(), guid: initialReq.headers.get('X-da-id') }; } else if (initialReq.status === 404) { return null; } else { @@ -215,11 +215,12 @@ export const persistence = { * @param {string} content - The content to store * @returns {object} The response from da-admin. */ - put: async (ydoc, content) => { + put: async (ydoc, content, guid) => { const blob = new Blob([content], { type: 'text/html' }); const formData = new FormData(); formData.append('data', blob); + formData.append('guid', guid); const opts = { method: 'PUT', body: formData }; const keys = Array.from(ydoc.conns.keys()); @@ -258,16 +259,64 @@ export const persistence = { * @param {WSSharedDoc} ydoc - the ydoc that has been updated. * @param {string} current - the current content of the document previously * obtained from da-admin + * @param {object} guidHolder - an object containing the guid of the document. + * If the document exists, it will hold its guid. If the document does not yet + * exists, it will be modified to set its guid in this method so that its known + * for subsequent calls. * @returns {string} - the new content of the document in da-admin. */ - update: async (ydoc, current) => { + update: async (ydoc, current, guidHolder) => { let closeAll = false; try { - const content = doc2aem(ydoc); + const { guid } = guidHolder; + + // The guid array contains the known guids. We sort it by timestamp so that we + // know to find the latest. Any other guids are considered stale. + // Objects on the guid array may also contain a newDoc flag, which is set to true + // when the document is just opened in the browser. + const guidArray = ydoc.getArray('prosemirror-guids'); + const copy = [...guidArray]; + if (copy.length === 0) { + // eslint-disable-next-line no-console + console.log('No guid array found in update. Ignoring.'); + return current; + } + copy.sort((a, b) => a.ts - b.ts); + const { newDoc, guid: curGuid, ts: createdTS } = copy.pop(); + + if (guid && curGuid !== guid) { + // Guid mismatch, need to update the editor + ydoc.transact(() => { + guidArray.delete(0, guidArray.length); // Delete the entire array + guidArray.push([{ guid, ts: createdTS + 1 }]); + }); + return current; + } + + if (!newDoc && !guid) { + // Someone is still editing a document in the browser that has since been deleted + // we know it's deleted because guid from da-admin is not set. + // eslint-disable-next-line no-console + console.log('Document GUID mismatch, da-admin guid:', guid, 'edited guid:', curGuid); + showError(ydoc, { message: 'This document has since been deleted, your edits are not persisted' }); + return current; + } + + const content = doc2aem(ydoc, curGuid); if (current !== content) { // Only store the document if it was actually changed. - const { ok, status, statusText } = await persistence.put(ydoc, content); + const { ok, status, statusText } = await persistence.put(ydoc, content, curGuid); + if (newDoc) { + // Update the guid in the guidHolder so that in subsequent calls we know what it is + // eslint-disable-next-line no-param-reassign + guidHolder.guid = curGuid; + // Remove the stale guids, and set the array to the current + ydoc.transact(() => { + guidArray.delete(0, guidArray.length); // Delete the entire array + guidArray.push([{ guid: curGuid, ts: createdTS }]); + }); + } if (!ok) { closeAll = (status === 401 || status === 403); throw new Error(`${status} - ${statusText}`); @@ -300,11 +349,18 @@ export const persistence = { let timingDaAdminGetDuration; let current; + let guid; let restored = false; // True if restored from worker storage try { let newDoc = false; const timingBeforeDaAdminGet = Date.now(); - current = await persistence.get(docName, conn.auth, ydoc.daadmin); + const cur = await persistence.get(docName, conn.auth, ydoc.daadmin); + if (cur === null) { + current = null; + } else { + current = cur?.text; + guid = cur?.guid; + } timingDaAdminGetDuration = Date.now() - timingBeforeDaAdminGet; const timingBeforeReadState = Date.now(); @@ -327,7 +383,7 @@ export const persistence = { // Check if the state from the worker storage is the same as the current state in da-admin. // So for example if da-admin doesn't have the doc any more, or if it has been altered in // another way, we don't use the state of the worker storage. - const fromStorage = doc2aem(ydoc); + const fromStorage = doc2aem(ydoc, guid); if (fromStorage === current) { restored = true; @@ -347,21 +403,25 @@ export const persistence = { showError(ydoc, error); } - if (!restored) { + if (!restored && guid) { // The doc was not restored from worker persistence, so read it from da-admin, - // but do this async to give the ydoc some time to get synced up first. Without - // this timeout, the ydoc can get confused which may result in duplicated content. + // but only if the doc actually exists in da-admin (guid has a value). + // If it's a brand new document, subsequent update() calls will set it in + // da-admin and provide the guid to use. + + // Do this async to give the ydoc some time to get synced up first. Without this + // timeout, the ydoc can get confused which may result in duplicated content. // eslint-disable-next-line no-console console.log('Could not be restored, trying to restore from da-admin', docName); setTimeout(() => { if (ydoc === docs.get(docName)) { - const rootType = ydoc.getXmlFragment('prosemirror'); + const rootType = ydoc.getXmlFragment(`prosemirror-${guid}`); ydoc.transact(() => { try { // clear document rootType.delete(0, rootType.length); // restore from da-admin - aem2doc(current, ydoc); + aem2doc(current, ydoc, guid); // eslint-disable-next-line no-console console.log('Restored from da-admin', docName); @@ -382,11 +442,15 @@ export const persistence = { } }); + // Use a holder for the guid. This is needed in case the guid is not known yet + // for a new document so that it can be updated later once its known. + const guidHolder = { guid }; + ydoc.on('update', debounce(async () => { // If we receive an update on the document, store it in da-admin, but debounce it // to avoid excessive da-admin calls. if (ydoc === docs.get(docName)) { - current = await persistence.update(ydoc, current); + current = await persistence.update(ydoc, current, guidHolder); } }, 2000, { maxWait: 10000 })); @@ -544,6 +608,29 @@ export const messageListener = (conn, doc, message) => { } }; +export const deleteFromAdmin = async (docName) => { + // eslint-disable-next-line no-console + console.log('Delete from Admin received', docName); + const ydoc = docs.get(docName); + if (ydoc) { + // empty out all known docs, should normally just be one + for (const { guid } of ydoc.getArray('prosemirror-guids')) { + ydoc.transact(() => { + const rootType = ydoc.getXmlFragment(`prosemirror-${guid}`); + rootType.delete(0, rootType.length); + }); + } + + // Reset the connections to flush the guids + ydoc.conns.forEach((_, c) => closeConn(ydoc, c)); + return true; + } else { + // eslint-disable-next-line no-console + console.log('Document not found', docName); + } + return false; +}; + /** * Invalidate the worker storage for the document, which will ensure that when accessed * the worker will fetch the latest version of the document from the da-admin. diff --git a/test/collab.test.js b/test/collab.test.js index 5155196..44ddca6 100644 --- a/test/collab.test.js +++ b/test/collab.test.js @@ -38,8 +38,8 @@ describe('Parsing test suite', () => { html = collapseWhitespace(html); const yDoc = new Y.Doc(); - aem2doc(html, yDoc); - const result = doc2aem(yDoc); + aem2doc(html, yDoc, 'test-guid'); + const result = doc2aem(yDoc, 'test-guid'); assert.equal(collapseWhitespace(result), html); }); @@ -64,8 +64,8 @@ describe('Parsing test suite', () => { html = collapseWhitespace(html); const yDoc = new Y.Doc(); - aem2doc(html, yDoc); - const result = doc2aem(yDoc); + aem2doc(html, yDoc, 'some-guid'); + const result = doc2aem(yDoc, 'some-guid'); assert.equal(collapseWhitespace(result), html); }); @@ -79,8 +79,8 @@ describe('Parsing test suite', () => { `; const yDoc = new Y.Doc(); - aem2doc(html, yDoc); - const result = doc2aem(yDoc); + aem2doc(html, yDoc, 'some-guid'); + const result = doc2aem(yDoc, 'some-guid'); console.log(result); assert.equal(result, html); }) @@ -101,8 +101,8 @@ describe('Parsing test suite', () => { `; const yDoc = new Y.Doc(); - aem2doc(html, yDoc); - const result = doc2aem(yDoc); + aem2doc(html, yDoc, 'some-guid'); + const result = doc2aem(yDoc, 'some-guid'); assert.equal(result, expectedResult); }); @@ -122,8 +122,8 @@ describe('Parsing test suite', () => { `; const yDoc = new Y.Doc(); - aem2doc(html, yDoc); - const result = doc2aem(yDoc); + aem2doc(html, yDoc, 'some-guid'); + const result = doc2aem(yDoc, 'some-guid'); assert.equal(result, expectedResult); }); @@ -136,8 +136,8 @@ describe('Parsing test suite', () => { `; const yDoc = new Y.Doc(); - aem2doc(html, yDoc); - const result = doc2aem(yDoc); + aem2doc(html, yDoc, 'my-guid'); + const result = doc2aem(yDoc, 'my-guid'); assert.equal(result, html); }); @@ -150,8 +150,8 @@ describe('Parsing test suite', () => { `; const yDoc = new Y.Doc(); - aem2doc(html, yDoc); - const result = doc2aem(yDoc); + aem2doc(html, yDoc, 'my-guid'); + const result = doc2aem(yDoc, 'my-guid'); assert.equal(result, html); }); @@ -164,8 +164,8 @@ describe('Parsing test suite', () => { `; const yDoc = new Y.Doc(); - aem2doc(html, yDoc); - const result = doc2aem(yDoc); + aem2doc(html, yDoc, 'my-guid'); + const result = doc2aem(yDoc, 'my-guid'); assert.equal(result, html); }); @@ -178,8 +178,8 @@ describe('Parsing test suite', () => { `; const yDoc = new Y.Doc(); - aem2doc(html, yDoc); - const result = doc2aem(yDoc); + aem2doc(html, yDoc, 'my-guid'); + const result = doc2aem(yDoc, 'my-guid'); assert.equal(result, html); }); @@ -192,8 +192,8 @@ describe('Parsing test suite', () => { `; const yDoc = new Y.Doc(); - aem2doc(html, yDoc); - const result = doc2aem(yDoc); + aem2doc(html, yDoc, 'my-guid'); + const result = doc2aem(yDoc, 'my-guid'); assert.equal(result, html); }); it('Test simple block roundtrip', async () => { @@ -205,8 +205,8 @@ it('Test simple block roundtrip', async () => { `; const yDoc = new Y.Doc(); -aem2doc(html, yDoc); -const result = doc2aem(yDoc); +aem2doc(html, yDoc, 'my-guid'); +const result = doc2aem(yDoc, 'my-guid'); assert.equal(result, html); }); it('Test complex block roundtrip', async () => { @@ -218,8 +218,8 @@ it('Test complex block roundtrip', async () => { `; const yDoc = new Y.Doc(); -aem2doc(html, yDoc); -const result = doc2aem(yDoc); +aem2doc(html, yDoc, 'my-guid'); +const result = doc2aem(yDoc, 'my-guid'); console.log(result); assert.equal(result, html); }); @@ -232,8 +232,8 @@ it('Test linebreak roundtrip', async () => { `; const yDoc = new Y.Doc(); -aem2doc(html, yDoc); -const result = doc2aem(yDoc); +aem2doc(html, yDoc, 'my-guid'); +const result = doc2aem(yDoc, 'my-guid'); console.log(result); assert.equal(result, html); }); @@ -247,8 +247,8 @@ assert.equal(result, html); `; const yDoc = new Y.Doc(); - aem2doc(html, yDoc); - const result = doc2aem(yDoc); + aem2doc(html, yDoc, 'my-guid'); + const result = doc2aem(yDoc, 'my-guid'); console.log(result); assert.equal(result, html); }); @@ -256,8 +256,8 @@ assert.equal(result, html); it('Test regional edit table parsing', async () => { const html = readFileSync('./test/mocks/regional-edit-1.html', 'utf-8'); const yDoc = new Y.Doc(); - aem2doc(html, yDoc); - const result = doc2aem(yDoc); + aem2doc(html, yDoc, 'my-guid'); + const result = doc2aem(yDoc, 'my-guid'); assert.equal(collapseWhitespace(result.trim()), collapseWhitespace(html.trim())); }); @@ -327,8 +327,8 @@ assert.equal(result, html); `; html = collapseWhitespace(html); const yDoc = new Y.Doc(); - aem2doc(html, yDoc); - const result = collapseWhitespace(doc2aem(yDoc)); + aem2doc(html, yDoc, 'my-guid'); + const result = collapseWhitespace(doc2aem(yDoc, 'my-guid')); assert.equal(result, html); }); @@ -341,8 +341,8 @@ assert.equal(result, html); `; const yDoc = new Y.Doc(); - aem2doc(html, yDoc); - const result = doc2aem(yDoc); + aem2doc(html, yDoc, 'my-guid'); + const result = doc2aem(yDoc, 'my-guid'); console.log(result); assert.equal(result, html); }); @@ -363,8 +363,8 @@ assert.equal(result, html); `; const yDoc = new Y.Doc(); - aem2doc(htmlIn, yDoc); - const result = doc2aem(yDoc); + aem2doc(htmlIn, yDoc, 'my-guid'); + const result = doc2aem(yDoc, 'my-guid'); console.log(result); assert.equal(result, htmlOut, 'The horizontal line should have been converted to a section break'); @@ -461,8 +461,8 @@ assert.equal(result, html); html = collapseWhitespace(html); const yDoc = new Y.Doc(); - aem2doc(html, yDoc); - const result = doc2aem(yDoc); + aem2doc(html, yDoc, 'my-guid'); + const result = doc2aem(yDoc, 'my-guid'); assert.equal(collapseWhitespace(result), html); }); @@ -487,8 +487,8 @@ assert.equal(result, html); html = collapseWhitespace(html); const yDoc = new Y.Doc(); - aem2doc(html, yDoc); - const result = doc2aem(yDoc); + aem2doc(html, yDoc, 'my-guid'); + const result = doc2aem(yDoc, 'my-guid'); assert.equal(collapseWhitespace(result), html); }); @@ -518,8 +518,8 @@ assert.equal(result, html); html = collapseWhitespace(html); const yDoc = new Y.Doc(); - aem2doc(html, yDoc); - const result = doc2aem(yDoc); + aem2doc(html, yDoc, 'my-guid'); + const result = doc2aem(yDoc, 'my-guid'); assert.equal(collapseWhitespace(result), html); }); @@ -542,8 +542,8 @@ assert.equal(result, html);
`; const yDoc = new Y.Doc(); - aem2doc(html, yDoc); - const result = doc2aem(yDoc); + aem2doc(html, yDoc, 'my-guid'); + const result = doc2aem(yDoc, 'my-guid'); assert.equal(collapseWhitespace(result), collapseWhitespace(html)); }); @@ -574,11 +574,11 @@ assert.equal(result, html);
`; const yDoc = new Y.Doc(); - aem2doc(html, yDoc); - const result = doc2aem(yDoc); + aem2doc(html, yDoc, 'my-guid'); + const result = doc2aem(yDoc, 'my-guid'); assert.equal(collapseWhitespace(result), collapseWhitespace(html)); }); - + it('can parse empty doc', async () => { const html = EMPTY_DOC; const yDoc = new Y.Doc(); @@ -598,8 +598,8 @@ assert.equal(result, html); it('can parse no main - results should remain unchanged - doc2aem wraps content into main', async () => { const html = '

Hello

World

'; const yDoc = new Y.Doc(); - aem2doc(html, yDoc); - const result = doc2aem(yDoc); + aem2doc(html, yDoc, 'my-guid'); + const result = doc2aem(yDoc, 'my-guid'); assert.equal(collapseWhitespace(result), collapseWhitespace('

Hello

World

')); }); }); diff --git a/test/collab2.test.js b/test/collab2.test.js index b1d0e80..56fd473 100644 --- a/test/collab2.test.js +++ b/test/collab2.test.js @@ -183,8 +183,8 @@ describe('Parsing test suite', () => { html = collapseWhitespace(html); const yDoc = new Y.Doc(); - aem2doc(html, yDoc); - const result = doc2aem(yDoc); + aem2doc(html, yDoc, 'my-guid'); + const result = doc2aem(yDoc, 'my-guid'); const expected = readFileSync('./test/mocks/expected-table.html', 'utf-8'); assert.equal(collapseWhitespace(result), collapseWhitespace(expected)); }); diff --git a/test/shareddoc.test.js b/test/shareddoc.test.js index 97ca695..4b66625 100644 --- a/test/shareddoc.test.js +++ b/test/shareddoc.test.js @@ -14,7 +14,7 @@ import assert from 'assert'; import esmock from 'esmock'; import { - closeConn, getYDoc, invalidateFromAdmin, messageListener, persistence, + closeConn, deleteFromAdmin, getYDoc, invalidateFromAdmin, messageListener, persistence, readState, setupWSConnection, setYDoc, showError, storeState, updateHandler, WSSharedDoc, } from '../src/shareddoc.js'; import { aem2doc, doc2aem, EMPTY_DOC } from '../src/collab.js'; @@ -144,10 +144,13 @@ describe('Collab Test Suite', () => { assert.equal(url, 'foo'); assert.equal(opts.method, undefined); assert(opts.headers === undefined); - return { ok: true, text: async () => 'content', status: 200, statusText: 'OK' }; + const headers = new Headers(); + headers.set('X-da-id', 'x1234y5678'); + return { ok: true, text: async () => 'content', status: 200, statusText: 'OK', headers }; }; const result = await persistence.get('foo', undefined, daadmin); - assert.equal(result, 'content'); + assert.equal(result.text, 'content'); + assert.equal(result.guid, 'x1234y5678'); }); it('Test persistence get auth', async () => { @@ -156,10 +159,10 @@ describe('Collab Test Suite', () => { assert.equal(url, 'foo'); assert.equal(opts.method, undefined); assert.equal(opts.headers.get('authorization'), 'auth'); - return { ok: true, text: async () => 'content', status: 200, statusText: 'OK' }; + return { ok: true, text: async () => 'content', status: 200, statusText: 'OK', headers: new Headers() }; }; const result = await persistence.get('foo', 'auth', daadmin); - assert.equal(result, 'content'); + assert.equal(result.text, 'content'); }); it('Test persistence get 404', async () => { @@ -290,13 +293,14 @@ describe('Collab Test Suite', () => { const mockYDoc = { conns: { keys() { return [ {} ] }}, name: 'http://foo.bar/0/123.html', + getArray() { return [{ ts: Date.now(), guid: '132' }]; }, }; pss.persistence.put = async (ydoc, content) => { assert.fail("update should not have happend"); } - const result = await pss.persistence.update(mockYDoc, 'Svr content'); + const result = await pss.persistence.update(mockYDoc, 'Svr content', { guid: '132' }); assert.equal(result, 'Svr content'); }); @@ -312,6 +316,7 @@ describe('Collab Test Suite', () => { const mockYDoc = { conns: { keys() { return [ {} ] }}, name: 'http://foo.bar/0/123.html', + getArray() { return [{ ts: Date.now(), guid: '437' }]; }, }; let called = false; @@ -327,7 +332,7 @@ describe('Collab Test Suite', () => { calledCloseCon = true; } - const result = await pss.persistence.update(mockYDoc, 'Svr content'); + const result = await pss.persistence.update(mockYDoc, 'Svr content', { guid: '437' }); assert.equal(result, 'Svr content update'); assert(called); assert(!calledCloseCon); @@ -345,6 +350,7 @@ describe('Collab Test Suite', () => { const mockYDoc = { conns: new Map().set('foo', 'bar'), name: 'http://foo.bar/0/123.html', + getArray() { return [{ ts: Date.now(), guid: '1234' }]; }, getMap(nm) { return nm === 'error' ? new Map() : null }, transact: (f) => f(), }; @@ -364,7 +370,7 @@ describe('Collab Test Suite', () => { calledCloseCon = true; } - const result = await pss.persistence.update(mockYDoc, 'Svr content'); + const result = await pss.persistence.update(mockYDoc, 'Svr content', { guid: '1234' }); assert.equal(result, 'Svr content'); assert(called); assert(calledCloseCon); @@ -375,6 +381,88 @@ describe('Collab Test Suite', () => { await testCloseAllOnAuthFailure(403); }); + it('Test no guid array in update does nothing', async () => { + const docName = 'http://a.b.c/my/doc.html'; + const ydoc = new WSSharedDoc(docName); + + const doc2aemCalled = []; + const mockDoc2Aem = () => { + doc2aemCalled.push('doc2aem'); + }; + const pss = await esmock( + '../src/shareddoc.js', { + '../src/collab.js': { + doc2aem: mockDoc2Aem + } + }); + + const res = await pss.persistence.update(ydoc, 'My Content', { guid: '1234' }); + assert.equal(0, doc2aemCalled.length, 'doc2aem should not have been called'); + assert.equal(res, 'My Content', 'Should not have changed the content, as the guid array was not set'); + }); + + it('Test update document deleted from admin produces error and is ignored', async () => { + const docName = 'http://a.b.c/my/doc.html'; + const ydoc = new WSSharedDoc(docName); + ydoc.transact = (f) => f(); + + const doc2aemCalled = []; + const mockDoc2Aem = () => { + doc2aemCalled.push('doc2aem'); + }; + const pss = await esmock( + '../src/shareddoc.js', { + '../src/collab.js': { + doc2aem: mockDoc2Aem + } + }); + + assert.equal(0, ydoc.getMap('error').size, 'Precondition'); + ydoc.getArray('prosemirror-guids').push([{ ts: Date.now(), guid: '1234' }]); + const res = await pss.persistence.update(ydoc, 'My Content', {}); + assert(ydoc.getMap('error').size > 0, 'Should have set the error'); + assert(ydoc.getMap('error').get('message').length > 0, 'Should have set the error message'); + assert.equal(0, doc2aemCalled.length, 'doc2aem should not have been called'); + assert.equal(res, 'My Content', 'Should not have changed the content, as the guid array was not set'); + }); + + it('Test update new doc with guid provided', async () => { + const docName = 'http://a.b.c/my/doc.html'; + + const initialContent = ` + +
+

Hello

+
+ +`; + + const ydoc = new WSSharedDoc(docName); + ydoc.transact = (f) => f(); + aem2doc(initialContent, ydoc, '3-3'); + + const pss = await esmock( + '../src/shareddoc.js', { + }); + pss.persistence.put = async (ydoc, content, guid) => { + if (guid == '3-3' && content.includes('Hello')) { + return { ok: true, status: 200, statusText: 'OK - Stored'}; + } + }; + + assert.equal(0, ydoc.getMap('error').size, 'Precondition'); + const ga = ydoc.getArray('prosemirror-guids'); + ga.push([{ ts: 222, guid: '2-2' }]); + ga.push([{ newDoc: true, ts: 333, guid: '3-3' }]); + ga.push([{ ts: 111, guid: '1-1' }]); + + const guidHolder = {}; + const res = await pss.persistence.update(ydoc, 'My Old Content', guidHolder); + assert.deepStrictEqual(guidHolder, { guid: '3-3' }); + assert.deepStrictEqual([{ guid: '3-3', ts: 333 }], [...ydoc.getArray('prosemirror-guids')]); + assert(res.includes('Hello'), 'Should have kept the content'); + }); + it('Test invalidateFromAdmin', async () => { const docName = 'http://blah.di.blah/a/ha.html'; @@ -400,6 +488,44 @@ describe('Collab Test Suite', () => { || res2.toString() === closeCalled.toString()); }); + it('Test deleteFromAdmin', async() => { + const docName = 'https://do.re.mi/so/la/ti.html'; + + const deleteCalled = []; + const rt1234 = { + length: 7, + delete: (i, l) => { deleteCalled.push(`delete(${i},${l})`)} + }; + const rt5678 = { + length: 19, + delete: (i, l) => { deleteCalled.push(`delete(${i},${l})`)} + }; + const rtMap = new Map(); + rtMap.set('prosemirror-g1234', rt1234); + rtMap.set('prosemirror-g5678', rt5678); + + const closeCalled = []; + const conn = { close: () => closeCalled.push('close') }; + const conns = new Map(); + conns.set(conn, new Set()); + + const testYDoc = new WSSharedDoc(docName); + testYDoc.getXmlFragment = (k) => rtMap.get(k); + const guids = testYDoc.getArray('prosemirror-guids'); + guids.push([{guid: 'g1234', ts: 999}]); + guids.push([{guid: 'g5678', ts: 777}]); + testYDoc.transact = (f) => f(); + testYDoc.conns = conns; + const m = setYDoc(docName, testYDoc); + + assert(m.has(docName), 'Precondition'); + deleteFromAdmin(docName); + assert(!m.has(docName), 'Document should have been removed from global map'); + + assert.deepStrictEqual(['close'], closeCalled); + assert.deepStrictEqual(['delete(0,7)', 'delete(0,19)'], deleteCalled); + }); + it('Test close connection', async () => { const awarenessEmitted = [] const mockDoc = { @@ -471,7 +597,10 @@ describe('Collab Test Suite', () => { const mockStorage = { list: () => new Map() }; - pss.persistence.get = async (nm, au, ad) => `Get: ${nm}-${au}-${ad}`; + pss.persistence.get = async (nm, au, ad) => ({ + guid: 'abc987', + text: `Get: ${nm}-${au}-${ad}` + }); const updated = new Map(); pss.persistence.update = async (d, v) => updated.set(d, v); @@ -554,24 +683,32 @@ describe('Collab Test Suite', () => { const storage = { list: async () => stored }; const savedGet = persistence.get; + const savedSetTimeout = globalThis.setTimeout; try { + const setTimeoutCalled = []; + globalThis.setTimeout = () => setTimeoutCalled.push('setTimeout'); persistence.get = (d) => { if (d === docName) { - return ` + return { + guid: 'abcd-efgh-1234-5678', + text: `
-`; +`}; } }; await persistence.bindState(docName, ydoc, conn, storage); assert.equal('somevalue', ydoc.getMap('foo').get('someattr')); + assert.equal(0, setTimeoutCalled.length, + 'Should not have called setTimeout as there is no document to restore from da-admin'); } finally { persistence.get = savedGet; + globalThis.setTimeout = savedSetTimeout; } }); @@ -587,15 +724,47 @@ describe('Collab Test Suite', () => { try { globalThis.setTimeout = (f) => f(); // run timeout method instantly - persistence.get = async () => ` + persistence.get = async () => ({ + guid: 'my-id', + text: ` + +
+
From daadmin
+
+ `}); + await persistence.bindState(docName, ydoc, {}, storage); + + assert(doc2aem(ydoc, 'my-id').includes('

From daadmin

')); + } finally { + persistence.get = savedGet; + globalThis.setTimeout = savedSetTimeout; + } + }); + + it('Test mismatch ID', async () => { + const docName = 'https://admin.da.live/source/foo/bar.html'; + const ydoc = new Y.Doc(); + setYDoc(docName, ydoc); + + const storage = { list: async () => { throw new Error('yikes') } }; + + const savedGet = persistence.get; + const savedSetTimeout = globalThis.setTimeout; + try { + globalThis.setTimeout = (f) => f(); // run timeout method instantly + + persistence.get = async () => ({ + guid: 'my-id', + text: `
From daadmin
- `; + `}); await persistence.bindState(docName, ydoc, {}, storage); - assert(doc2aem(ydoc).includes('

From daadmin

')); + assert(!doc2aem(ydoc, 'my-other-id').includes('

From daadmin

'), + 'The mismatched ID should not provide the daadmin content'); } finally { persistence.get = savedGet; globalThis.setTimeout = savedSetTimeout; @@ -620,6 +789,8 @@ describe('Collab Test Suite', () => { updObservers.push(fun); } }; + const ga = ydoc.getArray('prosemirror-guids'); + ga.push([{ ts: Date.now(), guid: '222-222-2222' }]); pss.setYDoc(docName, ydoc); const savedSetTimeout = globalThis.setTimeout; @@ -632,7 +803,10 @@ describe('Collab Test Suite', () => { f(); }; - pss.persistence.get = async () => '
oldcontent
'; + pss.persistence.get = async () => ({ + guid: '222-222-2222', + text: '
oldcontent
' + }); const putCalls = [] pss.persistence.put = async (yd, c) => { if (yd === ydoc && c.includes('newcontent')) { @@ -643,7 +817,7 @@ describe('Collab Test Suite', () => { await pss.persistence.bindState(docName, ydoc, {}, storage); - aem2doc('
newcontent
', ydoc); + aem2doc('
newcontent
', ydoc, '222-222-2222'); assert.equal(2, updObservers.length); await updObservers[0](); @@ -736,7 +910,6 @@ describe('Collab Test Suite', () => { await persistence.bindState(docName, ydoc, conn, storage); assert.deepStrictEqual([true], deleteAllCalled); - assert.equal(1, setTimeoutCalled.length, 'SetTimeout should have been called to update the doc'); } finally { globalThis.setTimeout = savedSetTimeout; } @@ -771,10 +944,13 @@ describe('Collab Test Suite', () => { globalThis.setTimeout = savedSetTimeout; f(); }; - persistence.get = async () => '
myinitial
'; + persistence.get = async () => ({ + guid: '1234-5678-9012', + text: '
myinitial
' + }); await persistence.bindState(docName, ydoc, conn, storage); - assert(doc2aem(ydoc).includes('myinitial')); + assert(doc2aem(ydoc, '1234-5678-9012').includes('myinitial')); assert.equal(2, updObservers.length); ydoc.getMap('yah').set('a', 'bcd'); @@ -789,7 +965,7 @@ describe('Collab Test Suite', () => { Y.applyUpdate(ydoc2, called[1].docstore); assert.equal('bcd', ydoc2.getMap('yah').get('a')); - assert(doc2aem(ydoc2).includes('myinitial')); + assert(doc2aem(ydoc, '1234-5678-9012').includes('myinitial')); } finally { globalThis.setTimeout = savedSetTimeout; persistence.get = savedGet; From 4fa6f12a644ea0de906210035bc0baba99160e73 Mon Sep 17 00:00:00 2001 From: David Bosschaert Date: Sat, 6 Sep 2025 18:57:51 +0100 Subject: [PATCH 2/3] chore: add unit test --- src/shareddoc.js | 19 ++++++++++--------- test/shareddoc.test.js | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 9 deletions(-) diff --git a/src/shareddoc.js b/src/shareddoc.js index 862f7d2..d286fd4 100644 --- a/src/shareddoc.js +++ b/src/shareddoc.js @@ -180,6 +180,13 @@ export const showError = (ydoc, err) => { } }; +const resetGuidArray = (ydoc, guidArray, guid, ts) => { + ydoc.transact(() => { + guidArray.delete(0, guidArray.length); // Delete the entire array + guidArray.push([{ guid, ts }]); + }); +}; + export const persistence = { closeConn: closeConn.bind(this), @@ -285,11 +292,8 @@ export const persistence = { const { newDoc, guid: curGuid, ts: createdTS } = copy.pop(); if (guid && curGuid !== guid) { - // Guid mismatch, need to update the editor - ydoc.transact(() => { - guidArray.delete(0, guidArray.length); // Delete the entire array - guidArray.push([{ guid, ts: createdTS + 1 }]); - }); + // Guid mismatch, need to update the editor to the guid from da-admin + resetGuidArray(ydoc, guidArray, guid, createdTS + 1); return current; } @@ -312,10 +316,7 @@ export const persistence = { guidHolder.guid = curGuid; // Remove the stale guids, and set the array to the current - ydoc.transact(() => { - guidArray.delete(0, guidArray.length); // Delete the entire array - guidArray.push([{ guid: curGuid, ts: createdTS }]); - }); + resetGuidArray(ydoc, guidArray, curGuid, createdTS); } if (!ok) { closeAll = (status === 401 || status === 403); diff --git a/test/shareddoc.test.js b/test/shareddoc.test.js index 4b66625..8a33948 100644 --- a/test/shareddoc.test.js +++ b/test/shareddoc.test.js @@ -458,11 +458,51 @@ describe('Collab Test Suite', () => { const guidHolder = {}; const res = await pss.persistence.update(ydoc, 'My Old Content', guidHolder); + assert.equal(0, ydoc.getMap('error').size, 'Should have been no errors'); assert.deepStrictEqual(guidHolder, { guid: '3-3' }); assert.deepStrictEqual([{ guid: '3-3', ts: 333 }], [...ydoc.getArray('prosemirror-guids')]); assert(res.includes('Hello'), 'Should have kept the content'); }); + it('Doc mismatch with existing doc in admin takes the doc from admin', async () => { + const docName = 'http://a.b.c/my/doc.html'; + + const initialContent = ` + +
+

Hi

+
+ +`; + + const ydoc = new WSSharedDoc(docName); + ydoc.transact = (f) => f(); + aem2doc(initialContent, ydoc, '4-4'); + + const pss = await esmock( + '../src/shareddoc.js', { + }); + pss.persistence.put = () => { + throw new Error('should not have put document'); + } + + assert.equal(0, ydoc.getMap('error').size, 'Precondition'); + const ga = ydoc.getArray('prosemirror-guids'); + ga.push([{ ts: 444, guid: '4-4'}]); + ga.push([{ ts: 333, guid: '3-3'}]); + + // The guidHolder holds the guid as it comes from da-admin. This one is different to what + // came in from the editor + const guidHolder = { guid: '5-5' }; + + const res = await pss.persistence.update(ydoc, 'My Admin Content', guidHolder); + assert.equal(0, ydoc.getMap('error').size, 'Should have been no errors'); + assert.equal(res, 'My Admin Content', 'Should have kept the content from da-admin'); + assert.equal(1, ga.length, 'Should have reset the guid array'); + assert.equal('5-5', ga.get(0).guid, 'The guid from da-admin should have been used'); + assert(ga.get(0).ts > 444, 'Should have made the timestamp newer'); + }); + it('Test invalidateFromAdmin', async () => { const docName = 'http://blah.di.blah/a/ha.html'; From c97cab8b144289a789384d7decbd2ec4eac16bd7 Mon Sep 17 00:00:00 2001 From: David Bosschaert Date: Tue, 9 Sep 2025 12:02:51 +0100 Subject: [PATCH 3/3] extra logging --- src/shareddoc.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/shareddoc.js b/src/shareddoc.js index d286fd4..6547390 100644 --- a/src/shareddoc.js +++ b/src/shareddoc.js @@ -293,6 +293,8 @@ export const persistence = { if (guid && curGuid !== guid) { // Guid mismatch, need to update the editor to the guid from da-admin + // eslint-disable-next-line no-console + console.log('Document GUID mismatch, da-admin guid:', guid, 'edited guid:', curGuid); resetGuidArray(ydoc, guidArray, guid, createdTS + 1); return current; } @@ -301,11 +303,13 @@ export const persistence = { // Someone is still editing a document in the browser that has since been deleted // we know it's deleted because guid from da-admin is not set. // eslint-disable-next-line no-console - console.log('Document GUID mismatch, da-admin guid:', guid, 'edited guid:', curGuid); + console.log('Document does not exist any more and is not new, ignoring. GUID: ', curGuid); showError(ydoc, { message: 'This document has since been deleted, your edits are not persisted' }); return current; } + // eslint-disable-next-line no-console + console.log('Document', ydoc.name, 'has guid:', curGuid); const content = doc2aem(ydoc, curGuid); if (current !== content) { // Only store the document if it was actually changed.