[AIT-38] Protocol v6 and support for partial sync of objects#2152
[AIT-38] Protocol v6 and support for partial sync of objects#2152
Conversation
WalkthroughThe changes implement support for partial object synchronization in the AblyJS SDK by bumping the protocol version to 6, replacing the SyncObjectsDataPool utility with an internal Map-based accumulation structure in RealtimeObject, and adding comprehensive tests for handling partial OBJECT_SYNC messages across multiple sequential messages. Changes
Sequence DiagramsequenceDiagram
participant Network
participant RealtimeObject
participant _syncObjectsPool
participant LiveObjects
Network->>RealtimeObject: OBJECT_SYNC (partial: root entry)
RealtimeObject->>_syncObjectsPool: accumulate root
Network->>RealtimeObject: OBJECT_SYNC (partial: counter)
RealtimeObject->>_syncObjectsPool: accumulate counter entry
Network->>RealtimeObject: OBJECT_SYNC (partial: map entries)
RealtimeObject->>_syncObjectsPool: merge map state
RealtimeObject->>RealtimeObject: _mergeMapSyncState()
RealtimeObject->>RealtimeObject: _applyObjectSyncMessages()
RealtimeObject->>LiveObjects: materialize accumulated state
RealtimeObject->>_syncObjectsPool: clear() on sync end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/plugins/liveobjects/realtimeobject.ts`:
- Around line 372-381: The code uses objectState.objectId as a Map key without
validation which can cause messages to pile up under undefined; inside the loop
in realtimeobject.ts where objectState and objectId are extracted (symbols:
objectState, objectId, this._syncObjectsPool), add a guard that checks if
objectId is null/undefined and if so skip storing/merging that message and emit
a warning/error (use the existing logger on the class, e.g., this.logger.warn or
console.warn) before continue; ensure no entry is created in _syncObjectsPool
for missing objectId and include the offending message or minimal context in the
log to aid debugging.
🧹 Nitpick comments (1)
test/realtime/liveobjects.test.js (1)
1182-1242: Consider asserting on a post-sync root to avoid timing races.
The assertions currently useentryPathObjectcreated before the partial sync; if OBJECT_SYNC application ever becomes async, this can become flaky. Grabbing the root after the final sync message guarantees you’re asserting on the fresh state.♻️ Suggested tweak
- expect(entryPathObject.get('stringKey').value()).to.equal('hello', 'Check root has correct string value'); - expect(entryPathObject.get('counter').value()).to.equal(15, 'Check counter has correct aggregated value'); - expect(entryPathObject.get('map').get('foo').value()).to.equal('bar', 'Check map has initial entries'); - expect(entryPathObject.get('map').get('baz').value()).to.equal('qux', 'Check map has materialised entries'); + const root = await channel.object.get(); // ensure OBJECT_SYNC has fully applied + expect(root.get('stringKey').value()).to.equal('hello', 'Check root has correct string value'); + expect(root.get('counter').value()).to.equal(15, 'Check counter has correct aggregated value'); + expect(root.get('map').get('foo').value()).to.equal('bar', 'Check map has initial entries'); + expect(root.get('map').get('baz').value()).to.equal('qux', 'Check map has materialised entries');
| * for the same object, so only the entries need to be merged. | ||
| * @spec RTO5b2b1 | ||
| */ | ||
| private _mergeMapSyncState(existingEntry: ObjectMessage, newObjectMessage: ObjectMessage): void { |
There was a problem hiding this comment.
Question on spec re which ObjectMessage's serialTimestamp property should be kept: ably/specification#413 (comment)
There was a problem hiding this comment.
What was your motivation for removing this class? I'm a bit surprised that you did it as part of this work since it seems that it would have been an ideal place to put the new logic for merging ObjectMessages into the pool, instead of expanding RealtimeObject.
| } | ||
|
|
||
| if (newObjectState.map?.entries) { | ||
| // During partial sync, no two messages contain the same map key, |
There was a problem hiding this comment.
Worth mentioning in the spec, I'd say
| }, | ||
|
|
||
| { | ||
| description: 'partial OBJECT_SYNC builds object tree across multiple messages', |
There was a problem hiding this comment.
I feel I must have missed something with this first test — in what sense is it testing a partial object sync? There doesn't seem to be any object whose state is split across multiple messages, which I thought was the defining characteristic of a partial sync.
| initialEntries: { | ||
| initialKey: { timeserial: lexicoTimeserial('aaa', 0, 0), data: { string: 'initial' } }, | ||
| }, | ||
| // materialisedEntries are merged across partial messages |
There was a problem hiding this comment.
- the usage of "are" here is inconsistent — in the first one it describes a characteristic of the data sent by the server (makes sense), and in the second it describes an expected behaviour of the SDK (this note would be more appropriate against the assertions I think)
- I think that the injection of the final three messages would be easier to read if it were done in a loop in which the only thing that varies is the
syncSerialand thematerialisedEntries
2 participants
Spec for this ably/specification#413.
Resolves AIT-38.
This also removes unnecessary
SyncObjectsDataPoolclass and simplifies a sync logic a bit by moving it toRealtimeObject.Summary by CodeRabbit
New Features
Updates
✏️ Tip: You can customize this high-level summary in your review settings.