Skip to content
Snippets Groups Projects
Unverified Commit e360b5d6 authored by Robert Roland's avatar Robert Roland Committed by GitHub
Browse files

Uppercase certain identifiers for snowflake (#12832)

Snowflake stores warehouse, database and schema names in uppper case.
When creating the connection details, uppercase them.

Adds a forced-US Locale upper-case function to avoids potential bugs with languages that don't use Latin characters

Resolves #9511

[ci snowflake]
parent 61048d8b
No related branches found
No related tags found
No related merge requests found
......@@ -24,6 +24,7 @@ driver:
- dbname
- name: db
required: true
display-name: Database name (case sensitive)
- name: regionid
display-name: Region ID
placeholder: my_region
......
......@@ -28,16 +28,26 @@
[i18n :refer [tru]]])
(:import [java.sql ResultSet Types]
[java.time OffsetDateTime ZonedDateTime]
metabase.util.honeysql_extensions.Identifier
net.snowflake.client.jdbc.SnowflakeSQLException))
metabase.util.honeysql_extensions.Identifier))
(driver/register! :snowflake, :parent #{:sql-jdbc ::legacy/use-legacy-classes-for-read-and-set})
(defmethod driver/humanize-connection-error-message :snowflake
[_ message]
(log/spy :error (type message))
(condp re-matches message
#"(?s).*Object does not exist.*$"
(driver.common/connection-error-messages :database-name-incorrect)
#"(?s).*" ; default - the Snowflake errors have a \n in them
message))
(defmethod sql-jdbc.conn/connection-details->spec :snowflake
[_ {:keys [account regionid], :as opts}]
(let [host (if regionid
(str account "." regionid)
account)]
account)
upcase-not-nil (fn [s] (when s (u/upper-case-en s)))]
;; it appears to be the case that their JDBC driver ignores `db` -- see my bug report at
;; https://support.snowflake.net/s/question/0D50Z00008WTOMCSA5/
(-> (merge {:classname "net.snowflake.client.jdbc.SnowflakeDriver"
......@@ -58,6 +68,9 @@
;; original version of the Snowflake driver incorrectly used `dbname` in the details fields instead of
;; `db`. If we run across `dbname`, correct our behavior
(set/rename-keys {:dbname :db})
;; see https://github.com/metabase/metabase/issues/9511
(update :warehouse upcase-not-nil)
(update :schema upcase-not-nil)
(dissoc :host :port :timezone)))
(sql-jdbc.common/handle-additional-options opts))))
......@@ -273,12 +286,8 @@
(and ((get-method driver/can-connect? :sql-jdbc) driver details)
(let [spec (sql-jdbc.conn/details->connection-spec-for-testing-connection driver details)
sql (format "SHOW OBJECTS IN DATABASE \"%s\";" db)]
(try
(jdbc/query spec sql)
true
(catch SnowflakeSQLException e
(log/error e (tru "Snowflake Database does not exist."))
false)))))
(jdbc/query spec sql)
true)))
(defmethod unprepare/unprepare-value [:snowflake OffsetDateTime]
[_ t]
......
......@@ -129,10 +129,10 @@
(is (= true
(can-connect? (:details (mt/db))))
"can-connect? should return true for normal Snowflake DB details")
(is (= false
(mt/suppress-output
(can-connect? (assoc (:details (mt/db)) :db (mt/random-name)))))
"can-connect? should return false for Snowflake databases that don't exist (#9041)"))))
(is (thrown? net.snowflake.client.jdbc.SnowflakeSQLException
(mt/suppress-output
(can-connect? (assoc (:details (mt/db)) :db (mt/random-name)))))
"can-connect? should throw for Snowflake databases that don't exist (#9511)"))))
(deftest report-timezone-test
(mt/test-driver :snowflake
......
......@@ -45,13 +45,15 @@
{:account (tx/db-test-env-var-or-throw :snowflake :account)
:user (tx/db-test-env-var-or-throw :snowflake :user)
:password (tx/db-test-env-var-or-throw :snowflake :password)
:warehouse (tx/db-test-env-var-or-throw :snowflake :warehouse)
;; this lowercasing this value is part of testing the fix for
;; https://github.com/metabase/metabase/issues/9511
:warehouse (u/lower-case-en (tx/db-test-env-var-or-throw :snowflake :warehouse))
;; SESSION parameters
:timezone "UTC"}
;; Snowflake JDBC driver ignores this, but we do use it in the `query-db-name` function in
;; `metabase.driver.snowflake`
(when (= context :db)
{:db (qualified-db-name database-name)})))
{:db (qualified-db-name (u/lower-case-en database-name))})))
;; Snowflake requires you identify an object with db-name.schema-name.table-name
(defmethod sql.tx/qualified-name-components :snowflake
......@@ -74,7 +76,7 @@
(jdbc/with-db-metadata [metadata db-spec]
;; for whatever dumb reason the Snowflake JDBC driver always returns these as uppercase despite us making them
;; all lower-case
(set (map str/lower-case (sql-jdbc.sync/get-catalogs metadata))))))
(set (map u/lower-case-en (sql-jdbc.sync/get-catalogs metadata))))))
(let [datasets (atom nil)]
(defn- existing-datasets []
......
......@@ -736,6 +736,14 @@
[^CharSequence s]
(.. s toString (toLowerCase (Locale/US))))
(defn upper-case-en
"Locale-agnostic version of `clojure.string/upper-case`.
`clojure.string/upper-case` uses the default locale in conversions, turning
`id` into `İD`, in the Turkish locale. This function always uses the
`Locale/US` locale."
[^CharSequence s]
(.. s toString (toUpperCase (Locale/US))))
(defn lower-case-map-keys
"Changes the keys of a given map to lower case."
[m]
......
......@@ -3,8 +3,9 @@
(:require [clojure.test :refer :all]
[clojure.tools.macro :as tools.macro]
[flatland.ordered.map :refer [ordered-map]]
[metabase.util :as u])
(:import java.util.Locale))
[metabase
[test :as mt]
[util :as u]]))
(defn- are+-message [expr arglist args]
(pr-str
......@@ -207,14 +208,14 @@
nil nil))
(deftest lower-case-en-test
(let [original-locale (Locale/getDefault)]
(try
(Locale/setDefault (Locale/forLanguageTag "tr"))
;; `(str/lower-case "ID")` returns "ıd" in the Turkish locale
(is (= "id"
(u/lower-case-en "ID")))
(finally
(Locale/setDefault original-locale)))))
(mt/with-locale "tr"
(is (= "id"
(u/lower-case-en "ID")))))
(deftest upper-case-en-test
(mt/with-locale "tr"
(is (= "ID"
(u/upper-case-en "id")))))
(deftest parse-currency-test
(are+ [s expected] (= expected
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment