Merge pull request #458 from streamich/fix-deleted-container-contents

fix: make sure obj, vec, arr, and val nodes don't throw on .view() or .toString() calls
streamich · Nov 25, 2023 · 8c2dcfe · 8c2dcfe
2 parents f83faca + 397468d
commit 8c2dcfe
Show file tree

Hide file tree

Showing 5 changed files with 101 additions and 13 deletions.
diff --git a/src/json-crdt/model/__tests__/Model.node-deletion.spec.ts b/src/json-crdt/model/__tests__/Model.node-deletion.spec.ts
@@ -1,3 +1,4 @@
+import {s} from '../../../json-crdt-patch';
 import {ValNode} from '../../nodes';
 import {Model} from '../Model';
 
@@ -160,3 +161,74 @@ test('removes from index recursively after LWW register write', () => {
   expect(!!doc.index.get(val3)).toBe(false);
   expect(!!doc.index.get(val4)).toBe(false);
 });
+
+test('calling .view() on dangling "obj" when it was deleted, should not throw', () => {
+  const doc = Model.withLogicalClock().setSchema(
+    s.obj({
+      foo: s.obj({
+        bar: s.obj({
+          baz: s.con(123),
+          qux: s.str('asdf'),
+        }),
+      }),
+    }),
+  );
+
+  const bar = doc.root.child()!.get('foo')!.get('bar')!;
+  const baz = bar.get('baz')!;
+  const qux = bar.get('qux')!;
+  expect(bar.view()).toStrictEqual({
+    baz: 123,
+    qux: 'asdf',
+  });
+  doc.api.obj(['foo']).del(['bar']);
+  expect(bar.view()).toStrictEqual({});
+  expect((bar + '').includes(bar.id.time + '')).toBe(true);
+  expect(baz.view()).toBe(123);
+  expect(qux.view()).toBe('asdf');
+});
+
+test('calling .view() on dangling "arr" when it was deleted, should not throw', () => {
+  const doc = Model.withLogicalClock().setSchema(
+    s.obj({
+      foo: s.obj({
+        bar: s.arr([s.con(123), s.str('asdf')]),
+      }),
+    }),
+  );
+  const bar = doc.root.child()!.get('foo')!.get('bar')!;
+  expect(bar.view()).toStrictEqual([123, 'asdf']);
+  doc.api.obj(['foo']).del(['bar']);
+  expect(bar.view()).toStrictEqual([]);
+  expect((bar + '').includes(bar.id.time + '')).toBe(true);
+});
+
+test('calling .view() on dangling "vec" when it was deleted, should not throw', () => {
+  const doc = Model.withLogicalClock().setSchema(
+    s.obj({
+      foo: s.obj({
+        bar: s.vec(s.con(123), s.str('asdf')),
+      }),
+    }),
+  );
+  const bar = doc.root.child()!.get('foo')!.get('bar')!;
+  expect(bar.view()).toStrictEqual([123, 'asdf']);
+  doc.api.obj(['foo']).del(['bar']);
+  expect(bar.view()).toStrictEqual([undefined, undefined]);
+  expect((bar + '').includes(bar.id.time + '')).toBe(true);
+});
+
+test('calling .view() on dangling "val" when it was deleted, should not throw', () => {
+  const doc = Model.withLogicalClock().setSchema(
+    s.obj({
+      foo: s.obj({
+        bar: s.val(s.str('asdf')),
+      }),
+    }),
+  );
+  const bar = doc.root.child()!.get('foo')!.get('bar')!;
+  expect(bar.view()).toStrictEqual('asdf');
+  doc.api.obj(['foo']).del(['bar']);
+  expect(bar.view()).toStrictEqual(undefined);
+  expect((bar + '').includes(bar.id.time + '')).toBe(true);
+});
diff --git a/src/json-crdt/nodes/arr/ArrNode.ts b/src/json-crdt/nodes/arr/ArrNode.ts
@@ -155,7 +155,12 @@ export class ArrNode<Element extends JsonNode = JsonNode>
     for (let chunk = this.first(); chunk; chunk = this.next(chunk)) {
       if (chunk.del) continue;
       for (const node of chunk.data!) {
-        const element = index.get(node)!.view() as JsonNodeView<Element>;
+        const elementNode = index.get(node);
+        if (!elementNode) {
+          useCache = false;
+          continue;
+        }
+        const element = elementNode.view() as JsonNodeView<Element>;
         if (_view[view.length] !== element) useCache = false;
         view.push(element);
       }
@@ -188,9 +193,10 @@ export class ArrNode<Element extends JsonNode = JsonNode>
       const index = this.doc.index;
       valueTree = printTree(
         tab,
-        chunk.data!.map(
-          (id, i) => (tab) => `[${pos + i}]: ${index.get(id)!.toString(tab + '    ' + ' '.repeat(String(i).length))}`,
-        ),
+        chunk
+          .data!.map((id) => index.get(id))
+          .filter((node) => !!node)
+          .map((node, i) => (tab) => `[${pos + i}]: ${node!.toString(tab + '    ' + ' '.repeat(String(i).length))}`),
       );
     }
     return (

diff --git a/src/json-crdt/nodes/obj/ObjNode.ts b/src/json-crdt/nodes/obj/ObjNode.ts
@@ -111,7 +111,12 @@ export class ObjNode<Value extends Record<string, JsonNode> = Record<string, Jso
     const index = doc.index;
     let useCache = true;
     this.keys.forEach((id, key) => {
-      const value = index.get(id)!.view();
+      const valueNode = index.get(id);
+      if (!valueNode) {
+        useCache = false;
+        return;
+      }
+      const value = valueNode.view();
       if (value !== undefined) {
         if (_view[key] !== value) useCache = false;
         (<any>view)[key] = value;
@@ -137,11 +142,13 @@ export class ObjNode<Value extends Record<string, JsonNode> = Record<string, Jso
       header +
       printTree(
         tab,
-        [...this.keys.entries()].map(
-          ([key, id]) =>
-            (tab) =>
-              JSON.stringify(key) + printTree(tab + ' ', [(tab) => this.doc.index.get(id)!.toString(tab)]),
-        ),
+        [...this.keys.entries()]
+          .filter(([, id]) => !!this.doc.index.get(id))
+          .map(
+            ([key, id]) =>
+              (tab) =>
+                JSON.stringify(key) + printTree(tab + ' ', [(tab) => this.doc.index.get(id)!.toString(tab)]),
+          ),
       )
     );
   }

diff --git a/src/json-crdt/nodes/val/ValNode.ts b/src/json-crdt/nodes/val/ValNode.ts
@@ -45,13 +45,13 @@ export class ValNode<Value extends JsonNode = JsonNode> implements JsonNode<Read
    * @returns The latest value of the node.
    */
   public node(): Value {
-    return this.val.sid === SESSION.SYSTEM ? <any>UNDEFINED : this.child()!;
+    return this.val.sid === SESSION.SYSTEM ? <any>UNDEFINED : this.child();
   }
 
   // ----------------------------------------------------------------- JsonNode
 
   public view(): Readonly<JsonNodeView<Value>> {
-    return this.node().view() as Readonly<JsonNodeView<Value>>;
+    return this.node()?.view() as Readonly<JsonNodeView<Value>>;
   }
 
   /**

diff --git a/src/json-crdt/nodes/vec/VecNode.ts b/src/json-crdt/nodes/vec/VecNode.ts
@@ -183,12 +183,15 @@ export class VecNode<Value extends JsonNode[] = JsonNode[]>
     if (extNode) {
       return this.child()!.toString(tab);
     }
+    const index = this.doc.index;
     return (
       header +
       printTree(tab, [
         ...this.elements.map(
           (id, i) => (tab: string) =>
-            `${i}: ${!id ? 'nil' : this.doc.index.get(id)!.toString(tab + '  ' + ' '.repeat(('' + i).length))}`,
+            `${i}: ${
+              !id ? 'nil' : index.get(id) ? index.get(id)!.toString(tab + '  ' + ' '.repeat(('' + i).length)) : 'nil'
+            }`,
         ),
         ...(extNode ? [(tab: string) => `${this.child()!.toString(tab)}`] : []),
       ])