diff --git a/src/metabase/driver/sql.clj b/src/metabase/driver/sql.clj index 060f72af448777c05093443a559825b8b8946d6e..d7b32bdbc76ca8ad9c727bf366b91fc3c3eb0c55 100644 --- a/src/metabase/driver/sql.clj +++ b/src/metabase/driver/sql.clj @@ -64,7 +64,7 @@ :params params))) ;; `:sql` drivers almost certainly don't need to override this method, and instead can implement -;; `unprepare/unprepare-value` for specific classes, or, in extereme cases, `unprepare/unprepare` itself. +;; `unprepare/unprepare-value` for specific classes, or, in extreme cases, `unprepare/unprepare` itself. (defmethod driver/splice-parameters-into-native-query :sql [driver {:keys [params], sql :query, :as query}] (cond-> query diff --git a/src/metabase/models/query_field.clj b/src/metabase/models/query_field.clj index 1b12517b1c605ed1a2139f2efb937a4383bcc5c1..307315a9b63f19eff3719590eb46c1310589c642 100644 --- a/src/metabase/models/query_field.clj +++ b/src/metabase/models/query_field.clj @@ -24,7 +24,7 @@ [query] (case (lib/normalized-query-type query) :native (try - (query-analyzer/field-ids-for-sql query) + (query-analyzer/field-ids-for-native query) (catch Exception e (log/error e "Error parsing SQL" query))) :query {:explicit (mbql.u/referenced-field-ids query)} @@ -47,7 +47,7 @@ query-field-rows (concat (map (partial id->row true) explicit) (map (partial id->row false) implicit))] - ;; when response is `nil`, it's a disabled parser, not unknown columns + ;; when the response is `nil`, it's a disabled parser, not unknown columns (when (some? res) (t2/with-transaction [_conn] (let [existing (t2/select :model/QueryField :card_id card-id) @@ -57,7 +57,7 @@ {:id-fn :field_id :to-compare #(dissoc % :id :card_id :field_id)})] (when (seq to-delete) - ;; this delete seems to break transaction (implicit commit or something) on MySQL, and this `diff` + ;; this deletion seems to break transaction (implicit commit or something) on MySQL, and this `diff` ;; algo drops its frequency by a lot - which should help with transactions affecting each other a ;; lot. Parallel tests in `metabase.models.query.permissions-test` were breaking when delete was ;; executed unconditionally on every query change. diff --git a/src/metabase/native_query_analyzer.clj b/src/metabase/native_query_analyzer.clj index 6d48804e830a1f238afe15cbad13fc463ac13b54..390b25a1a876cae51c911686d36f64fa8349a690 100644 --- a/src/metabase/native_query_analyzer.clj +++ b/src/metabase/native_query_analyzer.clj @@ -15,6 +15,7 @@ [clojure.string :as str] [macaw.core :as macaw] [metabase.config :as config] + [metabase.driver :as driver] [metabase.driver.util :as driver.u] [metabase.native-query-analyzer.impl :as nqa.impl] [metabase.native-query-analyzer.parameter-substitution :as nqa.sub] @@ -138,24 +139,32 @@ [:= :f.active true] (into [:or] (map table-query tables))]})))) -(defn field-ids-for-sql +(defn- field-ids-for-sql "Returns a `{:explicit #{...} :implicit #{...}}` map with field IDs that (may) be referenced in the given card's query. Errs on the side of optimism: i.e., it may return fields that are *not* in the query, and is unlikely to fail to return fields that are in the query. Explicit references are columns that are named in the query; implicit ones are from wildcards. If a field could be both explicit and implicit, it will *only* show up in the `:explicit` set." + [driver query] + (let [db-id (:database query) + macaw-opts (nqa.impl/macaw-options driver) + sql-string (:query (nqa.sub/replace-tags query)) + parsed-query (macaw/query->components (macaw/parsed-query sql-string) macaw-opts) + explicit-ids (explicit-field-ids-for-query parsed-query db-id) + implicit-ids (set/difference + (implicit-field-ids-for-query parsed-query db-id) + explicit-ids)] + {:explicit explicit-ids + :implicit implicit-ids})) + +(defn field-ids-for-native + "Returns a `{:explicit #{...} :implicit #{...}}` map with field IDs that (may) be referenced in the given card's + query. Currently only support SQL-based dialects." [query] - (when (and (active?) - (:native query)) - (let [db-id (:database query) - sql-string (:query (nqa.sub/replace-tags query)) - driver (driver.u/database->driver db-id) - macaw-opts (nqa.impl/macaw-options driver) - parsed-query (macaw/query->components (macaw/parsed-query sql-string) macaw-opts) - explicit-ids (explicit-field-ids-for-query parsed-query db-id) - implicit-ids (set/difference - (implicit-field-ids-for-query parsed-query db-id) - explicit-ids)] - {:explicit explicit-ids - :implicit implicit-ids}))) + (when (and (active?) (:native query)) + (let [driver (driver.u/database->driver (:database query))] + ;; TODO this approach is not extensible, we need to move to multimethods. + ;; See https://github.com/metabase/metabase/issues/43516 for long term solution. + (when (isa? driver/hierarchy driver :sql) + (field-ids-for-sql driver query))))) diff --git a/test/metabase/native_query_analyzer_test.clj b/test/metabase/native_query_analyzer_test.clj index c7202a5004d3de526288d788f4d45ff5ef0a6a7d..983eeb5f62ea53871f660671824275ff773e28f0 100644 --- a/test/metabase/native_query_analyzer_test.clj +++ b/test/metabase/native_query_analyzer_test.clj @@ -33,7 +33,7 @@ (deftest ^:parallel field-matching-test (binding [query-analyzer/*parse-queries-in-test?* true] (let [q (fn [sql] - (#'query-analyzer/field-ids-for-sql (mt/native-query {:query sql})))] + (#'query-analyzer/field-ids-for-native (mt/native-query {:query sql})))] (testing "simple query matches" (is (= {:explicit #{(mt/id :venues :id)} :implicit nil} (q "select id from venues"))))