Skip to content

Commit

Permalink
Ensure all nodes are included in overallOrder when cycles are allowed.
Browse files Browse the repository at this point in the history 
Including when there are several disconnected subgraphs, one or more of them having a cycle.

Fixes #33
  • Loading branch information
@jriecken
jriecken committed Dec 2, 2019
1 parent f32dc28 commit ed6477d
Show file tree
Hide file tree
Showing 2 changed files with 88 additions and 50 deletions.
49 changes: 31 additions & 18 deletions lib/dep_graph.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,13 +49,13 @@ DepGraph.prototype = {
/**
* The number of nodes in the graph.
*/
size:function () {
size: function () {
return Object.keys(this.nodes).length;
},
/**
* Add a node to the dependency graph. If a node already exists, this method will do nothing.
*/
addNode:function (node, data) {
addNode: function (node, data) {
if (!this.hasNode(node)) {
// Checking the arguments length allows the user to add a node with undefined data
if (arguments.length === 2) {
Expand All @@ -70,7 +70,7 @@ DepGraph.prototype = {
/**
* Remove a node from the dependency graph. If a node does not exist, this method will do nothing.
*/
removeNode:function (node) {
removeNode: function (node) {
if (this.hasNode(node)) {
delete this.nodes[node];
delete this.outgoingEdges[node];
Expand All @@ -88,13 +88,13 @@ DepGraph.prototype = {
/**
* Check if a node exists in the graph
*/
hasNode:function (node) {
hasNode: function (node) {
return this.nodes.hasOwnProperty(node);
},
/**
* Get the data associated with a node name
*/
getNodeData:function (node) {
getNodeData: function (node) {
if (this.hasNode(node)) {
return this.nodes[node];
} else {
Expand All @@ -104,7 +104,7 @@ DepGraph.prototype = {
/**
* Set the associated data for a given node name. If the node does not exist, this method will throw an error
*/
setNodeData:function (node, data) {
setNodeData: function (node, data) {
if (this.hasNode(node)) {
this.nodes[node] = data;
} else {
Expand All @@ -115,7 +115,7 @@ DepGraph.prototype = {
* Add a dependency between two nodes. If either of the nodes does not exist,
* an Error will be thrown.
*/
addDependency:function (from, to) {
addDependency: function (from, to) {
if (!this.hasNode(from)) {
throw new Error('Node does not exist: ' + from);
}
Expand All @@ -133,7 +133,7 @@ DepGraph.prototype = {
/**
* Remove a dependency between two nodes.
*/
removeDependency:function (from, to) {
removeDependency: function (from, to) {
var idx;
if (this.hasNode(from)) {
idx = this.outgoingEdges[from].indexOf(to);
Expand All @@ -153,7 +153,7 @@ DepGraph.prototype = {
* Return a clone of the dependency graph. If any custom data is attached
* to the nodes, it will only be shallow copied.
*/
clone:function () {
clone: function () {
var source = this;
var result = new DepGraph();
var keys = Object.keys(source.nodes);
Expand All @@ -172,7 +172,7 @@ DepGraph.prototype = {
* If `leavesOnly` is true, only nodes that do not depend on any other nodes will be returned
* in the array.
*/
dependenciesOf:function (node, leavesOnly) {
dependenciesOf: function (node, leavesOnly) {
if (this.hasNode(node)) {
var result = [];
var DFS = createDFS(this.outgoingEdges, leavesOnly, result, this.circular);
Expand All @@ -194,7 +194,7 @@ DepGraph.prototype = {
*
* If `leavesOnly` is true, only nodes that do not have any dependants will be returned in the array.
*/
dependantsOf:function (node, leavesOnly) {
dependantsOf: function (node, leavesOnly) {
if (this.hasNode(node)) {
var result = [];
var DFS = createDFS(this.incomingEdges, leavesOnly, result, this.circular);
Expand All @@ -215,19 +215,21 @@ DepGraph.prototype = {
*
* If `leavesOnly` is true, only nodes that do not depend on any other nodes will be returned.
*/
overallOrder:function (leavesOnly) {
overallOrder: function (leavesOnly) {
var self = this;
var result = [];
var keys = Object.keys(this.nodes);
if (keys.length === 0) {
return result; // Empty graph
} else {
// Look for cycles - we run the DFS starting at all the nodes in case there
// are several disconnected subgraphs inside this dependency graph.
var CycleDFS = createDFS(this.outgoingEdges, false, [], this.circular);
keys.forEach(function(n) {
CycleDFS(n);
});
if (!this.circular) {
// Look for cycles - we run the DFS starting at all the nodes in case there
// are several disconnected subgraphs inside this dependency graph.
var CycleDFS = createDFS(this.outgoingEdges, false, [], this.circular);
keys.forEach(function (n) {
CycleDFS(n);
});
}

var DFS = createDFS(this.outgoingEdges, leavesOnly, result, this.circular);
// Find all potential starting points (nodes with nothing depending on them) an
Expand All @@ -238,6 +240,17 @@ DepGraph.prototype = {
DFS(n);
});

// If we're allowing cycles - we need to run the DFS against any remaining
// nodes that did not end up in the initial result (as they are part of a
// subgraph that does not have a clear starting point)
if (this.circular) {
keys.filter(function (node) {
return result.indexOf(node) === -1;
}).forEach(function (n) {
DFS(n);
});
}

return result;
}
}
Expand Down
89 changes: 57 additions & 32 deletions specs/dep_graph_spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ describe('DepGraph', function () {
var falsyData = ['', 0, null, undefined, false];
graph.addNode('Foo');

falsyData.forEach(function(data) {
falsyData.forEach(function (data) {
graph.setNodeData('Foo', data);

expect(graph.hasNode('Foo')).toBeTrue();
Expand Down Expand Up @@ -104,7 +104,7 @@ describe('DepGraph', function () {
graph.addNode('a');
graph.addNode('b');

graph.addDependency('a','b');
graph.addDependency('a', 'b');

graph.addNode('a');

Expand All @@ -131,8 +131,8 @@ describe('DepGraph', function () {
graph.addNode('b');
graph.addNode('c');

graph.addDependency('a','b');
graph.addDependency('a','c');
graph.addDependency('a', 'b');
graph.addDependency('a', 'c');

expect(graph.dependenciesOf('a')).toEqual(['b', 'c']);
});
Expand All @@ -143,7 +143,7 @@ describe('DepGraph', function () {
graph.addNode('a');

expect(function () {
graph.addDependency('a','b');
graph.addDependency('a', 'b');
}).toThrow(new Error('Node does not exist: b'));
});

Expand Down Expand Up @@ -182,6 +182,31 @@ describe('DepGraph', function () {
expect(graph.overallOrder()).toEqual(['c', 'b', 'a', 'd']);
});

it('should include all nodes in overall order even from ' +
'cycles in disconnected subgraphs when circular is true', function () {
var graph = new DepGraph({ circular: true });

graph.addNode('2a');
graph.addNode('2b');
graph.addNode('2c');
graph.addDependency('2a', '2b');
graph.addDependency('2b', '2c');
graph.addDependency('2c', '2a');

graph.addNode('1a');
graph.addNode('1b');
graph.addNode('1c');
graph.addNode('1d');
graph.addNode('1e');

graph.addDependency('1a', '1b');
graph.addDependency('1a', '1c');
graph.addDependency('1b', '1c');
graph.addDependency('1c', '1d');

expect(graph.overallOrder()).toEqual(['1d', '1c', '1b', '1a', '1e', '2c', '2b', '2a']);
});

it('should detect cycles in overall order', function () {
var graph = new DepGraph();

Expand Down Expand Up @@ -217,24 +242,24 @@ describe('DepGraph', function () {
});

it('should detect cycles in overall order when there are several ' +
'disconnected subgraphs (with one that does not have a cycle', function () {
var graph = new DepGraph();

graph.addNode('a_1');
graph.addNode('a_2');
graph.addNode('b_1');
graph.addNode('b_2');
graph.addNode('b_3');

graph.addDependency('a_1', 'a_2');
graph.addDependency('b_1', 'b_2');
graph.addDependency('b_2', 'b_3');
graph.addDependency('b_3', 'b_1');

expect(function () {
graph.overallOrder();
}).toThrow(new dep_graph.DepGraphCycleError(['b_1', 'b_2', 'b_3', 'b_1']));
});
'disconnected subgraphs (with one that does not have a cycle', function () {
var graph = new DepGraph();

graph.addNode('a_1');
graph.addNode('a_2');
graph.addNode('b_1');
graph.addNode('b_2');
graph.addNode('b_3');

graph.addDependency('a_1', 'a_2');
graph.addDependency('b_1', 'b_2');
graph.addDependency('b_2', 'b_3');
graph.addDependency('b_3', 'b_1');

expect(function () {
graph.overallOrder();
}).toThrow(new dep_graph.DepGraphCycleError(['b_1', 'b_2', 'b_3', 'b_1']));
});

it('should retrieve dependencies and dependants in the correct order', function () {
var graph = new DepGraph();
Expand All @@ -255,8 +280,8 @@ describe('DepGraph', function () {
expect(graph.dependenciesOf('d')).toEqual(['c', 'b']);

expect(graph.dependantsOf('a')).toEqual([]);
expect(graph.dependantsOf('b')).toEqual(['a','d']);
expect(graph.dependantsOf('c')).toEqual(['a','d','b']);
expect(graph.dependantsOf('b')).toEqual(['a', 'd']);
expect(graph.dependantsOf('c')).toEqual(['a', 'd', 'b']);
expect(graph.dependantsOf('d')).toEqual(['a']);
});

Expand Down Expand Up @@ -373,7 +398,7 @@ describe('DepGraph', function () {
it('should only be a shallow clone', function () {
var graph = new DepGraph();

var data = {a: 42};
var data = { a: 42 };
graph.addNode('a', data);

var cloned = graph.clone();
Expand All @@ -383,27 +408,27 @@ describe('DepGraph', function () {
graph.getNodeData('a').a = 43;
expect(cloned.getNodeData('a').a).toBe(43);

cloned.setNodeData('a', {a: 42});
cloned.setNodeData('a', { a: 42 });
expect(cloned.getNodeData('a').a).toBe(42);
expect(graph.getNodeData('a') === cloned.getNodeData('a')).toBeFalse();
});
});

describe('DepGraphCycleError', function() {
describe('DepGraphCycleError', function () {
var DepGraphCycleError = dep_graph.DepGraphCycleError;
it('should have a message', function() {

it('should have a message', function () {
var err = new DepGraphCycleError(['a', 'b', 'c', 'a']);
expect(err.message).toEqual('Dependency Cycle Found: a -> b -> c -> a');
});

it('should be an instanceof DepGraphCycleError', function() {
it('should be an instanceof DepGraphCycleError', function () {
var err = new DepGraphCycleError(['a', 'b', 'c', 'a']);
expect(err instanceof DepGraphCycleError).toBeTrue();
expect(err instanceof Error).toBeTrue();
});

it('should have a cyclePath', function() {
it('should have a cyclePath', function () {
var cyclePath = ['a', 'b', 'c', 'a'];
var err = new DepGraphCycleError(cyclePath);
expect(err.cyclePath).toEqual(cyclePath);
Expand Down

0 comments on commit ed6477d

Please sign in to comment.