Optimize DB calls of dashboard card updates some more - 2 (#29252)

* Transform card data into the same format as that which comes out of the DB

* Use `from-json` instead of toucan transformations

* Remove unused require

* Update docstring and rename to from-parsed-json

* Oops, fixed

* Remove now

* Add docstring

* Fix order of docstring

* Don't update created_at and updated_at

src/metabase/api/dashboard.clj

@@ -572,7 +572,9 @@
   {cards (su/non-empty [UpdatedDashboardCard])}
   (let [dashboard (api/check-not-archived (api/write-check Dashboard id))]
     (check-updated-parameter-mapping-permissions id cards)
-    (dashboard/update-dashcards! dashboard cards)
+    ;; transform the card data to the format of the DashboardCard model
+    ;; so update-dashcards! can compare them with existing cards
+    (dashboard/update-dashcards! dashboard (map dashboard-card/from-parsed-json cards))
     (events/publish-event! :dashboard-reposition-cards {:id id, :actor_id api/*current-user-id*, :dashcards cards})
     {:status :ok}))
 

src/metabase/models/dashboard_card.clj

@@ -53,6 +53,27 @@
                            :visualization_settings :visualization-settings})
   :pre-insert pre-insert})
 
+(defn from-parsed-json
+  "Convert a map with dashboard-card into a Toucan instance assuming it came from parsed JSON and the map keys have
+   been keywordized. This is useful if the data from a request body inside a `defendpoint` body, and you need it in the
+   same format as if it were selected from the DB with toucan. It doesn't transform the `:created_at` or `:updated_at`
+   fields, as the types of timestamp values differ by the application database driver.
+
+   For example:
+   ```
+   (= dashcard ;; from toucan select, excluding :created_at and :updated_at
+      (-> (json/generate-string dashcard)
+          (json/parse-string true)
+          from-parsed-json))
+   =>
+   true
+   ```"
+  [dashboard-card]
+  (t2/instance DashboardCard
+               (-> dashboard-card
+                   (m/update-existing :parameter_mappings mi/normalize-parameters-list)
+                   (m/update-existing :visualization_settings mi/normalize-visualization-settings))))
+
 (defmethod serdes/hash-fields DashboardCard
   [_dashboard-card]
   [(serdes/hydrated-hash :card) ; :card is optional, eg. text cards
@@ -162,7 +183,8 @@
                     ;; This is to preserve the existing behavior of questions and card_id
                     ;; I don't know why card_id couldn't be changed for cards though.
                      action_id (conj :card_id))
-         updates (shallow-updates (select-keys dashboard-card update-ks) (select-keys old-dashboard-card update-ks))]
+         updates (shallow-updates (select-keys dashboard-card update-ks)
+                                  (select-keys old-dashboard-card update-ks))]
      (when (seq updates)
        (db/update! DashboardCard id updates))
      (when (not= (:series dashboard-card [])

src/metabase/models/interface.clj

@@ -145,9 +145,10 @@
   :in  (comp json-in normalize-metric-segment-definition)
   :out (comp (catch-normalization-exceptions normalize-metric-segment-definition) json-out-with-keywordization))
 
-(defn- normalize-visualization-settings [viz-settings]
-  ;; frontend uses JSON-serialized versions of MBQL clauses as keys in `:column_settings`; we need to normalize them
-  ;; to modern MBQL clauses so things work correctly
+(defn normalize-visualization-settings
+  "The frontend uses JSON-serialized versions of MBQL clauses as keys in `:column_settings`. This normalizes them
+   to modern MBQL clauses so things work correctly."
+  [viz-settings]
   (letfn [(normalize-column-settings-key [k]
             (some-> k u/qualified-name json/parse-string mbql.normalize/normalize json/generate-string))
           (normalize-column-settings [column-settings]

test/metabase/models/dashboard_card_test.clj

@@ -173,18 +173,18 @@
                (remove-ids-and-timestamps (dashboard-card/retrieve-dashboard-card dashcard-id)))))
       (testing "return value from the update call should be nil"
         (is (nil? (dashboard-card/update-dashboard-card!
-                    {:id                     dashcard-id
-                     :actor_id               (mt/user->id :rasta)
-                     :dashboard_id           nil
-                     :card_id                nil
-                     :size_x                 5
-                     :size_y                 3
-                     :row                    1
-                     :col                    1
-                     :parameter_mappings     [{:foo "barbar"}]
-                     :visualization_settings {}
-                     :series                 [card-id-2 card-id-1]}
-                    dashboard-card))))
+                   {:id                     dashcard-id
+                    :actor_id               (mt/user->id :rasta)
+                    :dashboard_id           nil
+                    :card_id                nil
+                    :size_x                 5
+                    :size_y                 3
+                    :row                    1
+                    :col                    1
+                    :parameter_mappings     [{:foo "barbar"}]
+                    :visualization_settings {}
+                    :series                 [card-id-2 card-id-1]}
+                   dashboard-card))))
       (testing "validate db captured everything"
         (is (= {:size_x                 5
                 :size_y                 3
@@ -214,34 +214,34 @@
                     DashboardCard [dashcard-3 {:dashboard_id dashboard-id, :card_id card-id}]
                     Card          [{series-id-1 :id} {:name "Series Card 1"}]
                     Card          [{series-id-2 :id} {:name "Series Card 2"}]]
-      (testing "Should have fewer DB calls if there's no changes to the dashcards"
-       (db/with-call-counting [call-count]
-         (dashboard/update-dashcards! dashboard [dashcard-1 dashcard-2 dashcard-3])
-         (is (= 6 (call-count)))))
-      (testing "Should have more calls if there's changes to the dashcards"
-       (db/with-call-counting [call-count]
-         (dashboard/update-dashcards! dashboard [{:id     (:id dashcard-1)
-                                                  :cardId card-id
-                                                  :row    1
-                                                  :col    2
-                                                  :size_x 3
-                                                  :size_y 4
-                                                  :series [{:id series-id-1}]}
-                                                 {:id     (:id dashcard-2)
-                                                  :cardId card-id
-                                                  :row    1
-                                                  :col    2
-                                                  :size_x 3
-                                                  :size_y 4
-                                                  :series [{:id series-id-2}]}
-                                                 {:id     (:id dashcard-3)
-                                                  :cardId card-id
-                                                  :row    1
-                                                  :col    2
-                                                  :size_x 3
-                                                  :size_y 4
-                                                  :series []}])
-         (is (= 15 (call-count))))))))
+      (testing "Should have fewer DB calls if there are no changes to the dashcards"
+        (db/with-call-counting [call-count]
+          (dashboard/update-dashcards! dashboard [dashcard-1 dashcard-2 dashcard-3])
+          (is (= 6 (call-count)))))
+      (testing "Should have more calls if there are changes to the dashcards"
+        (db/with-call-counting [call-count]
+          (dashboard/update-dashcards! dashboard [{:id     (:id dashcard-1)
+                                                   :cardId card-id
+                                                   :row    1
+                                                   :col    2
+                                                   :size_x 3
+                                                   :size_y 4
+                                                   :series [{:id series-id-1}]}
+                                                  {:id     (:id dashcard-2)
+                                                   :cardId card-id
+                                                   :row    1
+                                                   :col    2
+                                                   :size_x 3
+                                                   :size_y 4
+                                                   :series [{:id series-id-2}]}
+                                                  {:id     (:id dashcard-3)
+                                                   :cardId card-id
+                                                   :row    1
+                                                   :col    2
+                                                   :size_x 3
+                                                   :size_y 4
+                                                   :series []}])
+          (is (= 15 (call-count))))))))
 
 (deftest normalize-parameter-mappings-test
   (testing "DashboardCard parameter mappings should get normalized when coming out of the DB"
@@ -307,3 +307,28 @@
         (is (= "1311d6dc"
                (serdes/raw-hash [(serdes/identity-hash card) (serdes/identity-hash dash) {} 6 3 now])
                (serdes/identity-hash dashcard)))))))
+
+(deftest from-decoded-json-test
+  (testing "Dashboard Cards should remain the same if they are serialized to JSON,
+            deserialized, and finally transformed with `from-parsed-json`."
+    (mt/with-temp* [Dashboard     [dash     {:name "my dashboard"}]
+                    Card          [card     {:name "some question"}]
+                    DashboardCard [dashcard {:card_id (:id card)
+                                             :dashboard_id (:id dash)
+                                             :visualization_settings {:click_behavior {:type         "link",
+                                                                                       :linkType     "url",
+                                                                                       :linkTemplate "/dashboard/1?year={{column:Year}}"}}
+                                             :parameter_mappings     [{:card_id (:id card)
+                                                                       :parameter_id "-1419866742"
+                                                                       :target [:dimension [:field 1 nil]]}]
+                                             :row                    4
+                                             :col                    3}]]
+      ;; NOTE: we need to remove `:created_at` and `:updated_at` because they are not
+      ;; transformed by `from-parsed-json`
+      (let [dashcard     (dissoc (db/select-one DashboardCard :id (u/the-id dashcard))
+                                 :created_at :updated_at)
+            serialized   (json/generate-string dashcard)
+            deserialized (json/parse-string serialized true)
+            transformed  (dashboard-card/from-parsed-json deserialized)]
+        (is (= dashcard
+               transformed))))))