Merge pull request #2268 from weaveworks/fix-graph-layout-edge-directionality

Fix edge directionality in graph layout

client/app/scripts/selectors/nodes-chart-layout.js

@@ -3,41 +3,12 @@ import { createSelector, createStructuredSelector } from 'reselect';
 import { Map as makeMap } from 'immutable';
 import timely from 'timely';
 
-import { EDGE_ID_SEPARATOR } from '../constants/naming';
+import { initEdgesFromNodes, collapseMultiEdges } from '../utils/layouter-utils';
 import { doLayout } from '../charts/nodes-layout';
 
 const log = debug('scope:nodes-chart');
 
 
-function initEdgesFromNodes(nodes) {
-  let edges = makeMap();
-
-  nodes.forEach((node, nodeId) => {
-    const adjacency = node.get('adjacency');
-    if (adjacency) {
-      adjacency.forEach((adjacent) => {
-        const edge = nodeId < adjacent ? [nodeId, adjacent] : [adjacent, nodeId];
-        const edgeId = edge.join(EDGE_ID_SEPARATOR);
-
-        if (!edges.has(edgeId)) {
-          const source = edge[0];
-          const target = edge[1];
-          if (nodes.has(source) && nodes.has(target)) {
-            edges = edges.set(edgeId, makeMap({
-              id: edgeId,
-              value: 1,
-              source,
-              target
-            }));
-          }
-        }
-      });
-    }
-  });
-
-  return edges;
-}
-
 // TODO: Make all the selectors below pure (so that they only depend on the global state).
 
 const layoutOptionsSelector = createStructuredSelector({
@@ -86,8 +57,9 @@ export const graphLayout = createSelector(
     log(`graph layout calculation took ${timedLayouter.time}ms`);
 
     return {
+      // NOTE: This might be a good place to add (some of) nodes/edges decorators.
       layoutNodes: graph.nodes,
-      layoutEdges: graph.edges,
+      layoutEdges: collapseMultiEdges(graph.edges),
     };
   }
 );

client/app/scripts/utils/__tests__/layouter-utils-test.js

@@ -0,0 +1,41 @@
+import { fromJS } from 'immutable';
+
+import {
+  initEdgesFromNodes,
+  collapseMultiEdges,
+} from '../layouter-utils';
+
+
+describe('LayouterUtils', () => {
+  describe('initEdgesFromNodes', () => {
+    it('should return map of edges', () => {
+      const input = fromJS({
+        a: { adjacency: ['b', 'c'] },
+        b: { adjacency: ['a', 'b'] },
+        c: {}
+      });
+      expect(initEdgesFromNodes(input).toJS()).toEqual({
+        'a-b': { id: 'a-b', source: 'a', target: 'b', value: 1 },
+        'a-c': { id: 'a-c', source: 'a', target: 'c', value: 1 },
+        'b-a': { id: 'b-a', source: 'b', target: 'a', value: 1 },
+        'b-b': { id: 'b-b', source: 'b', target: 'b', value: 1 },
+      });
+    });
+  });
+
+  describe('collapseMultiEdges', () => {
+    it('should return collapsed multi-edges', () => {
+      const input = fromJS({
+        'a-b': { id: 'a-b', source: 'a', target: 'b' },
+        'a-c': { id: 'a-c', source: 'a', target: 'c' },
+        'b-a': { id: 'b-a', source: 'b', target: 'a' },
+        'b-b': { id: 'b-b', source: 'b', target: 'b' },
+      });
+      expect(collapseMultiEdges(input).toJS()).toEqual({
+        'a-b': { id: 'a-b', source: 'a', target: 'b', bidirectional: true },
+        'a-c': { id: 'a-c', source: 'a', target: 'c' },
+        'b-b': { id: 'b-b', source: 'b', target: 'b' },
+      });
+    });
+  });
+});

client/app/scripts/utils/layouter-utils.js

@@ -0,0 +1,55 @@
+import { Map as makeMap } from 'immutable';
+
+import { EDGE_ID_SEPARATOR } from '../constants/naming';
+
+
+function constructEdgeId(source, target) {
+  return [source, target].join(EDGE_ID_SEPARATOR);
+}
+
+// Constructs the edges for the layout engine from the nodes' adjacency table.
+// We don't collapse edge pairs (A->B, B->A) here as we want to let the layout
+// engine decide how to handle bidirectional edges.
+export function initEdgesFromNodes(nodes) {
+  let edges = makeMap();
+
+  nodes.forEach((node, nodeId) => {
+    (node.get('adjacency') || []).forEach((adjacentId) => {
+      const source = nodeId;
+      const target = adjacentId;
+
+      if (nodes.has(target)) {
+        // The direction source->target is important since dagre takes
+        // directionality into account when calculating the layout.
+        const edgeId = constructEdgeId(source, target);
+        const edge = makeMap({ id: edgeId, value: 1, source, target });
+        edges = edges.set(edgeId, edge);
+      }
+    });
+  });
+
+  return edges;
+}
+
+// Replaces all pairs of edges (A->B, B->A) with a single A->B edge that is marked as
+// bidirectional. We do this to prevent double rendering of edges between the same nodes.
+export function collapseMultiEdges(directedEdges) {
+  let collapsedEdges = makeMap();
+
+  directedEdges.forEach((edge, edgeId) => {
+    const source = edge.get('source');
+    const target = edge.get('target');
+    const reversedEdgeId = constructEdgeId(target, source);
+
+    if (collapsedEdges.has(reversedEdgeId)) {
+      // If the edge between the same nodes with the opposite direction already exists,
+      // mark it as bidirectional and don't add any other edges (making graph simple).
+      collapsedEdges = collapsedEdges.setIn([reversedEdgeId, 'bidirectional'], true);
+    } else {
+      // Otherwise just copy the edge.
+      collapsedEdges = collapsedEdges.set(edgeId, edge);
+    }
+  });
+
+  return collapsedEdges;
+}