From 230d6871d5e8cbea8a7b41c60d791ce740762e30 Mon Sep 17 00:00:00 2001
From: Cam Saul <cammsaul@gmail.com>
Date: Mon, 13 May 2019 13:08:22 -0700
Subject: [PATCH] Fix sql-jdbc QP error responses not being properly reported
:warning:
---
src/metabase/driver/sql_jdbc/execute.clj | 10 ++-
test/metabase/driver/postgres_test.clj | 61 ++++++++++--------
.../metabase/driver/sql_jdbc/execute_test.clj | 64 ++++++++++++++++++-
test/metabase/query_processor/test_util.clj | 9 ++-
4 files changed, 109 insertions(+), 35 deletions(-)
diff --git a/src/metabase/driver/sql_jdbc/execute.clj b/src/metabase/driver/sql_jdbc/execute.clj
index ea49bb7e11d..954d571ee42 100644
--- a/src/metabase/driver/sql_jdbc/execute.clj
+++ b/src/metabase/driver/sql_jdbc/execute.clj
@@ -180,10 +180,7 @@
;; This is what does the real work of canceling the query. We aren't checking the result of
;; `query-future` but this will cause an exception to be thrown, saying the query has been cancelled.
(.cancel stmt)
- (throw e))
- (catch Exception e
- (u/ignore-exceptions (.cancel stmt))
- e)))))
+ (throw e))))))
(defn- run-query
"Run the query itself."
@@ -248,8 +245,9 @@
(defn- set-timezone!
"Set the timezone for the current connection."
- [driver settings connection]
- (let [timezone (u/prog1 (:report-timezone settings)
+ {:arglists '([driver settings connection])}
+ [driver {:keys [report-timezone]} connection]
+ (let [timezone (u/prog1 report-timezone
(assert (re-matches #"[A-Za-z\/_]+" <>)))
format-string (set-timezone-sql driver)
sql (format format-string (str \' timezone \'))]
diff --git a/test/metabase/driver/postgres_test.clj b/test/metabase/driver/postgres_test.clj
index c49aa40faff..138666efa1f 100644
--- a/test/metabase/driver/postgres_test.clj
+++ b/test/metabase/driver/postgres_test.clj
@@ -23,7 +23,7 @@
[data :as data]
[util :as tu]]
[metabase.test.data
- [datasets :refer [expect-with-driver]]
+ [datasets :as datasets]
[interface :as tx]]
[toucan.db :as db]
[toucan.util.test :as tt]))
@@ -62,7 +62,7 @@
:user "camsaul"}))
;; Verify that we identify JSON columns and mark metadata properly during sync
-(expect-with-driver :postgres
+(datasets/expect-with-driver :postgres
:type/SerializedJSON
(data/with-temp-db
[_
@@ -84,7 +84,7 @@
[#uuid "84ed434e-80b4-41cf-9c88-e334427104ae"]]]])
;; Check that we can load a Postgres Database with a :type/UUID
-(expect-with-driver :postgres
+(datasets/expect-with-driver :postgres
[{:name "id", :base_type :type/Integer}
{:name "user_id", :base_type :type/UUID}]
(->> (data/dataset metabase.driver.postgres-test/with-uuid
@@ -95,21 +95,21 @@
;; Check that we can filter by a UUID Field
-(expect-with-driver :postgres
+(datasets/expect-with-driver :postgres
[[2 #uuid "4652b2e7-d940-4d55-a971-7e484566663e"]]
(rows (data/dataset metabase.driver.postgres-test/with-uuid
(data/run-mbql-query users
{:filter [:= $user_id "4652b2e7-d940-4d55-a971-7e484566663e"]}))))
;; check that a nil value for a UUID field doesn't barf (#2152)
-(expect-with-driver :postgres
+(datasets/expect-with-driver :postgres
[]
(rows (data/dataset metabase.driver.postgres-test/with-uuid
(data/run-mbql-query users
{:filter [:= $user_id nil]}))))
;; Check that we can filter by a UUID for SQL Field filters (#7955)
-(expect-with-driver :postgres
+(datasets/expect-with-driver :postgres
[[#uuid "4f01dcfd-13f7-430c-8e6f-e505c0851027" 1]]
(data/dataset metabase.driver.postgres-test/with-uuid
(rows (qp/process-query {:database (data/id)
@@ -132,7 +132,7 @@
["four_loko"]
["ouija_board"]]]])
-(expect-with-driver :postgres
+(datasets/expect-with-driver :postgres
{:columns ["id" "dotted.name"]
:rows [[1 "toucan_cage"]
[2 "four_loko"]
@@ -153,7 +153,7 @@
{:field-name "bird_id", :base-type :type/Integer, :fk :birds}]
[["Cam" 1]]]])
-(expect-with-driver :postgres
+(datasets/expect-with-driver :postgres
{:columns ["name" "name_2"]
:rows [["Cam" "Rasta"]]}
(-> (data/dataset metabase.driver.postgres-test/duplicate-names
@@ -171,7 +171,7 @@
;; Filtering by inet columns should add the appropriate SQL cast, e.g. `cast('192.168.1.1' AS inet)` (otherwise this
;; wouldn't work)
-(expect-with-driver :postgres
+(datasets/expect-with-driver :postgres
[[1]]
(rows (data/dataset metabase.driver.postgres-test/ip-addresses
(data/run-mbql-query addresses
@@ -200,7 +200,7 @@
;; Check that we properly fetch materialized views.
;; As discussed in #2355 they don't come back from JDBC `DatabaseMetadata` so we have to fetch them manually.
-(expect-with-driver :postgres
+(datasets/expect-with-driver :postgres
{:tables #{(default-table-result "test_mview")}}
(do
(drop-if-exists-and-create-db! "materialized_views_test")
@@ -213,7 +213,7 @@
(driver/describe-database :postgres database)))))
;; Check that we properly fetch foreign tables.
-(expect-with-driver :postgres
+(datasets/expect-with-driver :postgres
{:tables (set (map default-table-result ["foreign_table" "local_table"]))}
(do
(drop-if-exists-and-create-db! "fdw_test")
@@ -232,7 +232,7 @@
;; make sure that if a view is dropped and recreated that the original Table object is marked active rather than a new
;; one being created (#3331)
-(expect-with-driver :postgres
+(datasets/expect-with-driver :postgres
[{:name "angry_birds", :active true}]
(let [details (tx/dbdef->connection-details :postgres :db {:database-name "dropped_views_test"})
spec (sql-jdbc.conn/connection-details->spec :postgres details)
@@ -270,18 +270,18 @@
{:query "SELECT current_setting('TIMEZONE') AS timezone;"}))))
;; check that if we set report-timezone to US/Pacific that the session timezone is in fact US/Pacific
-(expect-with-driver :postgres
+(datasets/expect-with-driver :postgres
"US/Pacific"
(get-timezone-with-report-timezone "US/Pacific"))
;; check that we can set it to something else: America/Chicago
-(expect-with-driver :postgres
+(datasets/expect-with-driver :postgres
"America/Chicago"
(get-timezone-with-report-timezone "America/Chicago"))
;; ok, check that if we try to put in a fake timezone that the query still reëxecutes without a custom timezone. This
;; should give us the same result as if we didn't try to set a timezone at all
-(expect-with-driver :postgres
+(datasets/expect-with-driver :postgres
(get-timezone-with-report-timezone nil)
(get-timezone-with-report-timezone "Crunk Burger"))
@@ -299,12 +299,12 @@
:dbname "cool"
:additional-options "prepareThreshold=0"}))
-(expect-with-driver :postgres
+(datasets/expect-with-driver :postgres
"UTC"
(tu/db-timezone-id))
;; Make sure we're able to fingerprint TIME fields (#5911)
-(expect-with-driver :postgres
+(datasets/expect-with-driver :postgres
#{#metabase.models.field.FieldInstance{:name "start_time", :fingerprint {:global {:distinct-count 1
:nil% 0.0}
:type {:type/DateTime {:earliest "1970-01-01T22:00:00.000Z", :latest "1970-01-01T22:00:00.000Z"}}}}
@@ -366,14 +366,14 @@
(f database)))
;; check that we can actually fetch the enum types from a DB
-(expect-with-driver :postgres
+(datasets/expect-with-driver :postgres
#{(keyword "bird type") :bird_status}
(do-with-enums-db
(fn [db]
(#'postgres/enum-types :postgres db))))
;; check that describe-table properly describes the database & base types of the enum fields
-(expect-with-driver :postgres
+(datasets/expect-with-driver :postgres
{:name "birds"
:fields #{{:name "name",
:database-type "varchar"
@@ -390,7 +390,7 @@
(driver/describe-table :postgres db {:name "birds"}))))
;; check that when syncing the DB the enum types get recorded appropriately
-(expect-with-driver :postgres
+(datasets/expect-with-driver :postgres
#{{:name "name", :database_type "varchar", :base_type :type/Text}
{:name "type", :database_type "bird type", :base_type :type/PostgresEnum}
{:name "status", :database_type "bird_status", :base_type :type/PostgresEnum}}
@@ -402,12 +402,12 @@
;; check that values for enum types get wrapped in appropriate CAST() fn calls in `->honeysql`
-(expect-with-driver :postgres
- {:name :cast, :args ["toucan" (keyword "bird type")]}
- (sql.qp/->honeysql :postgres [:value "toucan" {:database_type "bird type", :base_type :type/PostgresEnum}]))
+(datasets/expect-with-driver :postgres
+ {:name :cast, :args ["toucan" (keyword "bird type")]}
+ (sql.qp/->honeysql :postgres [:value "toucan" {:database_type "bird type", :base_type :type/PostgresEnum}]))
;; End-to-end check: make sure everything works as expected when we run an actual query
-(expect-with-driver :postgres
+(datasets/expect-with-driver :postgres
{:rows [["Rasta" "good bird" "toucan"]]
:native_form {:query (str "SELECT \"public\".\"birds\".\"name\" AS \"name\","
" \"public\".\"birds\".\"status\" AS \"status\","
@@ -430,7 +430,7 @@
(select-keys [:rows :native_form]))))))
;; make sure schema/table/field names with hyphens in them work correctly (#8766)
-(expect-with-driver :postgres
+(datasets/expect-with-driver :postgres
[["Bird Hat"]]
(metabase.driver/with-driver :postgres
[{:name "angry_birds", :active true}]
@@ -453,3 +453,14 @@
:type :query
:query {:source-table (db/select-one-id Table :name "presents-and-gifts")}})
rows))))))
+
+;; If the DB throws an exception, is it properly returned by the query processor? Is it status :failed? (#9942)
+(datasets/expect-with-driver :postgres
+ {:status :failed
+ :class org.postgresql.util.PSQLException
+ :error "ERROR: column \"adsasdasd\" does not exist\n Position: 20"}
+ (-> (qp/process-query
+ {:database (data/id)
+ :type :native
+ :native {:query "SELECT adsasdasd;"}})
+ (select-keys [:status :class :error])))
diff --git a/test/metabase/driver/sql_jdbc/execute_test.clj b/test/metabase/driver/sql_jdbc/execute_test.clj
index 6c201b7900e..af3d81165f4 100644
--- a/test/metabase/driver/sql_jdbc/execute_test.clj
+++ b/test/metabase/driver/sql_jdbc/execute_test.clj
@@ -7,11 +7,16 @@
[query-processor :as qp]
[util :as u]]
[metabase.driver.sql-jdbc-test :as sql-jdbc-test]
+ [metabase.driver.sql-jdbc.execute :as sql-jdbc.execute]
+ [metabase.query-processor
+ [store :as qp.store]
+ [test-util :as qp.tu]]
[metabase.test
[data :as data]
[util :as tu]]
[metabase.test.data.datasets :as datasets]
- [metabase.test.util.log :as tu.log])
+ [metabase.test.util.log :as tu.log]
+ [schema.core :as s])
(:import [java.sql PreparedStatement ResultSet]))
(defn- do-with-max-rows [f]
@@ -185,3 +190,60 @@
(query-thunk))
(catch Throwable e
(throw e)))))))))
+
+;; If the DB throws an exception, is it properly returned by the query processor? Is it status :failed? (#9942)
+(tu/expect-schema
+ {:status (s/eq :failed)
+ :class (s/eq java.lang.Exception)
+ :error (s/eq "Column \"ADSASDASD\" not found")
+ :stacktrace [s/Str]
+ :query s/Any}
+ (qp/process-query
+ {:database (data/id)
+ :type :native
+ :native {:query "SELECT adsasdasd;"}}))
+
+;; do we run query with a timezone if one is present in the Settings?
+(defn- ran-with-timezone? [driver query]
+ (let [ran-with-timezone? (promise)
+ timezone (promise)]
+ (with-redefs [sql-jdbc.execute/run-query-without-timezone
+ (let [orig @#'sql-jdbc.execute/run-query-without-timezone]
+ (fn [& args]
+ (deliver ran-with-timezone? false)
+ (deliver timezone nil)
+ (apply orig args)))
+
+ sql-jdbc.execute/run-query-with-timezone
+ (let [orig @#'sql-jdbc.execute/run-query-with-timezone]
+ (fn [& args]
+ (deliver ran-with-timezone? true)
+ (apply orig args)))
+
+ sql-jdbc.execute/set-timezone!
+ (let [orig @#'sql-jdbc.execute/set-timezone!]
+ (fn [driver {:keys [report-timezone], :as settings} connection]
+ (deliver timezone report-timezone)
+ (orig driver settings connection)))]
+ (qp.tu/with-everything-store
+ (qp.store/store-database! (data/db))
+ (sql-jdbc.execute/execute-query driver query))
+ {:ran-with-timezone? (u/deref-with-timeout ran-with-timezone? 1000)
+ :timezone (u/deref-with-timeout timezone 1000)})))
+
+(expect
+ {:ran-with-timezone? false, :timezone nil}
+ (ran-with-timezone?
+ :h2
+ {:database (data/id)
+ :type :native
+ :native {:query "SELECT * FROM VENUES LIMIT 1;"}}))
+
+(expect
+ {:ran-with-timezone? true, :timezone "US/Pacific"}
+ (ran-with-timezone?
+ :h2
+ {:database (data/id)
+ :type :native
+ :native {:query "SELECT * FROM VENUES LIMIT 1;"}
+ :settings {:report-timezone "US/Pacific"}}))
diff --git a/test/metabase/query_processor/test_util.clj b/test/metabase/query_processor/test_util.clj
index fa3f2138948..8de6987a1e0 100644
--- a/test/metabase/query_processor/test_util.clj
+++ b/test/metabase/query_processor/test_util.clj
@@ -12,16 +12,19 @@
[f]
(with-redefs [qp.store/table (fn [table-id]
(or (get-in @@#'qp.store/*store* [:tables table-id])
- (db/select-one (vec (cons Table qp.store/table-columns-to-fetch)), :id table-id)))
+ (db/select-one (into [Table] qp.store/table-columns-to-fetch), :id table-id)))
qp.store/field (fn [field-id]
(or (get-in @@#'qp.store/*store* [:fields field-id])
- (db/select-one (vec (cons Field qp.store/field-columns-to-fetch)), :id field-id)))]
+ (db/select-one (into [Field] qp.store/field-columns-to-fetch), :id field-id)))]
(qp.store/with-store
(f))))
(defmacro with-everything-store
"When testing a specific piece of middleware, you often need to load things into the QP store, but doing so can be
tedious. This macro swaps out the normal QP store backend with one that fetches Tables and Fields from the DB
- on-demand, making tests a lot nicer to write."
+ on-demand, making tests a lot nicer to write.
+
+ You still need to store the Database yourself, because the getter function takes no args; we don't know which DB
+ you'd want to fetch."
[& body]
`(do-with-everything-store (fn [] ~@body)))
--
GitLab