From 12a861d01af6031729bbdb71b2f636dc9b83cee3 Mon Sep 17 00:00:00 2001 From: Sebastien DUMETZ Date: Mon, 8 Jan 2024 17:42:48 +0100 Subject: [PATCH] [WIP] POC structured merge with array double-push handling --- source/server/package.json | 1 + source/server/utils/merge/apply.ts | 35 +++++--- source/server/utils/merge/merge.test.ts | 102 +++++++++++++++++++----- 3 files changed, 104 insertions(+), 34 deletions(-) diff --git a/source/server/package.json b/source/server/package.json index 6559ec38..ea08982f 100755 --- a/source/server/package.json +++ b/source/server/package.json @@ -7,6 +7,7 @@ "scripts": { "start": "ROOT_DIR=\"$(pwd)/../..\" node ./dist/server/index.js", "test": "NODE_NO_WARNINGS=1 mocha", + "test-watch": "NODE_NO_WARNINGS=1 mocha --parallel --watch --reporter min", "build": "tsc -b ." }, "nodemonConfig": { diff --git a/source/server/utils/merge/apply.ts b/source/server/utils/merge/apply.ts index 8cab1f85..c8b83910 100644 --- a/source/server/utils/merge/apply.ts +++ b/source/server/utils/merge/apply.ts @@ -3,7 +3,7 @@ import {Diff, DELETE_KEY} from "./types.js"; /** * Deep assign two or more objects - * Like `Object.assign()` but recursive + * Like `Object.assign()` but recursive (modifies objects in-place) * It's currently quite simplified and doesn't handle strings splicing * @param into Object to merge into (will be mutated in-place) * @returns into, merged with source(s) @@ -29,7 +29,13 @@ export default function apply>(into :T, ...diffs : return into; } -//Handle special merge conditions for arrays +/** + * Handle special merge conditions for arrays + * In particular: + * - for arrays of native types, replace the whole array + * - for arrays of objects with a "name" or "id", tries to merge objects with a matching id/name, appends the rest + * + */ function apply_array>(into :T, ...diffs :Diff[]):T{ for (const diff of diffs){ for(const idx in diff){ @@ -38,17 +44,18 @@ function apply_array>(into :T, ...diffs :Diff[]):T{ continue; } if(typeof into[idx] !== undefined){ - if(value.id){ - const ref_id = into[idx].id; - if(ref_id && ref_id != value.id){ - into.push(value); - continue; - } + //When an item already exist, check + let append = ["id", "name"].some(key =>{ + if(!value[key]) return false; + const ref_id = into[idx][key]; + if(!ref_id || ref_id == value[key]) return false; + return true; + }); + + if(append){ + into.push(value); + continue; } - /** - * @todo names should more or less follow the same rules but are trickier - * Because they require a parent lookup to also reorder affected nodes. - */ } //Default case : recurse as apply() would. into[idx] ??= {} as any; @@ -58,7 +65,9 @@ function apply_array>(into :T, ...diffs :Diff[]):T{ return into; } - +/** + * Return true if the value was applied, false if it needs further processing + */ function apply_core>(into :T, key:keyof T, value :any):boolean{ if(value === DELETE_KEY){ diff --git a/source/server/utils/merge/merge.test.ts b/source/server/utils/merge/merge.test.ts index b0741c70..ffb4f4ac 100644 --- a/source/server/utils/merge/merge.test.ts +++ b/source/server/utils/merge/merge.test.ts @@ -7,7 +7,7 @@ const thisDir = path.dirname(fileURLToPath(import.meta.url)); import { IDocument, INode } from "../schema/document.js"; -import {DELETE_KEY, apply, applyDoc, diff} from "./index.js"; +import {DELETE_KEY, apply, applyDoc, diff, diffDoc} from "./index.js"; @@ -124,18 +124,69 @@ describe("three-way merge", function(){ expect(apply(current, diff(ref, next))).to.deep.equal({greet:"Hi", name:"Universe"}); }); - it.skip("ignores camera translation/rotation changes", function(){ - //This is a potential optimization - //The camera node's position can safely be ignored as it will get reset on load, in favor of `scene.setups[].navigation` - }); + describe("Arrays", function(){ + + it("Overrides array double-push for native types", function(){ + const ref = {a:[1]}; + const current = {a:[1, 2]}; + const next = {a:[1,3]}; + expect(apply(current, diff(ref, next))).to.deep.equal({a:[1,3]}); + }); + + describe("deduplicates by ID", function(){ + //Here it would be possible to differentiate annotations or articles by their id + //thus detecting a double-push and resolving it + let A:{}[] = []; + beforeEach(function(){ + A.push( + { "id": "mMzG2tLjmIst", "titles": { "EN": "A1"} }, + { "id": "FiIaONzRIbL4", "titles": { "EN": "A2"} }, + { "id": "rJpCltJjxyyL", "titles": { "EN": "A3"} } + ); + }); + + it("handle double array push (annotations)", function(){ + const ref = {annotations: [A[0]]}; + const current = {annotations: [A[0], A[1]]}; + const next = {annotations: [A[0], A[2]]}; + + const d = diff(ref, next); + expect(d).to.deep.equal({annotations: {1: A[2]}}); + expect(apply(current, d)).to.deep.equal({annotations: [A[0], A[1], A[2]]}); + }); + + it("handle array modify in place", function(){ + const m = {...A[0], titles: {EN: "A1 modified"}} + const ref = {annotations: [A[0]]}; + const current = {annotations: [A[0], A[1]]}; + const next = {annotations: [m]}; + + const d = diff(ref, next); + expect(apply(current, d)).to.deep.equal({annotations: [m, A[1]]}); - describe("fields ignore optimization", function(){ - //This is a potential optimization - //The camera node's position can safely be ignored as it will get reset on load, in favor of `scene.setups[].navigation` - it("ignores camera translation changes", function(){ - const ref = {nodes:[{name:"camera", translation:[1,0,0]}]}; - const next = {nodes:[{name:"camera", translation:[0,1,0]}]}; - expect(apply(ref, diff(ref, next))).to.deep.equal({nodes:[{name:"camera", translation:[1,0,0]}]}); + }); + }); + describe("deduplicates by name", function(){ + //Here it would be possible to differentiate annotations or articles by their id + //thus detecting a double-push and resolving it + let N:{}[] = []; + beforeEach(function(){ + N.push( + { "name": "Node1"}, + { "name": "Node2"}, + { "name": "Node3"}, + ); + }); + + it("handle double array push", function(){ + const ref = {nodes: [N[0]]}; + const current = {nodes: [N[0], N[1]]}; + const next = {nodes: [N[0], N[2]]}; + + const d = diff(ref, next); + expect(d).to.deep.equal({nodes: {1: N[2]}}); + expect(apply(current, d)).to.deep.equal({nodes: [N[0], N[1], N[2]]}); + }); }); }); }); @@ -151,7 +202,7 @@ describe("merge documents", function(){ }); it("merge simple document changes", function(){ const current = JSON.parse(docString); - current.lights.push({type:"ambient"}); + current.lights.push({type:"ambiant"}); current.nodes.push({ "name": "Ambiant Light", @@ -168,19 +219,28 @@ describe("merge documents", function(){ "light": 1, }); next.nodes.find( (n:INode) =>n.name=="Lights").children.push(next.nodes.length-1); - const d = diff(doc, next); - console.log("Diff : ", d); + const d = diffDoc(doc, next); + const result = applyDoc(current, d); - expect(result.nodes).to.deep.equal([ - ...(doc as any).nodes, - { + + expect(result.nodes, "merged nodes").to.deep.equal([ + (doc.nodes as any)[0], //The camera + {name: "Lights", children: [2, 3, 4]}, + (doc.nodes as any)[2], //The base light (index 2) + { //index 3 "name": "Ambiant Light", "light": 1, }, - { + { //index 4 "name": "Directional Light", - "light": 1, - } + "light": 2, + }, + (doc.nodes as any)[3], //The model ]); + expect(result.lights).to.deep.equal([ + ...(doc.lights as any), + {type: "ambiant"}, + {type:"directional"} + ]) }) }) \ No newline at end of file