fix(json-crdt-patch): 🐛 prevent recursion through new_val operation

streamich · streamich · commit 8ab4e2c70417 · 2023-11-05T14:06:21.000+01:00
diff --git a/src/json-crdt-patch/Patch.ts b/src/json-crdt-patch/Patch.ts
@@ -1,20 +1,4 @@
-import {
-  NewConOp,
-  NewObjOp,
-  NewValOp,
-  NewVecOp,
-  NewStrOp,
-  NewBinOp,
-  NewArrOp,
-  InsValOp,
-  InsObjOp,
-  InsVecOp,
-  InsStrOp,
-  InsBinOp,
-  InsArrOp,
-  DelOp,
-  NopOp,
-} from './operations';
+import * as operations from './operations';
 import {ITimestampStruct, ts, toDisplayString} from './clock';
 import {SESSION} from './constants';
 import {encode, decode} from './codec/binary';
@@ -24,21 +8,21 @@ import type {Printable} from '../util/print/types';
  * A union type of all possible JSON CRDT patch operations.
  */
 export type JsonCrdtPatchOperation =
-  | NewConOp
-  | NewValOp
-  | NewVecOp
-  | NewObjOp
-  | NewStrOp
-  | NewBinOp
-  | NewArrOp
-  | InsValOp
-  | InsObjOp
-  | InsVecOp
-  | InsStrOp
-  | InsBinOp
-  | InsArrOp
-  | DelOp
-  | NopOp;
+  | operations.NewConOp
+  | operations.NewValOp
+  | operations.NewVecOp
+  | operations.NewObjOp
+  | operations.NewStrOp
+  | operations.NewBinOp
+  | operations.NewArrOp
+  | operations.InsValOp
+  | operations.InsObjOp
+  | operations.InsVecOp
+  | operations.InsStrOp
+  | operations.InsBinOp
+  | operations.InsArrOp
+  | operations.DelOp
+  | operations.NopOp;
 
 /**
  * Represents a JSON CRDT patch.
@@ -135,27 +119,27 @@ export class Patch implements Printable {
     const patchOps = patch.ops;
     for (let i = 0; i < length; i++) {
       const op = ops[i];
-      if (op instanceof DelOp) patchOps.push(new DelOp(ts(op.id), ts(op.obj), op.what));
-      else if (op instanceof NewConOp) patchOps.push(new NewConOp(ts(op.id), op.val));
-      else if (op instanceof NewVecOp) patchOps.push(new NewVecOp(ts(op.id)));
-      else if (op instanceof NewValOp) patchOps.push(new NewValOp(ts(op.id), ts(op.val)));
-      else if (op instanceof NewObjOp) patchOps.push(new NewObjOp(ts(op.id)));
-      else if (op instanceof NewStrOp) patchOps.push(new NewStrOp(ts(op.id)));
-      else if (op instanceof NewBinOp) patchOps.push(new NewBinOp(ts(op.id)));
-      else if (op instanceof NewArrOp) patchOps.push(new NewArrOp(ts(op.id)));
-      else if (op instanceof InsArrOp) patchOps.push(new InsArrOp(ts(op.id), ts(op.obj), ts(op.ref), op.data.map(ts)));
-      else if (op instanceof InsStrOp) patchOps.push(new InsStrOp(ts(op.id), ts(op.obj), ts(op.ref), op.data));
-      else if (op instanceof InsBinOp) patchOps.push(new InsBinOp(ts(op.id), ts(op.obj), ts(op.ref), op.data));
-      else if (op instanceof InsValOp) patchOps.push(new InsValOp(ts(op.id), ts(op.obj), ts(op.val)));
-      else if (op instanceof InsObjOp)
+      if (op instanceof operations.DelOp) patchOps.push(new operations.DelOp(ts(op.id), ts(op.obj), op.what));
+      else if (op instanceof operations.NewConOp) patchOps.push(new operations.NewConOp(ts(op.id), op.val));
+      else if (op instanceof operations.NewVecOp) patchOps.push(new operations.NewVecOp(ts(op.id)));
+      else if (op instanceof operations.NewValOp) patchOps.push(new operations.NewValOp(ts(op.id)));
+      else if (op instanceof operations.NewObjOp) patchOps.push(new operations.NewObjOp(ts(op.id)));
+      else if (op instanceof operations.NewStrOp) patchOps.push(new operations.NewStrOp(ts(op.id)));
+      else if (op instanceof operations.NewBinOp) patchOps.push(new operations.NewBinOp(ts(op.id)));
+      else if (op instanceof operations.NewArrOp) patchOps.push(new operations.NewArrOp(ts(op.id)));
+      else if (op instanceof operations.InsArrOp) patchOps.push(new operations.InsArrOp(ts(op.id), ts(op.obj), ts(op.ref), op.data.map(ts)));
+      else if (op instanceof operations.InsStrOp) patchOps.push(new operations.InsStrOp(ts(op.id), ts(op.obj), ts(op.ref), op.data));
+      else if (op instanceof operations.InsBinOp) patchOps.push(new operations.InsBinOp(ts(op.id), ts(op.obj), ts(op.ref), op.data));
+      else if (op instanceof operations.InsValOp) patchOps.push(new operations.InsValOp(ts(op.id), ts(op.obj), ts(op.val)));
+      else if (op instanceof operations.InsObjOp)
         patchOps.push(
-          new InsObjOp(
+          new operations.InsObjOp(
             ts(op.id),
             ts(op.obj),
             op.data.map(([key, value]) => [key, ts(value)]),
           ),
         );
-      else if (op instanceof NopOp) patchOps.push(new NopOp(ts(op.id), op.len));
+      else if (op instanceof operations.NopOp) patchOps.push(new operations.NopOp(ts(op.id), op.len));
     }
     return patch;
   }
diff --git a/src/json-crdt-patch/PatchBuilder.ts b/src/json-crdt-patch/PatchBuilder.ts
@@ -154,11 +154,12 @@ export class PatchBuilder {
    *
    * @param val Reference to another object.
    * @returns ID of the new operation.
+   * @todo Rename to `newVal`.
    */
