Fix retry on push with objects (#907)

instead of modifying the doc, just do the bare minimum re-applying of the push & return that value.

Author: seefeldb

runner/src/cell.ts

@@ -400,13 +400,6 @@ function createRegularCell<T>(
         }
       });
 
-      // Hacky retry logic for push only. See storage.ts for details on this
-      // retry approach and what we should really be doing instead.
-      if (!ref.cell.retry) ref.cell.retry = [];
-      ref.cell.retry.push((newBaseValue: any[]) =>
-        diffAndUpdate(ref, [...newBaseValue, ...valuesToWrite], log, cause)
-      );
-
       // If there is no array yet, create it first. We have to do this as a
       // separate operation, so that in the next steps [ID] is properly anchored
       // in the array.
@@ -417,6 +410,27 @@ function createRegularCell<T>(
 
       // Append the new values to the array.
       diffAndUpdate(ref, [...array, ...valuesToWrite], log, cause);
+
+      const appended = ref.cell.getAtPath(ref.path).slice(
+        -valuesToWrite.length,
+      );
+
+      // Hacky retry logic for push only. See storage.ts for details on this
+      // retry approach and what we should really be doing instead.
+      if (!ref.cell.retry) ref.cell.retry = [];
+      ref.cell.retry.push((newBaseValue: any[]) => {
+        // Unlikely, but maybe the conflict reset to undefined?
+        if (newBaseValue === undefined) {
+          newBaseValue = Array.isArray(schema?.default) ? schema.default : [];
+        }
+
+        // Serialize cell links that were appended during the push. This works
+        // because of the .toJSON() method on Cell.
+        const newValues = JSON.parse(JSON.stringify(appended));
+
+        // Reappend the new values.
+        return [...newBaseValue, ...newValues];
+      });
     },
     equals: (other: Cell<any>) =>
       JSON.stringify(self) === JSON.stringify(other),

runner/test/push-conflict.test.ts

@@ -1,9 +1,11 @@
 import { describe, it } from "@std/testing/bdd";
 import { expect } from "@std/expect";
+import { ID } from "@commontools/builder";
+import { Identity } from "@commontools/identity";
 import { storage } from "../src/storage.ts";
-import { getDoc } from "@commontools/runner";
+import { getDoc } from "../src/doc.ts";
+import { isCellLink } from "../src/cell.ts";
 import { VolatileStorageProvider } from "../src/storage/volatile.ts";
-import { Identity } from "@commontools/identity";
 
 storage.setRemoteStorage(new URL(`volatile:`));
 storage.setSigner(await Identity.fromPassphrase("test operator"));
@@ -51,7 +53,7 @@ describe("Push conflict", () => {
       "name",
       "push and set",
     );
-    const listDoc = getDoc<any[]>([], "list", "push and set");
+    const listDoc = getDoc<any[]>([], "list 2", "push and set");
 
     const name = nameDoc.asCell();
     const list = listDoc.asCell();
@@ -96,4 +98,64 @@ describe("Push conflict", () => {
     // Retry list should be empty now, since the change was applied.
     expect(!!listDoc.retry?.length).toBe(false);
   });
+
+  it("should resolve push conflicts with ID among other conflicts", async () => {
+    const nameDoc = getDoc<string | undefined>(
+      undefined,
+      "name 2",
+      "push and set",
+    );
+    const listDoc = getDoc<any[]>([], "list 3", "push and set");
+
+    const name = nameDoc.asCell();
+    const list = listDoc.asCell();
+
+    await storage.syncCell(name);
+    await storage.syncCell(list);
+
+    const memory = new VolatileStorageProvider("push and set");
+
+    // Update memory without notifying main storage
+    await memory.sync(nameDoc.entityId, true); // Get current value
+    await memory.sync(listDoc.entityId, true); // Get current value
+    await memory.send<any>([{
+      entityId: nameDoc.entityId,
+      value: { value: "foo" },
+    }, {
+      entityId: listDoc.entityId,
+      value: { value: [{ n: 1 }, { n: 2 }, { n: 3 }] },
+    }], true); // true = do not notify main storage
+
+    let retryCalled = 0;
+    listDoc.retry = [(value) => {
+      retryCalled++;
+      return value;
+    }];
+
+    name.set("bar");
+    list.push({ n: 4, [ID]: "4" });
+
+    // This is locally ahead of the db, and retry wasn't called yet.
+    expect(name.get()).toEqual("bar");
+    expect(list.get()).toEqual([{ n: 4 }]);
+    expect(isCellLink(listDoc.get()?.[0])).toBe(true);
+    const entry = listDoc.get()[0].cell?.asCell();
+    expect(retryCalled).toEqual(0);
+
+    await storage.synced();
+
+    // We successfully replayed the change on top of the db:
+    expect(name.get()).toEqual("foo");
+    expect(
+      list.asSchema({
+        type: "array",
+        items: { type: "object", properties: { n: { type: "number" } } },
+      }).get(),
+    ).toEqual([{ n: 1 }, { n: 2 }, { n: 3 }, { n: 4 }]);
+    expect(retryCalled).toEqual(1);
+    expect(!!listDoc.retry?.length).toBe(false);
+
+    // Check that the ID is still there
+    expect(entry.equals(listDoc.get()[3])).toBe(true);
+  });
 });