diff --git a/src/metabase/api/meta/dataset.clj b/src/metabase/api/meta/dataset.clj
index 9950cb538af4668aa6cb15f141dab80ce738a707..b5e094628beaab7e73b42584473323f627f09ea2 100644
--- a/src/metabase/api/meta/dataset.clj
+++ b/src/metabase/api/meta/dataset.clj
@@ -5,6 +5,8 @@
[metabase.driver :as driver]))
(defendpoint POST "/" [:as {:keys [body]}]
- (driver/dataset-query body {:executed_by *current-user-id*}))
+ (let [{:keys [status] :as response} (driver/dataset-query body {:executed_by *current-user-id*})]
+ {:status (if (= status :completed) 200 400)
+ :body response}))
(define-routes)
diff --git a/src/metabase/driver.clj b/src/metabase/driver.clj
index 0b84d9120ad1f48267eb7ad4d68847f84d729864..d32e67612db03dacd26e579c63938d9087d2e4e0 100644
--- a/src/metabase/driver.clj
+++ b/src/metabase/driver.clj
@@ -135,9 +135,9 @@
(defn query-fail
"Save QueryExecution state and construct a failed query response"
- [query-execution msg]
- (let [updates {:status "failed"
- :error msg
+ [query-execution error-message]
+ (let [updates {:status :failed
+ :error error-message
:finished_at (util/new-sql-timestamp)
:running_time (- (System/currentTimeMillis) (:start_time_millis query-execution))}]
;; record our query execution and format response
@@ -146,7 +146,8 @@
(merge updates)
(save-query-execution)
;; this is just for the response for clien
- (assoc :row_count 0
+ (assoc :error error-message
+ :row_count 0
:data {:rows []
:cols []
:columns []}))))
diff --git a/src/metabase/driver/generic_sql/native.clj b/src/metabase/driver/generic_sql/native.clj
index 70f762ce1046f263cd6c9eeb8e2abec08e600ba0..6fdb8a7c5f0a7b1ed73904dd502e2ecf07d2608c 100644
--- a/src/metabase/driver/generic_sql/native.clj
+++ b/src/metabase/driver/generic_sql/native.clj
@@ -47,6 +47,7 @@
columns first-row)}})
(catch java.sql.SQLException e
{:status :failed
- :error (->> (.getMessage e) ; error message comes back like 'Column "ZID" not found; SQL statement: ... [error-code]
- (re-find #"^(.*);") ; the user already knows the SQL, and error code is meaningless
- second)}))) ; so just return the part of the exception that is relevant
+ :error (or (->> (.getMessage e) ; error message comes back like 'Column "ZID" not found; SQL statement: ... [error-code]' sometimes
+ (re-find #"^(.*);") ; the user already knows the SQL, and error code is meaningless
+ second) ; so just return the part of the exception that is relevant
+ (.getMessage e))})))
diff --git a/src/metabase/driver/generic_sql/query_processor.clj b/src/metabase/driver/generic_sql/query_processor.clj
index cfddf3737a9ea54894d23ce9199d86579cf77cb8..3d2526b8f20b79b4c6225accf330cf2cd650a57e 100644
--- a/src/metabase/driver/generic_sql/query_processor.clj
+++ b/src/metabase/driver/generic_sql/query_processor.clj
@@ -47,10 +47,17 @@
{:pre [(integer? (:database query)) ; double check that the query being passed is valid
(map? (:query query))
(= (name (:type query)) "query")]}
- (->> (process query)
- eval
- (post-process query)
- (annotate/annotate query)))
+ (try
+ (->> (process query)
+ eval
+ (post-process query)
+ (annotate/annotate query))
+ (catch java.sql.SQLException e
+ {:status :failed
+ :error (or (->> (.getMessage e) ; error message comes back like "Error message ... [status-code]" sometimes
+ (re-find #"(?s)(^.*)\s+\[[\d-]+\]$") ; status code isn't useful and makes unit tests hard to write so strip it off
+ second)
+ (.getMessage e))}))) ; (?s) = Pattern.DOTALL - tell regex `.` to match newline characters as well
(defn process-and-run
diff --git a/src/metabase/models/query_execution.clj b/src/metabase/models/query_execution.clj
index 6cfff0b19d176f5e65abb4dec8d53e5c04a19c14..fe0ca1deb2ec870e32fc746b0c0b555c9823430e 100644
--- a/src/metabase/models/query_execution.clj
+++ b/src/metabase/models/query_execution.clj
@@ -45,17 +45,18 @@
:result_rows
:query_id])
-(defmethod pre-insert QueryExecution [_ {:keys [json_query] :as query-execution}]
- (assoc query-execution :json_query (if (string? json_query) json_query
- (json/write-str json_query))))
+(defmethod pre-insert QueryExecution [_ {:keys [status json_query] :as query-execution}]
+ (cond-> (assoc query-execution :json_query (if (string? json_query) json_query
+ (json/write-str json_query)))
+ status (assoc :status (name status))))
(defmethod post-select QueryExecution [_ {:keys [query_id result_rows] :as query-execution}]
(-> query-execution
- (realize-json :json_query :result_data)
- (assoc* :row_count (or result_rows 0) ; sadly we have 2 ways to reference the row count :(
- :query (delay
- (check query_id 500 "Can't get execution: QueryExecution doesn't have a :query_id.")
- (sel :one Query :id query_id)))))
+ (realize-json :json_query :result_data)
+ (assoc* :row_count (or result_rows 0) ; sadly we have 2 ways to reference the row count :(
+ :query (delay
+ (check query_id 500 "Can't get execution: QueryExecution doesn't have a :query_id.")
+ (sel :one Query :id query_id)))))
(defn build-response
diff --git a/test/metabase/driver/generic_sql/query_processor_test.clj b/test/metabase/driver/generic_sql/query_processor_test.clj
index 25780c14f1b9a53419fea6693c02c4d48113c333..c24ec10463c18acafbd2ff5700e226f3c379be21 100644
--- a/test/metabase/driver/generic_sql/query_processor_test.clj
+++ b/test/metabase/driver/generic_sql/query_processor_test.clj
@@ -436,3 +436,18 @@
;; Rows come back like `[value timestamp]` but it is hard to compare timestamps directly since the values that come back are casted
;; to Dates and the exact value depends on the locale of the machine running the tests. So just drop the timestamps from the results.
(update-in [:data :rows] (partial map first))))
+
+
+;; # ERROR RESPONSES
+
+;; Check that we get an error response formatted the way we'd expect
+(expect
+ {:status :failed
+ :error "Column \"CHECKINS.NAME\" not found; SQL statement:\nSELECT \"CHECKINS\".* FROM \"CHECKINS\" WHERE (\"CHECKINS\".\"NAME\" = ?)"}
+ (process-and-run {:database @db-id
+ :type :query
+ :query {:source_table (table->id :checkins)
+ :filter ["=" (field->id :venues :name) 1] ; wrong Field
+ :aggregation ["rows"]
+ :breakout [nil]
+ :limit nil}}))