Support compiling SQL with inlined parameters directly (metabase#45008)

* Support compiling SQL with inlined parameters directly

* Test fix

* Make `format-honeysql` a multimethod

* Add test for metabase#32543

* Test also fixes metabase#44915

* Add explicit test for metabase#44915

* Better test for metabase#44915

* More test fixes 🔧

* Fix Kondo error

docs/developers-guide/driver-changelog.md

@@ -12,6 +12,48 @@ title: Driver interface changelog
   for better performance when saving Questions. Refer to the method docstring for more information and where to find
   an example implementation.
 
+- Prior to 0.51.0, to generate SQL queries with inline parameters, Metabase would generate a parameterized SQL string,
+  then attempt to parse the SQL replace and replace `?` placeholders with inline values from the driver method
+  `metabase.driver.sql.util.unprepare/unprepare-value`. In 0.51.0+, Metabase instead generates these queries using
+  Honey SQL 2's `:inline` option, eliminating the need to parse and replace `?` placeholders. As such, the
+  `metabase.driver.sql.util.unprepare` namespace has been deprecated; you should remove all usages of it in your driver.
+
+  - The `metabase.driver.sql.util.unprepare/unprepare-value` method has been replaced by the new method
+    `metabase.driver.sql.query-processor/inline-value`. The signatures of these two functions are the same, and you
+    should be able to simply change the all of your `unprepare-value` implementations to `inline-value` instead. See
+    [PR #45008](https://github.com/metabase/metabase/pull/45008) for examples of this change.
+
+    For the time being, implementations of `unprepare-value` are used as implementations of `inline-value`
+    automatically, but `unprepare-value` is slated for removal in 0.54.0.
+
+  - `metabase.driver.sql.query-processor/format-honeysql` is now a multimethod, mainly so you can bind
+    `*compile-with-inline-parameters*` if you need to always compile without parameterization.
+
+  - The dynamic variable `metabase.driver/*compile-with-inline-parameters*` (default `false`) has been added; drivers
+    that can generate parameterized queries should look at its value in their implementation of
+    `metabase.driver/mbql->native` and adjust their output accordingly. For `:sql-jdbc`-based drivers that support
+    parameterization, this is handled in the shared `metabase.driver.sql.query-processor` code, so you shouldn't need
+    to adjust anything here. For `:sql` drivers that do not support JDBC-style parameterized queries you can implement
+    `format-honeysql` and bind `*compile-with-inline-parameters*` as discussed above. See the `:athena` driver for an
+    example of how to do this.
+
+  - `metabase.driver.sql.util.unprepare/unprepare`, which took a parameterized SQL string and de-parameterized or
+    "unprepared" it, has been removed. Instead, if you need a query with parameters spliced directly into the SQL,
+    bind `metabase.driver/*compile-with-inline-parameters*` as discussed above.
+
+  - Similarly, the driver method `metabase.driver/splice-parameters-into-native-query` has been marked deprecated, and
+    the default implementation will throw an Exception if called. Rework code that generates parameterized queries and
+    then calls `unprepare` or `splice-parameters-into-native-query` with code that generates queries with inlined
+    parameters in the first place as discussed above. Tests can use
+    `metabase.query-processor.compile/compile-with-inline-parameters` if needed.
+
+   - `metabase.query-processor.compile/compile-and-splice-parameters` has been removed; replace usages with
+     `metabase.query-processor.compile/compile-with-inline-parameters`.
+
+- The three-arity of `metabase.driver.sql.query-processor/format-honeysql` (which had an additional parameter for
+  Honey SQL version) has been removed; replace all usages with the two-arity version. Honey SQL 2 has been the only
+  supported version since Metabase 0.49.0.
+
 ## Metabase 0.50.0
 
 - The Metabase `metabase.mbql.*` namespaces have been moved to `metabase.legacy-mbql.*`. You probably didn't need to

enterprise/backend/src/metabase_enterprise/llm/tasks/describe_question.clj

@@ -11,7 +11,7 @@
   "Create a data-oriented summary of a question as input to an LLM for summarization."
   [{:keys [display visualization_settings dataset_query result_metadata]}]
   (let [visualization_settings (u/remove-nils visualization_settings)
-        {:keys [query]} (qp.compile/compile-and-splice-parameters dataset_query)]
+        {:keys [query]} (qp.compile/compile-with-inline-parameters dataset_query)]
     (cond->
       {:sql_query           query
        :display_type        display

modules/drivers/athena/src/metabase/driver/athena.clj

@@ -14,7 +14,6 @@
    [metabase.driver.sql-jdbc.execute :as sql-jdbc.execute]
    [metabase.driver.sql-jdbc.sync :as sql-jdbc.sync]
    [metabase.driver.sql.query-processor :as sql.qp]
-   [metabase.driver.sql.util.unprepare :as unprepare]
    [metabase.public-settings.premium-features :as premium-features]
    [metabase.util :as u]
    [metabase.util.date-2 :as u.date]
@@ -146,7 +145,7 @@
   column, and you can only have `timestamp` columns when actually creating them."
   false)
 
-(defmethod unprepare/unprepare-value [:athena OffsetDateTime]
+(defmethod sql.qp/inline-value [:athena OffsetDateTime]
   [_driver t]
   ;; Timestamp literals do not support offsets, or at least they don't in `INSERT INTO ...` statements. I'm not 100%
   ;; sure what the correct thing to do here is then. The options are either:
@@ -163,7 +162,7 @@
     ;; when not loading data we can actually use timestamp with offset info.
     (format "timestamp '%s %s %s'" (t/local-date t) (t/local-time t) (t/zone-offset t))))
 
-(defmethod unprepare/unprepare-value [:athena ZonedDateTime]
+(defmethod sql.qp/inline-value [:athena ZonedDateTime]
   [driver t]
   ;; This format works completely fine for literals e.g.
   ;;
@@ -176,7 +175,7 @@
   ;; Athena (despite Athena only partially supporting TIMESTAMP WITH TIME ZONE) then you can use the commented out impl
   ;; to do it. That should work ok because it converts it to a UTC then to a LocalDateTime. -- Cam
   (if *loading-data*
-    (unprepare/unprepare-value driver (t/offset-date-time t))
+    (sql.qp/inline-value driver (t/offset-date-time t))
     (format "timestamp '%s %s %s'" (t/local-date t) (t/local-time t) (t/zone-id t))))
 
 ;;; for some evil reason Athena expects `OFFSET` *before* `LIMIT`, unlike every other database in the known universe; so
@@ -437,13 +436,13 @@
      (let [metadata (.getMetaData conn)]
        {:tables (fast-active-tables driver metadata details)}))))
 
-; Unsure if this is the right way to approach building the parameterized query...but it works
-(defn- prepare-query [driver {query :native, :as outer-query}]
-  (cond-> outer-query
-    (seq (:params query))
-    (merge {:native {:params nil
-                     :query (unprepare/unprepare driver (cons (:query query) (:params query)))}})))
+(defmethod sql.qp/format-honeysql :athena
+  [driver honeysql-form]
+  (binding [driver/*compile-with-inline-parameters* true]
+    ((get-method sql.qp/format-honeysql :sql) driver honeysql-form)))
 
 (defmethod driver/execute-reducible-query :athena
   [driver query context respond]
-  ((get-method driver/execute-reducible-query :sql-jdbc) driver (prepare-query driver query) context respond))
+  (assert (empty? (get-in query [:native :params]))
+          "Athena queries should not be parameterized; they should have been compiled with metabase.driver/*compile-with-inline-parameters*")
+  ((get-method driver/execute-reducible-query :sql-jdbc) driver query context respond))

modules/drivers/athena/test/metabase/driver/athena_test.clj

@@ -3,12 +3,12 @@
    [clojure.java.jdbc :as jdbc]
    [clojure.string :as str]
    [clojure.test :refer :all]
-   [honey.sql :as sql]
    [metabase.driver :as driver]
    [metabase.driver.athena :as athena]
    [metabase.driver.sql-jdbc.connection :as sql-jdbc.conn]
    [metabase.driver.sql-jdbc.execute :as sql-jdbc.execute]
    [metabase.driver.sql.query-processor :as sql.qp]
+   [metabase.lib.core :as lib]
    [metabase.lib.test-metadata :as meta]
    [metabase.lib.test-util :as lib.tu]
    [metabase.public-settings.premium-features :as premium-features]
@@ -85,8 +85,9 @@
       ;; we have code in place to work around that.
       (let [timestamp-tz [:raw "timestamp '2022-11-16 04:21:00 US/Pacific'"]
             time         [:raw "time '5:03:00'"]
-            [sql & args] (sql/format {:select [[timestamp-tz :timestamp-tz]
-                                               [time :time]]})
+            [sql & args] (sql.qp/format-honeysql :athena {:select [[timestamp-tz :timestamp-tz]
+                                                                   [time :time]]})
+            _            (assert (empty? args))
             query        (-> (mt/native-query {:query sql, :params args})
                              (assoc-in [:middleware :format-rows?] false))]
         (mt/with-native-query-testing-context query
@@ -102,8 +103,9 @@
     (testing "We should be able to handle TIME and TIMESTAMP WITH TIME ZONE parameters correctly"
       (let [timestamp-tz #t "2022-11-16T04:21:00.000-08:00[America/Los_Angeles]"
             time         #t "05:03"
-            [sql & args] (sql/format {:select [[timestamp-tz :timestamp-tz]
-                                               [time :time]]})
+            [sql & args] (sql.qp/format-honeysql :athena {:select [[timestamp-tz :timestamp-tz]
+                                                                   [time :time]]})
+            _            (assert (empty? args))
             query        (-> (mt/native-query {:query sql, :params args})
                              (assoc-in [:middleware :format-rows?] false))]
         (mt/with-native-query-testing-context query
@@ -253,3 +255,33 @@
                                    (jdbc/metadata-result rs))))))))
               (testing "`describe-table` returns the fields anyway"
                 (is (not-empty (:fields (driver/describe-table :athena db table)))))))))))
+
+(deftest column-name-with-question-mark-test
+  (testing "Column name with a question mark in it should be compiled correctly (#44915)"
+    (mt/test-driver :athena
+      (let [metadata-provider (lib.tu/merged-mock-metadata-provider meta/metadata-provider {:fields [{:id   (meta/id :venues :name)
+                                                                                                      :name "name?"}]})
+            query             (-> (lib/query metadata-provider (meta/table-metadata :venues))
+                                  (lib/with-fields [(meta/field-metadata :venues :name)])
+                                  (lib/filter (lib/= (meta/field-metadata :venues :name) "BBQ"))
+                                  (lib/limit 1))
+            executed-query    (atom nil)]
+        (with-redefs [sql-jdbc.execute/execute-reducible-query (let [orig sql-jdbc.execute/execute-reducible-query]
+                                                                 (fn [driver query context respond]
+                                                                   (reset! executed-query query)
+                                                                   (orig driver query context respond)))]
+          (try
+            (qp/process-query query)
+            (catch Throwable _))
+          (is (= {:query ["SELECT"
+                          "  \"PUBLIC\".\"VENUES\".\"name?\" AS \"name?\""
+                          "FROM"
+                          "  \"PUBLIC\".\"VENUES\""
+                          "WHERE"
+                          "  \"PUBLIC\".\"VENUES\".\"name?\" = 'BBQ'"
+                          "LIMIT"
+                          "  1"]
+                  :params nil}
+                 (-> @executed-query
+                     :native
+                     (update :query #(str/split-lines (driver/prettify-native-form :athena %)))))))))))

modules/drivers/athena/test/metabase/test/data/athena.clj

@@ -9,7 +9,6 @@
    [metabase.driver.sql-jdbc.connection :as sql-jdbc.conn]
    [metabase.driver.sql-jdbc.execute :as sql-jdbc.execute]
    [metabase.driver.sql-jdbc.sync :as sql-jdbc.sync]
-   [metabase.driver.sql.util.unprepare :as unprepare]
    [metabase.test.data.interface :as tx]
    [metabase.test.data.sql :as sql.tx]
    [metabase.test.data.sql-jdbc :as sql-jdbc.tx]
@@ -166,8 +165,8 @@
 ;; So go ahead and deparameterize all the statements for now.
 (defmethod ddl/insert-rows-ddl-statements :athena
   [driver table-identifier row-or-rows]
-  (for [sql+args ((get-method ddl/insert-rows-ddl-statements :sql-jdbc/test-extensions) driver table-identifier row-or-rows)]
-    (unprepare/unprepare driver sql+args)))
+  (binding [driver/*compile-with-inline-parameters* true]
+    ((get-method ddl/insert-rows-ddl-statements :sql-jdbc/test-extensions) driver table-identifier row-or-rows)))
 
 (doseq [[base-type sql-type] {:type/BigInteger     "BIGINT"
                               :type/Boolean        "BOOLEAN"

modules/drivers/bigquery-cloud-sdk/src/metabase/driver/bigquery_cloud_sdk/query_processor.clj

@@ -10,7 +10,6 @@
    [metabase.driver.sql.parameters.substitution :as sql.params.substitution]
    [metabase.driver.sql.query-processor :as sql.qp]
    [metabase.driver.sql.util :as sql.u]
-   [metabase.driver.sql.util.unprepare :as unprepare]
    [metabase.legacy-mbql.util :as mbql.u]
    [metabase.lib.metadata :as lib.metadata]
    [metabase.lib.schema.metadata :as lib.schema.metadata]
@@ -697,34 +696,34 @@
 ;; *  https://cloud.google.com/bigquery/docs/reference/standard-sql/date_functions
 ;; *  https://cloud.google.com/bigquery/docs/reference/standard-sql/datetime_functions
 
-(defmethod unprepare/unprepare-value [:bigquery-cloud-sdk String]
+(defmethod sql.qp/inline-value [:bigquery-cloud-sdk String]
   [_ s]
   ;; escape single-quotes like Cam's String -> Cam\'s String
   (str \' (str/replace s "'" "\\\\'") \'))
 
-(defmethod unprepare/unprepare-value [:bigquery-cloud-sdk LocalTime]
+(defmethod sql.qp/inline-value [:bigquery-cloud-sdk LocalTime]
   [_ t]
   (format "time \"%s\"" (u.date/format-sql t)))
 
-(defmethod unprepare/unprepare-value [:bigquery-cloud-sdk LocalDate]
+(defmethod sql.qp/inline-value [:bigquery-cloud-sdk LocalDate]
   [_ t]
   (format "date \"%s\"" (u.date/format-sql t)))
 
-(defmethod unprepare/unprepare-value [:bigquery-cloud-sdk LocalDateTime]
+(defmethod sql.qp/inline-value [:bigquery-cloud-sdk LocalDateTime]
   [_ t]
   (format "datetime \"%s\"" (u.date/format-sql t)))
 
-(defmethod unprepare/unprepare-value [:bigquery-cloud-sdk OffsetTime]
+(defmethod sql.qp/inline-value [:bigquery-cloud-sdk OffsetTime]
   [_ t]
   ;; convert to a LocalTime in UTC
   (let [local-time (t/local-time (t/with-offset-same-instant t (t/zone-offset 0)))]
     (format "time \"%s\"" (u.date/format-sql local-time))))
 
-(defmethod unprepare/unprepare-value [:bigquery-cloud-sdk OffsetDateTime]
+(defmethod sql.qp/inline-value [:bigquery-cloud-sdk OffsetDateTime]
   [_ t]
   (format "timestamp \"%s\"" (u.date/format-sql t)))
 
-(defmethod unprepare/unprepare-value [:bigquery-cloud-sdk ZonedDateTime]
+(defmethod sql.qp/inline-value [:bigquery-cloud-sdk ZonedDateTime]
   [_ t]
   (format "timestamp \"%s %s\"" (u.date/format-sql (t/local-date-time t)) (.getId (t/zone-id t))))
 

modules/drivers/bigquery-cloud-sdk/test/metabase/driver/bigquery_cloud_sdk/query_processor_test.clj

@@ -294,19 +294,19 @@
                  :info     {:executed-by 1000
                             :query-hash  (byte-array [1 2 3 4])}})))))))
 
-(deftest ^:parallel unprepare-params-test
-  (mt/test-driver :bigquery-cloud-sdk
-    (is (= [["Red Medicine"]]
-           (mt/rows
-             (qp/process-query
-              (mt/native-query
-                {:query  (with-test-db-name
-                           (str "SELECT `v4_test_data.venues`.`name` AS `name` "
-                                "FROM `v4_test_data.venues` "
-                                "WHERE `v4_test_data.venues`.`name` = ?"))
-                 :params ["Red Medicine"]}))))
-        (str "Do we properly unprepare, and can we execute, queries that still have parameters for one reason or "
-             "another? (EE #277)"))))
+(deftest ^:parallel query-with-params-test
+  (testing "Can we execute queries with parameters? (EE #277)"
+           (mt/test-driver :bigquery-cloud-sdk
+
+             (is (= [["Red Medicine"]]
+                    (mt/rows
+                     (qp/process-query
+                      (mt/native-query
+                        {:query  (with-test-db-name
+                                   (str "SELECT `v4_test_data.venues`.`name` AS `name` "
+                                        "FROM `v4_test_data.venues` "
+                                        "WHERE `v4_test_data.venues`.`name` = ?"))
+                         :params ["Red Medicine"]}))))))))
 
 (deftest ^:parallel temporal-type-test
   (testing "Make sure we can detect temporal types correctly"

modules/drivers/bigquery-cloud-sdk/test/metabase/driver/bigquery_cloud_sdk_test.clj

@@ -919,6 +919,6 @@
                   str/join
                   pretty-sql-lines)
              (->> (mt/mbql-query venues {:aggregation [:count]})
-                  qp.compile/compile-and-splice-parameters
+                  qp.compile/compile-with-inline-parameters
                   :query
                   pretty-sql-lines))))))

modules/drivers/druid-jdbc/src/metabase/driver/druid_jdbc.clj

@@ -12,7 +12,6 @@
    [metabase.driver.sql-jdbc.sync :as sql-jdbc.sync]
    [metabase.driver.sql.query-processor :as sql.qp]
    [metabase.driver.sql.query-processor.util :as sql.qp.u]
-   [metabase.driver.sql.util.unprepare :as unprepare]
    [metabase.lib.field :as lib.field]
    [metabase.lib.metadata :as lib.metadata]
    [metabase.query-processor.store :as qp.store]
@@ -119,15 +118,15 @@
   [driver ps i t]
   (sql-jdbc.execute/set-parameter driver ps i (format-datetime t)))
 
-(defmethod unprepare/unprepare-value [:druid-jdbc ZonedDateTime]
+(defmethod sql.qp/inline-value [:druid-jdbc ZonedDateTime]
   [_driver t]
   (format "'%s'" (format-datetime t)))
 
 (defmethod sql-jdbc.execute/set-parameter [:druid-jdbc LocalDateTime]
   [driver ps i t]
   (sql-jdbc.execute/set-parameter driver ps i (format-datetime t)))
 
-(defmethod unprepare/unprepare-value [:druid-jdbc LocalDateTime]
+(defmethod sql.qp/inline-value [:druid-jdbc LocalDateTime]
   [_driver t]
   (format "'%s'" (format-datetime t)))
 

modules/drivers/oracle/src/metabase/driver/oracle.clj

@@ -14,13 +14,10 @@
    [metabase.driver.sql-jdbc.execute :as sql-jdbc.execute]
    [metabase.driver.sql-jdbc.sync :as sql-jdbc.sync]
    [metabase.driver.sql-jdbc.sync.common :as sql-jdbc.sync.common]
-   [metabase.driver.sql-jdbc.sync.describe-table
-    :as sql-jdbc.describe-table]
+   [metabase.driver.sql-jdbc.sync.describe-table :as sql-jdbc.describe-table]
    [metabase.driver.sql.query-processor :as sql.qp]
-   [metabase.driver.sql.query-processor.empty-string-is-null
-    :as sql.qp.empty-string-is-null]
+   [metabase.driver.sql.query-processor.empty-string-is-null :as sql.qp.empty-string-is-null]
    [metabase.driver.sql.util :as sql.u]
-   [metabase.driver.sql.util.unprepare :as unprepare]
    [metabase.models.secret :as secret]
    [metabase.query-processor.timezone :as qp.timezone]
    [metabase.util.date-2 :as u.date]
@@ -622,20 +619,20 @@
   (fn []
     (u.date/parse (.getNString rs i))))
 
-(defmethod unprepare/unprepare-value [:oracle OffsetDateTime]
+(defmethod sql.qp/inline-value [:oracle OffsetDateTime]
   [_ t]
   ;; Oracle doesn't like `Z` to mean UTC
   (format "timestamp '%s'" (-> (t/format "yyyy-MM-dd HH:mm:ss.SSS ZZZZZ" t)
                                (str/replace #" Z$" " UTC"))))
 
-(defmethod unprepare/unprepare-value [:oracle ZonedDateTime]
+(defmethod sql.qp/inline-value [:oracle ZonedDateTime]
   [_ t]
   (format "timestamp '%s'" (-> (t/format "yyyy-MM-dd HH:mm:ss.SSS VV" t)
                                (str/replace #" Z$" " UTC"))))
 
-(defmethod unprepare/unprepare-value [:oracle Instant]
+(defmethod sql.qp/inline-value [:oracle Instant]
   [driver t]
-  (unprepare/unprepare-value driver (t/zoned-date-time t (t/zone-id "UTC"))))
+  (sql.qp/inline-value driver (t/zoned-date-time t (t/zone-id "UTC"))))
 
 ;; Oracle doesn't really support boolean types so use bits instead (See #11592, similar issue for SQL Server)
 (defmethod driver.sql/->prepared-substitution [:oracle Boolean]