Skip to content

Commit 50b0a31

Browse files
chriszaratechriszarate
authored
Apply only detected changes from the persisted CRDT document (#74668)
1 parent 22c5f89 commit 50b0a31

2 files changed

Lines changed: 96 additions & 106 deletions

File tree

‎packages/sync/src/manager.ts‎

Lines changed: 82 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -189,21 +189,7 @@ export function createSyncManager(): SyncManager {
189189
recordMetaMap.observe( onRecordMetaUpdate );
190190

191191
// Get and apply the persisted CRDT document, if it exists.
192-
const isInvalid = applyPersistedCrdtDoc( syncConfig, ydoc, record );
193-
194-
// If there is no persisted document or if it has been invalidated by out-of-
195-
// band updates, apply changes from the current entity record to the CRDT
196-
// document. This ensures that the CRDT document reflects the latest state of
197-
// the entity record.
198-
if ( isInvalid ) {
199-
ydoc.transact( () => {
200-
syncConfig.applyChangesToCRDTDoc( ydoc, record );
201-
}, LOCAL_SYNC_MANAGER_ORIGIN );
202-
203-
const meta = createEntityMeta( objectType, objectId );
204-
handlers.editRecord( { meta } );
205-
handlers.saveRecord();
206-
}
192+
applyPersistedCrdtDoc( objectType, objectId, record );
207193
}
208194

209195
/**
@@ -230,52 +216,99 @@ export function createSyncManager(): SyncManager {
230216
}
231217

232218
/**
233-
* Apply a persisted CRDT document to the current document, if it exists.
234-
* Return true if the document exists and is valid, otherwise false. Returning
235-
* a boolean allows us to destroy the temporary document and prevent it from
236-
* leaking out.
219+
* Load and inspect the persisted CRDT document. If supported and it exists,
220+
* compare it against the current entity record. If there are differences,
221+
* apply the changes from the entity record.
237222
*
238-
* @param {SyncConfig} syncConfig Sync configuration for the object type.
239-
* @param {CRDTDoc} targetDoc Target CRDT doc.
223+
* @param {ObjectType} objectType Object type.
224+
* @param {ObjectID} objectId Object ID.
240225
* @param {ObjectData} record Entity record representing this object type.
241-
* @return {boolean} Whether the persisted document is non-existent or invalid.
242226
*/
243227
function applyPersistedCrdtDoc(
244-
syncConfig: SyncConfig,
245-
targetDoc: CRDTDoc,
228+
objectType: ObjectType,
229+
objectId: ObjectID,
246230
record: ObjectData
247-
): boolean {
248-
if ( ! syncConfig.supports?.crdtPersistence ) {
249-
return true;
250-
}
251-
252-
// Get the persisted CRDT document, if it exists.
253-
const tempDoc = getPersistedCrdtDoc( record );
231+
): void {
232+
const entityId = getEntityId( objectType, objectId );
233+
const entityState = entityStates.get( entityId );
254234

255-
if ( ! tempDoc ) {
256-
return true;
235+
if ( ! entityState ) {
236+
return;
257237
}
258238

259-
// Apply the persisted document to the current document as a singular update.
260-
// This is done even if the persisted document has been invalidated. This
261-
// prevents a newly joining peer (or refreshing user) from re-initializing
262-
// the CRDT document (the "initialization problem").
263-
const update = Y.encodeStateAsUpdateV2( tempDoc );
239+
const {
240+
handlers,
241+
syncConfig: {
242+
applyChangesToCRDTDoc,
243+
getChangesFromCRDTDoc,
244+
supports,
245+
},
246+
ydoc: targetDoc,
247+
} = entityState;
248+
264249
targetDoc.transact( () => {
265-
Y.applyUpdateV2( targetDoc, update );
266-
}, LOCAL_SYNC_MANAGER_ORIGIN );
250+
if ( ! supports?.crdtPersistence ) {
251+
// Apply the current record as changes.
252+
applyChangesToCRDTDoc( targetDoc, record );
253+
return;
254+
}
255+
256+
// Get the persisted CRDT document, if it exists.
257+
const tempDoc = getPersistedCrdtDoc( record );
258+
259+
if ( ! tempDoc ) {
260+
// Apply the current record as changes and trigger a save, which will
261+
// persist the CRDT document. (The entity should call `createEntityMeta`
262+
// via its pre-persist hook.)
263+
applyChangesToCRDTDoc( targetDoc, record );
264+
handlers.saveRecord();
265+
return;
266+
}
267267

268-
// Check if the persisted doc has been invalidated by out-of-band updates
269-
// (e.g., a WP CLI command or direct database update) by determining if the
270-
// persisted document introduces any changes that are not present in the
271-
// current record. If it has been invalidated, then we return true as a
272-
// signal that we need to apply the entity record to the target document.
273-
const changes = syncConfig.getChangesFromCRDTDoc( tempDoc, record );
268+
// Apply the persisted document to the current document as a single update.
269+
// This is done even if the persisted document has been invalidated. This
270+
// prevents a newly joining peer (or refreshing user) from re-initializing
271+
// the CRDT document (the "initialization problem").
272+
const update = Y.encodeStateAsUpdateV2( tempDoc );
273+
Y.applyUpdateV2( targetDoc, update );
274274

275-
// Destroy the temporary document to prevent leaks.
276-
tempDoc.destroy();
275+
// Compute the differences between the persisted doc and the current
276+
// record. This can happen when:
277+
//
278+
// 1. The server makes updates on save that mutate the entity. Example: On
279+
// initial save, the server adds the "Uncategorized" category to the
280+
// post.
281+
// 2. An "out-of-band" update occurs. Example: a WP-CLI command or direct
282+
// database update mutates the entity.
283+
// 3. Unsaved changes are synced from a peer _before_ this code runs. We
284+
// can't control when (or if) remote changes are synced, so this is a
285+
// race condition.
286+
const invalidations = getChangesFromCRDTDoc( tempDoc, record );
287+
const invalidatedKeys = Object.keys( invalidations );
288+
289+
// Destroy the temporary document to prevent leaks.
290+
tempDoc.destroy();
291+
292+
if ( 0 === invalidatedKeys.length ) {
293+
// The persisted CRDT document is valid. There are no updates to apply.
294+
return;
295+
}
277296

278-
return Object.keys( changes ).length > 0;
297+
// Use the invalidated keys to get the updated values from the entity.
298+
const changes = invalidatedKeys.reduce(
299+
( acc, key ) =>
300+
Object.assign( acc, {
301+
[ key ]: record[ key ],
302+
} ),
303+
{}
304+
);
305+
306+
// Apply the changes and trigger a save, which will persist the CRDT
307+
// document. (The entity should call `createEntityMeta` via its pre-persist
308+
// hook.)
309+
applyChangesToCRDTDoc( targetDoc, changes );
310+
handlers.saveRecord();
311+
}, LOCAL_SYNC_MANAGER_ORIGIN );
279312
}
280313

