[serdes] extract tabId in click_behavior (#51588) (#51613)

also, somewhat relatedly, create `elide-fk` function to skip pieces of data holding FKs to deleted targets Co-authored-by: Alexander Solovyov <alexander@solovyov.net>
metabase · Dec 21, 2024 · 2f8c560 · 2f8c560
1 parent 97e8cae
commit 2f8c560
Show file tree

Hide file tree

Showing 7 changed files with 70 additions and 46 deletions.
diff --git a/enterprise/backend/test/metabase_enterprise/serialization/v2/load_test.clj b/enterprise/backend/test/metabase_enterprise/serialization/v2/load_test.clj
@@ -1,5 +1,6 @@
 (ns ^:mb/once metabase-enterprise.serialization.v2.load-test
   (:require
+   [cheshire.core :as json]
    [clojure.string :as str]
    [clojure.test :refer :all]
    [java-time.api :as t]
@@ -362,6 +363,7 @@
           field3s    (atom nil)
           dash1s     (atom nil)
           dash2s     (atom nil)
+          tab2s      (atom nil)
           card1s     (atom nil)
           dashcard1s (atom nil)
           user1s     (atom nil)
@@ -371,6 +373,7 @@
           field2d    (atom nil)
           user1d     (atom nil)
           dash1d     (atom nil)
+          tab2d      (atom nil)
           card1d     (atom nil)
           dashcard1d (atom nil)
           db2d       (atom nil)
@@ -389,6 +392,7 @@
             (reset! user1s   (ts/create! User  :first_name "Tom" :last_name "Scholz" :email "tom@bost.on"))
             (reset! dash1s   (ts/create! Dashboard :name "My Dashboard" :collection_id (:id @coll1s) :creator_id (:id @user1s)))
             (reset! dash2s   (ts/create! Dashboard :name "Linked dashboard" :collection_id (:id @coll1s) :creator_id (:id @user1s)))
+            (reset! tab2s    (ts/create! :model/DashboardTab :name "Tab for dash2" :dashboard_id (:id @dash2s) :position 0))
             (let [columns           [{:name     "SOME_FIELD"
                                       :fieldRef [:field (:id @field1s) nil]
                                       :enabled  true}
@@ -419,11 +423,12 @@
                                               :table.cell_column  "sum"
                                               :table.columns      columns
                                               :column_settings
-                                              {(str "[\"ref\",[\"field\"," (:id @field1s) ",null]]")
+                                              {(json/encode [:ref [:field (:id @field1s) nil]])
                                                {:click_behavior {:type     "link"
                                                                  :linkType "dashboard"
-                                                                 :targetId (:id @dash2s)}}
-                                               (str "[\"ref\",[\"field\"," (:id @field2s) ",null]]")
+                                                                 :targetId (:id @dash2s)
+                                                                 :tabId    (:id @tab2s)}}
+                                               (json/encode [:ref [:field (:id @field2s) nil]])
                                                {:column_title "Locus"
                                                 :click_behavior
                                                 {:type     "link"
@@ -433,7 +438,7 @@
                                                  {mapping-id {:id     mapping-id
                                                               :source {:type "column" :id "Category_ID" :name "Category ID"}
                                                               :target {:type "dimension" :id mapping-id :dimension mapping-dimension}}}}}
-                                               (str "[\"ref\",[\"field\"," (:id @field3s) ",null]]")
+                                               (json/encode [:ref [:field (:id @field3s) nil]])
                                                {:click_behavior
                                                 {:type     "link"
                                                  :linkType "question"
@@ -482,19 +487,22 @@
                                     :column_settings
                                     {"[\"ref\",[\"field\",[\"my-db\",null,\"orders\",\"invoice\"],null]]" {:column_title "Locus"}}}
                       dimension    [:dimension [:field ["my-db" nil "orders" "invoice"] {:source-field ["my-db" nil "orders" "subtotal"]}]]
-                      dimension-id "[\"dimension\",[\"fk->\",[\"field\",[\"my-db\",null,\"orders\",\"subtotal\"],null],[\"field\",[\"my-db\",null,\"orders\",\"invoice\"],null]]]"
+                      dimension-id (json/encode [:dimension [:fk->
+                                                             [:field [:my-db nil :orders :subtotal] nil]
+                                                             [:field [:my-db nil :orders :invoice] nil]]])
                       exp-dashcard (-> exp-card
                                        (assoc :click_behavior {:type     "link"
                                                                :linkType "question"
                                                                :targetId (:entity_id @card1s)})
                                        (assoc-in [:column_settings
-                                                  "[\"ref\",[\"field\",[\"my-db\",null,\"orders\",\"subtotal\"],null]]"
+                                                  (json/encode [:ref [:field [:my-db nil :orders :subtotal] nil]])
                                                   :click_behavior]
                                                  {:type     "link"
                                                   :linkType "dashboard"
-                                                  :targetId (:entity_id @dash2s)})
+                                                  :targetId (:entity_id @dash2s)
+                                                  :tabId    [(:entity_id @dash2s) (:entity_id @tab2s)]})
                                        (assoc-in [:column_settings
-                                                  "[\"ref\",[\"field\",[\"my-db\",null,\"orders\",\"invoice\"],null]]"
+                                                  (json/encode [:ref [:field [:my-db nil :orders :invoice] nil]])
                                                   :click_behavior]
                                                  {:type     "link"
                                                   :linkType "question"
@@ -505,7 +513,7 @@
                                                     :source {:type "column" :id "Category_ID" :name "Category ID"}
                                                     :target {:type "dimension" :id dimension-id :dimension dimension}}}})
                                        (assoc-in [:column_settings
-                                                  "[\"ref\",[\"field\",[\"my-db\",null,\"orders\",\"discount\"],null]]"
+                                                  (json/encode [:ref [:field [:my-db nil :orders :discount] nil]])
                                                   :click_behavior]
                                                  {:type "link"
                                                   :linkType "question"
@@ -538,6 +546,7 @@
             (reset! field1d    (t2/select-one Field :table_id (:id @table1d) :name "subtotal"))
             (reset! field2d    (t2/select-one Field :table_id (:id @table1d) :name "invoice"))
             (reset! dash1d     (t2/select-one Dashboard :name "My Dashboard"))
+            (reset! tab2d      (t2/select-one :model/DashboardTab :name "Tab for dash2"))
             (reset! card1d     (t2/select-one Card :name "The Card"))
             (reset! dashcard1d (t2/select-one DashboardCard :card_id (:id @card1d) :dashboard_id (:id @dash1d)))
 
@@ -559,7 +568,13 @@
               (is (= [{:parameter_id "deadbeef"
                        :card_id      (:id @card1d)
                        :target       [:dimension [:field (:id @field1d) {:source-field (:id @field2d)}]]}]
-                     (:parameter_mappings @dashcard1d))))))))))
+                     (:parameter_mappings @dashcard1d))))
+            (testing "DashboardTab was exported even though he wasn't mentioned by click_behavior"
+              (is (= (:entity_id @tab2s) (:entity_id @tab2d)))
+              (let [settings (-> @dashcard1d :visualization_settings :column_settings)
+                    setting  (get settings (json/encode [:ref [:field (:id @field1d) nil]]))]
+                (is (= (:id @tab2d)
+                       (-> setting :click_behavior :tabId)))))))))))
 
 (deftest timelines-test
   (testing "timelines"

diff --git a/..._usage_analytics/dashboards/Zf1jknwhPhazxIIJ2X6mX_dashboard_subscriptions_and_alerts.yaml b/..._usage_analytics/dashboards/Zf1jknwhPhazxIIJ2X6mX_dashboard_subscriptions_and_alerts.yaml
@@ -64,7 +64,7 @@ dashcards:
               target:
                 id: 6b3da96f
                 type: parameter
-          tabId: 1
+          tabId: ["DHMhMa1FYxiyIgM7_xdgR", "VLzpbzPlShrECFp-dG4pH"]
           targetId: DHMhMa1FYxiyIgM7_xdgR
           type: link
       '["name","name"]':
@@ -115,7 +115,7 @@ dashcards:
               target:
                 id: 6b3da96f
                 type: parameter
-          tabId: 1
+          tabId: ["DHMhMa1FYxiyIgM7_xdgR", "VLzpbzPlShrECFp-dG4pH"]
           targetId: DHMhMa1FYxiyIgM7_xdgR
           type: link
   serdes/meta:

diff --git a/...R8k-QddHWA7_47umn_usage_analytics/dashboards/jm7KgY6IuS6pQjkBZ7WUI_question_overview.yaml b/...R8k-QddHWA7_47umn_usage_analytics/dashboards/jm7KgY6IuS6pQjkBZ7WUI_question_overview.yaml
@@ -357,7 +357,7 @@ dashcards:
               target:
                 id: 6b3da96f
                 type: parameter
-          tabId: 1
+          tabId: ["DHMhMa1FYxiyIgM7_xdgR", "VLzpbzPlShrECFp-dG4pH"]
           targetId: DHMhMa1FYxiyIgM7_xdgR
           type: link
   serdes/meta:

diff --git a/...8R8k-QddHWA7_47umn_usage_analytics/dashboards/vFnGZMNN2K_KW1I0B52bq_metabase_metrics.yaml b/...8R8k-QddHWA7_47umn_usage_analytics/dashboards/vFnGZMNN2K_KW1I0B52bq_metabase_metrics.yaml
@@ -383,7 +383,7 @@ dashcards:
               target:
                 id: 6b3da96f
                 type: parameter
-          tabId: 1
+          tabId: ["DHMhMa1FYxiyIgM7_xdgR", "VLzpbzPlShrECFp-dG4pH"]
           targetId: DHMhMa1FYxiyIgM7_xdgR
           type: link
         column_title: Person Name
@@ -472,7 +472,7 @@ dashcards:
               target:
                 id: 6b3da96f
                 type: parameter
-          tabId: 1
+          tabId: ["DHMhMa1FYxiyIgM7_xdgR", "VLzpbzPlShrECFp-dG4pH"]
           targetId: DHMhMa1FYxiyIgM7_xdgR
           type: link
         column_title: Person Name
@@ -489,7 +489,7 @@ dashcards:
               target:
                 id: 6b3da96f
                 type: parameter
-          tabId: 1
+          tabId: ["DHMhMa1FYxiyIgM7_xdgR", "VLzpbzPlShrECFp-dG4pH"]
           targetId: DHMhMa1FYxiyIgM7_xdgR
           type: link
       ? '["ref",["field",["Internal Metabase Database","public","v_users","full_name"],{"base-type":"type/Text","join-alias":"Question 1"}]]'

diff --git a/src/metabase/models/serialization.clj b/src/metabase/models/serialization.clj
@@ -849,14 +849,15 @@
         (= (count path) 1) (first path)
         :else              path))))
 
