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

@@ -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,
+  },
 }

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;
 

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];
   }
 

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 */);
       }
 

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);
   }

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];

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.<Node.id>|String} [ids]  --> optional, can be array of nodeIds, can be string

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 = [];
   }
 }

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));

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

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();