From 24a9562b077e40868a9aa29abaac2f55fe81f962 Mon Sep 17 00:00:00 2001
From: Kegan Dougal <kegan@matrix.org>
Date: Tue, 25 Oct 2022 13:07:53 +0100
Subject: [PATCH] bugfix: allow subtly different DELETE/INSERT semantics

In sliding sync, with an empty list, it is possible for the proxy
to send back DELETE 0, INSERT 0 !room which has the net result of
`[!room]`. Previously, the JS SDK would not handle this correctly.
Now it does. With tests.
---
 spec/integ/sliding-sync.spec.ts | 85 +++++++++++++++++++++++++++++++++
 src/sliding-sync.ts             |  7 ++-
 2 files changed, 91 insertions(+), 1 deletion(-)

diff --git a/spec/integ/sliding-sync.spec.ts b/spec/integ/sliding-sync.spec.ts
index 5d5646a8d..1b3e0b07b 100644
--- a/spec/integ/sliding-sync.spec.ts
+++ b/spec/integ/sliding-sync.spec.ts
@@ -806,6 +806,91 @@ describe("SlidingSync", () => {
             await listPromise;
             slidingSync.stop();
         });
+
+        // Regression test to make sure things like DELETE 0 INSERT 0 work correctly and we don't
+        // end up losing room IDs.
+        it("should handle insertions with a spurious DELETE correctly", async () => {
+            slidingSync = new SlidingSync(proxyBaseUrl, [
+                {
+                    ranges: [[0, 20]],
+                },
+            ], {}, client!, 1);
+            // initially start with nothing
+            httpBackend!.when("POST", syncUrl).respond(200, {
+                pos: "a",
+                lists: [{
+                    count: 0,
+                    ops: [],
+                }],
+            });
+            slidingSync.start();
+            await httpBackend!.flushAllExpected();
+            expect(slidingSync.getListData(0)!.roomIndexToRoomId).toEqual({});
+
+            // insert a room
+            httpBackend!.when("POST", syncUrl).respond(200, {
+                pos: "b",
+                lists: [{
+                    count: 1,
+                    ops: [
+                        {
+                            op: "DELETE", index: 0,
+                        },
+                        {
+                            op: "INSERT", index: 0, room_id: roomA,
+                        },
+                    ],
+                }],
+            });
+            await httpBackend!.flushAllExpected();
+            expect(slidingSync.getListData(0)!.roomIndexToRoomId).toEqual({
+                0: roomA,
+            });
+
+            // insert another room
+            httpBackend!.when("POST", syncUrl).respond(200, {
+                pos: "c",
+                lists: [{
+                    count: 1,
+                    ops: [
+                        {
+                            op: "DELETE", index: 1,
+                        },
+                        {
+                            op: "INSERT", index: 0, room_id: roomB,
+                        },
+                    ],
+                }],
+            });
+            await httpBackend!.flushAllExpected();
+            expect(slidingSync.getListData(0)!.roomIndexToRoomId).toEqual({
+                0: roomB,
+                1: roomA,
+            });
+
+            // insert a final room
+            httpBackend!.when("POST", syncUrl).respond(200, {
+                pos: "c",
+                lists: [{
+                    count: 1,
+                    ops: [
+                        {
+                            op: "DELETE", index: 2,
+                        },
+                        {
+                            op: "INSERT", index: 0, room_id: roomC,
+                        },
+                    ],
+                }],
+            });
+            await httpBackend!.flushAllExpected();
+            expect(slidingSync.getListData(0)!.roomIndexToRoomId).toEqual({
+                0: roomC,
+                1: roomB,
+                2: roomA,
+            });
+            slidingSync.stop();
+        });
     });
 
     describe("transaction IDs", () => {
diff --git a/src/sliding-sync.ts b/src/sliding-sync.ts
index 947ac85d1..481a50a70 100644
--- a/src/sliding-sync.ts
+++ b/src/sliding-sync.ts
@@ -641,8 +641,12 @@ export class SlidingSync extends TypedEventEmitter<SlidingSyncEvent, SlidingSync
                             // starting at the gap so we can just shift each element in turn
                             this.shiftLeft(listIndex, op.index, gapIndex);
                         }
-                        gapIndex = -1; // forget the gap, we don't need it anymore.
                     }
+                    // forget the gap, we don't need it anymore. This is outside the check for
+                    // a room being present in this index position because INSERTs always universally
+                    // forget the gap, not conditionally based on the presence of a room in the INSERT
+                    // position. Without this, DELETE 0; INSERT 0; would do the wrong thing.
+                    gapIndex = -1;
                     this.lists[listIndex].roomIndexToRoomId[op.index] = op.room_id;
                     break;
                 }
@@ -686,6 +690,7 @@ export class SlidingSync extends TypedEventEmitter<SlidingSyncEvent, SlidingSync
             // Everything higher than the gap needs to be shifted left.
             this.removeEntry(listIndex, gapIndex);
         }
+        console.log("post-process", listIndex, JSON.stringify(this.lists[listIndex].roomIndexToRoomId));
     }
 
     /**