Add fix-bad-references middleware (#19612)

src/metabase/query_processor.clj

@@ -32,6 +32,7 @@
             [metabase.query-processor.middleware.desugar :as desugar]
             [metabase.query-processor.middleware.expand-macros :as expand-macros]
             [metabase.query-processor.middleware.fetch-source-query :as fetch-source-query]
+            [metabase.query-processor.middleware.fix-bad-references :as fix-bad-refs]
             [metabase.query-processor.middleware.format-rows :as format-rows]
             [metabase.query-processor.middleware.large-int-id :as large-int-id]
             [metabase.query-processor.middleware.limit :as limit]
@@ -91,6 +92,7 @@
    ;; yes, this is called a second time, because we need to handle any joins that got added
    (resolve 'ee.sandbox.rows/apply-row-level-permissions)
    #'viz-settings/update-viz-settings
+   #'fix-bad-refs/fix-bad-references-middleware
    #'resolve-joined-fields/resolve-joined-fields
    #'resolve-joins/resolve-joins
    #'add-implicit-joins/add-implicit-joins

src/metabase/query_processor/middleware/fix_bad_references.clj

+(ns metabase.query-processor.middleware.fix-bad-references
+  (:require [clojure.tools.logging :as log]
+            [clojure.walk :as walk]
+            [metabase.mbql.util :as mbql.u]
+            [metabase.query-processor.store :as qp.store]
+            [metabase.util :as u]
+            [metabase.util.i18n :refer [trs]]))
+
+(defn- find-source-table [{:keys [source-table source-query]}]
+  (or source-table
+      (when source-query
+        (recur source-query))))
+
+(defn- find-join-against-table [{:keys [joins source-query]} table-id]
+  (or (when source-query
+        (find-join-against-table source-query table-id))
+      (some (fn [join]
+              (when (= (find-source-table join) table-id)
+                join))
+            joins)))
+
+(defn- table [table-id]
+  (when table-id
+    (qp.store/fetch-and-store-tables! #{table-id})
+    (qp.store/table table-id)))
+
+(defn- fix-bad-references*
+  ([inner-query]
+   (fix-bad-references* inner-query inner-query (find-source-table inner-query)))
+
+  ([inner-query form source-table & sources]
+   (mbql.u/replace form
+     ;; don't replace anything inside source metadata.
+     (_ :guard (constantly ((set &parents) :source-metadata)))
+     &match
+
+     ;; if we have entered a join map and don't have `join-source` info yet, determine that and recurse.
+     (m :guard (every-pred map?
+                           :condition
+                           (fn [join]
+                             (let [join-source (find-source-table join)]
+                               (not (contains? (set sources) join-source))))))
+     (apply fix-bad-references* inner-query m source-table (cons (find-source-table m) sources))
+
+     ;; find Field ID fields whose Table IS NOT the source table (or not directly available in some `[:source-query+
+     ;; :source-table]` path that do not have `:join-alias` info
+     [:field
+      (id :guard (every-pred integer? (fn [id]
+                                        (let [{table-id :table_id} (qp.store/field id)]
+                                          (not (some (partial = table-id)
+                                                     (cons source-table sources)))))))
+      (opts :guard (complement :join-alias))]
+     (let [{table-id :table_id, :as field} (qp.store/field id)
+           {join-alias :alias}             (find-join-against-table inner-query table-id)]
+       (log/warn (u/colorize 'yellow (str (trs "Bad :field clause {0} for field {1} at {2}: clause should have a :join-alias."
+                                               (pr-str &match)
+                                               (pr-str (format "%s.%s"
+                                                               (:name (table table-id))
+                                                               (:name field)))
+                                               (pr-str &parents))
+                                          " "
+                                          (if join-alias
+                                            (trs "Guessing join {0}" (pr-str join-alias))
+                                            (trs "Unable to infer an appropriate join. Query may not work as expected.")))))
+       (if join-alias
+         [:field id (assoc opts :join-alias join-alias)]
+         &match)))))
+
+(defn fix-bad-references
+  "Walk `query` and look for `:field` ID clauses without `:join-alias` information that reference Fields belonging to
+  Tables other than the source Table (or an 'indirect' source Table that is available via source queries). Such
+  references are technically disallowed. Since we are nice we will look thru joins and try to figure out a join that
+  will work and add appropriate `:join-alias` information if we can.
+
+  This middleware performs a best-effort DWIM transformation, and isn't smart enough to fix every broken query out
+  there. If the query cannot be fixed, this log a warning and move on. See #19612 for more information."
+  [query]
+  (walk/postwalk
+   (fn [form]
+     (if (and (map? form)
+              ((some-fn :source-query :source-table) form)
+              (not (:condition form)))
+       (fix-bad-references* form)
+       form))
+   query))
+
+(defn fix-bad-references-middleware
+  "Middleware version of [[fix-bad-references]]."
+  [qp]
+  (fn [query rff context]
+    (qp (fix-bad-references query) rff context)))

test/metabase/query_processor/middleware/fix_bad_references_test.clj

+(ns metabase.query-processor.middleware.fix-bad-references-test
+  (:require [clojure.test :refer :all]
+            [metabase.query-processor.middleware.fix-bad-references :as fix-bad-refs]
+            [metabase.test :as mt]))
+
+(defn- fix-bad-refs [query]
+  (mt/with-everything-store
+    (fix-bad-refs/fix-bad-references query)))
+
+(deftest fix-bad-references-test
+  (mt/dataset sample-dataset
+    (is (query= (mt/mbql-query orders
+                  {:source-query {:source-table $$orders
+                                  :joins        [{:fields       :all
+                                                  :source-table $$products
+                                                  :condition    [:= $orders.product_id &P1.products.id]
+                                                  :alias        "P1"}]}
+                   :joins        [{:fields       :all
+                                   :condition    [:= &P1.products.category &Q2.products.category]
+                                   :alias        "Q2"
+                                   :source-table $$reviews}]})
+                (fix-bad-refs
+                 (mt/mbql-query orders
+                   {:source-query {:source-table $$orders
+                                   :joins        [{:source-table $$products
+                                                   :alias        "P1"
+                                                   :condition    [:= $orders.product_id &P1.products.id]
+                                                   :fields       :all}]}
+                    :joins        [{:fields       :all
+                                    :condition    [:= $products.category &Q2.products.category]
+                                    :alias        "Q2"
+                                    :source-table $$reviews}]}))))))