when no field-values exist for a field, clear out any previous field-values so...

when no field-values exist for a field, clear out any previous field-values so we don't leave stale data lying around.

src/metabase/driver/sync.clj

@@ -190,9 +190,10 @@
               (upd-non-nil-keys Field id
                 :preview_display preview-display
                 :special_type    special-type))
-            ;; looks like we found some field values
-            (when (and id values (< 0 (count (filter identity values))))
-              (field-values/save-field-values id values))))))))
+            ;; handle field values, setting them if applicable otherwise clearing them
+            (if (and id values (< 0 (count (filter identity values))))
+              (field-values/save-field-values id values)
+              (field-values/clear-field-values id))))))))
 
 
 (defn- sync-database-active-tables!

src/metabase/models/field_values.clj

 (ns metabase.models.field-values
   (:require [clojure.tools.logging :as log]
-            [metabase.db :refer [ins sel upd]]
+            [metabase.db :refer [ins sel upd cascade-delete]]
             [metabase.models.interface :as i]
             [metabase.util :as u]))
 
@@ -78,5 +78,11 @@
     (upd FieldValues (:id field-values) :values values)
     (ins FieldValues :field_id field-id, :values values)))
 
+(defn clear-field-values
+  "Remove the `FieldValues` for FIELD-ID."
+  [field-id]
+  {:pre [(integer? field-id)]}
+  (cascade-delete FieldValues :field_id field-id))
+
 
 (u/require-dox-in-this-namespace)

test/metabase/driver/sync_test.clj

@@ -147,6 +147,9 @@
                                    :engine  :sync-test
                                    :details {}}]
     (sync/sync-database! (SyncTestDriver.) fake-db)
+    ;; we are purposely running the sync twice to test for possible logic issues which only manifest
+    ;; on resync of a database, such as adding tables that already exist or duplicating fields
+    (sync/sync-database! (SyncTestDriver.) fake-db)
     (->> (sel :many Table :db_id (:id fake-db) (k/order :name))
          (mapv table-details))))
 
@@ -204,6 +207,28 @@
       (table-details (sel :one Table :id (:id fake-table))))))
 
 
+;; ## Test that we will remove field-values when they aren't appropriate
+
+(expect
+  [[1,2,3]
+   [1,2,3]]
+  (tu/with-temp Database [fake-db {:name    "sync-test"
+                                   :engine  :sync-test
+                                   :details {}}]
+    (tu/with-temp Table [fake-table {:name "movie"
+                                     :schema "default"
+                                     :db_id (:id fake-db)
+                                     :active true}]
+      (sync/sync-table! (SyncTestDriver.) fake-table)
+      (let [{:keys [id]} (sel :one Field :table_id (:id fake-table) :name "title")]
+        (tu/with-temp FieldValues [_ {:field_id id
+                                      :values   "[1,2,3]"}]
+          (let [starting (sel :one :field [FieldValues :values] :field_id id)]
+            (sync/sync-table! (SyncTestDriver.) fake-table)
+            [starting
+             (sel :one :field [FieldValues :values] :field_id id)]))))))
+
+
 ;; ## Individual Helper Fns
 
 ;; infer-field-special-type
@@ -279,18 +304,6 @@
        (driver/sync-table! table)
        (get-special-type-and-fk-exists?))]))
 
-;;; ## Tests for DETERMINE-FK-TYPE
-;;; Since COUNT(category_id) > COUNT(DISTINCT(category_id)) the FK relationship should be Mt1
-;(def determine-fk-type @(resolve 'metabase.driver.sync/determine-fk-type))
-;
-;(expect :Mt1
-;  (determine-fk-type (Field (id :venues :category_id))))
-;
-;;; Since COUNT(id) == COUNT(DISTINCT(id)) the FK relationship should be 1t1
-;;; (yes, ID isn't really a FK field, but determine-fk-type doesn't need to know that)
-;(expect :1t1
-;  (determine-fk-type (Field (id :venues :id))))
-
 
 ;;; ## FieldValues Syncing
 

test/metabase/models/field_values_test.clj

 (ns metabase.models.field-values-test
   (:require [expectations :refer :all]
-            [metabase.models.field-values :refer :all]))
+            [metabase.db :as db]
+            [metabase.models.database :refer [Database]]
+            [metabase.models.field :refer [Field]]
+            [metabase.models.field-values :refer :all]
+            [metabase.models.table :refer [Table]]
+            [metabase.test.util :as tu]))
 
 ;; ## TESTS FOR FIELD-SHOULD-HAVE-FIELD-VALUES?
 
@@ -28,3 +33,28 @@
   (field-should-have-field-values? {:special_type nil
                                     :field_type :info
                                     :base_type "BooleanField"}))
+
+
+(expect
+  [[1,2,3]
+   {:status 204, :body nil}
+   nil]
+  (tu/with-temp Database [{database-id :id} {:name      "FieldValues Test"
+                                             :engine    :yeehaw
+                                             :details   {}
+                                             :is_sample false}]
+    (tu/with-temp Table [{table-id :id} {:name   "FieldValues Test"
+                                         :db_id  database-id
+                                         :active true}]
+      (tu/with-temp Field [{field-id :id} {:table_id    table-id
+                                           :name        "FieldValues Test"
+                                           :base_type   :TextField
+                                           :field_type  :info
+                                           :active      true
+                                           :preview_display true
+                                           :position    1}]
+        (tu/with-temp FieldValues [_ {:field_id field-id
+                                      :values   "[1,2,3]"}]
+          [(db/sel :one :field [FieldValues :values] :field_id field-id)
+           (clear-field-values field-id)
+           (db/sel :one :field [FieldValues :values] :field_id field-id)])))))