-  public val(val: ITimestampStruct): ITimestampStruct {
+  public val(): ITimestampStruct {
     this.pad();
     const id = this.clock.tick(1);
-    this.patch.ops.push(new NewValOp(id, val));
+    this.patch.ops.push(new NewValOp(id));
     return id;
   }
 
@@ -351,7 +352,9 @@ export class PatchBuilder {
    */
   public jsonVal(value: unknown): ITimestampStruct {
     const id = this.const(value);
-    return this.val(id);
+    const valId = this.val();
+    this.setVal(valId, id);
+    return valId;
   }
 
   /**
diff --git a/src/json-crdt-patch/builder/schema.ts b/src/json-crdt-patch/builder/schema.ts
@@ -32,8 +32,10 @@ export namespace nodes {
 
     constructor(public readonly value: T) {
       super((builder) => {
+        const valId = builder.val();
         const valueId = value.build(builder);
-        return builder.val(valueId);
+        builder.setVal(valId, valueId);
+        return valId;
       });
     }
   }
diff --git a/src/json-crdt-patch/codec/__tests__/PatchFuzzer.ts b/src/json-crdt-patch/codec/__tests__/PatchFuzzer.ts
@@ -28,7 +28,7 @@ export class PatchFuzzer extends Fuzzer {
         () => builder.arr(),
         () => builder.str(),
         () => builder.bin(),
-        () => builder.val(ts()),
+        () => builder.val(),
         () => builder.const(RandomJson.generate()),
         () => builder.root(ts()),
         () =>
diff --git a/src/json-crdt-patch/codec/binary/Decoder.ts b/src/json-crdt-patch/codec/binary/Decoder.ts
@@ -73,7 +73,7 @@ export class Decoder extends CborDecoder<CrdtReader> {
         break;
       }
       case JsonCrdtPatchOpcode.new_val: {
-        builder.val(this.decodeId());
+        builder.val();
         break;
       }
       case JsonCrdtPatchOpcode.new_obj: {
diff --git a/src/json-crdt-patch/codec/binary/Encoder.ts b/src/json-crdt-patch/codec/binary/Encoder.ts
@@ -94,10 +94,7 @@ export class Encoder extends CborEncoder<CrdtWriter> {
         break;
       }
       case operations.NewValOp: {
-        const operation = <operations.NewValOp>op;
-        const val = operation.val;
         writer.u8(JsonCrdtPatchOpcode.new_val);
-        this.encodeId(val);
         break;
       }
       case operations.NewObjOp: {
diff --git a/src/json-crdt-patch/codec/compact/decode.ts b/src/json-crdt-patch/codec/compact/decode.ts
@@ -38,7 +38,7 @@ export const decode = (data: types.CompactCodecPatch): Patch => {
         break;
       }
       case JsonCrdtPatchOpcode.new_val: {
-        builder.val(timestamp(sid, op[1]));
+        builder.val();
         break;
       }
       case JsonCrdtPatchOpcode.new_obj: {
diff --git a/src/json-crdt-patch/codec/compact/encode.ts b/src/json-crdt-patch/codec/compact/encode.ts
@@ -47,7 +47,7 @@ export const encode = (patch: Patch): types.CompactCodecPatch => {
         res.push([JsonCrdtPatchOpcode.new_con, val]);
       }
     } else if (op instanceof operations.NewValOp) {
-      res.push([JsonCrdtPatchOpcode.new_val, timestamp(sid, op.val)]);
+      res.push([JsonCrdtPatchOpcode.new_val]);
     } else if (op instanceof operations.NewObjOp) {
       res.push([JsonCrdtPatchOpcode.new_obj]);
     } else if (op instanceof operations.NewVecOp) {
diff --git a/src/json-crdt-patch/codec/verbose/decode.ts b/src/json-crdt-patch/codec/verbose/decode.ts
@@ -30,7 +30,7 @@ export const decode = (data: types.JsonCodecPatch): Patch => {
         break;
       }
       case 'new_val': {
-        builder.val(decodeId(op.value));
+        builder.val();
         break;
       }
       case 'new_obj': {
diff --git a/src/json-crdt-patch/codec/verbose/encode.ts b/src/json-crdt-patch/codec/verbose/encode.ts
@@ -45,7 +45,7 @@ export const encode = (patch: Patch): types.JsonCodecPatch => {
     } else if (op instanceof operations.NewBinOp) {
       ops.push({op: 'new_bin'});
     } else if (op instanceof operations.NewValOp) {
-      ops.push({op: 'new_val', value: encodeTimestamp(op.val)});
+      ops.push({op: 'new_val'});
     } else if (op instanceof operations.InsObjOp) {
       ops.push({
         op: 'ins_obj',
diff --git a/src/json-crdt-patch/codec/verbose/types.ts b/src/json-crdt-patch/codec/verbose/types.ts
@@ -119,12 +119,7 @@ export interface JsonCodecNewConOperation extends JsonCodecOperationBase<'new_co
  * Operation which creates a new "val" CRDT data type, which is a
  * Last-Write-Wins Register of a pointer to another CRDT data type.
  */
-export interface JsonCodecNewValOperation extends JsonCodecOperationBase<'new_val'> {
-  /**
-   * ID of the "val" LWW-Register object latest value.
-   */
-  value: JsonCodecTimestamp;
-}
+export type JsonCodecNewValOperation = JsonCodecOperationBase<'new_val'>;
 
 /**
  * Operation which creates a new "object" CRDT data type, which is a map of
diff --git a/src/json-crdt-patch/operations.ts b/src/json-crdt-patch/operations.ts
@@ -37,7 +37,7 @@ export class NewConOp implements IJsonCrdtPatchOperation {
  * @category Operations
  */
 export class NewValOp implements IJsonCrdtPatchOperation {
-  constructor(public readonly id: ITimestampStruct, public readonly val: ITimestampStruct) {}
+  constructor(public readonly id: ITimestampStruct) {}
 
   public span(): number {
     return 1;
@@ -48,7 +48,7 @@ export class NewValOp implements IJsonCrdtPatchOperation {
   }
 
   public toString(): string {
-    return `"${this.name()}" ${toDisplayString(this.id)} { ${toDisplayString(this.val)} }`;
+    return `"${this.name()}" ${toDisplayString(this.id)}`;
   }
 }
 
@@ -165,6 +165,7 @@ export class NewArrOp implements IJsonCrdtPatchOperation {
 export class InsValOp implements IJsonCrdtPatchEditOperation {
   constructor(
     public readonly id: ITimestampStruct,
+    /** @todo Rename to `node`. */
     public readonly obj: ITimestampStruct,
     public readonly val: ITimestampStruct,
   ) {}
diff --git a/src/json-crdt/__tests__/recursion.spec.ts b/src/json-crdt/__tests__/recursion.spec.ts
@@ -108,37 +108,21 @@ describe('recursive node references are not allowed', () => {
   });
 
   describe('val', () => {
-    describe('new_val', () => {
-      test('reference to parent object in same patch', () => {
-        const model = Model.withLogicalClock();
-        const builder = model.api.builder;
-        const objId = builder.obj();
-        const valId = builder.val();
-        builder.insObj(objId, [['val', valId]]);
-        builder.root(objId);
-        const patch1 = builder.flush();
-        model.applyPatch(patch1);
-        console.log(model + '');
-        console.log(model.view());
-        expect(model.view()).toStrictEqual({val: undefined});
-      });
-    });
-
     describe('ins_val', () => {
       test('reference to parent object in second patch', () => {
         const model = Model.withLogicalClock();
         const builder = model.api.builder;
         const objId = builder.obj();
-        const conId = builder.const(3);
-        const valId = builder.val(conId);
+        const valId = builder.val();
         builder.insObj(objId, [['val', valId]]);
         builder.root(objId);
         const patch1 = builder.flush();
         model.applyPatch(patch1);
+        expect((model.view() as any).val).toStrictEqual(undefined);
         builder.setVal(valId, objId);
         const patch2 = builder.flush();
         model.applyPatch(patch2);
-        expect(model.view()).toStrictEqual({val: 3});
+        expect((model.view() as any).val).toStrictEqual(undefined);
       });
     });
   });
diff --git a/src/json-crdt/model/Model.ts b/src/json-crdt/model/Model.ts
@@ -187,10 +187,7 @@ export class Model<RootJsonNode extends JsonNode = JsonNode> implements Printabl
       if (!index.get(id)) index.set(id, new StrNode(id));
     } else if (op instanceof operations.NewValOp) {
       const id = op.id;
-      if (!index.get(id)) {
-        const val = index.get(op.val);
-        if (val) index.set(id, new ValNode(this, id, op.val));
-      }
+      if (!index.get(id)) index.set(id, new ValNode(this, id, ORIGIN));
     } else if (op instanceof operations.NewConOp) {
       const id = op.id;
       if (!index.get(id)) index.set(id, new ConNode(id, op.val));
diff --git a/src/json-crdt/model/__tests__/Model.value.spec.ts b/src/json-crdt/model/__tests__/Model.value.spec.ts
@@ -7,8 +7,9 @@ describe('Document', () => {
     test('can create a value', () => {
       const doc = Model.withLogicalClock();
       const builder = new PatchBuilder(doc.clock);
+      const numId = builder.val();
       const val = builder.const([1, 2, 3]);
-      const numId = builder.val(val);
+      builder.setVal(numId, val);
       doc.applyPatch(builder.patch);
       const obj = doc.index.get(numId);
       expect(obj).toBeInstanceOf(ValNode);
@@ -17,7 +18,8 @@ describe('Document', () => {
     test('can set value as document root', () => {
       const doc = Model.withLogicalClock();
       const builder = new PatchBuilder(doc.clock);
-      const numId = builder.val(builder.const(10_000));
+      const numId = builder.val();
+      builder.setVal(numId, builder.const(10_000));
       builder.root(numId);
       doc.applyPatch(builder.patch);
       expect(doc.view()).toEqual(10_000);
@@ -26,7 +28,8 @@ describe('Document', () => {
     test('can update value to a number', () => {
       const doc = Model.withLogicalClock();
       const builder = new PatchBuilder(doc.clock);
-      const objId = builder.val(builder.const(1));
+      const objId = builder.val();
+      builder.setVal(objId, builder.const(1));
       builder.setVal(objId, builder.const(2));
       builder.root(objId);
       doc.applyPatch(builder.patch);
@@ -36,7 +39,8 @@ describe('Document', () => {
     test('can update value to a string', () => {
       const doc = Model.withLogicalClock();
       const builder = new PatchBuilder(doc.clock);
-      const objId = builder.val(builder.const(1));
+      const objId = builder.val();
+      builder.setVal(objId, builder.const(1));
       builder.setVal(objId, builder.const('boom'));
       builder.root(objId);
       doc.applyPatch(builder.patch);
@@ -46,7 +50,8 @@ describe('Document', () => {
     test('can overwrite number value', () => {
       const doc = Model.withLogicalClock();
       const builder = new PatchBuilder(doc.clock);
-      const valId = builder.val(builder.const(-1));
+      const valId = builder.val();
+      builder.setVal(valId, builder.const(-1));
       builder.setVal(valId, builder.const(123));
       builder.setVal(valId, builder.const(5.5));
       builder.root(valId);
@@ -58,7 +63,8 @@ describe('Document', () => {
       const doc = Model.withLogicalClock();
       const builder = new PatchBuilder(doc.clock);
       const objId = builder.obj();
-      const valId = builder.val(builder.const(25));
+      const valId = builder.val();
+      builder.setVal(valId, builder.const(25));
       builder.insObj(objId, [['gg', valId]]);
       builder.setVal(valId, builder.const(123));
       builder.setVal(valId, builder.const(99));
@@ -71,7 +77,8 @@ describe('Document', () => {
       const doc = Model.withLogicalClock();
       const builder = new PatchBuilder(doc.clock);
       const objId = builder.obj();
-      const valId = builder.val(builder.const(25));
+      const valId = builder.val();
+      builder.setVal(valId, builder.const(25));
       builder.insObj(objId, [['gg', valId]]);
       builder.setVal(valId, builder.const(123));
       builder.setVal(valId, builder.const(true));
@@ -84,7 +91,8 @@ describe('Document', () => {
       const doc = Model.withLogicalClock();
       const builder = new PatchBuilder(doc.clock);
       const objId = builder.arr();
-      const valId = builder.val(builder.const(25));
+      const valId = builder.val();
+      builder.setVal(valId, builder.const(25));
       builder.insArr(objId, objId, [valId]);
       builder.setVal(valId, builder.const(123));
       builder.setVal(valId, builder.const(true));
diff --git a/src/json-crdt/nodes/root/RootNode.ts b/src/json-crdt/nodes/root/RootNode.ts
@@ -1,6 +1,6 @@
-import {ORIGIN, SESSION} from '../../../json-crdt-patch/constants';
+import {ORIGIN} from '../../../json-crdt-patch/constants';
 import {ValNode} from '../val/ValNode';
-import {Model, UNDEFINED} from '../../model/Model';
+import {Model} from '../../model/Model';
 import type {ITimestampStruct} from '../../../json-crdt-patch/clock';
 import type {JsonNode} from '../types';
 
@@ -18,8 +18,4 @@ export class RootNode<Value extends JsonNode = JsonNode> extends ValNode<Value>
   constructor(doc: Model<any>, val: ITimestampStruct) {
     super(doc, ORIGIN, val);
   }
-
-  public node(): Value {
-    return this.val.sid === SESSION.SYSTEM ? <any>UNDEFINED : super.node();
-  }
 }
diff --git a/src/json-crdt/nodes/val/ValNode.ts b/src/json-crdt/nodes/val/ValNode.ts

Original file line number	Diff line number	Diff line change
`@@ -32,8 +32,10 @@ export namespace nodes {`
`32`	`32`
`33`	`33`	`constructor(public readonly value: T) {`
`34`	`34`	`super((builder) => {`
	`35`	`+ const valId = builder.val();`
`35`	`36`	`const valueId = value.build(builder);`
`36`		`- return builder.val(valueId);`
	`37`	`+ builder.setVal(valId, valueId);`
	`38`	`+ return valId;`
`37`	`39`	`});`
`38`	`40`	`}`
`39`	`41`	`}`
Original file line number	Diff line number	Diff line change
`@@ -73,7 +73,7 @@ export class Decoder extends CborDecoder<CrdtReader> {`
`73`	`73`	`break;`
`74`	`74`	`}`
`75`	`75`	`case JsonCrdtPatchOpcode.new_val: {`
`76`		`- builder.val(this.decodeId());`
	`76`	`+ builder.val();`
`77`	`77`	`break;`
`78`	`78`	`}`
`79`	`79`	`case JsonCrdtPatchOpcode.new_obj: {`
Original file line number	Diff line number	Diff line change
`@@ -94,10 +94,7 @@ export class Encoder extends CborEncoder<CrdtWriter> {`
`94`	`94`	`break;`
`95`	`95`	`}`
`96`	`96`	`case operations.NewValOp: {`
`97`		`- const operation = <operations.NewValOp>op;`
`98`		`- const val = operation.val;`
`99`	`97`	`writer.u8(JsonCrdtPatchOpcode.new_val);`
`100`		`- this.encodeId(val);`
`101`	`98`	`break;`
`102`	`99`	`}`
`103`	`100`	`case operations.NewObjOp: {`
Original file line number	Diff line number	Diff line change
`@@ -38,7 +38,7 @@ export const decode = (data: types.CompactCodecPatch): Patch => {`
`38`	`38`	`break;`
`39`	`39`	`}`
`40`	`40`	`case JsonCrdtPatchOpcode.new_val: {`
`41`		`- builder.val(timestamp(sid, op[1]));`
	`41`	`+ builder.val();`
`42`	`42`	`break;`
`43`	`43`	`}`
`44`	`44`	`case JsonCrdtPatchOpcode.new_obj: {`
Original file line number	Diff line number	Diff line change
`@@ -47,7 +47,7 @@ export const encode = (patch: Patch): types.CompactCodecPatch => {`
`47`	`47`	`res.push([JsonCrdtPatchOpcode.new_con, val]);`
`48`	`48`	`}`
`49`	`49`	`} else if (op instanceof operations.NewValOp) {`
`50`		`- res.push([JsonCrdtPatchOpcode.new_val, timestamp(sid, op.val)]);`
	`50`	`+ res.push([JsonCrdtPatchOpcode.new_val]);`
`51`	`51`	`} else if (op instanceof operations.NewObjOp) {`
`52`	`52`	`res.push([JsonCrdtPatchOpcode.new_obj]);`
`53`	`53`	`} else if (op instanceof operations.NewVecOp) {`
Original file line number	Diff line number	Diff line change
`@@ -30,7 +30,7 @@ export const decode = (data: types.JsonCodecPatch): Patch => {`
`30`	`30`	`break;`
`31`	`31`	`}`
`32`	`32`	`case 'new_val': {`
`33`		`- builder.val(decodeId(op.value));`
	`33`	`+ builder.val();`
`34`	`34`	`break;`
`35`	`35`	`}`
`36`	`36`	`case 'new_obj': {`