-(defn- maybe-export-fk
-  "Exactly like the above `*export-fk*`, except returns `nil` if the target was not found"
-  [id model]
-  (try (*export-fk* id model)
-       (catch clojure.lang.ExceptionInfo e
-         (when-not (= (::type (ex-data e)) :target-not-found)
-           (throw e))
-         nil)))
+(defmacro ^:private fk-elide
+  "If a call to `*export-fk*` inside of this fails, do not export the whole data structure"
+  [& body]
+  `(try
+     ~@body
+     (catch clojure.lang.ExceptionInfo e#
+       (when-not (= (::type (ex-data e#)) :target-not-found)
+         (throw e#))
+       nil)))
 
 (defn ^:dynamic ^::cache *import-fk*
   "Given an identifier, and the model it represents (symbol, name or IModel), looks up the corresponding
@@ -1339,17 +1340,17 @@
       json/generate-string))
 
 (defn- export-viz-click-behavior-link
-  [{:keys [linkType type] old-target-id :targetId :as click-behavior}]
-  (if-not (= type "link")
-    click-behavior
-    ;; if the card doesn't exist anymore, just remove the entire click behavior
-    (when-let [new-target-id (maybe-export-fk old-target-id (link-card-model->toucan-model linkType))]
-      (assoc click-behavior :targetId new-target-id))))
+  [{:keys [linkType type] :as click-behavior}]
+  (fk-elide
+   (cond-> click-behavior
+     (= type "link") (-> (update :targetId *export-fk* (link-card-model->toucan-model linkType))
+                         (u/update-some :tabId *export-fk* :model/DashboardTab)))))
 
 (defn- import-viz-click-behavior-link
   [{:keys [linkType type] :as click-behavior}]
   (cond-> click-behavior
-    (= type "link") (update :targetId *import-fk* (link-card-model->toucan-model linkType))))
+    (= type "link") (-> (update :targetId *import-fk* (link-card-model->toucan-model linkType))
+                        (u/update-some :tabId *import-fk* :model/DashboardTab))))
 
 (defn- export-viz-click-behavior-mapping [mapping]
   (-> mapping
@@ -1391,11 +1392,8 @@
 
 (defn- export-viz-click-behavior [settings]
   (some-> settings
-          (m/update-existing    :click_behavior export-viz-click-behavior-link)
-          (m/update-existing-in [:click_behavior :parameterMapping] export-viz-click-behavior-mappings)
-          (as-> updated-settings
-                (cond-> updated-settings
-                  (nil? (:click_behavior updated-settings)) (dissoc :click_behavior)))))
+          (u/update-some :click_behavior export-viz-click-behavior-link)
+          (m/update-existing-in [:click_behavior :parameterMapping] export-viz-click-behavior-mappings)))
 
 (defn- import-viz-click-behavior [settings]
   (some-> settings
@@ -1520,10 +1518,11 @@
 
 (defn- viz-click-behavior-deps
   [settings]
-  (when-let [{:keys [linkType targetId type]} (:click_behavior settings)]
+  (let [{:keys [linkType targetId type]} (:click_behavior settings)
+        model (when linkType (link-card-model->toucan-model linkType))]
     (case type
-      "link" (when-let [model (some-> linkType link-card-model->toucan-model name)]
-               #{[{:model model
+      "link" (when model
+               #{[{:model (name model)
                    :id    targetId}]})
       ;; TODO: We might need to handle the click behavior that updates dashboard filters? I can't figure out how get
       ;; that to actually attach to a filter to check what it looks like.
@@ -1550,12 +1549,12 @@
          (reduce set/union #{}))))
 
 (defn- viz-click-behavior-descendants [{:keys [click_behavior]} src]
-  (when-let [{:keys [linkType targetId type]} click_behavior]
+  (let [{:keys [linkType targetId type]} click_behavior
+        model (when linkType (link-card-model->toucan-model linkType))]
     (case type
-      "link" (when-let [model (link-card-model->toucan-model linkType)]
-               ;; if the card was deleted, just ignore it.
-               (when (maybe-export-fk targetId model)
-                 {[(name model) targetId] src}))
+      "link" (when (and model
+                        (fk-elide (*export-fk* targetId model)))
+               {[(name model) targetId] src})
       ;; TODO: We might need to handle the click behavior that updates dashboard filters? I can't figure out how get
       ;; that to actually attach to a filter to check what it looks like.
       nil)))

diff --git a/src/metabase/task.clj b/src/metabase/task.clj
@@ -327,8 +327,9 @@
   "Check whether there is a Job with the given key."
   [job-key]
   (boolean
-   (when-let [s (scheduler)]
-     (qs/get-job s (->job-key job-key)))))
+   (let [s (scheduler)]
+     (when (and s (not (.isShutdown s)))
+       (qs/get-job s (->job-key job-key))))))
 
 (defn job-info
   "Get info about a specific Job (`job-key` can be either a String or `JobKey`).

diff --git a/src/metabase/util.cljc b/src/metabase/util.cljc
@@ -1158,3 +1158,12 @@
   (let [cnt (volatile! 0)]
     (reduce (fn [_ item] (vswap! cnt inc) (proc item)) nil reducible)
     @cnt))
+
+(defn update-some
+  "Update a value by key in the `m`, if it's `some?`. If `nil` is returned, dissoc it instead"
+  [m k f & args]
+  (let [v (get m k)
+        res (when v (apply f v args))]
+    (if res
+      (assoc m k res)
+      (dissoc m k))))