Don't log a full exception when fetching a dashboard that has a stray param (#33613)

e2e/test/scenarios/dashboard-filters/reproductions/25322-loading-list-values.cy.spec.js

@@ -6,7 +6,7 @@ import {
 } from "e2e/support/helpers";
 import { SAMPLE_DATABASE } from "e2e/support/cypress_sample_database";
 
-const { ORDERS, ORDERS_ID } = SAMPLE_DATABASE;
+const { PEOPLE, PEOPLE_ID } = SAMPLE_DATABASE;
 
 const parameterDetails = {
   name: "Location",
@@ -17,8 +17,8 @@ const parameterDetails = {
 };
 
 const questionDetails = {
-  name: "Orders",
-  query: { "source-table": ORDERS_ID },
+  name: "People",
+  query: { "source-table": PEOPLE_ID },
 };
 
 const dashboardDetails = {
@@ -58,7 +58,7 @@ const createDashboard = () => {
                 {
                   card_id,
                   parameter_id: parameterDetails.id,
-                  target: ["dimension", ["field", ORDERS.STATE, null]],
+                  target: ["dimension", ["field", PEOPLE.STATE, null]],
                 },
               ],
             },

src/metabase/models/params.clj

@@ -111,16 +111,20 @@
   "Parse a Card parameter `target` form, which looks something like `[:dimension [:field-id 100]]`, and return the Field
   ID it references (if any)."
   [target card]
-  (let [target (mbql.normalize/normalize-tokens target :ignore-path)]
+  (let [target (mbql.normalize/normalize target)]
     (when (mbql.u/is-clause? :dimension target)
-      (let [[_ dimension] target]
-        (try
-          (unwrap-field-or-expression-clause
-           (if (mbql.u/is-clause? :template-tag dimension)
-             (template-tag->field-form dimension card)
-             dimension))
-          (catch Throwable e
-            (log/error e "Could not find matching Field ID for target:" target)))))))
+      (let [[_ dimension] target
+            field-form    (if (mbql.u/is-clause? :template-tag dimension)
+                            (template-tag->field-form dimension card)
+                            dimension)]
+        ;; Being extra safe here since we've got many reports on this cause loading dashboard to fail
+        ;; for unknown reasons. See #8917
+        (if field-form
+          (try
+           (unwrap-field-or-expression-clause field-form)
+           (catch Exception e
+             (log/error e "Failed unwrap field form" field-form)))
+          (log/error "Could not find matching field clause for target:" target))))))
 
 (defn- pk-fields
   "Return the `fields` that are PK Fields."

test/metabase/api/dashboard_test.clj

@@ -47,6 +47,7 @@
    [metabase.query-processor.streaming.test-util :as streaming.test-util]
    [metabase.server.middleware.util :as mw.util]
    [metabase.test :as mt]
+   [metabase.test.fixtures :as fixtures]
    [metabase.util :as u]
    [ring.util.codec :as codec]
    [toucan2.core :as t2]
@@ -55,6 +56,10 @@
 
 (set! *warn-on-reflection* true)
 
+(use-fixtures
+  :once
+  (fixtures/initialize :test-users))
+
 ;;; +----------------------------------------------------------------------------------------------------------------+
 ;;; |                                              Helper Fns & Macros                                               |
 ;;; +----------------------------------------------------------------------------------------------------------------+
@@ -527,6 +532,29 @@
                  (into {} (for [[field-id m] response]
                             [field-id (update m :values (partial take 3))])))))))))
 
+(deftest fetch-a-dashboard-with-param-linked-to-a-field-filter-that-is-not-existed
+  (testing "when fetching a dashboard that has a param linked to a field filter that no longer exists, we shouldn't throw an error (#15494)"
+    (mt/with-temp
+      [:model/Card          {card-id :id} {:name "Native card"
+                                           :database_id   (mt/id)
+                                           :dataset_query (mt/native-query
+                                                           {:query "SELECT category FROM products LIMIT 10;"})
+                                           :dataset       true}
+       :model/Dashboard     {dash-id :id} {:parameters [{:name      "Text"
+                                                         :slug      "text"
+                                                         :id        "_TEXT_"
+                                                         :type      :string/=
+                                                         :sectionId "string"}]}
+       :model/DashboardCard {}            {:dashboard_id       dash-id
+                                           :card_id            card-id
+                                           :parameter_mappings [{:parameter_id "_TEXT_"
+                                                                 :card_id      card-id
+                                                                 :target       [:dimension [:template-tag "not-existed-filter"]]}]}]
+      (is (= (repeat 2 [:error nil
+                        "Could not find matching field clause for target: [:dimension [:template-tag not-existed-filter]]"])
+             (mt/with-log-messages-for-level ['metabase.models.params :error]
+               (is (some? (mt/user-http-request :rasta :get 200 (str "dashboard/" dash-id))))))))))
+
 (deftest dashboard-param-link-to-a-field-without-full-field-values-test
   (testing "GET /api/dashboard/:id"
     (t2.with-temp/with-temp
@@ -1782,6 +1810,37 @@
                       (t2/select [DashboardCard :size_x :size_y :col :row :parameter_mappings :visualization_settings]
                                  :dashboard_id dashboard-id)))))))))
 
+(deftest can-update-card-parameter-with-legacy-field-and-expression-test
+  (testing "PUT /api/dashboard/:id/cards accepts legacy field as parameter's target"
+    (mt/with-temp [:model/Dashboard {dashboard-id :id} {}
+                   :model/Card      {card-id :id}      {}]
+      (let [resp (:cards (mt/user-http-request :rasta :put 200 (format "dashboard/%d/cards" dashboard-id)
+                                               {:cards        [{:id                     -1
+                                                                :card_id                card-id
+                                                                :row                    0
+                                                                :col                    0
+                                                                :size_x                 4
+                                                                :size_y                 4
+                                                                :parameter_mappings     [{:parameter_id "abc"
+                                                                                          :card_id card-id
+                                                                                          :target [:dimension [:field-id (mt/id :venues :id)]]}]}]}))]
+        (is (some? (t2/select-one :model/DashboardCard (:id (first resp))))))))
+
+  (testing "PUT /api/dashboard/:id/cards accepts expression as parammeter's target"
+    (mt/with-temp [:model/Dashboard {dashboard-id :id} {}
+                   :model/Card      {card-id :id}      {:dataset_query (mt/mbql-query venues {:expressions {"A" [:+ (mt/$ids $venues.price) 1]}})}]
+      (let [resp (:cards (mt/user-http-request :rasta :put 200 (format "dashboard/%d/cards" dashboard-id)
+                                               {:cards        [{:id                     -1
+                                                                :card_id                card-id
+                                                                :row                    0
+                                                                :col                    0
+                                                                :size_x                 4
+                                                                :size_y                 4
+                                                                :parameter_mappings     [{:parameter_id "abc"
+                                                                                          :card_id card-id
+                                                                                          :target [:dimension [:expression "A"]]}]}]}))]
+        (is (some? (t2/select-one :model/DashboardCard (:id (first resp)))))))))
+
 (deftest new-dashboard-card-with-additional-series-test
   (mt/with-temp [Dashboard {dashboard-id :id} {}
                  Card      {card-id :id} {}