281314
/**

‎packages/sync/src/test/manager.ts‎

Lines changed: 14 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@ import {
2222
CRDT_RECORD_METADATA_MAP_KEY as RECORD_METADATA_MAP_KEY,
2323
CRDT_RECORD_METADATA_SAVED_AT_KEY as SAVED_AT_KEY,
2424
CRDT_RECORD_METADATA_SAVED_BY_KEY as SAVED_BY_KEY,
25-
WORDPRESS_META_KEY_FOR_CRDT_DOC_PERSISTENCE,
2625
} from '../config';
2726
import { createPersistedCRDTDoc } from '../persistence';
2827
import { getProviderCreators } from '../providers';
@@ -269,33 +268,16 @@ describe( 'SyncManager', () => {
269268
mockSyncConfig.applyChangesToCRDTDoc
270269
).toHaveBeenCalledWith( expect.any( Y.Doc ), mockRecord );
271270

272-
// Changes should be correctly applied.
273-
const mockCall =
274-
mockSyncConfig.applyChangesToCRDTDoc.mock.calls[ 0 ];
275-
const targetDoc = mockCall[ 0 ] as Y.Doc;
276-
const appliedChanges = mockCall[ 1 ] as ObjectData;
277-
expect(
278-
targetDoc.getMap( CRDT_RECORD_MAP_KEY ).get( 'title' )
279-
).toBeUndefined();
280-
expect( appliedChanges.title ).toStrictEqual( 'Test Post' );
281-
282271
// getChangesFromCRDTDoc should not be called since there was no persisted doc.
283272
expect(
284273
mockSyncConfig.getChangesFromCRDTDoc
285274
).not.toHaveBeenCalled();
286275

287276
// Verify a save operation occurred.
288-
expect( mockHandlers.editRecord ).toHaveBeenCalledTimes( 1 );
289-
expect( mockHandlers.editRecord ).toHaveBeenCalledWith( {
290-
meta: {
291-
[ WORDPRESS_META_KEY_FOR_CRDT_DOC_PERSISTENCE ]:
292-
expect.any( String ),
293-
},
294-
} );
295277
expect( mockHandlers.saveRecord ).toHaveBeenCalledTimes( 1 );
296278
} );
297279

298-
it( 'applies a valid persisted CRDT doc without applying the current record', async () => {
280+
it( 'accepts a valid persisted CRDT doc without applying changes', async () => {
299281
const record = createRecordWithPersistedCRDTDoc( mockRecord );
300282

301283
mockSyncConfig = {
@@ -313,7 +295,7 @@ describe( 'SyncManager', () => {
313295
mockHandlers
314296
);
315297

316-
// Current record should NOT be applied since the persisted doc is valid.
298+
// Changes should NOT be applied since the persisted doc is valid.
317299
expect(
318300
mockSyncConfig.applyChangesToCRDTDoc
319301
).not.toHaveBeenCalled();
@@ -331,9 +313,10 @@ describe( 'SyncManager', () => {
331313
expect( mockHandlers.saveRecord ).not.toHaveBeenCalled();
332314
} );
333315

334-
it( 'applies an invalid persisted CRDT doc, then applies the current record', async () => {
316+
it( 'applies an invalid CRDT doc, then applies changes', async () => {
335317
const record = createRecordWithPersistedCRDTDoc( mockRecord, {
336-
title: 'Title from persisted CRDT doc',
318+
...mockRecord,
319+
title: 'Invalidated title from persisted CRDT doc',
337320
} );
338321

339322
mockSyncConfig = {
@@ -351,23 +334,17 @@ describe( 'SyncManager', () => {
351334
mockHandlers
352335
);
353336

354-
// Current record should be applied since the persisted doc is invalid.
337+
// Changes should be applied for the invalidated properties.
338+
const expectedChanges = {
339+
title: mockRecord.title,
340+
};
341+
355342
expect(
356343
mockSyncConfig.applyChangesToCRDTDoc
357344
).toHaveBeenCalledTimes( 1 );
358345
expect(
359346
mockSyncConfig.applyChangesToCRDTDoc
360-
).toHaveBeenCalledWith( expect.any( Y.Doc ), record );
361-
362-
// Changes should be correctly applied.
363-
const mockCall =
364-
mockSyncConfig.applyChangesToCRDTDoc.mock.calls[ 0 ];
365-
const targetDoc = mockCall[ 0 ] as Y.Doc;
366-
const appliedChanges = mockCall[ 1 ] as ObjectData;
367-
expect(
368-
targetDoc.getMap( CRDT_RECORD_MAP_KEY ).get( 'title' )
369-
).toStrictEqual( 'Title from persisted CRDT doc' );
370-
expect( appliedChanges.title ).toStrictEqual( 'Test Post' );
347+
).toHaveBeenCalledWith( expect.any( Y.Doc ), expectedChanges );
371348

372349
// getChangesFromCRDTDoc should be called with the persisted doc and record.
373350
expect(
@@ -378,18 +355,12 @@ describe( 'SyncManager', () => {
378355
).toHaveBeenCalledWith( expect.any( Y.Doc ), record );
379356

380357
// Verify a save operation occurred.
381-
expect( mockHandlers.editRecord ).toHaveBeenCalledTimes( 1 );
382-
expect( mockHandlers.editRecord ).toHaveBeenCalledWith( {
383-
meta: {
384-
[ WORDPRESS_META_KEY_FOR_CRDT_DOC_PERSISTENCE ]:
385-
expect.any( String ),
386-
},
387-
} );
388358
expect( mockHandlers.saveRecord ).toHaveBeenCalledTimes( 1 );
389359
} );
390360

391361
it( 'ignores a persisted CRDT doc when CRDT persistence is not supported', async () => {
392362
const record = createRecordWithPersistedCRDTDoc( mockRecord, {
363+
...mockRecord,
393364
title: 'Persisted Title',
394365
} );
395366

@@ -411,27 +382,13 @@ describe( 'SyncManager', () => {
411382
mockSyncConfig.applyChangesToCRDTDoc
412383
).toHaveBeenCalledWith( expect.any( Y.Doc ), record );
413384

414-
// Changes should be correctly applied.
415-
const mockCall =
416-
mockSyncConfig.applyChangesToCRDTDoc.mock.calls[ 0 ];
417-
const targetDoc = mockCall[ 0 ] as Y.Doc;
418-
const appliedChanges = mockCall[ 1 ] as ObjectData;
419-
expect(
420-
targetDoc.getMap( CRDT_RECORD_MAP_KEY ).get( 'title' )
421-
).toBeUndefined();
422-
expect( appliedChanges.title ).toStrictEqual( 'Test Post' );
423-
424385
// getChangesFromCRDTDoc should not be called since the persisted doc is igored.
425386
expect(
426387
mockSyncConfig.getChangesFromCRDTDoc
427388
).not.toHaveBeenCalled();
428389

429-
// Verify a save operation occurred.
430-
expect( mockHandlers.editRecord ).toHaveBeenCalledTimes( 1 );
431-
expect( mockHandlers.editRecord ).toHaveBeenCalledWith( {
432-
meta: {},
433-
} );
434-
expect( mockHandlers.saveRecord ).toHaveBeenCalledTimes( 1 );
390+
// Verify no save operation occurred because persistence is not supported.
391+
expect( mockHandlers.saveRecord ).not.toHaveBeenCalled();
435392
} );
436393
} );
437394
} );

0 commit comments

Comments
 (0)