From b46a6592e700c9f69ad5f633913fcdd87fa674a8 Mon Sep 17 00:00:00 2001
From: Case Nelson <case@metabase.com>
Date: Wed, 4 Sep 2024 16:31:26 -0600
Subject: [PATCH] fix: sqlserver handle uniqueidentifier uuids (#47544)
* fix: sqlserver handle uniqueidentifier uuids
Fixes #46148
Include sqlserver in `uuid-type` handling as its `uniqueidentifier` type
stores uuids.
* Don't be so precise with varchar size
* Add seam for drivers to cast to text type
* Fix arg order
---
.../athena/test/metabase/test/data/athena.clj | 1 +
.../oracle/src/metabase/driver/oracle.clj | 4 +++
.../src/metabase/driver/sqlserver.clj | 29 +++++++++++++++++--
.../test/metabase/test/data/sqlserver.clj | 1 +
src/metabase/driver/sql/query_processor.clj | 11 +++++--
test/metabase/driver/sql_jdbc_test.clj | 5 +++-
test/metabase/test/data/h2.clj | 1 +
7 files changed, 47 insertions(+), 5 deletions(-)
diff --git a/modules/drivers/athena/test/metabase/test/data/athena.clj b/modules/drivers/athena/test/metabase/test/data/athena.clj
index 20531d04335..db4b4ea7d74 100644
--- a/modules/drivers/athena/test/metabase/test/data/athena.clj
+++ b/modules/drivers/athena/test/metabase/test/data/athena.clj
@@ -182,6 +182,7 @@
:type/Float "DOUBLE"
:type/Integer "INT"
:type/Text "STRING"
+ :type/UUID "UUID"
:type/Time "TIMESTAMP"}]
(defmethod sql.tx/field-base-type->sql-type [:athena base-type] [_ _] sql-type))
diff --git a/modules/drivers/oracle/src/metabase/driver/oracle.clj b/modules/drivers/oracle/src/metabase/driver/oracle.clj
index cc9db027322..ac662866c46 100644
--- a/modules/drivers/oracle/src/metabase/driver/oracle.clj
+++ b/modules/drivers/oracle/src/metabase/driver/oracle.clj
@@ -476,6 +476,10 @@
[_ bool]
[:inline (if bool 1 0)])
+(defmethod sql.qp/->honeysql [:sql ::sql.qp/cast-to-text]
+ [driver [_ expr]]
+ (sql.qp/->honeysql driver [::sql.qp/cast expr "varchar"]))
+
(defmethod driver/humanize-connection-error-message :oracle
[_ message]
;; if the connection error message is caused by the assertion above checking whether sid or service-name is set,
diff --git a/modules/drivers/sqlserver/src/metabase/driver/sqlserver.clj b/modules/drivers/sqlserver/src/metabase/driver/sqlserver.clj
index bf9e0eb60a3..cbf55a5cff4 100644
--- a/modules/drivers/sqlserver/src/metabase/driver/sqlserver.clj
+++ b/modules/drivers/sqlserver/src/metabase/driver/sqlserver.clj
@@ -20,18 +20,27 @@
[metabase.lib.util.match :as lib.util.match]
[metabase.query-processor.interface :as qp.i]
[metabase.query-processor.timezone :as qp.timezone]
+ [metabase.util :as u]
[metabase.util.honey-sql-2 :as h2x]
[metabase.util.log :as log])
(:import
(java.sql Connection ResultSet Time)
- (java.time LocalDate LocalDateTime LocalTime OffsetDateTime OffsetTime ZonedDateTime)
- (java.time.format DateTimeFormatter)))
+ (java.time
+ LocalDate
+ LocalDateTime
+ LocalTime
+ OffsetDateTime
+ OffsetTime
+ ZonedDateTime)
+ (java.time.format DateTimeFormatter)
+ [java.util UUID]))
(set! *warn-on-reflection* true)
(driver/register! :sqlserver, :parent :sql-jdbc)
(doseq [[feature supported?] {:case-sensitivity-string-filter-options false
+ :uuid-type true
:convert-timezone true
:datetime-diff true
:index-info true
@@ -620,6 +629,10 @@
args)]
((get-method sql.qp/->honeysql [:sql-jdbc op]) driver clause)))))
+(defmethod sql.qp/->honeysql [:sqlserver ::sql.qp/cast-to-text]
+ [driver [_ expr]]
+ (sql.qp/->honeysql driver [::sql.qp/cast expr "varchar(256)"]))
+
(defmethod driver/db-default-timezone :sqlserver
[driver database]
(sql-jdbc.execute/do-with-connection-with-options
@@ -782,6 +795,18 @@
[driver ps i t]
(sql-jdbc.execute/set-parameter driver ps i (t/local-time (t/with-offset-same-instant t (t/zone-offset 0)))))
+(defmethod sql-jdbc.execute/read-column-thunk [:sqlserver java.sql.Types/CHAR]
+ [driver ^java.sql.ResultSet rs ^java.sql.ResultSetMetaData rsmeta ^Integer i]
+ (condp = (u/lower-case-en (.getColumnTypeName rsmeta i))
+ "uniqueidentifier"
+ (fn read-column-as-UUID []
+ (when-let [s (.getObject rs i)]
+ (try
+ (UUID/fromString s)
+ (catch IllegalArgumentException _
+ s))))
+ ((get-method sql-jdbc.execute/read-column-thunk [:sql-jdbc java.sql.Types/CHAR]) driver rs rsmeta i)))
+
;; instead of default `microsoft.sql.DateTimeOffset`
(defmethod sql-jdbc.execute/read-column-thunk [:sqlserver microsoft.sql.Types/DATETIMEOFFSET]
[_ ^ResultSet rs _ ^Integer i]
diff --git a/modules/drivers/sqlserver/test/metabase/test/data/sqlserver.clj b/modules/drivers/sqlserver/test/metabase/test/data/sqlserver.clj
index 6328577e065..2d52635f413 100644
--- a/modules/drivers/sqlserver/test/metabase/test/data/sqlserver.clj
+++ b/modules/drivers/sqlserver/test/metabase/test/data/sqlserver.clj
@@ -17,6 +17,7 @@
:type/Decimal "DECIMAL"
:type/Float "FLOAT"
:type/Integer "INTEGER"
+ :type/UUID "UNIQUEIDENTIFIER"
;; TEXT is considered deprecated -- see
;; https://msdn.microsoft.com/en-us/library/ms187993.aspx
:type/Text "VARCHAR(1024)"
diff --git a/src/metabase/driver/sql/query_processor.clj b/src/metabase/driver/sql/query_processor.clj
index 594476a8103..2b5560f0db3 100644
--- a/src/metabase/driver/sql/query_processor.clj
+++ b/src/metabase/driver/sql/query_processor.clj
@@ -1332,7 +1332,7 @@
(not (uuid? (->honeysql driver arg)))
;; Check for inlined values
(not (= (:database-type (h2x/type-info (->honeysql driver arg))) "uuid")))
- [::cast field "varchar"]
+ [::cast-to-text field]
field))
(mu/defn- maybe-cast-uuid-for-text-compare
@@ -1340,13 +1340,20 @@
Comparing UUID fields against with these operations requires casting as the right side will have `%` for `LIKE` operations."
[field]
(if (uuid-field? field)
- [::cast field "varchar"]
+ [::cast-to-text field]
field))
(defmethod ->honeysql [:sql ::cast]
[driver [_ expr database-type]]
(h2x/maybe-cast database-type (->honeysql driver expr)))
+(defmethod ->honeysql [:sql ::cast-to-text]
+ [driver [_ expr]]
+ ;; Oracle does not support text type,
+ ;; sqlserver limits varchar to 30 in casts,
+ ;; athena cannot cast uuid to bounded varchars
+ (->honeysql driver [::cast expr "text"]))
+
(defmethod ->honeysql [:sql :starts-with]
[driver [_ field arg options]]
(like-clause (->honeysql driver (maybe-cast-uuid-for-text-compare field))
diff --git a/test/metabase/driver/sql_jdbc_test.clj b/test/metabase/driver/sql_jdbc_test.clj
index fd768ee9d69..c4bdf789bb6 100644
--- a/test/metabase/driver/sql_jdbc_test.clj
+++ b/test/metabase/driver/sql_jdbc_test.clj
@@ -14,6 +14,7 @@
[metabase.query-processor.compile :as qp.compile]
[metabase.test :as mt]
[metabase.test.data.dataset-definition-test :as dataset-definition-test]
+ [metabase.test.data.sql :as sql.tx]
[metabase.util :as u]
[toucan2.core :as t2]
[toucan2.tools.with-temp :as t2.with-temp]))
@@ -213,7 +214,9 @@
(mt/sql-jdbc-drivers)
(mt/normal-drivers-with-feature :uuid-type))
(let [uuid (random-uuid)
- uuid-query (mt/native-query {:query (format "select cast('%s' as uuid) as x" uuid)})
+ uuid-query (mt/native-query {:query (format "select cast('%s' as %s) as x"
+ uuid
+ (sql.tx/field-base-type->sql-type driver/*driver* :type/UUID))})
results (qp/process-query uuid-query)
result-metadata (get-in results [:data :results_metadata :columns])
col-metadata (first result-metadata)]
diff --git a/test/metabase/test/data/h2.clj b/test/metabase/test/data/h2.clj
index a8c83118789..6e438401721 100644
--- a/test/metabase/test/data/h2.clj
+++ b/test/metabase/test/data/h2.clj
@@ -29,6 +29,7 @@
:type/Float "FLOAT"
:type/Integer "INTEGER"
:type/Text "VARCHAR"
+ :type/UUID "UUID"
:type/Time "TIME"}]
(defmethod sql.tx/field-base-type->sql-type [:h2 base-type] [_ _] database-type))
--
GitLab