From eac5f29ae625a91dbc00e7b10749839e957e8c5e Mon Sep 17 00:00:00 2001 From: Brian Broll Date: Wed, 7 Oct 2020 16:31:09 -0500 Subject: [PATCH] Fix copyNode in TwoPhaseCore. Fixes #1955 (#1956) * Fix copyNode in 2PC. Fixes #1955 * Fix failing tests --- src/plugins/TwoPhaseCommit/CreatedNode.js | 3 +- src/plugins/TwoPhaseCommit/StagedChanges.js | 61 +++--- src/plugins/TwoPhaseCommit/TwoPhaseCore.js | 187 +++++++++--------- .../TwoPhaseCommit/TwoPhaseCore.spec.js | 89 +++++++++ 4 files changed, 214 insertions(+), 126 deletions(-) diff --git a/src/plugins/TwoPhaseCommit/CreatedNode.js b/src/plugins/TwoPhaseCommit/CreatedNode.js index 6b906cf1e..2ebce3e44 100644 --- a/src/plugins/TwoPhaseCommit/CreatedNode.js +++ b/src/plugins/TwoPhaseCommit/CreatedNode.js @@ -41,7 +41,8 @@ define([ return (typeof id === 'string') && (id.indexOf(CreatedNode.CREATE_PREFIX) === 0); } } - CreatedNode.CREATE_PREFIX = 'created_node_'; + + CreatedNode.CREATE_PREFIX = '__created_node_'; class InheritedNode extends CreatedNode { async toGMENode (rootNode, core) { diff --git a/src/plugins/TwoPhaseCommit/StagedChanges.js b/src/plugins/TwoPhaseCommit/StagedChanges.js index b6ab32dd9..6e8888bc4 100644 --- a/src/plugins/TwoPhaseCommit/StagedChanges.js +++ b/src/plugins/TwoPhaseCommit/StagedChanges.js @@ -7,35 +7,19 @@ define([ assert, ) { - function StagedChanges(createdNodes, changes, deletions, idDict) { - this.createdNodes = createdNodes; - this.changes = changes; - this.deletions = deletions; + function StagedChanges(idDict, predecessor) { + assert(idDict); + this.createdNodes = []; + this.changes = {}; + this.deletions = []; this._createdGMEIds = idDict; + this.predecessor = predecessor; } StagedChanges.prototype.getCreatedNode = function(id) { return this.createdNodes.find(node => node.id === id); }; - StagedChanges.prototype.onNodeCreated = function(createdNode, nodeId) { - // Update newly created node - const tmpId = createdNode.id; - if (this.changes[tmpId]) { - assert(!this.changes[nodeId], - `Newly created node cannot already have changes! (${nodeId})`); - this.changes[nodeId] = this.changes[tmpId]; - - delete this.changes[tmpId]; - } - - // Update any deletions - let index = this.deletions.indexOf(tmpId); - if (index !== -1) { - this.deletions.splice(index, 1, nodeId); - } - }; - StagedChanges.prototype.resolveCreateIds = function() { const changedIds = Object.keys(this.changes) .filter(id => CreatedNode.isCreateId(id)); @@ -77,12 +61,13 @@ define([ }; StagedChanges.prototype.tryGetNodeEdits = function(id) { - id = CreatedNode.isCreateId(id) ? this.tryResolveCreateId(id) : id; - if (id) { - return null; + const ids = [id]; + if (CreatedNode.isCreateId(id) && this.tryResolveCreateId(id)) { + ids.push(this.tryResolveCreateId(id)); } - - return this.changes[id]; + return ids + .map(id => this.changes[id]) + .find(changes => changes) || null; }; StagedChanges.prototype.getModifiedNodeIds = function() { @@ -96,5 +81,27 @@ define([ return Promise.all(gmeNodes); }; + StagedChanges.prototype.getChangesForNode = function (nodeId) { + if (!this.changes[nodeId]) { + this.changes[nodeId] = { + attr: {}, + ptr: {}, + }; + } + + return this.changes[nodeId]; + }; + + StagedChanges.prototype.next = function() { + return new StagedChanges(this._createdGMEIds, this); + }; + + StagedChanges.prototype.changesets = function() { + if (this.predecessor) { + return this.predecessor.changesets().concat([this]); + } + return [this]; + }; + return StagedChanges; }); diff --git a/src/plugins/TwoPhaseCommit/TwoPhaseCore.js b/src/plugins/TwoPhaseCommit/TwoPhaseCore.js index 02887504e..b718cf535 100644 --- a/src/plugins/TwoPhaseCommit/TwoPhaseCore.js +++ b/src/plugins/TwoPhaseCommit/TwoPhaseCore.js @@ -13,12 +13,11 @@ define([ function TwoPhaseCore(logger, core) { this.logger = logger; this.core = core; - this.changes = {}; - this.createdNodes = []; - this.deletions = []; this.queuedChanges = []; - this._events = {}; this._createdGMEIds = {}; + this.stagedChanges = new StagedChanges( + this._createdGMEIds + ); } TwoPhaseCore.prototype.unwrap = function () { @@ -45,7 +44,7 @@ define([ TwoPhaseCore.prototype.loadByPath = async function (node, id) { ensureNode(node, 'loadByPath'); if (CreatedNode.isCreateId(id)) { - const changesets = this.queuedChanges.concat(this); + const changesets = this._getAllChangesets(); for (let i = 0; i < changesets.length; i++) { const createdNodes = changesets[i].createdNodes; const node = createdNodes.find(node => node.id === id); @@ -155,21 +154,20 @@ define([ TwoPhaseCore.prototype._forAllNodeChanges = function (node, fn) { const nodeId = this.getPath(node); - for (let i = 0; i < this.queuedChanges.length; i++) { - const changes = this.queuedChanges[i].tryGetNodeEdits(nodeId); - if (changes) { - fn(changes); + const changes = this._getAllChangesets(); + for (let i = 0; i < changes.length; i++) { + const nodeEdits = changes[i].tryGetNodeEdits(nodeId); + if (nodeEdits) { + fn(nodeEdits); } } - const changes = this.getChangesForNode(node); - if (changes) { - fn(changes); - } }; TwoPhaseCore.prototype.getBase = function (node) { ensureNode(node, 'getBase'); - if (node instanceof CreatedNode) { + if (node instanceof CreatedNode.CopiedNode) { + return this.getBase(node.original); + } else if (node instanceof CreatedNode) { return node.base; } return this.core.getBase(node); @@ -183,18 +181,18 @@ define([ return this.core.isTypeOf(node, base); }; + TwoPhaseCore.prototype._getAllChangesets = function () { + return this.queuedChanges.concat(this.stagedChanges) + .flatMap(changes => changes.changesets()); + }; + TwoPhaseCore.prototype.getStagedChanges = function () { - const changes = new StagedChanges( - this.createdNodes, - this.changes, - this.deletions, + const stagedChanges = this.stagedChanges; + this.queuedChanges.push(stagedChanges); + this.stagedChanges = new StagedChanges( this._createdGMEIds ); - this.createdNodes = []; - this.changes = {}; - this.deletions = []; - this.queuedChanges.push(changes); - return changes; + return stagedChanges; }; TwoPhaseCore.prototype.discard = function (changes) { @@ -206,31 +204,18 @@ define([ TwoPhaseCore.prototype.setPointer = function (node, name, target) { ensureNode(node, 'setPointer'); ensureNode(target, 'setPointer'); - const changes = this.getChangesForNode(node); - changes.ptr[name] = target; - }; - - TwoPhaseCore.prototype.getChangesForNode = function (node) { const nodeId = this.getPath(node); - - if (!this.changes[nodeId]) { - this.changes[nodeId] = { - attr: {}, - ptr: {}, - }; - } - - return this.changes[nodeId]; + const changes = this.stagedChanges.getChangesForNode(nodeId); + changes.ptr[name] = target; }; TwoPhaseCore.prototype.copyNode = function (node, parent) { - assert(node, 'Cannot copy invalid node'); - assert(parent, 'Cannot copy node without parent'); ensureNode(node, 'copyNode'); ensureNode(parent, 'copyNode'); - const newNode = new CreatedNode(node, parent); - this.createdNodes.push(newNode); + const newNode = new CreatedNode.CopiedNode(node, parent); + this.stagedChanges = this.stagedChanges.next(); + this.stagedChanges.createdNodes.push(newNode); return newNode; }; @@ -243,13 +228,13 @@ define([ const node = new CreatedNode(base, parent); this.logger.info(`Creating ${node.id} in ${parentId}`); - this.createdNodes.push(node); + this.stagedChanges.createdNodes.push(node); return node; }; TwoPhaseCore.prototype.deleteNode = function (node) { ensureNode(node, 'deleteNode'); - this.deletions.push(node); + this.stagedChanges.deletions.push(node); }; TwoPhaseCore.prototype.loadChildren = async function (node) { @@ -257,9 +242,8 @@ define([ const getId = node => node instanceof CreatedNode ? node.id : this.core.getPath(node); const nodeId = getId(node); - const allCreatedNodes = this.queuedChanges.concat([this]) - .map(changes => changes.createdNodes) - .reduce((l1, l2) => l1.concat(l2)); + const allCreatedNodes = this._getAllChangesets() + .flatMap(changes => changes.createdNodes); let children = allCreatedNodes.filter(node => getId(node.parent) === nodeId); if (node instanceof CreatedNode) { @@ -281,9 +265,10 @@ define([ value !== undefined, `Cannot set attribute to undefined value (${attr})` ); + const nodeId = this.getPath(node); this.logger.info(`setting ${attr} to ${value}`); - const changes = this.getChangesForNode(node); + const changes = this.stagedChanges.getChangesForNode(nodeId); changes.attr[attr] = value; }; @@ -311,56 +296,63 @@ define([ TwoPhaseCore.prototype.getAttribute = function (node, attr) { ensureNode(node, 'getAttribute'); - var nodeId; - - // Check if it was newly created - if (node instanceof CreatedNode) { - nodeId = node._nodeId || node.id; - node = node.base; - } else { - nodeId = this.core.getPath(node); - } - - assert(this.deletions.indexOf(nodeId) === -1, - `Cannot get ${attr} from deleted node ${nodeId}`); // Check the most recent changes, then the staged changes, then the model - let value = this._getValueFrom(nodeId, attr, node, this.changes); - - if (value === undefined) { - for (let i = this.queuedChanges.length; i--;) { - const changes = this.queuedChanges[i]; - value = this._getValueFrom(nodeId, attr, node, changes.getAllNodeEdits()); - if (value !== undefined) { - return value; - } - } - } + const changes = this._getAllChangesets() + .reverse(); + + const nodeId = this.getPath(node); + const isDeleted = changes + .find(changes => changes.deletions.includes(nodeId)); + assert(!isDeleted, `Cannot get ${attr} from deleted node ${nodeId}`); + let value = this._getValueFrom(node, attr, changes); if (value !== undefined) { return value; + } else if (node instanceof CreatedNode.CopiedNode) { + const hasCopyNodeEdit = changeset => node instanceof CreatedNode.CopiedNode && + changeset.createdNodes.includes(node); + const changesBeforeCopy = takeUntil(changes.reverse(), hasCopyNodeEdit) + .reverse(); + const value = this._getValueFrom(node.original, attr, changesBeforeCopy); + + if (value) { + return value; + } else { + return this.core.getAttribute(node.original, attr); + } + } else if (node instanceof CreatedNode) { + return this.getAttribute(node.base, attr); } return this.core.getAttribute(node, attr); }; - TwoPhaseCore.prototype._getValueFrom = function (nodeId, attr, node, changes) { - var base; - if (changes[nodeId] && changes[nodeId].attr[attr] !== undefined) { - // If deleted the attribute, get the default (inherited) value - if (changes[nodeId].attr[attr] === null) { - base = CreatedNode.isCreateId(nodeId) ? node : this.core.getBase(node); - let inherited = this.getAttribute(base, attr); - return inherited || null; + TwoPhaseCore.prototype._getValueFrom = function (node, attr, changes) { + const nodeId = this.getPath(node); + for (let i = changes.length; i--;) { + const changeset = changes[i]; + const nodeChanges = changeset.getAllNodeEdits(); + if (nodeChanges[nodeId] && nodeChanges[nodeId].attr[attr] !== undefined) { + // If deleted the attribute, get the default (inherited) value + if (nodeChanges[nodeId].attr[attr] === null) { + const base = this.getBase(node); + let inherited = this.getAttribute(base, attr); + return inherited || null; + } + return nodeChanges[nodeId].attr[attr]; } - return changes[nodeId].attr[attr]; } }; - TwoPhaseCore.prototype.apply = async function (rootNode, changes) { - await this.applyCreations(rootNode, changes); - await this.applyChanges(rootNode, changes); - await this.applyDeletions(rootNode, changes); + TwoPhaseCore.prototype.apply = async function (rootNode, stagedChanges) { + const changesets = stagedChanges.changesets(); + for (let i = 0; i < changesets.length; i++) { + const changes = changesets[i]; + await this.applyCreations(rootNode, changes); + await this.applyChanges(rootNode, changes); + await this.applyDeletions(rootNode, changes); + } }; TwoPhaseCore.prototype.applyCreations = async function (rootNode, changes) { @@ -368,29 +360,16 @@ define([ const createdNode = changes.createdNodes[i]; const node = await createdNode.toGMENode(rootNode, this.core); const nodeId = this.core.getPath(node); - this.emit('nodeCreated', createdNode, node); this._createdGMEIds[createdNode.id] = nodeId; } changes.resolveCreateIds(); }; - TwoPhaseCore.prototype.on = function(ev, cb) { - this._events[ev] = this._events[ev] || []; - this._events[ev].push(cb); - }; - - TwoPhaseCore.prototype.emit = function(ev) { - const args = Array.prototype.slice.call(arguments, 1); - const handlers = this._events[ev] || []; - handlers.forEach(fn => fn.apply(this, args)); - }; - - TwoPhaseCore.prototype.applyChanges = async function (rootNode,changes) { + TwoPhaseCore.prototype.applyChanges = async function (rootNode, changes) { const nodeIds = changes.getModifiedNodeIds(); this.logger.info('Collecting changes to apply in commit'); - this.currentChanges = this.changes; for (let i = nodeIds.length; i--;) { const id = nodeIds[i]; const edits = changes.getNodeEdits(id); @@ -399,7 +378,6 @@ define([ assert(node, `node is ${node} (${id})`); await this._applyNodeChanges(rootNode, node, edits); } - this.currentChanges = {}; }; TwoPhaseCore.prototype._applyNodeChanges = async function (rootNode, node, edits) { @@ -455,5 +433,18 @@ define([ }; } + function takeUntil(list, fn) { + const result = []; + for (let i = 0; i < list.length; i++) { + if (!fn(list[i])) { + result.push(list[i]); + } else { + break; + } + } + + return result; + } + return TwoPhaseCore; }); diff --git a/test/unit/plugins/TwoPhaseCommit/TwoPhaseCore.spec.js b/test/unit/plugins/TwoPhaseCommit/TwoPhaseCore.spec.js index 7cd569204..40752397a 100644 --- a/test/unit/plugins/TwoPhaseCommit/TwoPhaseCore.spec.js +++ b/test/unit/plugins/TwoPhaseCommit/TwoPhaseCore.spec.js @@ -229,6 +229,95 @@ describe('TwoPhaseCore', function() { assert(core.getAllMetaNodes(newNode)); }); }); + + describe('copyNode', function() { + it('should copy existing node', function() { + const {META, core, rootNode} = plugin; + const node = META['pipeline.Operation']; + const newNode = core.copyNode(node, rootNode); + + assert(newNode); + }); + + it('should copy new node', function() { + const {META, rootNode, core} = plugin; + const {FCO} = META; + const parent = rootNode; + const newNode = core.createNode({base: FCO, parent}); + const copy = core.copyNode(newNode, rootNode); + + assert(copy); + }); + + it('should get attribute from copy of new node', function() { + const {META, rootNode, core} = plugin; + const {FCO} = META; + const parent = rootNode; + const newNode = core.createNode({base: FCO, parent}); + core.setAttribute(newNode, 'name', 'newName!'); + const copy = core.copyNode(newNode, rootNode); + + assert.equal( + core.getAttribute(copy, 'name'), + 'newName!' + ); + }); + + it('should include added attributes in copy', function() { + const {META, rootNode, core} = plugin; + const {FCO} = META; + const parent = rootNode; + const newNode = core.unwrap().createNode({base: FCO, parent}); + core.setAttribute(newNode, 'name', 'newName!'); + const copy = core.copyNode(newNode, rootNode); + + assert.equal( + core.getAttribute(copy, 'name'), + 'newName!' + ); + }); + + it('should get updated attribute in copy', function() { + const {META, rootNode, core} = plugin; + const {FCO} = META; + const parent = rootNode; + const newNode = core.unwrap().createNode({base: FCO, parent}); + const copy = core.copyNode(newNode, rootNode); + core.setAttribute(copy, 'name', 'newName!'); + + assert.equal( + core.getAttribute(copy, 'name'), + 'newName!' + ); + }); + + it('should not include later attributes in copy', function() { + const {META, rootNode, core} = plugin; + const {FCO} = META; + const parent = rootNode; + const newNode = core.unwrap().createNode({base: FCO, parent}); + const copy = core.copyNode(newNode, rootNode); + core.setAttribute(newNode, 'name', 'newName!'); + + assert.notEqual( + core.getAttribute(copy, 'name'), + 'newName!' + ); + }); + + it('should set correct base in copied node', function() { + const {META, rootNode, core} = plugin; + const {FCO} = META; + const parent = rootNode; + const newNode = core.unwrap().createNode({base: FCO, parent}); + const copy = core.copyNode(newNode, rootNode); + + assert.equal( + core.getPath(core.getBase(copy)), + core.getPath(FCO) + ); + }); + }); }); describe('argument validation', function() {