Fixup human readable values when field values change [ci drivers]

Field values and human readable values are zipped together to support
remapping. If the field values change (i.e. new values, removing
values etc) then the pairings of those field values and their remapped
values can be wrong. This commit adds some code to reconcile that. If
a new field value is found, a stringified version of that field value
is used as the human readable version (the user can update/change this
later).

Fixes #5782

src/metabase/models/field_values.clj

@@ -79,6 +79,16 @@
       (when (values-less-than-total-max-length? values)
         values))))
 
+(defn- fixup-human-readable-values
+  "Field values and human readable values are lists that are zipped
+  together. If the field values have changes, the human readable
+  values will need to change too. This function reconstructs the
+  human_readable_values to reflect `NEW-VALUES`. If a new field value
+  is found, a string version of that is used"
+  [{old-values :values, old-hrv :human_readable_values} new-values]
+  (when (seq old-hrv)
+    (let [orig-remappings (zipmap old-values old-hrv)]
+      (map #(get orig-remappings % (str %)) new-values))))
 
 (defn create-or-update-field-values!
   "Create or update the FieldValues object for FIELD."
@@ -91,8 +101,9 @@
       (and field-values values)
       (do
         (log/debug (format "Storing updated FieldValues for Field %s..." field-name))
-        (db/update! FieldValues (u/get-id field-values)
-          :values values))
+        (db/update-non-nil-keys! FieldValues (u/get-id field-values)
+          :values                values
+          :human_readable_values (fixup-human-readable-values field-values values)))
       ;; if FieldValues object doesn't exist create one
       values
       (do

test/metabase/models/field_values_test.clj

 (ns metabase.models.field-values-test
-  (:require [expectations :refer :all]
+  (:require [clojure.java.jdbc :as jdbc]
+            [expectations :refer :all]
+            [metabase
+             [db :as mdb]
+             [sync :as sync]
+             [util :as u]]
             [metabase.models
              [database :refer [Database]]
              [field :refer [Field]]
@@ -50,3 +55,57 @@
      (do
        (clear-field-values! field-id)
        (db/select-one-field :values FieldValues, :field_id field-id))]))
+
+(defn- find-values [field-values-id]
+  (-> (db/select-one FieldValues :id field-values-id)
+      (select-keys [:values :human_readable_values])))
+
+(defn- sync-and-find-values [db field-values-id]
+  (sync/sync-database! db)
+  (find-values field-values-id))
+
+;; Test "fixing" of human readable values when field values change
+(expect
+  (concat (repeat 2 {:values [1 2 3] :human_readable_values ["a" "b" "c"]})
+          (repeat 2 {:values [-2 -1 0 1 2 3] :human_readable_values ["-2" "-1" "0" "a" "b" "c"]})
+          [{:values [-2 -1 0] :human_readable_values ["-2" "-1" "0"]}])
+
+  (binding [mdb/*allow-potentailly-unsafe-connections* true]
+    ;; Create a temp warehouse database that can have it's field values change
+    (jdbc/with-db-connection [conn {:classname "org.h2.Driver", :subprotocol "h2", :subname "mem:temp"}]
+      (jdbc/execute! conn ["drop table foo if exists"])
+      (jdbc/execute! conn ["create table foo (id integer primary key, category_id integer not null, desc text)"])
+      (jdbc/insert-multi! conn :foo [{:id 1 :category_id 1 :desc "foo"}
+                                     {:id 2 :category_id 2 :desc "bar"}
+                                     {:id 3 :category_id 3 :desc "baz"}])
+
+      ;; Create a new in the Database table for this newly created temp database
+      (tt/with-temp Database [db {:engine       :h2
+                                  :name         "foo"
+                                  :is_full_sync true
+                                  :details      "{\"db\": \"mem:temp\"}"}]
+
+        ;; Sync the database so we have the new table and it's fields
+        (do (sync/sync-database! db)
+            (let [table-id        (db/select-one-field :id Table :db_id (u/get-id db) :name "FOO")
+                  field-id        (db/select-one-field :id Field :table_id table-id :name "CATEGORY_ID")
+                  field-values-id (db/select-one-field :id FieldValues :field_id field-id)]
+              ;; Add in human readable values for remapping
+              (db/update! FieldValues field-values-id {:human_readable_values "[\"a\",\"b\",\"c\"]"})
+
+              ;; This is the starting point, the original catgory ids and their remapped values
+              [(find-values field-values-id)
+               ;; There should be no changes to human_readable_values  when resync'd
+               (sync-and-find-values db field-values-id)
+               (do
+                 ;; Add new rows that will have new field values
+                 (jdbc/insert-multi! conn :foo [{:id 4 :category_id -2 :desc "foo"}
+                                                {:id 5 :category_id -1 :desc "bar"}
+                                                {:id 6 :category_id 0 :desc "baz"}])
+                 ;; Sync to pickup the new field values and rebuild the human_readable_values
+                 (sync-and-find-values db field-values-id))
+               ;; Resyncing this (with the new field values) should result in the same human_readable_values
+               (sync-and-find-values db field-values-id)
+               ;; Test that field values can be removed and the corresponding human_readable_values are removed as well
+               (do (jdbc/delete! conn :foo ["id in (?,?,?)" 1 2 3])
+                   (sync-and-find-values db field-values-id))]))))))