Merge pull request #9791 from metabase/middleware-cleanup

QP Middleware cleanup

src/metabase/query_processor.clj

@@ -135,7 +135,6 @@
       log-query/log-initial-query
       ;; TODO - bind `*query*` here ?
       cache/maybe-return-cached-results
-      log-query/log-results-metadata
       validate/validate-query
       normalize/normalize
       catch-exceptions/catch-exceptions

src/metabase/query_processor/middleware/catch_exceptions.clj

@@ -2,31 +2,41 @@
   "Middleware for catching exceptions thrown by the query processor and returning them in a friendlier format."
   (:require [metabase.util :as u]
             schema.utils)
-  (:import [schema.utils NamedError ValidationError]))
+  (:import clojure.lang.ExceptionInfo
+           [schema.utils NamedError ValidationError]))
 
 (def ^:dynamic ^:private *add-preprocessed-queries?* true)
 
-(defn- fail [{query-type :type, :as query}, ^Throwable e, & [additional-info]]
-  (merge {:status     :failed
-          :class      (class e)
-          :error      (or (.getMessage e) (str e))
-          :stacktrace (u/filtered-stacktrace e)
-          ;; TODO - removing this stuff is not really needed anymore since `:database` is just the ID and not the
-          ;; entire map including `:details`
-          :query      (dissoc query :database :driver)}
-         ;; add the fully-preprocessed and native forms to the error message for MBQL queries, since they're extremely
-         ;; useful for debugging purposes. Since generating them requires us to recursively run the query processor,
-         ;; make sure we can skip adding them if we end up back here so we don't recurse forever
-         (when (and (= (keyword query-type) :query)
-                    *add-preprocessed-queries?*)
-           (binding [*add-preprocessed-queries?* false]
-             {:preprocessed (u/ignore-exceptions
-                              ((resolve 'metabase.query-processor/query->preprocessed) query))
-              :native       (u/ignore-exceptions
-                              ((resolve 'metabase.query-processor/query->native) query))}))
-         (when-let [data (ex-data e)]
-           {:ex-data (dissoc data :schema)})
-         additional-info))
+(defmulti ^:private format-exception
+  "Format an Exception thrown by the Query Processor."
+  {:arglists '([query e])}
+  (fn [_ e]
+    (class e)))
+
+(defmethod format-exception Throwable
+  [{query-type :type, :as query}, ^Throwable e]
+  (merge
+   {:status     :failed
+    :class      (class e)
+    :error      (or (.getMessage e) (str e))
+    :stacktrace (u/filtered-stacktrace e)
+    ;; TODO - removing this stuff is not really needed anymore since `:database` is just the ID and not the
+    ;; entire map including `:details`
+    :query      (dissoc query :database :driver)}
+   ;; add the fully-preprocessed and native forms to the error message for MBQL queries, since they're extremely
+   ;; useful for debugging purposes. Since generating them requires us to recursively run the query processor,
+   ;; make sure we can skip adding them if we end up back here so we don't recurse forever
+   (when (and (= (keyword query-type) :query)
+              *add-preprocessed-queries?*)
+     ;; obviously we do not want to get core.async channels back for preprocessed & native, so run the preprocessing
+     ;; steps synchronously
+     (let [query (dissoc query :async?)]
+       (binding [*add-preprocessed-queries?* false]
+         {:preprocessed (u/ignore-exceptions
+                          ((resolve 'metabase.query-processor/query->preprocessed) query))
+          :native       (u/ignore-exceptions
+                          ((resolve 'metabase.query-processor/query->native) query))})))))
+
 
 (defn- explain-schema-validation-error
   "Return a nice error message to explain the schema validation error."
@@ -61,15 +71,22 @@
                 msg)))
           explanation))))
 
+(defmethod format-exception ExceptionInfo
+  [query e]
+  (let [{error :error, error-type :type, :as data} (ex-data e)]
+    (merge
+     ((get-method format-exception Throwable) query e)
+     (when-let [error-msg (and (= error-type :schema.core/error)
+                               (explain-schema-validation-error error))]
+       {:error error-msg})
+     {:ex-data (dissoc data :schema)})))
+
+
 (defn catch-exceptions
   "Middleware for catching exceptions thrown by the query processor and returning them in a normal format."
   [qp]
   (fn [query]
-    (try (qp query)
-         (catch clojure.lang.ExceptionInfo e
-           (let [{error :error, error-type :type, :as data} (ex-data e)]
-             (fail query e (when-let [error-msg (and (= error-type :schema.core/error)
-                                                     (explain-schema-validation-error error))]
-                             {:error error-msg}))))
-         (catch Throwable e
-           (fail query e)))))
+    (try
+      (qp query)
+      (catch Throwable e
+        (format-exception query e)))))

src/metabase/query_processor/middleware/log.clj

@@ -2,7 +2,6 @@
   "Middleware for logging a query before it is processed.
    (Various other middleware functions log the query as well in different stages.)"
   (:require [clojure.tools.logging :as log]
-            [medley.core :as m]
             [metabase.query-processor.interface :as i]
             [metabase.util :as u]))
 
@@ -16,16 +15,3 @@
   "Middleware for logging a query when it is very first encountered, before it is expanded."
   [qp]
   (comp qp log-initial-query*))
-
-
-(defn- log-results-metadata* [results]
-  (u/prog1 results
-    (when-not i/*disable-qp-logging*
-      (log/debug "Result Metadata:\n"
-                 (u/pprint-to-str 'blue (for [col (get-in <> [:data :cols])]
-                                          (m/filter-vals (complement nil?) col)))))))
-
-(defn log-results-metadata
-  "Middleware that logs the column metadata that comes back with the results."
-  [qp]
-  (comp log-results-metadata* qp))