Fix n+1 when getting field values for dashboard (#47113)

src/metabase/models/field_values.clj

@@ -394,22 +394,30 @@
       nil)))
 
 (defn- delete-duplicates-and-return-latest!
-  "This is a workaround for the issue of stale FieldValues rows (metabase#668)
-  In order to mitigate the impact of duplicates, we return the most recently updated row, and delete the older rows."
-  [rows]
-  (if (<= (count rows) 1)
-    (first rows)
-    (let [[latest & duplicates] (sort-by :updated_at u/reverse-compare rows)]
-      (t2/delete! FieldValues :id [:in (map :id duplicates)])
-      latest)))
-
-(defn get-latest-field-values
+  "Takes a list of field values, return a map of field-id -> latest FieldValues.
+
+  If a field has more than one Field Values, delete the old ones. This is a workaround for the issue of stale FieldValues rows (metabase#668)
+  In order to mitigate the impact of duplicates, we return the most recently updated row, and delete the older rows.
+
+  It assumes that all rows are of the same type. Rows could be from multiple field-ids."
+  [fvs]
+  (let [fvs-grouped-by-field-id (update-vals (group-by :field_id fvs)
+                                             #(sort-by :updated_at u/reverse-compare %))
+        to-delete-fv-ids        (->> (vals fvs-grouped-by-field-id)
+                                     (mapcat rest)
+                                     (map :id))]
+    (when (seq to-delete-fv-ids)
+      (t2/delete! :model/FieldValues :id [:in to-delete-fv-ids]))
+    (update-vals fvs-grouped-by-field-id first)))
+
+(defn- get-latest-field-values
   "This returns the FieldValues with the given :type and :hash_key for the given Field.
-   This may implicitly delete shadowed entries in the database, see [[delete-duplicates-and-return-latest!]]"
+  This may implicitly delete shadowed entries in the database, see [[delete-duplicates-and-return-latest!]]"
   [field-id type hash]
   (assert (= (nil? hash) (= type :full)) ":hash_key must be nil iff :type is :full")
-  (delete-duplicates-and-return-latest!
-   (t2/select FieldValues :field_id field-id :type type :hash_key hash)))
+  (-> (t2/select :model/FieldValues :field_id field-id :type type :hash_key hash)
+      delete-duplicates-and-return-latest!
+      (get field-id)))
 
 (defn get-latest-full-field-values
   "This returns the full FieldValues for the given Field.
@@ -417,6 +425,14 @@
   [field-id]
   (get-latest-field-values field-id :full nil))
 
+(defn batched-get-latest-full-field-values
+  "Batched version of [[get-latest-full-field-values]] .
+  Takes a list of field-ids and returns a map of field-id -> full FieldValues.
+  This may implicitly delete shadowed entries in the database, see [[delete-duplicates-and-return-latest!]]"
+  [field-ids]
+  (delete-duplicates-and-return-latest!
+   (t2/select :model/FieldValues :field_id [:in field-ids] :type :full :hash_key nil)))
+
 (defn create-or-update-full-field-values!
   "Create or update the full FieldValues object for `field`. If the FieldValues object already exists, then update values for
    it; otherwise create a new FieldValues object with the newly fetched values. Returns whether the field values were

src/metabase/models/params/field_values.clj

@@ -2,7 +2,6 @@
   "Code related to fetching FieldValues for Fields to populate parameter widgets. Always used by the field
   values (`GET /api/field/:id/values`) endpoint; used by the chain filter endpoints under certain circumstances."
   (:require
-   [medley.core :as m]
    [metabase.db.query :as mdb.query]
    [metabase.models.field :as field]
    [metabase.models.field-values :as field-values :refer [FieldValues]]
@@ -46,15 +45,12 @@
   "OSS implementation; used as a fallback for the EE implementation for any fields that aren't subject to sandboxing."
   [field-ids]
   (when (seq field-ids)
-    (not-empty
-     (let [fields       (-> (t2/select :model/Field :id [:in (set field-ids)])
-                            (field/readable-fields-only)
-                            (t2/hydrate :values))
-           field-values (->> (map #(select-keys (field-values/get-latest-full-field-values (:id %))
-                                                [:field_id :human_readable_values :values])
-                                  fields)
-                             (keep not-empty))]
-       (m/index-by :field_id field-values)))))
+    (let [field-ids (->> (t2/select :model/Field :id [:in (set field-ids)])
+                         field/readable-fields-only
+                         (map :id))]
+      (when (seq field-ids)
+        (update-vals (field-values/batched-get-latest-full-field-values field-ids)
+                     #(select-keys % [:field_id :human_readable_values :values]))))))
 
 (defenterprise field-id->field-values-for-current-user
   "Fetch *existing* FieldValues for a sequence of `field-ids` for the current User. Values are returned as a map of

test/metabase/models/field_values_test.clj

@@ -211,6 +211,28 @@
             ;; double check that we deleted the correct row
             (is (= ["C" "D"] (:human_readable_values (field-values/get-latest-full-field-values field-id))))))))))
 
+(deftest implicit-deduplication-batched-test
+  (let [before (t/zoned-date-time)
+        after  (t/plus before (t/millis 1))
+        later  (t/plus after (t/millis 1))]
+    (mt/with-temp [:model/Database    {database-id :id} {}
+                   :model/Table       {table-id :id}    {:db_id database-id}
+                   ;; will have duplicated field values
+                   :model/Field       {field-id-1 :id}     {:table_id table-id}
+                   ;; have only one field values
+                   :model/Field       {field-id-2 :id}     {:table_id table-id}
+                   ;; doesn't have a a field values
+                   :model/Field       {field-id-3 :id}     {:table_id table-id}
+                   :model/FieldValues _                 {:field_id field-id-1 :type :full :values ["a" "b"] :human_readable_values ["A" "B"] :created_at before :updated_at before}
+                   :model/FieldValues _                 {:field_id field-id-1 :type :full :values ["c" "d"] :human_readable_values ["C" "D"] :created_at before :updated_at later}
+                   :model/FieldValues _                 {:field_id field-id-2 :type :full :values ["e" "f"] :human_readable_values ["E" "F"] :created_at after :updated_at after}]
+      (testing "When we have multiple FieldValues rows in the database, we always return the most recently updated row"
+        (is (=? {field-id-1 {:values ["c" "d"]}
+                 field-id-2 {:values ["e" "f"]}}
+               (field-values/batched-get-latest-full-field-values [field-id-1 field-id-2 field-id-3])))
+        (testing "and older values are implicitly deleted"
+          (is (= 1 (count (t2/select FieldValues :field_id field-id-1 :type :full)))))))))
+
 (deftest get-or-create-full-field-values!-test
   (mt/dataset test-data
     (testing "create a full Fieldvalues if it does not exist"