From d3614361fa2ddffbc185bbd05a1ac1c4f13f400a Mon Sep 17 00:00:00 2001 From: lpoulain Date: Mon, 30 Oct 2023 09:46:51 -0400 Subject: [PATCH] Support JDBC explicitPrepare flag Support JDBC explicitPrepare flag --- Makefile | 4 +- app_versions.json | 2 +- drivers/starburst/deps.edn | 2 +- .../starburst/resources/metabase-plugin.yaml | 6 +++ .../driver/implementation/connectivity.clj | 8 +++- .../driver/implementation/execute.clj | 45 +++++++++++++------ .../driver/implementation/messages.clj | 18 ++++++++ .../src/metabase/driver/starburst.clj | 21 +++++++++ .../test/metabase/test/data/starburst.clj | 1 + 9 files changed, 88 insertions(+), 19 deletions(-) create mode 100644 drivers/starburst/src/metabase/driver/implementation/messages.clj diff --git a/Makefile b/Makefile index 5fb75a5..12766b7 100644 --- a/Makefile +++ b/Makefile @@ -78,7 +78,9 @@ update_deps_files: test: start_trino_if_missing link_to_driver update_deps_files @echo "Testing Starburst driver..." - cd $(makefile_dir)/metabase/; DRIVERS=starburst MB_STARBURST_TEST_PORT=$(trino_port) clojure -X:dev:drivers:drivers-dev:test + cd $(makefile_dir)/metabase/; \ + DRIVERS=starburst MB_STARBURST_TEST_PORT=$(trino_port) clojure -X:dev:drivers:drivers-dev:test; \ + DRIVERS=starburst MB_STARBURST_TEST_PORT=$(trino_port) clojure -J-DexplicitPrepare=true -X:dev:drivers:drivers-dev:test build: clone_metabase_if_missing update_deps_files link_to_driver front_end driver diff --git a/app_versions.json b/app_versions.json index 3a22793..4ed2b9a 100644 --- a/app_versions.json +++ b/app_versions.json @@ -1,5 +1,5 @@ { - "trino": "426", + "trino": "431", "clojure": "1.11.1.1262", "metabase": "v1.47.2" } diff --git a/drivers/starburst/deps.edn b/drivers/starburst/deps.edn index 608dfaf..d6c508f 100644 --- a/drivers/starburst/deps.edn +++ b/drivers/starburst/deps.edn @@ -15,7 +15,7 @@ ["src" "resources"] :deps - {io.trino/trino-jdbc {:mvn/version "426"}} + {io.trino/trino-jdbc {:mvn/version "431"}} ;; build the driver with clojure -X:build :aliases diff --git a/drivers/starburst/resources/metabase-plugin.yaml b/drivers/starburst/resources/metabase-plugin.yaml index ef5b88f..d0d4de4 100644 --- a/drivers/starburst/resources/metabase-plugin.yaml +++ b/drivers/starburst/resources/metabase-plugin.yaml @@ -42,6 +42,12 @@ driver: type: boolean required: false description: Impersonate end user when running queries + - name: prepared-optimized + display-name: Optimized prepared statements + type: boolean + required: false + default: true + description: Use Starburst optimized prepared statements (version 418+) - advanced-options-start - merge: - additional-options diff --git a/drivers/starburst/src/metabase/driver/implementation/connectivity.clj b/drivers/starburst/src/metabase/driver/implementation/connectivity.clj index dfca653..ef6a8ed 100644 --- a/drivers/starburst/src/metabase/driver/implementation/connectivity.clj +++ b/drivers/starburst/src/metabase/driver/implementation/connectivity.clj @@ -109,6 +109,9 @@ (:impersonation details-map) (not= (get (deref api/*current-user*) :email) (:user details-map)))) +(defn assoc-if [coll key value condition] + (if condition (assoc coll key value) coll)) + (defmethod sql-jdbc.conn/connection-details->spec :starburst [_ details-map] (let [props (-> details-map @@ -121,7 +124,8 @@ (update :kerberos-delegation bool->str) (assoc :SSL (:ssl details-map)) (assoc :source "Starburst Metabase 4.0.0") - (assoc :clientInfo (if (:impersonation details-map) "impersonate:true" "")) + (assoc-if :clientInfo "impersonate:true" (:impersonation details-map)) + (assoc-if :explicitPrepare "false" (:prepared-optimized details-map)) (dissoc (if (remove-role? details-map) :roles :test)) ;; remove any Metabase specific properties that are not recognized by the Trino JDBC driver, which is @@ -136,6 +140,6 @@ :source :applicationNamePrefix ::accessToken :SSL :SSLVerification :SSLKeyStorePath :SSLKeyStorePassword :SSLKeyStoreType :SSLTrustStorePath :SSLTrustStorePassword :SSLTrustStoreType :SSLUseSystemTrustStore :extraCredentials :roles :sessionProperties :externalAuthentication :externalAuthenticationTokenCache :disableCompression - :assumeLiteralNamesInMetadataCallsForNonConformingClients] + :explicitPrepare :assumeLiteralNamesInMetadataCallsForNonConformingClients] (keys kerb-props->url-param-names))))] (jdbc-spec props))) diff --git a/drivers/starburst/src/metabase/driver/implementation/execute.clj b/drivers/starburst/src/metabase/driver/implementation/execute.clj index c42866d..9b2f7af 100644 --- a/drivers/starburst/src/metabase/driver/implementation/execute.clj +++ b/drivers/starburst/src/metabase/driver/implementation/execute.clj @@ -17,6 +17,7 @@ [clojure.tools.logging :as log] [java-time :as t] [metabase.driver.implementation.sync :refer [starburst-type->base-type]] + [metabase.driver.implementation.messages :as msg] [metabase.driver.sql-jdbc.execute :as sql-jdbc.execute] [metabase.driver.sql.parameters.substitution :as sql.params.substitution] [metabase.query-processor.timezone :as qp.timezone] @@ -168,10 +169,13 @@ ; Metabase tests require a specific error when an invalid number of parameters are passed (defn handle-execution-error [e] - (log/fatalf (.getMessage e)) - (if (clojure.string/includes? (.getMessage e) "Incorrect number of parameters") - (throw (Exception. "It looks like we got more parameters than we can handle, remember that parameters cannot be used in comments or as identifiers.")) - (throw e))) + (let [message (.getMessage e)] + (cond + (clojure.string/includes? message "Expecting: 'USING'") + (throw (Exception. (str message msg/STARBURST_MAYBE_INCOMPATIBLE))) + (clojure.string/includes? message "Incorrect number of parameters") + (throw (Exception. msg/TOO_MANY_PARAMETERS)) + :else (throw e)))) (defmethod sql-jdbc.execute/prepared-statement :starburst [driver ^Connection conn ^String sql params] @@ -187,16 +191,29 @@ (.setFetchDirection stmt ResultSet/FETCH_FORWARD) (catch Throwable e (log/debug e (trs "Error setting prepared statement fetch direction to FETCH_FORWARD")))) - (sql-jdbc.execute/set-parameters! driver stmt params) - (proxy [java.sql.PreparedStatement] [] - (executeQuery [] - (try - (let [rs (.executeQuery stmt)] - (remove-impersonation conn) - rs) - (catch Throwable e (handle-execution-error e)))) - (setMaxRows [nb] (.setMaxRows stmt nb)) - (close [] (.close stmt))) + (let [ps (proxy [java.sql.PreparedStatement] [] + (executeQuery [] + (try + (let [rs (.executeQuery stmt)] + (remove-impersonation conn) + rs) + (catch Throwable e (handle-execution-error e)))) + (setMaxRows [nb] (.setMaxRows stmt nb)) + (setObject + ([index obj] (.setObject stmt index obj)) + ([index obj type] (.setObject stmt index obj type))) + (setTime + ([index val] (.setTime stmt index val)) + ([index val cal] (.setTime stmt index val cal))) + (setTimestamp + ([index val] (.setTimestamp stmt index val)) + ([index val cal] (.setTimestamp stmt index val cal))) + (setDate + ([index val] (.setDate stmt index val)) + ([index val cal] (.setDate stmt index val cal))) + (close [] (.close stmt)))] + (sql-jdbc.execute/set-parameters! driver ps params) + ps) (catch Throwable e (remove-impersonation conn) (.close stmt) diff --git a/drivers/starburst/src/metabase/driver/implementation/messages.clj b/drivers/starburst/src/metabase/driver/implementation/messages.clj new file mode 100644 index 0000000..8197793 --- /dev/null +++ b/drivers/starburst/src/metabase/driver/implementation/messages.clj @@ -0,0 +1,18 @@ +;; +;; Licensed under the Apache License, Version 2.0 (the "License"); +;; you may not use this file except in compliance with the License. +;; You may obtain a copy of the License at + +;; http://www.apache.org/licenses/LICENSE-2.0 + +;; Unless required by applicable law or agreed to in writing, software +;; distributed under the License is distributed on an "AS IS" BASIS, +;; WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +;; See the License for the specific language governing permissions and +;; limitations under the License. +;; +(ns metabase.driver.implementation.messages "Starburst messages") + +(def STARBURST_INCOMPATIBLE_WITH_OPTIMIZED_PREPARED "Your version of Starburst does not seem to be recent enough to support the 'Optimized prepared statements' option. Make sure to use Requires Starburst Galaxy or Starburst Enterprise 418+") +(def STARBURST_MAYBE_INCOMPATIBLE ". If the database has the 'Optimized prepared statements' option on, make sure to use Starburst Galaxy or Starburst Enterprise 418+") +(def TOO_MANY_PARAMETERS "It looks like we got more parameters than we can handle, remember that parameters cannot be used in comments or as identifiers.") diff --git a/drivers/starburst/src/metabase/driver/starburst.clj b/drivers/starburst/src/metabase/driver/starburst.clj index a12943d..7cf77b3 100644 --- a/drivers/starburst/src/metabase/driver/starburst.clj +++ b/drivers/starburst/src/metabase/driver/starburst.clj @@ -14,8 +14,13 @@ (ns metabase.driver.starburst "Starburst driver." (:require [metabase.driver :as driver] + [clojure.java.jdbc :as jdbc] + [clojure.tools.logging :as log] + [metabase.util.ssh :as ssh] + [metabase.driver.sql-jdbc.connection :as connection] [metabase.query-processor.util :as qp.util] [metabase.public-settings :as public-settings] + [metabase.driver.implementation.messages :as msg] [metabase.driver.sql-jdbc.execute.legacy-impl :as sql-jdbc.legacy])) (driver/register! :starburst, :parent #{::sql-jdbc.legacy/use-legacy-classes-for-read-and-set}) @@ -35,6 +40,22 @@ (format-field "dashboardID" dashboard-id) (format-field "cardID" card-id))) +(defn handle-execution-error + [e details] + (let [message (.getMessage e) + execute-immediate (get details :prepared-optimized false)] + (cond + (and (clojure.string/includes? message "Expecting: 'USING'") execute-immediate) + (throw (Exception. msg/STARBURST_INCOMPATIBLE_WITH_OPTIMIZED_PREPARED)) + :else (throw e)))) + +(defmethod driver/can-connect? :starburst + [driver details] + (try + (connection/with-connection-spec-for-testing-connection [jdbc-spec [driver details]] + (connection/can-connect-with-spec? jdbc-spec)) + (catch Throwable e (handle-execution-error e details)))) + ;;; +----------------------------------------------------------------------------------------------------------------+ ;;; | Load implemetation files | ;;; +----------------------------------------------------------------------------------------------------------------+ diff --git a/drivers/starburst/test/metabase/test/data/starburst.clj b/drivers/starburst/test/metabase/test/data/starburst.clj index c41b993..aee28e0 100644 --- a/drivers/starburst/test/metabase/test/data/starburst.clj +++ b/drivers/starburst/test/metabase/test/data/starburst.clj @@ -61,6 +61,7 @@ :port (tx/db-test-env-var :starburst :port "8082") :user (tx/db-test-env-var-or-throw :starburst :user "metabase") :additional-options (tx/db-test-env-var :starburst :additional-options nil) + :prepared-optimized (tx/db-test-env-var :starburst :prepared-optimized (not= (System/getProperty "explicitPrepare" "false") "true")) :ssl (tx/db-test-env-var :starburst :ssl "false") :kerberos (tx/db-test-env-var :starburst :kerberos "false") :kerberos-principal (tx/db-test-env-var :starburst :kerberos-principal nil)