From 34be40d106395d44e22f12db8bf14daf316a1074 Mon Sep 17 00:00:00 2001 From: Torrent Glenn Date: Mon, 12 Sep 2016 08:17:17 -0700 Subject: [PATCH] Fixed bug: a node can be containedBy two rack --- lib/services/nodes-api-service.js | 114 +++++++++------ spec/lib/services/nodes-api-service-spec.js | 152 ++++++++------------ 2 files changed, 136 insertions(+), 130 deletions(-) diff --git a/lib/services/nodes-api-service.js b/lib/services/nodes-api-service.js index 186478906..c155d9e68 100644 --- a/lib/services/nodes-api-service.js +++ b/lib/services/nodes-api-service.js @@ -79,34 +79,29 @@ function nodeApiServiceFactory( * of a node delete it * @param {Object} node node whose relation needs to be updated * @param {String} type relation type that needs to be updated - * @param {String[]} targets - node ids in the relation that needs to be deleted - * @return {Promise} node after removing relation + * @param {String[] | Object[]} targets - nodes or ids in the relation + * that needs to be deleted + * @return {Object} node after removing relation */ NodeApiService.prototype._removeRelation = function removeRelation(node, type, targets) { if (!node || !type || !_.has(node, 'relations')) { - return Promise.resolve(); + return; } var index = _.findIndex(node.relations, { relationType: type }); if (index === -1 || !_.has(node.relations[index], 'targets')) { - return Promise.resolve(); + return; } - if ((Constants.NodeRelations[type].relationClass === 'component') && - (type.indexOf('By') !== -1)) { - // Its components need to be deleted - return this.removeNode(node, type); - } // Remove target node id in relation field - targets = [].concat(targets); + targets = [].concat(targets).map(function(node) {return node.id || node;}); node.relations[index].targets = _.difference(node.relations[index].targets, targets); - // Remove the type of relation if no targets in it if (node.relations[index].targets.length === 0) { _.pull(node.relations, node.relations[index]); } - return waterline.nodes.updateByIdentifier(node.id, {relations: node.relations}); + return node; }; /** @@ -115,24 +110,35 @@ function nodeApiServiceFactory( * create it, otherwise append to the existing one. * @param {Object} node - node whose relation needs to be updated * @param {String} type - relation type that needs to be updated - * @param {String | String[]} targets - node ids in relation type that needs to be added - * @return {Promise} a promise to update the node + * @param {String[] | Object[]} targets - nodes or ids in relation type that needs to be added + * @return {Object} the updated node */ NodeApiService.prototype._addRelation = function addRelation(node, type, targets) { if (!(node && type && targets)) { - return Promise.resolve(); + return; } node.relations = (node.relations || []); - targets = [].concat(targets); + + targets = _.map([].concat(targets), function(targetNode) { + targetNode = targetNode.id || targetNode; + if(targetNode === node.id ) { + throw new Error('Node cannot have relationship '+type+' with itself'); + } + return targetNode; + }); var index = _.findIndex(node.relations, { relationType: type }); if (index === -1) { node.relations.push({relationType: type, targets: _.uniq(targets)}); } else { node.relations[index].targets = _.uniq(node.relations[index].targets.concat(targets)); } + index = index === -1 ? node.relations.length - 1: index; + if (type === 'containedBy' && node.relations[index].targets.length > 1) { + throw new Error("Node "+node.id+" can only be contained by one node"); + } - return waterline.nodes.updateByIdentifier(node.id, {relations: node.relations}); + return node; }; /** @@ -180,17 +186,34 @@ function nodeApiServiceFactory( } // Otherwise update targets node in its "relationType" return self._findTargetNodes(node.relations, type) - .tap(function(targetNodes) { - return Promise.map(targetNodes, function(node) { - return self._delValidityCheck(node.id); - }); - }) - .map(function (targetNode) { - return self._removeRelation( - targetNode, - Constants.NodeRelations[type].mapping, - node.id - ); + .then(function(targetNodes) { + if(Constants.NodeRelations[type].relationClass === 'component' && + type.indexOf('By') === -1) { + + return Promise.map(targetNodes, function(targetNode) { + return self._delValidityCheck(targetNode.id); + }).then(function() { + return Promise.map(targetNodes, function(targetNode) { + return self.removeNode( + targetNode, Constants.NodeRelations[type].mapping + ); + }); + }); + } else { + return Promise.map(targetNodes, function(targetNode) { + var updatedNode = self._removeRelation( + targetNode, + Constants.NodeRelations[type].mapping, + node.id + ); + if (!updatedNode) { + return; + } + return waterline.nodes.updateByIdentifier( + updatedNode.id, {relations: updatedNode.relations} + ); + }); + } }); }); }) @@ -302,7 +325,8 @@ function nodeApiServiceFactory( }; /** - * Edit the relations of a given node, delegating the updates to the given handler. + * Edit the relations of a given node, delegating object manipulations to the given handler. + * Handle the update with the handler's output * * @param {String} id - a node id * @param {Object} body - an object with relation types as keys and arrays of target @@ -330,29 +354,37 @@ function nodeApiServiceFactory( targetRelations = _.chunk(targetRelations, 2); //divide the array into subArray chunks //of [[targetNodes], relationType] - var handlerArgs = _.transform(targetRelations, function(result, relationSet) { + var updatedNodes = _.transform(targetRelations, function(result, relationSet) { var targetNodes = relationSet[0]; var relationType = relationSet[1]; if (!Constants.NodeRelations[relationType]) { return; } - result.push( - [ - parentNode, + result[parentNode.id] = handler.call( + self, + result[parentNode.id] || parentNode, relationType, - _.map(targetNodes, function(node) {return node.id;}) - ] + targetNodes ); _.forEach(targetNodes, function(node) { - result.push( - [node, Constants.NodeRelations[relationType].mapping, parentNode.id] + result[node.id] = handler.call( + self, + result[node.id] || node, + Constants.NodeRelations[relationType].mapping, + parentNode ); }); - }); - return Promise.map(handlerArgs, function(args) { - return handler.apply(self, args); - }, {concurrency: 1}).then(_.compact); + }, {}); + + return Promise.all(_.compact(_.map(updatedNodes, function(updatedNode) { + if (!updatedNode) { + return; + } + return waterline.nodes.updateByIdentifier( + updatedNode.id, {relations: updatedNode.relations} + ); + }))); }); }; diff --git a/spec/lib/services/nodes-api-service-spec.js b/spec/lib/services/nodes-api-service-spec.js index 95603303a..7cbaf0fc1 100644 --- a/spec/lib/services/nodes-api-service-spec.js +++ b/spec/lib/services/nodes-api-service-spec.js @@ -416,56 +416,44 @@ describe("Http.Services.Api.Nodes", function () { }); describe("_removeRelation", function() { - before("_removeRelation before", function() { - }); beforeEach(function() { updateByIdentifier.resolves(); }); it("_removeRelation should return undefined if target node is null", function() { - return nodeApiService._removeRelation(null, 'encloses', computeNode.id) - .then(function() { - expect(updateByIdentifier).to.not.be.called; - }); + expect(nodeApiService._removeRelation(null, 'encloses', computeNode)) + .to.equal(undefined); }); it("_removeRelation should fail if relation type is null", function() { - var enclNodes = [ - { - id: '1234abcd1234abcd1234abcd', - type: 'enclNode' - } - ]; + var enclNode = { + id: '1234abcd1234abcd1234abcd', + type: 'enclNode' + }; - return nodeApiService._removeRelation(enclNodes, 'encloses', computeNode.id) - .then(function() { - expect(updateByIdentifier).to.not.be.called; - }); + expect(nodeApiService._removeRelation(enclNode, null, computeNode)) + .to.equal(undefined); }); it("_removeRelation should fail if relationType is incorrect", function() { - var enclNodes = [ - { + var enclNode = { id: '1234abcd1234abcd1234abcd', - type: 'compute', + type: 'enclosure', relations: [ { "relationType": "enclosedBy", + "targets": [computeNode.id] } ] - } - ]; + }; - return nodeApiService._removeRelation(enclNodes, 'encloses', computeNode.id) - .then(function() { - expect(updateByIdentifier).to.not.be.called; - }); + expect(nodeApiService._removeRelation(enclNode, 'encloses', computeNode)) + .to.equal(undefined); }); it("_removeRelation should fail if don't have targets", function() { - var enclNodes = [ - { + var enclNode = { id: '1234abcd1234abcd1234abcd', type: 'enclosure', relations: [ @@ -473,25 +461,18 @@ describe("Http.Services.Api.Nodes", function () { "relationType": "encloses" } ] - } - ]; + }; - return nodeApiService._removeRelation(enclNodes, 'encloses', computeNode.id) - .then(function () { - expect(updateByIdentifier).to.not.have.been.called; - }); + expect(nodeApiService._removeRelation(enclNode, 'encloses', computeNode)) + .to.equal(undefined); }); it("_removeRelation should remove related id", function() { var enclRelationAfter = _.cloneDeep(enclosureNode.relations); _.pull(enclRelationAfter[0].targets, computeNode.id); - return nodeApiService._removeRelation(enclosureNode, 'encloses', computeNode.id) - .then(function () { - expect(updateByIdentifier).to.have.been - .calledWith(enclosureNode.id, - {relations: enclRelationAfter}); - }); + expect(nodeApiService._removeRelation(enclosureNode, 'encloses', computeNode).relations) + .to.deep.equal(enclRelationAfter); }); it("_removeRelation should remove one relation when no target", function() { @@ -515,12 +496,8 @@ describe("Http.Services.Api.Nodes", function () { }; var enclRelationAfter = _.cloneDeep(enclNode.relations); _.pull(enclRelationAfter, enclRelationAfter[0]); - return nodeApiService._removeRelation(enclNode, 'encloses', computeNode.id) - .then(function () { - expect(updateByIdentifier) - .to.have.been.calledWith(enclNode.id, - {relations: enclRelationAfter}); - }); + expect(nodeApiService._removeRelation(enclNode, 'encloses', computeNode).relations) + .to.deep.equal(enclRelationAfter); }); it("_removeRelation should remain empty relation field when no relation", function() { @@ -537,23 +514,8 @@ describe("Http.Services.Api.Nodes", function () { ] }; var enclRelationAfter = []; - return nodeApiService._removeRelation(enclNode, 'encloses', computeNode.id) - .then(function () { - expect(updateByIdentifier) - .to.have.been.calledWith(enclNode.id, - {relations: enclRelationAfter}); - }); - }); - - it("should delete component nodes", function() { - this.sandbox.stub(nodeApiService, 'removeNode').resolves(); - return nodeApiService._removeRelation( - computeNode, 'enclosedBy', "1234abcd1234abcd1234abcf" - ).then(function() { - expect(nodeApiService.removeNode).to.be.calledWithExactly( - computeNode, 'enclosedBy' - ); - }); + expect(nodeApiService._removeRelation(enclNode, 'encloses', computeNode).relations) + .to.deep.equal(enclRelationAfter); }); }); @@ -577,32 +539,42 @@ describe("Http.Services.Api.Nodes", function () { it("should do nothing if arguments are missing", function() { var argList = [rackNode, "contains", [computeNode]]; - return Promise.map(argList, function(arg, index) { + expect(_.map(argList, function(arg, index) { var argsCopy = [].concat(argList); argsCopy[index] = undefined; - return nodeApiService._addRelation(argsCopy[0], argsCopy[1], argsCopy[2]) - .then(function() { - expect(updateByIdentifier).to.not.be.called; - }); - }); + return nodeApiService._addRelation(argsCopy[0], argsCopy[1], argsCopy[2]); + }) + ).to.deep.equal([undefined, undefined, undefined]); }); - it("should return a promise for an update to the relations field of a node", function() { - return nodeApiService._addRelation( - rackNode, 'contains', [computeNode.id, computeNode2.id] - ).then(function() { - expect(updateByIdentifier).to.be.calledWithExactly(rackNode.id, {relations: - [{relationType: 'contains', targets: [computeNode.id, computeNode2.id]}] - }); - }).then(function() { - rackNode.relations = [{relationType: 'contains', targets: [computeNode.id]}]; - updateByIdentifier.reset(); - return nodeApiService._addRelation(rackNode, 'contains', [computeNode2.id]); - }).then(function() { - expect(updateByIdentifier).to.be.calledWithExactly(rackNode.id, {relations: - [{relationType: 'contains', targets: [computeNode.id, computeNode2.id]}] - }); - }); + it("should return the node updated with new relations", function() { + expect(nodeApiService._addRelation( + rackNode, 'contains', [computeNode, computeNode2]).relations) + .to.deep.equal( + [{relationType: 'contains', targets: [computeNode.id, computeNode2.id]}] + ); + + rackNode.relations = [{relationType: 'contains', targets: [computeNode.id]}]; + expect(nodeApiService._addRelation(rackNode, 'contains', [computeNode2]).relations) + .to.deep.equal( + [{relationType: 'contains', targets: [computeNode.id, computeNode2.id]}] + ); + }); + + it('should fail to relate a node to itself', function() { + expect(function() { + return nodeApiService._addRelation(rackNode, 'contains', rackNode.id); + }).to.throw("Node cannot have relationship contains with itself"); + }); + + it('should fail to set a node as containedBy two other nodes', function() { + var rackNode2 = _.cloneDeep(rackNode); + rackNode2.id = 'secondRackNodeId'; + expect(function() { + return nodeApiService._addRelation( + computeNode, 'containedBy', [rackNode.id, rackNode2.id] + ); + }).to.throw("Node "+computeNode.id+" can only be contained by one node"); }); }); @@ -646,6 +618,7 @@ describe("Http.Services.Api.Nodes", function () { error = new Errors.NotFoundError(); this.sandbox.stub(nodeApiService, "_needTargetNodes"); waterline.nodes.needByIdentifier.resolves(rackNode); + waterline.nodes.updateByIdentifier.resolves(); }); afterEach(function() { @@ -657,13 +630,13 @@ describe("Http.Services.Api.Nodes", function () { return nodeApiService.editNodeRelations(rackNode.id, body, handler) .then(function() { expect(handler).to.be.calledWithExactly( - rackNode, 'contains', [computeNode.id, computeNode2.id] + rackNode, 'contains', [computeNode, computeNode2] ); expect(handler).to.be.calledWithExactly( - computeNode, 'containedBy', rackNode.id + computeNode, 'containedBy', rackNode ); expect(handler).to.be.calledWithExactly( - computeNode2, 'containedBy', rackNode.id + computeNode2, 'containedBy', rackNode ); }); }); @@ -824,6 +797,8 @@ describe("Http.Services.Api.Nodes", function () { var computeNode2After = _.cloneDeep(computeNode2); delete computeNodeAfter.relations; delete computeNode2After.relations; + this.sandbox.stub(nodeApiService, '_findTargetNodes') + .resolves([computeNode, computeNode2]); findActiveGraphForTarget.resolves(''); needByIdentifier.withArgs(computeNode.id).resolves(computeNode); @@ -884,7 +859,6 @@ describe("Http.Services.Api.Nodes", function () { needByIdentifier.withArgs(computeNode2.id).resolves(computeNode2); needByIdentifier.withArgs(pduNode.id).resolves(pduNode); updateByIdentifier.withArgs(pduNode.id).resolves(pduNodeAfter); - return nodeApiService.removeNode(enclosureNode) .then(function (node){ expect(node).to.equal(enclosureNode); @@ -1048,7 +1022,7 @@ describe("Http.Services.Api.Nodes", function () { }); describe('Obms', function() { - + var node = { id: '1234abcd1234abcd1234abcd', name: 'name',