From 54191de47126f67752249b6a081ba5aeaf4adad3 Mon Sep 17 00:00:00 2001 From: Sebastien DUMETZ Date: Mon, 13 Nov 2023 10:11:47 +0100 Subject: [PATCH] working prototype with basic dumb merge --- source/client/components/CVDocument.ts | 7 +- source/client/schema/document.ts | 1 + source/server/package-lock.json | 37 ---------- source/server/package.json | 1 - source/server/routes/scenes/get/document.ts | 17 +++-- source/server/routes/scenes/index.ts | 2 - source/server/routes/scenes/put/document.ts | 10 ++- source/server/utils/merge.test.ts | 76 ++++++++++++++++++++- 8 files changed, 96 insertions(+), 55 deletions(-) diff --git a/source/client/components/CVDocument.ts b/source/client/components/CVDocument.ts index fa8f151c..fc873c33 100755 --- a/source/client/components/CVDocument.ts +++ b/source/client/components/CVDocument.ts @@ -58,6 +58,7 @@ export default class CVDocument extends CRenderGraph protected titles: Dictionary = {}; protected meta: CVMeta = null; + protected ref_id :number = -1; protected static readonly ins = { dumpJson: types.Event("Document.DumpJSON"), @@ -173,6 +174,7 @@ export default class CVDocument extends CRenderGraph */ openDocument(documentData: IDocument, assetPath?: string, mergeParent?: boolean | NVNode | NVScene) { + console.time("openDocument"); if (ENV_DEVELOPMENT) { console.log("CVDocument.openDocument - assetPath: %s, mergeParent: %s", assetPath, mergeParent); } @@ -215,6 +217,8 @@ export default class CVDocument extends CRenderGraph this.outs.assetPath.setValue(assetPath); this.name = this.getMainComponent(CVAssetManager).getAssetName(assetPath); } + this.ref_id = documentData.asset.id; + console.timeEnd("openDocument"); } appendModel(assetPath: string, quality?: EDerivativeQuality | string, parent?: NVNode | NVScene) : CVModel2 @@ -266,7 +270,8 @@ export default class CVDocument extends CRenderGraph type: CVDocument.mimeType, version: CVDocument.version, generator: "Voyager", - copyright: "(c) Smithsonian Institution. All rights reserved." + copyright: "(c) Smithsonian Institution. All rights reserved.", + id: this.ref_id, }, scene: 0, scenes: [], diff --git a/source/client/schema/document.ts b/source/client/schema/document.ts index 2220bcf3..33ccfc9c 100644 --- a/source/client/schema/document.ts +++ b/source/client/schema/document.ts @@ -54,6 +54,7 @@ export interface IDocumentAsset version: string; copyright?: string; generator?: string; + id?: number; } export interface IScene diff --git a/source/server/package-lock.json b/source/server/package-lock.json index 2729ddb3..b3cf6115 100644 --- a/source/server/package-lock.json +++ b/source/server/package-lock.json @@ -10,7 +10,6 @@ "license": "Apache-2.0", "dependencies": { "body-parser": "^1.20.1", - "cookie-parser": "^1.4.6", "cookie-session": "^2.0.0", "express": "^4.17.1", "express-rate-limit": "^7.1.2", @@ -1331,26 +1330,6 @@ "node": ">= 0.6" } }, - "node_modules/cookie-parser": { - "version": "1.4.6", - "resolved": "https://registry.npmjs.org/cookie-parser/-/cookie-parser-1.4.6.tgz", - "integrity": "sha512-z3IzaNjdwUC2olLIB5/ITd0/setiaFMLYiZJle7xg5Fe9KWAceil7xszYfHHBtDFYLSgJduS2Ty0P1uJdPDJeA==", - "dependencies": { - "cookie": "0.4.1", - "cookie-signature": "1.0.6" - }, - "engines": { - "node": ">= 0.8.0" - } - }, - "node_modules/cookie-parser/node_modules/cookie": { - "version": "0.4.1", - "resolved": "https://registry.npmjs.org/cookie/-/cookie-0.4.1.tgz", - "integrity": "sha512-ZwrFkGJxUR3EIoXtO+yVE69Eb7KlixbaeAWfBQB9vVsNn/o+Yw69gBWSSDK825hQNdN+wF8zELf3dFNl/kxkUA==", - "engines": { - "node": ">= 0.6" - } - }, "node_modules/cookie-session": { "version": "2.0.0", "resolved": "https://registry.npmjs.org/cookie-session/-/cookie-session-2.0.0.tgz", @@ -5354,22 +5333,6 @@ "resolved": "https://registry.npmjs.org/cookie/-/cookie-0.5.0.tgz", "integrity": "sha512-YZ3GUyn/o8gfKJlnlX7g7xq4gyO6OSuhGPKaaGssGB2qgDUS0gPgtTvoyZLTt9Ab6dC4hfc9dV5arkvc/OCmrw==" }, - "cookie-parser": { - "version": "1.4.6", - "resolved": "https://registry.npmjs.org/cookie-parser/-/cookie-parser-1.4.6.tgz", - "integrity": "sha512-z3IzaNjdwUC2olLIB5/ITd0/setiaFMLYiZJle7xg5Fe9KWAceil7xszYfHHBtDFYLSgJduS2Ty0P1uJdPDJeA==", - "requires": { - "cookie": "0.4.1", - "cookie-signature": "1.0.6" - }, - "dependencies": { - "cookie": { - "version": "0.4.1", - "resolved": "https://registry.npmjs.org/cookie/-/cookie-0.4.1.tgz", - "integrity": "sha512-ZwrFkGJxUR3EIoXtO+yVE69Eb7KlixbaeAWfBQB9vVsNn/o+Yw69gBWSSDK825hQNdN+wF8zELf3dFNl/kxkUA==" - } - } - }, "cookie-session": { "version": "2.0.0", "resolved": "https://registry.npmjs.org/cookie-session/-/cookie-session-2.0.0.tgz", diff --git a/source/server/package.json b/source/server/package.json index 5ab2b98e..6559ec38 100755 --- a/source/server/package.json +++ b/source/server/package.json @@ -39,7 +39,6 @@ }, "dependencies": { "body-parser": "^1.20.1", - "cookie-parser": "^1.4.6", "cookie-session": "^2.0.0", "express": "^4.17.1", "express-rate-limit": "^7.1.2", diff --git a/source/server/routes/scenes/get/document.ts b/source/server/routes/scenes/get/document.ts index a0513a7c..a3d54f80 100644 --- a/source/server/routes/scenes/get/document.ts +++ b/source/server/routes/scenes/get/document.ts @@ -1,6 +1,5 @@ -import path from "path"; -import { getUserId, getVfs } from "../../../utils/locals.js"; +import { getVfs } from "../../../utils/locals.js"; import { Request, Response } from "express"; import { createHash } from "crypto"; @@ -14,12 +13,16 @@ export default async function handleGetDocument(req :Request, res :Response){ let scene = await vfs.getScene(scene_name); let f = await vfs.getDoc(scene.id); - let hash = createHash("sha256").update(f.data).digest("base64url"); - let data = Buffer.from(f.data); - //Use this to know the client's document generation if he submits a change - res.cookie("docID", `${f.id}`, {sameSite: "strict", path: path.dirname(req.originalUrl)}); + let doc = JSON.parse(f.data); + //Inject document id to know the client's reference document if he submits a change + doc.asset.id = f.id; - res.set("ETag", `W/${hash}`); + let data = Buffer.from(JSON.stringify(doc)); + + let hash = createHash("sha256").update(data).digest("base64url"); + + + res.set("ETag", hash); res.set("Last-Modified", f.mtime.toUTCString()); if(req.fresh){ return res.status(304).send("Not Modified"); diff --git a/source/server/routes/scenes/index.ts b/source/server/routes/scenes/index.ts index 2fdb3c46..74f48d9f 100644 --- a/source/server/routes/scenes/index.ts +++ b/source/server/routes/scenes/index.ts @@ -1,7 +1,6 @@ import { Router } from "express"; import bodyParser from "body-parser"; -import cookieParser from "cookie-parser"; import {handlePropfind} from "./propfind.js"; import {handlePutFile, handlePutDocument} from "./put/index.js"; @@ -38,7 +37,6 @@ router.get("/:scene/:file(*.svx.json)", wrap(handleGetDocument)); router.put("/:scene/:file(*.svx.json)", canWrite, bodyParser.json({type:["application/si-dpo-3d.document+json", "application/json"]}), - cookieParser(), wrap(handlePutDocument) ); router.copy("/:scene/:file(*.svx.json)", canWrite, wrap(handleCopyDocument)); diff --git a/source/server/routes/scenes/put/document.ts b/source/server/routes/scenes/put/document.ts index f65811ed..626aca6a 100644 --- a/source/server/routes/scenes/put/document.ts +++ b/source/server/routes/scenes/put/document.ts @@ -38,17 +38,16 @@ export default async function handlePutDocument(req :Request, res :Response){ const uid = getUserId(req); const {scene} = req.params; const newDoc = req.body; - - const refId = parseInt(req.cookies["docID"] ?? "0"); + const refId = newDoc?.asset?.id; if(!refId) return await overwritePutDocument(req, res); - + else delete newDoc.asset.id; //Don't write this to DB await getVfs(req).isolate(async (tr)=>{ // perform a diff of the document with the reference one const {id: scene_id} = await tr.getScene(scene); const {data: currentDocString} = await tr.getDoc(scene_id); const currentDoc = JSON.parse(currentDocString); - const {data:refDocString} = await tr.getDocById(refId); + const {data: refDocString} = await tr.getDocById(refId); const refDoc = JSON.parse(refDocString); const docDiff = merge.diff(refDoc, newDoc); @@ -59,10 +58,9 @@ export default async function handlePutDocument(req :Request, res :Response){ } console.log("Merge changes : ", JSON.stringify(docDiff)); const mergedDoc = merge.apply(currentDoc, docDiff); - let s = JSON.stringify(mergedDoc, null, 2); + let s = JSON.stringify(mergedDoc); if(s == "{}") throw new BadRequestError(`Invalid json document`); let id = await tr.writeDoc(s, scene, uid); - res.cookie("docID", `${id}`, {sameSite: "strict", path: path.dirname(req.originalUrl)}); res.status(204).send(); }) diff --git a/source/server/utils/merge.test.ts b/source/server/utils/merge.test.ts index 19f117c7..43d8e69d 100644 --- a/source/server/utils/merge.test.ts +++ b/source/server/utils/merge.test.ts @@ -59,6 +59,10 @@ describe("merge.diff()", function(){ expect(diff({a:["foo", "bar", "baz"]}, {a:["foo", "baz"]})).to.deep.equal({a:{1: "baz", 2: DELETE_KEY}}); }); + it("handle deep changes", function(){ + expect(diff({a:[{v:"foo"}, {v:"bar"}]}, {a:[{v:"foo"}, {v:"baz"}]})).to.deep.equal({a:{1:{v:"baz"}}}); + }); + it("throws when diffing an array with an object", function(){ expect(()=>diff({a:[]}, {a:{}})).to.throw("Can't diff an array with an object"); }); @@ -87,11 +91,19 @@ describe("merge.apply()", function(){ it("merges updated arrays", function(){ expect(apply({a:[1,2]}, {a:{0:1, 1:3}})).to.deep.equal({a:[1,3]}); }); + + }); describe("three-way merge", function(){ - // This is the real point of this module + /* This is the real point of this module + * For consistency, tests should follow the following naming scheme: + * a *ref* as the common source, + * a *current* as the document that have been saved in-between + * a *next* as the document that is being saved now + */ + it("string update", function(){ /** @TODO could try to do string splicing for finer results */ const ref = {greet:"Hello", name:"World"}; @@ -99,4 +111,66 @@ describe("three-way merge", function(){ const next = {greet:"Hello", name:"Universe"}; 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.skip("conflict resolution", function(){ + //Test only cases where "smart" conflict resolution is possible + describe("by ID", function(){ + //Here it would be possible to differentiate annotations or articles by their id + //thus detecting a double-push and resolving it + const A1 = { "id": "mMzG2tLjmIst", "titles": { "EN": "A1"} } + const A2 = { "id": "FiIaONzRIbL4", "titles": { "EN": "A2"} } + const A3 = { "id": "rJpCltJjxyyL", "titles": { "EN": "A3"} } + it("handle double array push (annotations)", function(){ + const ref = {annotations: [A1], title: "foo"}; + const current = {annotations: [A1,A2]}; + const next = {annotations: [A1,A3]}; + + const d = diff(ref, next); + expect(d).to.deep.equal({annotations: {1: A3}}); + expect(apply(current, d)).to.deep.equal({annotations: [A1,A2,A3], title: "foo"}); + }); + + it.skip("handle double array push (articles)", function(){ + const ref = {articles: [A1], title: "foo"}; + const current = {articles: [A1,A2]}; + const next = {articles: [A1,A3]}; + expect(apply(current, diff(ref, next))).to.deep.equal({articles: [A1,A2,A3], title: "foo"}); + }); + }) + + describe.skip("by name", function(){ + + it("reassign scene nodes appropriately", function(){ + //Differentiate nodes by their names + //However, we also need to handle possible conflict in `scenes[x].nodes` if we reorder nodes. + const N1 = { "name": "n1"}; + const N2 = { "name": "n2"}; + const N3 = { "name": "n3"}; + const ref = {nodes: [N1], scenes:[{nodes:[0]}] }; + const current = {nodes: [N1,N2], scenes:[{nodes:[0, 1]}]}; + const next = {nodes: [N1,N3], scenes:[{nodes:[0, 1]}]}; + + expect(apply(current, diff(ref, next))).to.deep.equal({nodes: [N1,N2,N3], scenes:[{nodes:[0, 1, 2]}]}); + //FIXME a node's children might also be affected. + }); + + it("reassign nodes children appropriately", function(){ + + }); + + ["lights", "cameras", "models"].forEach(type=>{ + it(`reassign ${type} appropriately`, function(){ + //Lights have no property that could be used to differentiate them + //We can only suppose that if someone adds a node, the light attached to this node is unique to this one and should stay with it. + }); + }); + }); + + + }); });