Skip to content

Commit

Permalink
Adjust for-in loops so they can deal with added properties in Array a…
Browse files Browse the repository at this point in the history 
…nd Object prototypes (almende#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
  • Loading branch information
@wimrijnders
wimrijnders authored and Primoz Susa committed Jan 3, 2019
1 parent d32e0e1 commit 1023058
Show file tree
Hide file tree
Showing 11 changed files with 177 additions and 154 deletions.
9 changes: 6 additions & 3 deletions .eslintrc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
},
}
4 changes: 2 additions & 2 deletions lib/graph3d/Settings.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -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;

Expand Down
3 changes: 3 additions & 0 deletions lib/network/Network.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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];
}

Expand Down
121 changes: 57 additions & 64 deletions lib/network/modules/Clustering.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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];
Expand Down Expand Up @@ -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;
}
Expand All @@ -1211,31 +1209,27 @@ 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
*
* @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);
}
});
};


Expand All @@ -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) {
Expand All @@ -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]];
}

Expand All @@ -1270,88 +1265,82 @@ 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);
}

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


//
Expand All @@ -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) {
Expand All @@ -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
Expand All @@ -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 */);
}

Expand Down
23 changes: 8 additions & 15 deletions lib/network/modules/EdgesHandler.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand All @@ -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);
}
}
});
}

/**
Expand Down Expand Up @@ -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);
}
Expand Down
Loading

0 comments on commit 1023058

Please sign in to comment.