Merge pull request #268 from metabase/better_errors_for_qp

Structured QP now returns useful error responses

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)

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 []}))))

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))})))

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

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

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}}))