From 0f3789a150d73b4efc880127c8238e53f07e6973 Mon Sep 17 00:00:00 2001 From: wimrijnders Date: Fri, 22 Sep 2017 15:50:30 +0200 Subject: [PATCH] Adjust for-in loops so they can deal with added properties in Array and Object prototypes (#3471) * Added unit test for Array.prototype mangling - first passing version * Enhanced unit test for prototype stressing to catch more illegal for-in loops * Fixed all for-in linting violations --- .eslintrc | 9 +- lib/graph3d/Settings.js | 4 +- lib/network/Network.js | 3 + lib/network/modules/Clustering.js | 121 +++++++++--------- lib/network/modules/EdgesHandler.js | 23 ++-- lib/network/modules/LayoutEngine.js | 31 +++-- lib/network/modules/NodesHandler.js | 12 +- .../modules/components/nodes/Cluster.js | 43 +++---- lib/shared/Validator.js | 2 +- lib/util.js | 2 +- test/Network.test.js | 81 +++++++++--- 11 files changed, 177 insertions(+), 154 deletions(-) diff --git a/.eslintrc b/.eslintrc index 520db094..9853e411 100644 --- a/.eslintrc +++ b/.eslintrc @@ -18,7 +18,11 @@ "max-statements": [2, 115], "no-unreachable": 1, "no-useless-escape": 0, + "no-console": 0, + // To flag presence of console.log without breaking linting: + //"no-console": ["warn", { allow: ["warn", "error"] }], + "require-jsdoc": ["error", { "require": { "FunctionDeclaration": true, @@ -33,7 +37,6 @@ "requireParamDescription": false, "requireReturnType": true }], - } - // To flag presence of console.log without breaking linting: - //"no-console": ["warn", { allow: ["warn", "error"] }], + "guard-for-in": 1, + }, } diff --git a/lib/graph3d/Settings.js b/lib/graph3d/Settings.js index 560543a3..7b2c0c9c 100755 --- a/lib/graph3d/Settings.js +++ b/lib/graph3d/Settings.js @@ -174,7 +174,7 @@ function forceCopy(src, dst, fields, prefix) { var srcKey; var dstKey; - for (var i in fields) { + for (var i = 0; i < fields.length; ++i) { srcKey = fields[i]; dstKey = prefixFieldName(prefix, srcKey); @@ -198,7 +198,7 @@ function safeCopy(src, dst, fields, prefix) { var srcKey; var dstKey; - for (var i in fields) { + for (var i = 0; i < fields.length; ++i) { srcKey = fields[i]; if (src[srcKey] === undefined) continue; diff --git a/lib/network/Network.js b/lib/network/Network.js index fbe2b9de..f5980b20 100644 --- a/lib/network/Network.js +++ b/lib/network/Network.js @@ -403,9 +403,12 @@ Network.prototype.destroy = function () { delete this.images; for (var nodeId in this.body.nodes) { + if (!this.body.nodes.hasOwnProperty(nodeId)) continue; delete this.body.nodes[nodeId]; } + for (var edgeId in this.body.edges) { + if (!this.body.edges.hasOwnProperty(edgeId)) continue; delete this.body.edges[edgeId]; } diff --git a/lib/network/modules/Clustering.js b/lib/network/modules/Clustering.js index 5b9ad960..9a0b0144 100644 --- a/lib/network/modules/Clustering.js +++ b/lib/network/modules/Clustering.js @@ -160,23 +160,19 @@ class ClusterEngine { let childEdgesObj = {}; // collect the nodes that will be in the cluster - for (let nodeId in this.body.nodes) { - if (!this.body.nodes.hasOwnProperty(nodeId)) continue; - - let node = this.body.nodes[nodeId]; + util.forEach(this.body.nodes, (node, nodeId) => { let clonedOptions = NetworkUtil.cloneOptions(node); if (options.joinCondition(clonedOptions) === true) { - childNodesObj[nodeId] = this.body.nodes[nodeId]; + childNodesObj[nodeId] = node; // collect the edges that will be in the cluster - for (let i = 0; i < node.edges.length; i++) { - let edge = node.edges[i]; + util.forEach(node.edges, (edge) => { if (this.clusteredEdges[edge.id] === undefined) { childEdgesObj[edge.id] = edge; } - } + }); } - } + }); this._cluster(childNodesObj, childEdgesObj, options, refreshData); } @@ -249,7 +245,7 @@ class ClusterEngine { * @returns {Boolean} true if no joinCondition, otherwise return value of joinCondition */ var findClusterData = function() { - for (var n in clusters) { + for (let n = 0; n < clusters.length; ++n) { // Search for a cluster containing any of the node id's for (var m in childNodesObj) { if (clusters[n].nodes[m] !== undefined) { @@ -379,6 +375,8 @@ class ClusterEngine { }) for (childNode in childNodesObj) { + if (!childNodesObj.hasOwnProperty(childNode)) continue; + var childNode = childNodesObj[childNode]; for (var y=0; y < childNode.edges.length; y++){ var childEdge = childNode.edges[y]; @@ -1197,11 +1195,11 @@ class ClusterEngine { _filter(arr, callback) { let ret = []; - for (var n in arr) { - if (callback(arr[n])) { - ret.push(arr[n]); + util.forEach(arr, (item) => { + if (callback(item)) { + ret.push(item); } - } + }); return ret; } @@ -1211,18 +1209,15 @@ class ClusterEngine { * Scan all edges for changes in clustering and adjust this if necessary. * * Call this (internally) after there has been a change in node or edge data. + * + * Pre: States of this.body.nodes and this.body.edges consistent + * Pre: this.clusteredNodes and this.clusteredEdge consistent with containedNodes and containedEdges + * of cluster nodes. */ _updateState() { - // Pre: States of this.body.nodes and this.body.edges consistent - // Pre: this.clusteredNodes and this.clusteredEdge consistent with containedNodes and containedEdges - // of cluster nodes. let nodeId; - let edgeId; - let m, n; let deletedNodeIds = []; let deletedEdgeIds = []; - let self = this; - /** * Utility function to iterate over clustering nodes only @@ -1230,12 +1225,11 @@ class ClusterEngine { * @param {Function} callback function to call for each cluster node */ let eachClusterNode = (callback) => { - for (nodeId in this.body.nodes) { - let node = this.body.nodes[nodeId]; - if (node.isCluster !== true) continue; - - callback(node); - } + util.forEach(this.body.nodes, (node) => { + if (node.isCluster === true) { + callback(node); + } + }); }; @@ -1245,6 +1239,7 @@ class ClusterEngine { // Determine the deleted nodes for (nodeId in this.clusteredNodes) { + if (!this.clusteredNodes.hasOwnProperty(nodeId)) continue; let node = this.body.nodes[nodeId]; if (node === undefined) { @@ -1254,13 +1249,13 @@ class ClusterEngine { // Remove nodes from cluster nodes eachClusterNode(function(clusterNode) { - for (n in deletedNodeIds) { + for (let n = 0; n < deletedNodeIds.length; n++) { delete clusterNode.containedNodes[deletedNodeIds[n]]; } }); // Remove nodes from cluster list - for (n in deletedNodeIds) { + for (let n = 0; n < deletedNodeIds.length; n++) { delete this.clusteredNodes[deletedNodeIds[n]]; } @@ -1270,44 +1265,40 @@ class ClusterEngine { // // Add the deleted clustered edges to the list - for (edgeId in this.clusteredEdges) { + util.forEach(this.clusteredEdges, (edgeId) => { let edge = this.body.edges[edgeId]; if (edge === undefined || !edge.endPointsValid()) { deletedEdgeIds.push(edgeId); } - } + }); // Cluster nodes can also contain edges which are not clustered, // i.e. nodes 1-2 within cluster with an edge in between. // So the cluster nodes also need to be scanned for invalid edges eachClusterNode(function(clusterNode) { - for (edgeId in clusterNode.containedEdges) { - let edge = clusterNode.containedEdges[edgeId]; + util.forEach(clusterNode.containedEdges, (edge, edgeId) => { if (!edge.endPointsValid() && deletedEdgeIds.indexOf(edgeId) === -1) { deletedEdgeIds.push(edgeId); } - } + }); }); // Also scan for cluster edges which need to be removed in the active list. // Regular edges have been removed beforehand, so this only picks up the cluster edges. - for (edgeId in this.body.edges) { - let edge = this.body.edges[edgeId]; - + util.forEach(this.body.edges, (edge, edgeId) => { // Explicitly scan the contained edges for validity let isValid = true; let replacedIds = edge.clusteringEdgeReplacingIds; if (replacedIds !== undefined) { let numValid = 0; - for (let n in replacedIds) { - let containedEdgeId = replacedIds[n]; + util.forEach(replacedIds, (containedEdgeId) => { let containedEdge = this.body.edges[containedEdgeId]; if (containedEdge !== undefined && containedEdge.endPointsValid()) { numValid += 1; } - } + }); isValid = (numValid > 0); } @@ -1315,43 +1306,41 @@ class ClusterEngine { if (!edge.endPointsValid() || !isValid) { deletedEdgeIds.push(edgeId); } - } + }); // Remove edges from cluster nodes - eachClusterNode(function(clusterNode) { - for (n in deletedEdgeIds) { - let deletedEdgeId = deletedEdgeIds[n]; + eachClusterNode((clusterNode) => { + util.forEach(deletedEdgeIds, (deletedEdgeId) => { delete clusterNode.containedEdges[deletedEdgeId]; - for (m in clusterNode.edges) { - let edge = clusterNode.edges[m]; - + util.forEach(clusterNode.edges, (edge, m) => { if (edge.id === deletedEdgeId) { clusterNode.edges[m] = null; // Don't want to directly delete here, because in the loop - continue; + return; } - edge.clusteringEdgeReplacingIds = self._filter(edge.clusteringEdgeReplacingIds, function(id) { + edge.clusteringEdgeReplacingIds = this._filter(edge.clusteringEdgeReplacingIds, function(id) { return deletedEdgeIds.indexOf(id) === -1; }); - } + }); // Clean up the nulls - clusterNode.edges = self._filter(clusterNode.edges, function(item) {return item !== null}); - } + clusterNode.edges = this._filter(clusterNode.edges, function(item) {return item !== null}); + }); }); + // Remove from cluster list - for (n in deletedEdgeIds) { - delete this.clusteredEdges[deletedEdgeIds[n]]; - } + util.forEach(deletedEdgeIds, (edgeId) => { + delete this.clusteredEdges[edgeId]; + }); // Remove cluster edges from active list (this.body.edges). // deletedEdgeIds still contains id of regular edges, but these should all // be gone when you reach here. - for (n in deletedEdgeIds) { - delete this.body.edges[deletedEdgeIds[n]]; - } + util.forEach(deletedEdgeIds, (edgeId) => { + delete this.body.edges[edgeId]; + }); // @@ -1360,13 +1349,12 @@ class ClusterEngine { // Iterating over keys here, because edges may be removed in the loop let ids = Object.keys(this.body.edges); - for (n in ids) { - let edgeId = ids[n]; + util.forEach(ids, (edgeId) => { let edge = this.body.edges[edgeId]; let shouldBeClustered = this._isClusteredNode(edge.fromId) || this._isClusteredNode(edge.toId); if (shouldBeClustered === this._isClusteredEdge(edge.id)) { - continue; // all is well + return; // all is well } if (shouldBeClustered) { @@ -1382,11 +1370,16 @@ class ClusterEngine { } // TODO: check that it works for both edges clustered + // (This might be paranoia) } else { - // undo clustering for this edge + // This should not be happening, the state should + // be properly updated at this point. + // + // If it *is* reached during normal operation, then we have to implement + // undo clustering for this edge here. throw new Error('remove edge from clustering not implemented!'); } - } + }); // Clusters may be nested to any level. Keep on opening until nothing to open @@ -1405,7 +1398,7 @@ class ClusterEngine { }); // Open them - for (let n in clustersToOpen) { + for (let n = 0; n < clustersToOpen.length; ++n) { this.openCluster(clustersToOpen[n], {}, false /* Don't refresh, we're in an refresh/update already */); } diff --git a/lib/network/modules/EdgesHandler.js b/lib/network/modules/EdgesHandler.js index c74c55be..66ed8ff0 100644 --- a/lib/network/modules/EdgesHandler.js +++ b/lib/network/modules/EdgesHandler.js @@ -353,13 +353,12 @@ class EdgesHandler { if (ids.length === 0) return; // early out var edges = this.body.edges; - for (var i = 0; i < ids.length; i++) { - var id = ids[i]; + util.forEach(ids, (id) => { var edge = edges[id]; if (edge !== undefined) { edge.remove(); } - } + }); if (emit) { this.body.emitter.emit("_dataChanged"); @@ -370,17 +369,12 @@ class EdgesHandler { * Refreshes Edge Handler */ refresh() { - let edges = this.body.edges; - for (let edgeId in edges) { - let edge = undefined; - if (edges.hasOwnProperty(edgeId)) { - edge = edges[edgeId]; - } + util.forEach(this.body.edges, (edge, edgeId) => { let data = this.body.data.edges._data[edgeId]; - if (edge !== undefined && data !== undefined) { + if (data !== undefined) { edge.setOptions(data); } - } + }); } /** @@ -451,21 +445,20 @@ class EdgesHandler { _updateState() { let edgesToDelete = []; - for(let id in this.body.edges) { - let edge = this.body.edges[id]; + util.forEach(this.body.edges, (edge, id) => { let toNode = this.body.nodes[edge.toId]; let fromNode = this.body.nodes[edge.fromId]; // Skip clustering edges here, let the Clustering module handle those if ((toNode !== undefined && toNode.isCluster === true) || (fromNode !== undefined && fromNode.isCluster === true)) { - continue; + return; } if (toNode === undefined || fromNode === undefined) { edgesToDelete.push(id); } - } + }); this.remove(edgesToDelete, false); } diff --git a/lib/network/modules/LayoutEngine.js b/lib/network/modules/LayoutEngine.js index 296d04ba..7dc04b7e 100644 --- a/lib/network/modules/LayoutEngine.js +++ b/lib/network/modules/LayoutEngine.js @@ -1371,12 +1371,11 @@ class LayoutEngine { _getActiveEdges(node) { let result = []; - for (let j in node.edges) { - let edge = node.edges[j]; + util.forEach(node.edges, (edge) => { if (this.body.edgeIndices.indexOf(edge.id) !== -1) { result.push(edge); } - } + }); return result; } @@ -1392,18 +1391,18 @@ class LayoutEngine { let hubSizes = {}; let nodeIds = this.body.nodeIndices; - for (let i in nodeIds) { - let nodeId = nodeIds[i]; + util.forEach(nodeIds, (nodeId) => { let node = this.body.nodes[nodeId]; let hubSize = this._getActiveEdges(node).length; hubSizes[hubSize] = true; - } + }); // Make an array of the size sorted descending let result = []; - for (let size in hubSizes) { + util.forEach(hubSizes, (size) => { result.push(Number(size)); - } + }); + result.sort(function(a, b) { return b - a; }); @@ -1428,15 +1427,13 @@ class LayoutEngine { let hubSize = hubSizes[i]; if (hubSize === 0) break; - let nodeIds = this.body.nodeIndices; - for (let j in nodeIds) { - let nodeId = nodeIds[j]; + util.forEach(this.body.nodeIndices, (nodeId) => { let node = this.body.nodes[nodeId]; if (hubSize === this._getActiveEdges(node).length) { this._crawlNetwork(levelDownstream, nodeId); } - } + }); } } @@ -1485,19 +1482,21 @@ class LayoutEngine { /** * Check if there is an edge going the opposite direction for given edge + * + * @param {Edge} edge edge to check + * @returns {boolean} true if there's another edge going into the opposite direction */ - let self = this; let isBidirectional = (edge) => { - for (let key in self.body.edges) { - let otherEdge = self.body.edges[key]; + util.forEach(this.body.edges, (otherEdge) => { if (otherEdge.toId === edge.fromId && otherEdge.fromId === edge.toId) { return true; } - } + }); return false; }; + let levelByDirection = (nodeA, nodeB, edge) => { let levelA = this.hierarchical.levels[nodeA.id]; let levelB = this.hierarchical.levels[nodeB.id]; diff --git a/lib/network/modules/NodesHandler.js b/lib/network/modules/NodesHandler.js index c43a0a73..6f820419 100644 --- a/lib/network/modules/NodesHandler.js +++ b/lib/network/modules/NodesHandler.js @@ -368,23 +368,19 @@ class NodesHandler { * @param {boolean} [clearPositions=false] */ refresh(clearPositions = false) { - let nodes = this.body.nodes; - for (let nodeId in nodes) { - let node = undefined; - if (nodes.hasOwnProperty(nodeId)) { - node = nodes[nodeId]; - } + util.forEach(this.body.nodes, (node, nodeId) => { let data = this.body.data.nodes.get(nodeId); - if (node !== undefined && data !== undefined) { + if (data !== undefined) { if (clearPositions === true) { node.setOptions({x:null, y:null}); } node.setOptions({ fixed: false }); node.setOptions(data); } - } + }); } + /** * Returns the positions of the nodes. * @param {Array.|String} [ids] --> optional, can be array of nodeIds, can be string diff --git a/lib/network/modules/components/nodes/Cluster.js b/lib/network/modules/components/nodes/Cluster.js index 60eb0343..b05ae50a 100644 --- a/lib/network/modules/components/nodes/Cluster.js +++ b/lib/network/modules/components/nodes/Cluster.js @@ -1,4 +1,5 @@ -import Node from '../Node' +let util = require("../../../../util"); +let Node = require("../Node").default; /** * A Cluster is a special Node that allows a group of Nodes positioned closely together @@ -41,44 +42,40 @@ class Cluster extends Node { // Disconnect child cluster from current cluster delete this.containedNodes[childClusterId]; - for(let n in childCluster.edges) { - let edgeId = childCluster.edges[n].id; - delete this.containedEdges[edgeId]; - } + util.forEach(childCluster.edges, (edge) => { + delete this.containedEdges[edge.id]; + }); // Transfer nodes and edges - for (let nodeId in childCluster.containedNodes) { - this.containedNodes[nodeId] = childCluster.containedNodes[nodeId]; - } + util.forEach(childCluster.containedNodes, (node, nodeId) => { + this.containedNodes[nodeId] = node; + }); childCluster.containedNodes = {}; - for (let edgeId in childCluster.containedEdges) { - this.containedEdges[edgeId] = childCluster.containedEdges[edgeId]; - } + util.forEach(childCluster.containedEdges, (edge, edgeId) => { + this.containedEdges[edgeId] = edge; + }); childCluster.containedEdges = {}; // Transfer edges within cluster edges which are clustered - for (let n in childCluster.edges) { - let clusterEdge = childCluster.edges[n]; - - for (let m in this.edges) { - let parentClusterEdge = this.edges[m]; + util.forEach(childCluster.edges, (clusterEdge) => { + util.forEach(this.edges, (parentClusterEdge) => { + // Assumption: a clustered edge can only be present in a single clustering edge + // Not tested here let index = parentClusterEdge.clusteringEdgeReplacingIds.indexOf(clusterEdge.id); - if (index === -1) continue; + if (index === -1) return; - for (let n in clusterEdge.clusteringEdgeReplacingIds) { - let srcId = clusterEdge.clusteringEdgeReplacingIds[n]; + util.forEach(clusterEdge.clusteringEdgeReplacingIds, (srcId) => { parentClusterEdge.clusteringEdgeReplacingIds.push(srcId); // Maintain correct bookkeeping for transferred edge this.body.edges[srcId].edgeReplacedById = parentClusterEdge.id; - } + }); // Remove cluster edge from parent cluster edge parentClusterEdge.clusteringEdgeReplacingIds.splice(index, 1); - break; // Assumption: a clustered edge can only be present in a single clustering edge - } - } + }); + }); childCluster.edges = []; } } diff --git a/lib/shared/Validator.js b/lib/shared/Validator.js index 67facb52..c41066e8 100644 --- a/lib/shared/Validator.js +++ b/lib/shared/Validator.js @@ -234,7 +234,7 @@ class Validator { let closestMatchPath = []; let lowerCaseOption = option.toLowerCase(); let indexMatch = undefined; - for (let op in options) { + for (let op in options) { // eslint-disable-line guard-for-in let distance; if (options[op].__type__ !== undefined && recursive === true) { let result = Validator.findInOptions(option, options[op], util.copyAndExtendArray(path,op)); diff --git a/lib/util.js b/lib/util.js index cced85de..f21de8e8 100644 --- a/lib/util.js +++ b/lib/util.js @@ -640,7 +640,7 @@ exports.removeClassName = function (elem, classNames) { /** * For each method for both arrays and objects. - * In case of an array, the built-in Array.forEach() is applied. + * In case of an array, the built-in Array.forEach() is applied. (**No, it's not!**) * In case of an Object, the method loops over all properties of the object. * @param {Object | Array} object An Object or Array * @param {function} callback Callback method, called for each item in diff --git a/test/Network.test.js b/test/Network.test.js index 8d6e4b6e..ac22f050 100644 --- a/test/Network.test.js +++ b/test/Network.test.js @@ -292,6 +292,66 @@ describe('Network', function () { } +///////////////////////////////////////////////////// +// End Local helper methods for Edge and Node testing +///////////////////////////////////////////////////// + + + /** + * Helper function for clustering + */ + function clusterTo(network, clusterId, nodeList, allowSingle) { + var options = { + joinCondition: function(node) { + return nodeList.indexOf(node.id) !== -1; + }, + clusterNodeProperties: { + id: clusterId, + label: clusterId + } + } + + if (allowSingle === true) { + options.clusterNodeProperties.allowSingleNodeCluster = true + } + + network.cluster(options); + } + + + /** + * At time of writing, this test detected 22 out of 33 'illegal' loops. + * The real deterrent is eslint rule 'guard-for-in`. + */ + it('can deal with added fields in Array.prototype', function (done) { + Array.prototype.foo = 1; // Just add anything to the prototype + Object.prototype.bar = 2; // Let's screw up hashes as well + + // The network should just run without throwing errors + try { + var [network, data, numNodes, numEdges] = createSampleNetwork({}); + + // Do some stuff to trigger more errors + clusterTo(network, 'c1', [1,2,3]); + data.nodes.remove(1); + network.openCluster('c1'); + clusterTo(network, 'c1', [4], true); + clusterTo(network, 'c2', ['c1'], true); + clusterTo(network, 'c3', ['c2'], true); + data.nodes.remove(4); + + } catch(e) { + delete Array.prototype.foo; // Remove it again so as not to confuse other tests. + delete Object.prototype.bar; + assert(false, "Got exception:\n" + e.stack); + } + + delete Array.prototype.foo; // Remove it again so as not to confuse other tests. + delete Object.prototype.bar; + done(); + }); + + describe('Node', function () { it('has known font options', function () { @@ -555,27 +615,6 @@ describe('Edge', function () { describe('Clustering', function () { - /** - * Helper function for clustering - */ - function clusterTo(network, clusterId, nodeList, allowSingle) { - var options = { - joinCondition: function(node) { - return nodeList.indexOf(node.id) !== -1; - }, - clusterNodeProperties: { - id: clusterId, - label: clusterId - } - } - - if (allowSingle === true) { - options.clusterNodeProperties.allowSingleNodeCluster = true - } - - network.cluster(options); - } - it('properly handles options allowSingleNodeCluster', function() { var [network, data, numNodes, numEdges] = createSampleNetwork();