Merge pull request #676 from metabase/sync_field_values

Update FieldValues when DBs are synced 

src/metabase/driver/sync.clj

@@ -10,12 +10,13 @@
                              [query-processor :as qp])
             [metabase.driver.sync.queries :as queries]
             (metabase.models [field :refer [Field] :as field]
+                             [field-values :as field-values]
                              [foreign-key :refer [ForeignKey]]
                              [table :refer [Table]])
             [metabase.util :as u]))
 
 (declare auto-assign-field-special-type-by-name!
-         mark-category-field!
+         mark-category-field-or-update-field-values!
          mark-no-preview-display-field!
          mark-url-field!
          sync-database-active-tables!
@@ -258,7 +259,7 @@
   (log/debug (format "Syncing field '%s'..." @(:qualified-name field)))
   (sync-field->> field
                  (mark-url-field! driver)
-                 mark-category-field!
+                 mark-category-field-or-update-field-values!
                  (mark-no-preview-display-field! driver)
                  auto-assign-field-special-type-by-name!
                  (sync-field-nested-fields! driver)))
@@ -311,22 +312,31 @@
         (assoc field :special_type :url)))))
 
 
-;; ### mark-category-field!
+;; ### mark-category-field-or-update-field-values!
 
 (def ^:const ^:private low-cardinality-threshold
   "Fields with less than this many distinct values should automatically be marked with `special_type = :category`."
   40)
 
-(defn mark-category-field!
+(defn- mark-category-field!
   "If FIELD doesn't yet have a `special_type`, and has low cardinality, mark it as a category."
   [field]
-  (when-not (:special_type field)
-    (let [cardinality (queries/field-distinct-count field low-cardinality-threshold)]
-      (when (and (> cardinality 0)
-                 (< cardinality low-cardinality-threshold))
-        (log/info (u/format-color 'green "Field '%s' has %d unique values. Marking it as a category." @(:qualified-name field) cardinality))
-        (upd Field (:id field) :special_type :category)
-        (assoc field :special_type :category)))))
+  (let [cardinality (queries/field-distinct-count field low-cardinality-threshold)]
+    (when (and (> cardinality 0)
+               (< cardinality low-cardinality-threshold))
+      (log/info (u/format-color 'green "Field '%s' has %d unique values. Marking it as a category." @(:qualified-name field) cardinality))
+      (upd Field (:id field) :special_type :category)
+      (assoc field :special_type :category))))
+
+(defn- mark-category-field-or-update-field-values!
+  "If FIELD doesn't yet have a `special_type`, call `mark-category-field!` to (possibly) mark it as a `:category`.
+   Otherwise if FIELD is already a `:category` update its `FieldValues`."
+  [field]
+  (cond
+    (not (:special_type field))                          (mark-category-field! field)
+    (field-values/field-should-have-field-values? field) (do (log/debug (format "Updating values for field '%s'..." @(:qualified-name field)))
+                                                             (field-values/update-field-values! field)
+                                                             field)))
 
 
 ;; ### mark-no-preview-display-field!

src/metabase/models/field_values.clj

@@ -51,6 +51,16 @@
     :values                (field-distinct-values field)
     :human_readable_values human-readable-values))
 
+(defn update-field-values!
+  "Update the `FieldValues` for FIELD, creating them if needed"
+  [{field-id :id, :as field}]
+  {:pre [(integer? field-id)
+         (field-should-have-field-values? field)]}
+  (if-let [field-values (sel :one FieldValues :field_id field-id)]
+    (upd FieldValues (:id field-values)
+      :values (field-distinct-values field))
+    (create-field-values field)))
+
 (defn create-field-values-if-needed
   "Create `FieldValues` for a `Field` if they *should* exist but don't already exist.
    Returns the existing or newly created `FieldValues` for `Field`."

test/metabase/driver/sync_test.clj

@@ -7,6 +7,7 @@
                              [sync :as sync])
             [metabase.driver.generic-sql.util :refer [korma-entity]]
             (metabase.models [field :refer [Field]]
+                             [field-values :refer [FieldValues]]
                              [foreign-key :refer [ForeignKey]]
                              [table :refer [Table]])
             (metabase.test [data :refer :all]
@@ -89,3 +90,37 @@
 ;; (yes, ID isn't really a FK field, but determine-fk-type doesn't need to know that)
 (expect :1t1
   (sync/determine-fk-type (Field (id :venues :id))))
+
+
+
+;;; ## FieldValues Syncing
+
+(let [get-field-values    (fn [] (sel :one :field [FieldValues :values] :field_id (id :venues :price)))
+      get-field-values-id (fn [] (sel :one :id FieldValues :field_id (id :venues :price)))]
+  ;; Test that when we delete FieldValues syncing the Table again will cause them to be re-created
+  (expect
+      [[1 2 3 4]  ; 1
+       nil        ; 2
+       [1 2 3 4]] ; 3
+    [ ;; 1. Check that we have expected field values to start with
+     (get-field-values)
+     ;; 2. Delete the Field values, make sure they're gone
+     (do (cascade-delete FieldValues :id (get-field-values-id))
+         (get-field-values))
+     ;; 3. Now re-sync the table and make sure they're back
+     (do (driver/sync-table! @venues-table)
+         (get-field-values))])
+
+  ;; Test that syncing will cause FieldValues to be updated
+  (expect
+      [[1 2 3 4]  ; 1
+       [1 2 3]    ; 2
+       [1 2 3 4]] ; 3
+    [ ;; 1. Check that we have expected field values to start with
+     (get-field-values)
+     ;; 2. Update the FieldValues, remove one of the values that should be there
+     (do (upd FieldValues (get-field-values-id) :values [1 2 3])
+         (get-field-values))
+     ;; 3. Now re-sync the table and make sure the value is back
+     (do (driver/sync-table! @venues-table)
+         (get-field-values))]))