Skip to content

Commit

Permalink
Support JDBC explicitPrepare flag
Browse files Browse the repository at this point in the history
Support JDBC explicitPrepare flag
  • Loading branch information
lpoulain committed Nov 2, 2023
1 parent 7e384ae commit d361436
Show file tree
Hide file tree
Showing 9 changed files with 88 additions and 19 deletions.
4 changes: 3 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
2 changes: 1 addition & 1 deletion app_versions.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"trino": "426",
"trino": "431",
"clojure": "1.11.1.1262",
"metabase": "v1.47.2"
}
2 changes: 1 addition & 1 deletion drivers/starburst/deps.edn
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 6 additions & 0 deletions drivers/starburst/resources/metabase-plugin.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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)))
45 changes: 31 additions & 14 deletions drivers/starburst/src/metabase/driver/implementation/execute.clj
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -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]
Expand All @@ -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)
Expand Down
18 changes: 18 additions & 0 deletions drivers/starburst/src/metabase/driver/implementation/messages.clj
Original file line number Diff line number Diff line change
@@ -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.")
21 changes: 21 additions & 0 deletions drivers/starburst/src/metabase/driver/starburst.clj
Original file line number Diff line number Diff line change
Expand Up @@ -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})

Expand All @@ -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 |
;;; +----------------------------------------------------------------------------------------------------------------+
Expand Down
1 change: 1 addition & 0 deletions drivers/starburst/test/metabase/test/data/starburst.clj
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down

0 comments on commit d361436

Please sign in to comment.