From 37f8f736e0366403127bb535ae4d2ddf6a6a4cd5 Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Mon, 22 Aug 2022 14:37:10 +0100 Subject: [PATCH] sliding sync: handle lone DELETE and INSERT operations If you leave a room you can get a lone DELETE op. If you join a room you can get a lone INSERT op. Up until now, we've assumed these operations happen at the ends of the list (e.g [0] or [length-1]) which is not guaranteed as it depends on the sort order (e.g sort alphabetically and join a room called 'D'). In this scenario, the indexes would not be tracked correctly. Fixed with integration tests. --- spec/integ/sliding-sync.spec.ts | 147 ++++++++++++++++++++++++++++++++ src/sliding-sync.ts | 104 +++++++++++++++------- 2 files changed, 221 insertions(+), 30 deletions(-) diff --git a/spec/integ/sliding-sync.spec.ts b/spec/integ/sliding-sync.spec.ts index 8c4a7ad1254..4cfe392158c 100644 --- a/spec/integ/sliding-sync.spec.ts +++ b/spec/integ/sliding-sync.spec.ts @@ -558,6 +558,153 @@ describe("SlidingSync", () => { await httpBackend.flushAllExpected(); await responseProcessed; await listPromise; + }); + + // this refers to a set of operations where the end result is no change. + it("should handle net zero operations correctly", async () => { + const indexToRoomId = { + 0: roomB, + 1: roomC, + }; + expect(slidingSync.getListData(0).roomIndexToRoomId).toEqual(indexToRoomId); + httpBackend.when("POST", syncUrl).respond(200, { + pos: "f", + // currently the list is [B,C] so we will insert D then immediately delete it + lists: [{ + count: 500, + ops: [ + { + op: "DELETE", index: 2, + }, + { + op: "INSERT", index: 0, room_id: roomA, + }, + { + op: "DELETE", index: 0, + }, + ], + }, + { + count: 50, + }], + }); + const listPromise = listenUntil(slidingSync, "SlidingSync.List", + (listIndex, joinedCount, roomIndexToRoomId) => { + expect(listIndex).toEqual(0); + expect(joinedCount).toEqual(500); + expect(roomIndexToRoomId).toEqual(indexToRoomId); + return true; + }); + const responseProcessed = listenUntil(slidingSync, "SlidingSync.Lifecycle", (state) => { + return state === SlidingSyncState.Complete; + }); + await httpBackend.flushAllExpected(); + await responseProcessed; + await listPromise; + }); + + it("should handle deletions correctly", async () => { + expect(slidingSync.getListData(0).roomIndexToRoomId).toEqual({ + 0: roomB, + 1: roomC, + }); + httpBackend.when("POST", syncUrl).respond(200, { + pos: "g", + lists: [{ + count: 499, + ops: [ + { + op: "DELETE", index: 0, + }, + ], + }, + { + count: 50, + }], + }); + const listPromise = listenUntil(slidingSync, "SlidingSync.List", + (listIndex, joinedCount, roomIndexToRoomId) => { + expect(listIndex).toEqual(0); + expect(joinedCount).toEqual(499); + expect(roomIndexToRoomId).toEqual({ + 0: roomC, + }); + return true; + }); + const responseProcessed = listenUntil(slidingSync, "SlidingSync.Lifecycle", (state) => { + return state === SlidingSyncState.Complete; + }); + await httpBackend.flushAllExpected(); + await responseProcessed; + await listPromise; + }); + + it("should handle insertions correctly", async () => { + expect(slidingSync.getListData(0).roomIndexToRoomId).toEqual({ + 0: roomC, + }); + httpBackend.when("POST", syncUrl).respond(200, { + pos: "h", + lists: [{ + count: 500, + ops: [ + { + op: "INSERT", index: 1, room_id: roomA, + }, + ], + }, + { + count: 50, + }], + }); + let listPromise = listenUntil(slidingSync, "SlidingSync.List", + (listIndex, joinedCount, roomIndexToRoomId) => { + expect(listIndex).toEqual(0); + expect(joinedCount).toEqual(500); + expect(roomIndexToRoomId).toEqual({ + 0: roomC, + 1: roomA, + }); + return true; + }); + let responseProcessed = listenUntil(slidingSync, "SlidingSync.Lifecycle", (state) => { + return state === SlidingSyncState.Complete; + }); + await httpBackend.flushAllExpected(); + await responseProcessed; + await listPromise; + + httpBackend.when("POST", syncUrl).respond(200, { + pos: "h", + lists: [{ + count: 501, + ops: [ + { + op: "INSERT", index: 1, room_id: roomB, + }, + ], + }, + { + count: 50, + }], + }); + listPromise = listenUntil(slidingSync, "SlidingSync.List", + (listIndex, joinedCount, roomIndexToRoomId) => { + expect(listIndex).toEqual(0); + expect(joinedCount).toEqual(501); + expect(roomIndexToRoomId).toEqual({ + 0: roomC, + 1: roomB, + 2: roomA, + }); + return true; + }); + responseProcessed = listenUntil(slidingSync, "SlidingSync.Lifecycle", (state) => { + return state === SlidingSyncState.Complete; + }); + await httpBackend.flushAllExpected(); + await responseProcessed; + await listPromise; slidingSync.stop(); }); }); diff --git a/src/sliding-sync.ts b/src/sliding-sync.ts index 28026b3a999..08a29051506 100644 --- a/src/sliding-sync.ts +++ b/src/sliding-sync.ts @@ -530,6 +530,65 @@ export class SlidingSync extends TypedEventEmitter low; i--) { + if (this.lists[listIndex].isIndexInRange(i)) { + this.lists[listIndex].roomIndexToRoomId[i] = + this.lists[listIndex].roomIndexToRoomId[ + i - 1 + ]; + } + } + } + + private shiftLeft(listIndex: number, hi: number, low: number) { + // l h + // 0,1,2,3,4 <- before + // 0,1,3,4,4 <- after, low is deleted and hi is duplicated + for (let i = low; i < hi; i++) { + if (this.lists[listIndex].isIndexInRange(i)) { + this.lists[listIndex].roomIndexToRoomId[i] = + this.lists[listIndex].roomIndexToRoomId[ + i + 1 + ]; + } + } + } + + private removeEntry(listIndex: number, index: number) { + // work out the max index + let max = -1; + for (const n in this.lists[listIndex].roomIndexToRoomId) { + if (Number(n) > max) { + max = Number(n); + } + } + if (max < 0 || index > max) { + return; + } + // Everything higher than the gap needs to be shifted left. + this.shiftLeft(listIndex, max, index); + delete this.lists[listIndex].roomIndexToRoomId[max]; + } + + private addEntry(listIndex: number, index: number) { + // work out the max index + let max = -1; + for (const n in this.lists[listIndex].roomIndexToRoomId) { + if (Number(n) > max) { + max = Number(n); + } + } + if (max < 0 || index > max) { + return; + } + // Everything higher than the gap needs to be shifted right, +1 so we don't delete the highest element + this.shiftRight(listIndex, max+1, index); + } + private processListOps(list: ListResponse, listIndex: number): void { let gapIndex = -1; list.ops.forEach((op: Operation) => { @@ -537,6 +596,10 @@ export class SlidingSync extends TypedEventEmitter op.index) { + // we haven't been told where to shift from, so make way for a new room entry. + this.addEntry(listIndex, op.index); + } else if (gapIndex > op.index) { // the gap is further down the list, shift every element to the right // starting at the gap so we can just shift each element in turn: // [A,B,C,_] gapIndex=3, op.index=0 @@ -572,26 +624,13 @@ export class SlidingSync extends TypedEventEmitter op.index; i--) { - if (this.lists[listIndex].isIndexInRange(i)) { - this.lists[listIndex].roomIndexToRoomId[i] = - this.lists[listIndex].roomIndexToRoomId[ - i - 1 - ]; - } - } + this.shiftRight(listIndex, gapIndex, op.index); } else if (gapIndex < op.index) { // the gap is further up the list, shift every element to the left // starting at the gap so we can just shift each element in turn - for (let i = gapIndex; i < op.index; i++) { - if (this.lists[listIndex].isIndexInRange(i)) { - this.lists[listIndex].roomIndexToRoomId[i] = - this.lists[listIndex].roomIndexToRoomId[ - i + 1 - ]; - } - } + this.shiftLeft(listIndex, op.index, gapIndex); } + gapIndex = -1; // forget the gap, we don't need it anymore. } this.lists[listIndex].roomIndexToRoomId[op.index] = op.room_id; break; @@ -631,6 +670,11 @@ export class SlidingSync extends TypedEventEmitter