Don't fail entire query if we don't know how to convert value to literal (#11046)

* Don't fail entire query if we don't know how to convert result to literal

* Code cleanup 

src/metabase/driver/sql/util/unprepare.clj

@@ -4,13 +4,15 @@
   TODO - since this is no longer strictly a 'util' namespace (most `:sql-jdbc` drivers need to implement one or
   methods from here) let's rename this `metabase.driver.sql.unprepare` when we get a chance."
   (:require [clojure.string :as str]
+            [clojure.tools.logging :as log]
             [honeysql
              [core :as hsql]
              [format :as hformat]]
             [metabase.driver :as driver]
             [metabase.util
              [date :as du]
-             [honeysql-extensions :as hx]])
+             [honeysql-extensions :as hx]
+             [i18n :refer [trs]]])
   (:import java.sql.Time
            java.util.Date))
 
@@ -22,17 +24,28 @@
     [(driver/the-initialized-driver driver) (class value)])
   :hierarchy #'driver/hierarchy)
 
-(defmethod unprepare-value [:sql nil] [_ _]
+(defmethod unprepare-value :default
+  [_ value]
+  ;; it's better return a slightly broken SQL query with a probably incorrect string representation of the value than
+  ;; to have the entire QP run fail because of an unknown type.
+  (log/warn (trs "Don''t know how to unprepare values of class {0}" (.getName (class value))))
+  (str value))
+
+(defmethod unprepare-value [:sql nil]
+  [_ _]
   "NULL")
 
-(defmethod unprepare-value [:sql String] [_ value]
+(defmethod unprepare-value [:sql String]
+  [_ value]
   ;; escape single-quotes like Cam's String -> Cam''s String
   (str \' (str/replace value "'" "''") \'))
 
-(defmethod unprepare-value [:sql Boolean] [_ value]
+(defmethod unprepare-value [:sql Boolean]
+  [_ value]
   (if value "TRUE" "FALSE"))
 
-(defmethod unprepare-value [:sql Number] [_ value]
+(defmethod unprepare-value [:sql Number]
+  [_ value]
   (str value))
 
 (defn unprepare-date-with-iso-8601-fn
@@ -42,12 +55,14 @@
   (hformat/to-sql
    (hsql/call iso-8601-fn (hx/literal (du/date->iso-8601 value)))))
 
-(defmethod unprepare-value [:sql Date] [_ value]
+(defmethod unprepare-value [:sql Date]
+  [_ value]
   (unprepare-date-with-iso-8601-fn :timestamp value))
 
 ;; default impl for Time is just converting the Time literal to a `1970-01-01T<time>` Timestamp and passing to impl
 ;; for `Date`, then wrapping entire expression in `time()`
-(defmethod unprepare-value [:sql Time] [driver value]
+(defmethod unprepare-value [:sql Time]
+  [driver value]
   (hformat/to-sql (hx/->time (hsql/raw (unprepare-value driver (du/->Timestamp value))))))
 
 

src/metabase/driver/sql_jdbc/execute.clj

@@ -277,7 +277,7 @@
 ;;; ------------------------------------------------- execute-query --------------------------------------------------
 
 (defn execute-query
-  "Process and run a native (raw SQL) QUERY."
+  "Process and run a native (raw SQL) `query`."
   [driver {settings :settings, query :native, :as outer-query}]
   (let [query (assoc query
                 :remark   (qputil/query->remark outer-query)

test/metabase/test/data/dataset_definitions.clj

@@ -104,7 +104,7 @@
          tabledef
          :field-definitions
          concat
-         [(tx/map->FieldDefinition {:field-name "null_only_date" :base-type :type/Date})]))
+         [(tx/map->FieldDefinition {:field-name "null_only_date", :base-type :type/Date})]))
       :rows
       (fn [rows]
         (for [row rows]

test/metabase/test/util.clj

@@ -265,7 +265,8 @@
                          (setting/get setting-k))]
     (try
       (setting/set! setting-k value)
-      (f)
+      (t/testing (format "Setting %s = %s" (keyword setting-k) value)
+        (f))
       (finally
         (setting/set! setting-k original-value)))))
 
@@ -278,8 +279,7 @@
   [[setting-k value & more :as bindings] & body]
   (if (empty? bindings)
     `(do ~@body)
-    (let [body `(t/testing ~(format "Setting %s = %s" (keyword setting-k) value)
-                  (do-with-temporary-setting-value ~(keyword setting-k) ~value (fn [] ~@body)))]
+    (let [body `(do-with-temporary-setting-value ~(keyword setting-k) ~value (fn [] ~@body))]
       (if (seq more)
         `(with-temporary-setting-values ~more ~body)
         body))))