diff --git a/src/metabase/api/database.clj b/src/metabase/api/database.clj
index 864ebd8b05bdfe8b109bec3511b89d739cf74239..3af72e3331fcdb135720485a9a6b70c9e4f56b30 100644
--- a/src/metabase/api/database.clj
+++ b/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)))
diff --git a/src/metabase/mbql/normalize.clj b/src/metabase/mbql/normalize.clj
index fd44ee2c21f481f1d1e3b66aa093ecd5b5fbcadd..86b7bd74a7282a3d54a4067bde2ed94cdce68bbc 100644
--- a/src/metabase/mbql/normalize.clj
+++ b/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
diff --git a/src/metabase/models/interface.clj b/src/metabase/models/interface.clj
index 8335c2f5ddacbf56dcdddda35953386c0ec87b72..205615e76658ed7cae42433f99beb9334d64159a 100644
--- a/src/metabase/models/interface.clj
+++ b/src/metabase/models/interface.clj
@@ -1,12 +1,14 @@
(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
diff --git a/test/metabase/models/interface_test.clj b/test/metabase/models/interface_test.clj
new file mode 100644
index 0000000000000000000000000000000000000000..d56f17a27f36a21f3fd556c4eba9f7f1f342866d
--- /dev/null
+++ b/test/metabase/models/interface_test.clj
@@ -0,0 +1,64 @@
+(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"]]}])))