rework scenes structured merge algorithm to keep arrays ordering and …

…add tests

source/server/__test_fixtures/documents/04_tours.svx.json

@@ -129,6 +129,31 @@
             "duration": 1.5
           }
         ]
-      }
+      },
+      "tours": [
+        {
+          "id": "9Ya68S8n2tnl",
+          "steps": [
+            {
+              "id": "MJ05xF",
+              "titles": {
+                "EN": "New Step #0"
+              }
+            },
+            {
+              "id": "xyhq5j",
+              "titles": {
+                "EN": "New Step #1"
+              }
+            }
+          ],
+          "titles": {
+            "EN": "New Tour #0"
+          },
+          "leads": {
+            "EN": ""
+          }
+        }
+        ]
    }]
 }

source/server/routes/history/diff/get.test.ts

@@ -1,9 +1,12 @@
+import path from "path";
+import { readFile } from "fs/promises";
 
 import request from "supertest";
 import Vfs from "../../../vfs/index.js";
 import User from "../../../auth/User.js";
 import UserManager from "../../../auth/UserManager.js";
 
+import { fixturesDir } from "../../../__test_fixtures/fixtures.js";
 
 
 /**
@@ -36,16 +39,37 @@ describe("GET /history/:scene/:id/diff", function(){
     .expect("Content-Type", "text/plain; charset=utf-8");
     expect(res.text).to.equal(`--- hello.txt\n+++ hello.txt\n@@ -1 +1 @@\n-Hello\n+Hello World\n`);
   });
+  describe("documents", function(){
+    it("get diff summary", async function(){
+      await vfs.writeDoc(`{"label":"foo"}`, {scene: scene_id, name: "scene.svx.json", mime: "application/si-dpo-3d.document+json", user_id: 0});
+      let ref = await vfs.writeDoc(`{"label":"bar"}`, {scene: scene_id, name: "scene.svx.json", mime: "application/si-dpo-3d.document+json", user_id: 0});
+      let res = await request(this.server).get(`/history/foo/${ref.id}/diff`)
+      .set("Accept", "application/json")
+      .expect(200)
+      .expect("Content-Type", "application/json; charset=utf-8");
+      expect(res.body).to.have.property("diff").match(/Couldn't compile .*diff/);
+      expect(res.body).to.have.property("src");
+      expect(res.body).to.have.property("dst");
+    });
+    it("stringifies DELETED_KEY symbol", async function(){
+      const docString = await readFile(path.join(fixturesDir, "documents", "01_simple.svx.json"), {encoding: "utf-8"});
+      let doc2 = JSON.parse(docString);
+      delete doc2.asset.version
+      await vfs.writeDoc(docString, {scene: scene_id, name: "scene.svx.json", mime: "application/si-dpo-3d.document+json", user_id: 0});
+      let ref = await vfs.writeDoc(JSON.stringify(doc2), {scene: scene_id, name: "scene.svx.json", mime: "application/si-dpo-3d.document+json", user_id: 0});
+      let res = await request(this.server).get(`/history/foo/${ref.id}/diff`)
+      .set("Accept", "application/json")
+      .expect(200)
+      .expect("Content-Type", "application/json; charset=utf-8");
+
+      expect(res.body).to.have.property("diff", "STRUCTURED CHANGES SUMMARY\n"+JSON.stringify({
+        "asset": {
+          "version": "*DELETED*"
+        }
+      }, null, 2));
+      expect(res.body).to.have.property("src");
+      expect(res.body).to.have.property("dst");
+    });
 
-  it("get diff summary for documents", async function(){
-    await vfs.writeDoc(`{"label":"foo"}`, {scene: scene_id, name: "scene.svx.json", mime: "application/si-dpo-3d.document+json", user_id: 0});
-    let ref = await vfs.writeDoc(`{"label":"bar"}`, {scene: scene_id, name: "scene.svx.json", mime: "application/si-dpo-3d.document+json", user_id: 0});
-    let res = await request(this.server).get(`/history/foo/${ref.id}/diff`)
-    .set("Accept", "application/json")
-    .expect(200)
-    .expect("Content-Type", "application/json; charset=utf-8");
-    expect(res.body).to.have.property("diff").match(/Couldn't compile diff/);
-    expect(res.body).to.have.property("src");
-    expect(res.body).to.have.property("dst");
   })
 });

source/server/routes/history/diff/get.ts

@@ -5,11 +5,13 @@ import {Request, Response} from "express";
 import { getVfs } from "../../../utils/locals.js";
 import { BadRequestError } from "../../../utils/errors.js";
 import { FileProps } from "../../../vfs/types.js";
-import { diffDoc } from "../../../utils/merge/index.js";
+import { DELETE_KEY, diffDoc } from "../../../utils/merge/index.js";
 import { fromPointers } from "../../../utils/merge/pointers/index.js";
 
 const sizeMax = 400*1000;
 
+
+
 /**
  * Computes a somewhat arbitrary text representation of the difference between 
  */
