fix: Fix countPathsToRoot for cyclic graphs

`countPathsToRoot` reported incorrect results for cyclic graphs due to the interaction between caching and cycle breaking. You can't easily cache counts for intermediate traversal paths due to contextual cycle breaking, so just cache the final traversal counts.
snyk · May 6, 2023 · 4b15635 · 4b15635
1 parent cf57280
commit 4b15635
Show file tree

Hide file tree

Showing 7 changed files with 308 additions and 141 deletions.
diff --git a/src/core/dep-graph.ts b/src/core/dep-graph.ts
@@ -152,9 +152,14 @@ class DepGraphImpl implements types.DepGraphInternal {
   public countPathsToRoot(pkg: types.Pkg): number {
     let count = 0;
     for (const nodeId of this.getPkgNodeIds(pkg)) {
-      count += this.countNodePathsToRoot(nodeId);
+      if (this._countNodePathsToRootCache.has(nodeId)) {
+        count += this._countNodePathsToRootCache.get(nodeId)!;
+      } else {
+        const c = this.countNodePathsToRoot(nodeId);
+        this._countNodePathsToRootCache.set(nodeId, c);
+        count += c;
+      }
     }
-
     return count;
   }
 
@@ -367,30 +372,17 @@ class DepGraphImpl implements types.DepGraphInternal {
     return allPaths;
   }
 
-  private countNodePathsToRoot(
-    nodeId: string,
-    ancestors: string[] = [],
-  ): number {
-    if (ancestors.includes(nodeId)) {
-      return 0;
-    }
-
-    if (this._countNodePathsToRootCache.has(nodeId)) {
-      return this._countNodePathsToRootCache.get(nodeId) || 0;
-    }
-
-    const parentNodesIds = this.getNodeParentsNodeIds(nodeId);
-    if (parentNodesIds.length === 0) {
-      this._countNodePathsToRootCache.set(nodeId, 1);
+  private countNodePathsToRoot(nodeId: string, visited: string[] = []): number {
+    if (nodeId === this._rootNodeId) {
       return 1;
     }
-
-    ancestors = ancestors.concat(nodeId);
-    const count = parentNodesIds.reduce((acc, parentNodeId) => {
-      return acc + this.countNodePathsToRoot(parentNodeId, ancestors);
-    }, 0);
-
-    this._countNodePathsToRootCache.set(nodeId, count);
+    visited = visited.concat(nodeId);
+    let count = 0;
+    for (const parentNodeId of this.getNodeParentsNodeIds(nodeId)) {
+      if (!visited.includes(parentNodeId)) {
+        count += this.countNodePathsToRoot(parentNodeId, visited);
+      }
+    }
     return count;
   }
 }
diff --git a/test/core/__snapshots__/cycles.test.ts.snap b/test/core/__snapshots__/cycles.test.ts.snap
diff --git a/test/core/__snapshots__/pkg-paths-to-root.test.ts.snap b/test/core/__snapshots__/pkg-paths-to-root.test.ts.snap
@@ -0,0 +1,234 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`pkgPathsToRoot cycles returns expected paths for all packages: a@1 1`] = `
+Array [
+  Array [
+    Object {
+      "name": "a",
+      "version": "1",
+    },
+  ],
+]
+`;
+
+exports[`pkgPathsToRoot cycles returns expected paths for all packages: b@2 1`] = `
+Array [
+  Array [
+    Object {
+      "name": "b",
+      "version": "2",
+    },
+    Object {
+      "name": "a",
+      "version": "1",
+    },
+  ],
+]
+`;
+
+exports[`pkgPathsToRoot cycles returns expected paths for all packages: c@3 1`] = `
+Array [
+  Array [
+    Object {
+      "name": "c",
+      "version": "3",
+    },
+    Object {
+      "name": "a",
+      "version": "1",
+    },
+  ],
+]
+`;
+
+exports[`pkgPathsToRoot cycles returns expected paths for all packages: d@4 1`] = `
+Array [
+  Array [
+    Object {
+      "name": "d",
+      "version": "4",
+    },
+    Object {
+      "name": "a",
+      "version": "1",
+    },
+  ],
+]
+`;
+
+exports[`pkgPathsToRoot cycles returns expected paths for all packages: e@5 1`] = `
+Array [
+  Array [
+    Object {
+      "name": "e",
+      "version": "5",
+    },
+    Object {
+      "name": "b",
+      "version": "2",
+    },
+    Object {
+      "name": "a",
+      "version": "1",
+    },
+  ],
+  Array [
+    Object {
+      "name": "e",
+      "version": "5",
+    },
+    Object {
+      "name": "c",
+      "version": "3",
+    },
+    Object {
+      "name": "a",
+      "version": "1",
+    },
+  ],
+  Array [
+    Object {
+      "name": "e",
+      "version": "5",
+    },
+    Object {
+      "name": "g",
+      "version": "7",
+    },
+    Object {
+      "name": "f",
+      "version": "6",
+    },
+    Object {
+      "name": "d",
+      "version": "4",
+    },
+    Object {
+      "name": "a",
+      "version": "1",
+    },
+  ],
+]
+`;
+
+exports[`pkgPathsToRoot cycles returns expected paths for all packages: f@6 1`] = `
+Array [
+  Array [
+    Object {
+      "name": "f",
+      "version": "6",
+    },
+    Object {
+      "name": "d",
+      "version": "4",
+    },
+    Object {
+      "name": "a",
+      "version": "1",
+    },
+  ],
+  Array [
+    Object {
+      "name": "f",
+      "version": "6",
+    },
+    Object {
+      "name": "e",
+      "version": "5",
+    },
+    Object {
+      "name": "b",
+      "version": "2",
+    },
+    Object {
+      "name": "a",
+      "version": "1",
+    },
+  ],
+  Array [
+    Object {
+      "name": "f",
+      "version": "6",
+    },
+    Object {
+      "name": "e",
+      "version": "5",
+    },
+    Object {
+      "name": "c",
+      "version": "3",
+    },
+    Object {
+      "name": "a",
+      "version": "1",
+    },
+  ],
+]
+`;
+
+exports[`pkgPathsToRoot cycles returns expected paths for all packages: g@7 1`] = `
+Array [
+  Array [
+    Object {
+      "name": "g",
+      "version": "7",
+    },
+    Object {
+      "name": "f",
+      "version": "6",
+    },
+    Object {
+      "name": "d",
+      "version": "4",
+    },
+    Object {
+      "name": "a",
+      "version": "1",
+    },
+  ],
+  Array [
+    Object {
+      "name": "g",
+      "version": "7",
+    },
+    Object {
+      "name": "f",
+      "version": "6",
+    },
+    Object {
+      "name": "e",
+      "version": "5",
+    },
+    Object {
+      "name": "b",
+      "version": "2",
+    },
+    Object {
+      "name": "a",
+      "version": "1",
+    },
+  ],
+  Array [
+    Object {
+      "name": "g",
+      "version": "7",
+    },
+    Object {
+      "name": "f",
+      "version": "6",
+    },
+    Object {
+      "name": "e",
+      "version": "5",
+    },
+    Object {
+      "name": "c",
+      "version": "3",
+    },
+    Object {
+      "name": "a",
+      "version": "1",
+    },
+  ],
+]
+`;