Use Tarjan's strongly connected component algorithm to compute cycles. (

#486)

* Use Tarjan's strongly connected component algorithm to compute cycles.

* cleanup

* cleanup

* bump version

core/BUILD

@@ -11,6 +11,7 @@ ts_library(
         "//sqlx",
         "@npm//@types/node",
         "@npm//protobufjs",
+        "@npm//tarjan-graph",
     ],
 )
 

core/core.package.json

@@ -6,6 +6,7 @@
   "dependencies": {
     "@dataform/protos": "$DF_VERSION",
     "@dataform/sqlx": "$DF_VERSION",
-    "protobufjs": "^6.8.8"
+    "protobufjs": "^6.8.8",
+    "tarjan-graph": "^2.0.0"
   }
 }

core/session.ts

@@ -7,6 +7,8 @@ import * as test from "@dataform/core/test";
 import * as utils from "@dataform/core/utils";
 import { dataform } from "@dataform/protos";
 import { util } from "protobufjs";
+import { Graph as TarjanGraph } from "tarjan-graph";
+import * as TarjanGraphConstructor from "tarjan-graph";
 
 interface IActionProto {
   name?: string;
@@ -502,26 +504,23 @@ export class Session {
 
   private checkCircularity(actions: IActionProto[]) {
     const allActionsByName = keyByName(actions);
-    const checkCircular = (action: IActionProto, dependents: IActionProto[]): boolean => {
-      if (dependents.indexOf(action) >= 0) {
-        const message = `Circular dependency detected in chain: [${dependents
-          .map(d => d.name)
-          .join(" > ")} > ${action.name}]`;
-        this.compileError(new Error(message), action.fileName);
-        return true;
-      }
-      return (action.dependencies || []).some(
-        dependencyName =>
-          allActionsByName[dependencyName] &&
-          checkCircular(allActionsByName[dependencyName], dependents.concat([action]))
-      );
-    };
 
-    for (const action of actions) {
-      if (checkCircular(action, [])) {
-        break;
-      }
-    }
+    // Type exports for tarjan-graph are unfortunately wrong, so we have to do this minor hack.
+    const tarjanGraph: TarjanGraph = new (TarjanGraphConstructor as any)();
+    actions.forEach(action => {
+      const cleanedDependencies = (action.dependencies || []).filter(
+        dependency => !!allActionsByName[dependency]
+      );
+      tarjanGraph.add(action.name, cleanedDependencies);
+    });
+    const cycles = tarjanGraph.getCycles();
+    cycles.forEach(cycle => {
+      const firstActionInCycle = allActionsByName[cycle[0].name];
+      const message = `Circular dependency detected in chain: [${cycle
+        .map(vertex => vertex.name)
+        .join(" > ")} > ${firstActionInCycle.name}]`;
+      this.compileError(new Error(message), firstActionInCycle.fileName);
+    });
   }
 }
 

package.json

@@ -74,6 +74,7 @@
     "source-map-loader": "^0.2.0",
     "sql-formatter": "^2.3.3",
     "style-loader": "^0.13.1",
+    "tarjan-graph": "^2.0.0",
     "tmp": "0.0.33",
     "ts-loader": "^5.3.1",
     "ts-mockito": "^2.3.1",

version.bzl

@@ -1,3 +1,3 @@
 # NOTE: If you change the format of this line, you must change the bash command
 # in /scripts/publish to extract the version string correctly.
-DF_VERSION = "1.3.8"
+DF_VERSION = "1.3.9"

yarn.lock

@@ -8399,6 +8399,11 @@ tar@^4:
     safe-buffer "^5.1.2"
     yallist "^3.0.3"
 
+tarjan-graph@^2.0.0:
+  version "2.0.0"
+  resolved "https://registry.yarnpkg.com/tarjan-graph/-/tarjan-graph-2.0.0.tgz#8d8e991f170600982f9b8081984112ea8390b4a3"
+  integrity sha512-fDe57nO2Ukw2A/jHwVeiEgERGrGHukf3aHmR/YZ9BrveOtHVlFs289AnVeb1wD2aj9g01ZZ6f7VyMJ2QxI2NBQ==
+
 tedious@^4.2.0:
   version "4.2.0"
   resolved "https://registry.yarnpkg.com/tedious/-/tedious-4.2.0.tgz#ced27d46c3e3c2f643888bb62f2ea4f463650793"