diff --git a/project.clj b/project.clj
index 2d4c3db0aca890c46c032e036ed3596775cbdb5d..c38b1496340722b63b51254b470343a52347577b 100644
--- a/project.clj
+++ b/project.clj
@@ -120,7 +120,8 @@
[org.eclipse.jetty/jetty-server "9.4.15.v20190215"] ; We require JDK 8 which allows us to run Jetty 9.4, ring-jetty-adapter runs on 1.7 which forces an older version
[ring/ring-json "0.4.0"] ; Ring middleware for reading/writing JSON automatically
[stencil "0.5.0"] ; Mustache templates for Clojure
- [toucan "1.11.0" :exclusions [org.clojure/java.jdbc honeysql]]] ; Model layer, hydration, and DB utilities
+ [toucan "1.11.0" :exclusions [org.clojure/java.jdbc honeysql]] ; Model layer, hydration, and DB utilities
+ [weavejester/dependency "0.2.1"]] ; Dependency graphs and topological sorting
:main ^:skip-aot metabase.core
diff --git a/src/metabase/query_processor/middleware/fetch_source_query.clj b/src/metabase/query_processor/middleware/fetch_source_query.clj
index 2813a3fa193dcc49e48641e866da4670fc0364da..e292b47f76ad74ba419b99097e494dfe8c77c22b 100644
--- a/src/metabase/query_processor/middleware/fetch_source_query.clj
+++ b/src/metabase/query_processor/middleware/fetch_source_query.clj
@@ -33,7 +33,8 @@
[i18n :refer [trs tru]]
[schema :as su]]
[schema.core :as s]
- [toucan.db :as db]))
+ [toucan.db :as db]
+ [weavejester.dependency :as dep]))
;; These next two schemas are for validating the intermediate stages of the middleware. We don't need to validate the
;; entire query
@@ -146,38 +147,47 @@
(dissoc m :source-table)
source-query-and-metadata)))
-(defn- check-for-circular-references
- "Make sure we aren't trying to resolve circular references such as a card whose source query is itself."
- [card-id already-resolved]
- (when (contains? (set already-resolved) card-id)
- (throw (ex-info (str (tru "Cannot resolve source query for Card {0}: circular references between source queries."
- card-id))
- {:resolving card-id, :already-resolved already-resolved}))))
-
(defn- resolve-all*
+ [m]
+ (mbql.u/replace m
+ map-with-card-id-source-table?
+ ;; if this is a map that has a Card ID `:source-table`, resolve that (replacing it with the appropriate
+ ;; `:source-query`, then recurse and resolve any nested-nested queries that need to be resolved too
+ (let [resolved (resolve-one &match)]
+ ;; wrap the recursive call in a try-catch; if the recursive resolution fails, add context about the
+ ;; resolution that were we in the process of
+ (try
+ (resolve-all* resolved)
+ (catch Throwable e
+ (throw (ex-info (.getMessage e)
+ {:resolving &match, :resolved resolved}
+ e)))))))
+
+(defn- check-for-circular-references
+ "Check that there are no circular dependencies among source cards. This is equivalent to
+ finding a topological sort of the dependency graph.
+ https://en.wikipedia.org/wiki/Topological_sorting"
([m]
- (resolve-all* m nil))
-
- ([m already-resolved]
- (mbql.u/replace m
- map-with-card-id-source-table?
- ;; if this is a map that has a Card ID `:source-table`, resolve that (replacing it with the appropriate
- ;; `:source-query`, then recurse and resolve any nested-nested queries that need to be resolved too
- (let [source-table-str (:source-table &match)
- card-id (source-table-str->card-id source-table-str)]
- (check-for-circular-references card-id already-resolved)
- ;; ok, now actually resolve the Card ID, then recursively resolve all the Card ID source tables inside the
- ;; newly resolved source query
- ;;
- (let [resolved (resolve-one &match)]
- ;; wrap the recursive call in a try-catch; if the recursive resolution fails, add context about the
- ;; resolution that were we in the process of
- (try
- (resolve-all* resolved (conj already-resolved card-id))
- (catch Throwable e
- (throw (ex-info (.getMessage e)
- {:resolving &match, :resolved resolved}
- e)))))))))
+ (check-for-circular-references (dep/graph) m)
+ m)
+ ([g m]
+ (transduce (comp (filter map-with-card-id-source-table?)
+ (map (comp card-id->source-query-and-metadata
+ source-table-str->card-id
+ :source-table)))
+ (fn
+ ([] g)
+ ([g source-query]
+ (-> g
+ (dep/depend m source-query)
+ ;; Recursive call will circuit break the moment there's a cycle, so no
+ ;; danger of unbounded recursion.
+ (check-for-circular-references source-query)))
+ ([g]
+ ;; This will throw if there's a cycle
+ (dep/topo-sort g)
+ g))
+ (tree-seq coll? identity m))))
(defn- copy-source-query-database-ids
"If `m` has the saved questions virtual `:database` ID, (recursively) look for actual resolved Database IDs in the
@@ -209,7 +219,9 @@
"Recursively replace all Card ID source tables in `m` with resolved `:source-query` and `:source-metadata`. Since
the `:database` is only useful for top-level source queries, we'll remove it from all other levels."
[query :- su/Map]
- (-> (resolve-all* query)
+ (-> query
+ check-for-circular-references
+ resolve-all*
copy-source-query-database-ids
remove-unneeded-database-ids))
diff --git a/test/metabase/query_processor/middleware/fetch_source_query_test.clj b/test/metabase/query_processor/middleware/fetch_source_query_test.clj
index a2a28bc3bf5a4f41559f9cfad68de58316ec3aad..4848653c6837dd22489bf28159aaa6b4f464ea36 100644
--- a/test/metabase/query_processor/middleware/fetch_source_query_test.clj
+++ b/test/metabase/query_processor/middleware/fetch_source_query_test.clj
@@ -236,3 +236,22 @@
(str "Failed to save Card:" e)))]
(or save-error
(resolve-card-id-source-tables (circular-source-query card-1-id)))))))
+
+;; Alow complex dependency topologies such as:
+;;
+;; A
+;: | \
+;; B |
+;; | /
+;; C
+;;
+(expect
+ (tt/with-temp* [Card [{card-1-id :id} {:dataset_query (data/mbql-query venues)}]
+ Card [{card-2-id :id} {:dataset_query (data/mbql-query nil
+ {:source-table (str "card__" card-1-id)})}]]
+ (resolve-card-id-source-tables
+ (data/mbql-query nil
+ {:source-table (str "card__" card-1-id)
+ :joins [{:alias "c"
+ :source-table (str "card__" card-2-id)
+ :condition [:= *ID/Number &c.venues.id]}]}))))