From b10abc158295f85032555e5a302518b947d97b60 Mon Sep 17 00:00:00 2001
From: BenBirt <benjamin.birt@gmail.com>
Date: Tue, 19 Nov 2019 17:32:18 +0000
Subject: [PATCH] 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             |  1 +
 core/core.package.json |  3 ++-
 core/session.ts        | 37 ++++++++++++++++++-------------------
 package.json           |  1 +
 version.bzl            |  2 +-
 yarn.lock              |  5 +++++
 6 files changed, 28 insertions(+), 21 deletions(-)

diff --git a/core/BUILD b/core/BUILD
index a4c66179d..84a157caa 100644
--- a/core/BUILD
+++ b/core/BUILD
@@ -11,6 +11,7 @@ ts_library(
         "//sqlx",
         "@npm//@types/node",
         "@npm//protobufjs",
+        "@npm//tarjan-graph",
     ],
 )
 
diff --git a/core/core.package.json b/core/core.package.json
index 458fb2053..f3f7743eb 100644
--- a/core/core.package.json
+++ b/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"
   }
 }
diff --git a/core/session.ts b/core/session.ts
index 52bd4d5dc..300f94dbb 100644
--- a/core/session.ts
+++ b/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);
+    });
   }
 }
 
diff --git a/package.json b/package.json
index 65aa5f15e..e2c73c27a 100644
--- a/package.json
+++ b/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",
diff --git a/version.bzl b/version.bzl
index c1cf47b9a..521fff8f1 100644
--- a/version.bzl
+++ b/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"
diff --git a/yarn.lock b/yarn.lock
index e8120ab93..4d04bcaa1 100644
--- a/yarn.lock
+++ b/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"