Merge pull request #8979 from metabase/gracefully-handle-normalization-failures

Gracefully handle normalization of invalid queries 

src/metabase/api/database.clj

@@ -70,8 +70,7 @@
 (defn- card-database-supports-nested-queries? [{{database-id :database} :dataset_query, :as card}]
   (when database-id
     (when-let [driver (driver/database-id->driver database-id)]
-      (and (driver/driver-supports? driver :nested-queries)
-           (mi/can-read? card)))))
+      (driver/driver-supports? driver :nested-queries))))
 
 (defn- card-has-ambiguous-columns?
   "We know a card has ambiguous columns if any of the columns that come back end in `_2` (etc.) because that's what
@@ -106,6 +105,7 @@
           :result_metadata [:not= nil] :archived false
           {:order-by [[:%lower.name :asc]]}) <>
     (filter card-database-supports-nested-queries? <>)
+    (filter mi/can-read? <>)
     (remove card-uses-unnestable-aggregation? <>)
     (remove card-has-ambiguous-columns? <>)
     (hydrate <> :collection)))

src/metabase/mbql/normalize.clj

@@ -200,7 +200,7 @@
               (let [tag-def (-> (normalize-tokens tag-def :ignore-path)
                                 (update :type mbql.u/normalize-token))]
                 (cond-> tag-def
-                  (:widget-type tag-def) (update :widget-type #(when % (mbql.u/normalize-token %)))))])))
+                  (:widget-type tag-def) (update :widget-type mbql.u/normalize-token)))])))
 
 (defn- normalize-query-parameter [param]
   (-> param

src/metabase/models/interface.clj

 (ns metabase.models.interface
   (:require [cheshire.core :as json]
             [clojure.core.memoize :as memoize]
+            [clojure.tools.logging :as log]
             [metabase.mbql.normalize :as normalize]
             [metabase.util :as u]
             [metabase.util
              [cron :as cron-util]
              [date :as du]
-             [encryption :as encryption]]
+             [encryption :as encryption]
+             [i18n :refer [tru]]]
             [schema.core :as s]
             [taoensso.nippy :as nippy]
             [toucan
@@ -59,11 +61,25 @@
 
 ;; `metabase-query` type is for *outer* queries like Card.dataset_query. Normalizes them on the way in & out
 (defn- maybe-normalize [query]
-  (when query (normalize/normalize query)))
+  (when query
+    (normalize/normalize query)))
+
+(defn- catch-normalization-exceptions
+  "Wraps normalization fn `f` and returns a version that gracefully handles Exceptions during normalization. When
+  invalid queries (etc.) come out of the Database, it's best we handle normalization failures gracefully rather than
+  letting the Exception cause the entire API call to fail because of one bad object. (See #8914 for more details.)"
+  [f]
+  (fn [query]
+    (try
+      (doall (f query))
+      (catch Throwable e
+        (log/error e (tru "Unable to normalize:") "\n"
+                   (u/pprint-to-str 'red query))
+        nil))))
 
 (models/add-type! :metabase-query
   :in  (comp json-in maybe-normalize)
-  :out (comp maybe-normalize json-out-with-keywordization))
+  :out (comp (catch-normalization-exceptions maybe-normalize) json-out-with-keywordization))
 
 ;; `metric-segment-definition` is, predicatbly, for Metric/Segment `:definition`s, which are just the inner MBQL query
 (defn- normalize-metric-segment-definition [definition]
@@ -73,7 +89,7 @@
 ;; For inner queries like those in Metric definitions
 (models/add-type! :metric-segment-definition
   :in  (comp json-in normalize-metric-segment-definition)
-  :out (comp normalize-metric-segment-definition json-out-with-keywordization))
+  :out (comp (catch-normalization-exceptions normalize-metric-segment-definition) json-out-with-keywordization))
 
 ;; For DashCard parameter lists
 (defn- normalize-parameter-mapping-targets [parameter-mappings]
@@ -83,14 +99,14 @@
 
 (models/add-type! :parameter-mappings
   :in  (comp json-in normalize-parameter-mapping-targets)
-  :out (comp normalize-parameter-mapping-targets json-out-with-keywordization))
+  :out (comp (catch-normalization-exceptions normalize-parameter-mapping-targets) json-out-with-keywordization))
 
 
 ;; json-set is just like json but calls `set` on it when coming out of the DB. Intended for storing things like a
 ;; permissions set
 (models/add-type! :json-set
   :in  json-in
-  :out #(when % (set (json-out-with-keywordization %))))
+  :out #(some-> % json-out-with-keywordization set))
 
 (models/add-type! :clob
   :in  identity

test/metabase/models/interface_test.clj

+(ns metabase.models.interface-test
+  (:require [cheshire.core :as json]
+            [expectations :refer [expect]]
+            [metabase.mbql.normalize :as normalize]
+            [metabase.test.util.log :as tu.log]
+            [toucan.models :as t.models]))
+
+;; let's make sure the `:metabase-query`/`:metric-segment-definition`/`:parameter-mappings` normalization functions
+;; respond gracefully to invalid stuff when pulling them out of the Database. See #8914
+
+(defn- type-fn [toucan-type in-or-out]
+  (-> @@#'t.models/type-fns toucan-type in-or-out))
+
+;; an empty template tag like the one below is invalid. Rather than potentially destroy an entire API response because
+;; of one malformed Card, dump the error to the logs and return nil.
+(expect
+  nil
+  (tu.log/suppress-output
+    ((type-fn :metabase-query :out)
+     (json/generate-string
+      {:database 1
+       :type     :native
+       :native   {:template-tags {"x" {}}}}))))
+
+;; on the other hand we should be a little more strict on the way and disallow you from saving the invalid stuff
+(expect
+  Exception
+  ((type-fn :metabase-query :in)
+   {:database 1
+    :type     :native
+    :native   {:template-tags {"x" {}}}}))
+
+;; `metric-segment-definition`s should avoid explosions coming out of the DB...
+(expect
+  nil
+  (tu.log/suppress-output
+    ((type-fn :metric-segment-definition :out)
+     (json/generate-string
+      {:filter 1000}))))
+
+;; ...but should still throw them coming in
+(expect
+  Exception
+  ((type-fn :metric-segment-definition :in)
+   {:filter 1000}))
+
+;; same for `:parameter-mappings`. I wasn't actually able to figure out any input that would make this fail, so cheat
+;; and override the `normalization-tokens` function to always throw an Exception so we can make sure the Toucan type
+;; fn handles the error gracefully
+(expect
+  nil
+  (tu.log/suppress-output
+    (with-redefs [normalize/normalize-tokens (fn [& _] (throw (Exception. "BARF")))]
+      (doall
+       ((type-fn :parameter-mappings :out)
+        (json/generate-string
+         [{:target [:dimension [:field-id "ABC"]]}]))))))
+
+;; should not eat Exceptions if normalization barfs when saving
+(expect
+  Exception
+  (with-redefs [normalize/normalize-tokens (fn [& _] (throw (Exception. "BARF")))]
+    ((type-fn :parameter-mappings :in)
+     [{:target [:dimension [:field-id "ABC"]]}])))