@@ -52,7 +54,10 @@ export default async function handleGetDiff(req :Request, res :Response){
     try{
       let fromDoc = JSON.parse(fromFile.data);
       let toDoc = JSON.parse(dstFile.data);
-      diff = `STRUCTURED CHANGES SUMMARY\n`+JSON.stringify(fromPointers(diffDoc(fromDoc, toDoc) as any), null, 2);
+      diff = `STRUCTURED CHANGES SUMMARY\n`+JSON.stringify(fromPointers(diffDoc(fromDoc, toDoc) as any), (key, value)=>{
+        if(value === DELETE_KEY) return "*DELETED*";
+        return value;
+      }, 2);
     }catch(e:any){
       console.error("Failed to compile scene diff : ", e);
       diff = `Couldn't compile diff from #${fromFile.id} to #${dstFile.id}: ${e.message}`;

source/server/routes/scenes/post.ts

@@ -42,7 +42,7 @@ export default async function postScenes(req :Request, res :Response){
       let m  = /^(?<contained>scenes\/)?(?<scene>[^/]+)(?:\/(?<name>.+))?\/?$/.exec(record.filename);
       const scene :string|undefined = m?.groups?.scene;
       const name :string|undefined = m?.groups?.name;
-      console.log("Parse %s %s from zip", scene, name);
+
       if(!scene){
         results.fail.push(`${record.filename}: not matching pattern`);
         continue;

source/server/routes/scenes/scene/files/put/document.ts

@@ -35,13 +35,17 @@ export default async function handlePutDocument(req :Request, res :Response){
     if(!refDocString) throw new BadRequestError(`Referenced document is not valid`);
     const refDoc = JSON.parse(refDocString);
 
+    console.log("Ref doc :", JSON.stringify(refDoc.setups![0].tours, null, 2));
+    console.log("New doc :", JSON.stringify(newDoc.setups![0].tours, null, 2));
     const docDiff = merge.diffDoc(refDoc, newDoc);
     if(Object.keys(docDiff).length == 0){
       //Nothing to do
       return res.status(204).send();
     }
 
+    console.log("Diff :", JSON.stringify(docDiff, (key, value)=> value === merge.DELETE_KEY? "*DELETED*":value, 2));
     const mergedDoc = merge.applyDoc(currentDoc, docDiff);
+    console.log("Merged doc :", JSON.stringify(mergedDoc.setups![0].tours, null, 2));
     let s = JSON.stringify(mergedDoc);
     await tr.writeDoc(s, {scene: scene, user_id: uid, name: "scene.svx.json", mime: "application/si-dpo-3d.document+json"});
     res.status(204).send();

source/server/utils/merge/apply.test.ts

@@ -1,5 +1,5 @@
 
-import {DELETE_KEY} from "./pointers/types.js";
+import {DELETE_KEY, SOURCE_INDEX} from "./pointers/types.js";
 import apply from "./apply.js";
 
 describe("merge.apply()", function(){
@@ -38,4 +38,8 @@ describe("merge.apply()", function(){
       {name: "C"} // C gets pushed-back to index 2
     ]});
   });
+
+  it("merges special SOURCE_INDEX symbol", function(){
+    expect(apply({a:{[SOURCE_INDEX]:0}}, {a:{[SOURCE_INDEX]:1}})).to.deep.equal({a:{[SOURCE_INDEX]:1}});
+  });
 });

source/server/utils/merge/apply.ts

@@ -1,5 +1,5 @@
 'use strict';
-import {Diff, DELETE_KEY} from "./pointers/types.js";
+import {Diff, DELETE_KEY, SOURCE_INDEX, withIndex} from "./pointers/types.js";
 
 /**
  * Deep assign two or more objects
@@ -10,6 +10,7 @@ import {Diff, DELETE_KEY} from "./pointers/types.js";
  */
 export default function apply<T extends Record<string, any>>(into :T, ...diffs :Diff<T>[]):T{
   for(const diff of diffs){
+    if(SOURCE_INDEX in diff) into = withIndex(into, diff[SOURCE_INDEX] as number);
     for(const key in diff){
       const value = diff[key] as T[Extract<keyof T, string>];
 

source/server/utils/merge/diff.test.ts

@@ -1,5 +1,5 @@
 
-import {DELETE_KEY} from "./pointers/types.js";
+import {DELETE_KEY, SOURCE_INDEX} from "./pointers/types.js";
 
 import diff from "./diff.js";
 
@@ -96,6 +96,16 @@ describe("merge.diff()", function(){
 
     it("handles object creation", function(){
       expect(diff({name:"A", articles: null}, {name:"A", articles: {id:"1"}})).to.deep.equal({articles: {id:"1"}});
+    });
+
+    it("handles the special SOURCE_INDEX symbol", function(){
+      // SOURCE_INDEX is used internally to reference an array's ordering
+      // We need this to restore the expected array order in the end
+      let res = diff<any>(
+        {v: {a: 1, [SOURCE_INDEX]: 1 }},
+        {v: {a: 1, [SOURCE_INDEX]: 2 }}
+      );
+      expect(res).to.deep.equal({v:{[SOURCE_INDEX]: 2}});
     })
   });
 });

source/server/utils/merge/diff.ts

@@ -1,27 +1,32 @@
-import {Diff, DELETE_KEY} from "./pointers/types.js";
+import {Diff, DELETE_KEY, SOURCE_INDEX, withIndex} from "./pointers/types.js";
 
 /**
  * Computes a diff between two objects.
  * Applying deepMerge(a, diff(a,b)) should yield b
  * deleted keys are represented using the DELETE_KEY symbol
- * It treats arrays as primitives (ie. will overwrite blindly any array)
+ * It treats arrays as primitives (ie. will overwrite blindly any array) so they should be pre-transformed into dictionaries where possible
+ * @warning it messes with properties iteration order by making a `Set<from.keys() ∪ to.keys()>`
  * @param from origin document
  * @param to target document
  * @returns 
  */
-export default function diff<T extends Record<string,any>>(from :T, to :T) :Diff<T>{
+export default function diff<T extends Record<string, any>&{[SOURCE_INDEX]?:number}>(from :T, to :T) :Diff<T>{
   const is_array = Array.isArray(from);
   if(is_array && !Array.isArray(to)) throw new Error("Can't diff an array with an object");
 
   let r :Diff<T> = {} as any;
+
+  if(SOURCE_INDEX in to && to[SOURCE_INDEX] !== from[SOURCE_INDEX]){
+    r = withIndex(r, to[SOURCE_INDEX] as any);
+  }
+
   const keys :Set<keyof T>= new Set([...Object.keys(from), ...Object.keys(to)]);
   for(const key of keys.values()){
 
     if(typeof to[key] == "undefined"){
       if(typeof from[key] != "undefined") r[key] = DELETE_KEY;
       continue;
     }
-
     if(typeof from[key] != "object"){
       if(from[key] === to[key]) continue;
       //Simple case with primitive values
@@ -44,13 +49,11 @@ export default function diff<T extends Record<string,any>>(from :T, to :T) :Diff
       continue;
     }
 
-    const d = diff(from[key] as any, to[key] as any);
-    if(Object.keys(d).length){
+    const d = diff(from[key], to[key]);
+    if(Object.keys(d).length || typeof (d as any)[SOURCE_INDEX] !== "undefined"){
       //console.log("Diffing", key, from[key], to[key]);
       r[key] = d;
     }
-
-
   }
   return r;
 }

source/server/utils/merge/index.ts

@@ -14,6 +14,8 @@ export {
 
 /**
  * like `diff()` but dereferences a document's pointers first to make a cleaner diff
+ * 
+ * `diff()` holds the generalized diff logic while `toPointers` is in charge of the implementation details regarding the document's schema
  * @see diff
  */
 export function diffDoc(from:IDocument,to:IDocument){
@@ -22,6 +24,8 @@ export function diffDoc(from:IDocument,to:IDocument){
 
 /**
  * like `apply()` but dereferences a document's pointers first to make a cleaner diff, then re-references the result
+ * 
+ * `apply()` holds the generalized merge logic while `toPointers` and `fromPointers` are in charge of the implementation details regarding the document's schema
  * @see apply
  * @see fromPointers
  * @see toPointers