Remove unused Table.rows column & sync code (#12797)

* Remove Table.rows entirely

* Lots of test updates/modernization

* Test fixes 

resources/migrations/000_migrations.yaml

@@ -6240,6 +6240,9 @@ databaseChangeLog:
       id: 165
       author: sb
       comment: 'Added field_order to Table and database_position to Field'
+      validCheckSum:
+        - 8:474a357368a665d5e0701b9eb5f313f9
+        - 8:8848644da9dd9e40924ae71ac4c7c370
       changes:
         - addColumn:
             tableName: metabase_field
@@ -6376,3 +6379,16 @@ databaseChangeLog:
             tableName: query_execution
             columnName: started_at
             newDataType: ${timestamp_type}
+
+# Remove `Table.rows`, which hasn't been used for years now. Older versions of
+# Metabase used to store the row count in this column but we disabled it a long
+# time ago for performance reasons. Now it's time to remove it entirely.
+
+  - changeSet:
+      id: 169
+      author: camsaul
+      comment: Added 0.36.0
+      changes:
+        - dropColumn:
+            tableName: metabase_table
+            columnName: rows

src/metabase/api/table.clj

@@ -37,13 +37,9 @@
 (api/defendpoint GET "/"
   "Get all `Tables`."
   []
-  (for [table (-> (db/select Table, :active true, {:order-by [[:name :asc]]})
-                  (hydrate :db))
-        :when (mi/can-read? table)]
-    ;; if for some reason a Table doesn't have rows set then set it to 0 so UI doesn't barf.
-    ;; TODO - should that be part of `post-select` instead?
-    (update table :rows (fn [n]
-                          (or n 0)))))
+  (as-> (db/select Table, :active true, {:order-by [[:name :asc]]}) tables
+    (hydrate tables :db)
+    (filterv mi/can-read? tables)))
 
 (api/defendpoint GET "/:id"
   "Get `Table` with ID."

src/metabase/sync/analyze.clj

@@ -76,10 +76,8 @@
   (update-last-analyzed! tables))
 
 (s/defn analyze-table!
-  "Perform in-depth analysis for a TABLE."
+  "Perform in-depth analysis for a `table`."
   [table :- i/TableInstance]
-  ;; Table row count disabled for now because of performance issues
-  #_(table-row-count/update-row-count! table)
   (fingerprint/fingerprint-fields! table)
   (classify/classify-fields! table)
   (classify/classify-table! table)

src/metabase/sync/analyze/table_row_count.clj

-(ns metabase.sync.analyze.table-row-count
-  "Logic for updating a Table's row count by running appropriate MBQL queries."
-  (:require [clojure.tools.logging :as log]
-            [metabase.db.metadata-queries :as queries]
-            [metabase.models.table :refer [Table]]
-            [metabase.sync
-             [interface :as i]
-             [util :as sync-util]]
-            [metabase.util :as u]
-            [schema.core :as s]
-            [toucan.db :as db]))
-
-(s/defn ^:private table-row-count :- (s/maybe s/Int)
-  "Determine the count of rows in TABLE by running a simple structured MBQL query."
-  [table :- i/TableInstance]
-  (sync-util/with-error-handling (format "Unable to determine row count for %s" (sync-util/name-for-logging table))
-    (queries/table-row-count table)))
-
-(s/defn update-row-count!
-  "Update the cached row count (`rows`) for a single TABLE."
-  [table :- i/TableInstance]
-  (sync-util/with-error-handling (format "Error setting table row count for %s" (sync-util/name-for-logging table))
-    (when-let [row-count (table-row-count table)]
-      (log/debug (format "Set table row count for %s to %d" (sync-util/name-for-logging table) row-count))
-      (db/update! Table (u/get-id table)
-        :rows row-count))))

test/expectations.clj

@@ -137,8 +137,8 @@
     ;; expecting it.
     (when-not (env/env :drivers)
       (t/testing "Don't write any new tests using expect!"
-        (t/is (<= total-expect-forms 1778))
-        (t/is (<= total-namespaces-using-expect 114))))))
+        (t/is (<= total-expect-forms 1716))
+        (t/is (<= total-namespaces-using-expect 108))))))
 
 (defmacro ^:deprecated expect
   "Simple macro that simulates converts an Expectations-style `expect` form into a `clojure.test` `deftest` form."

test/metabase/api/database_test.clj

@@ -59,7 +59,7 @@
 (defn- table-details [table]
   (-> (merge (mt/obj->json->obj (mt/object-defaults Table))
              (select-keys table [:active :created_at :db_id :description :display_name :entity_name :entity_type
-                                 :fields_hash :id :name :rows :schema :updated_at :visibility_type]))
+                                 :fields_hash :id :name :schema :updated_at :visibility_type]))
       (update :entity_type #(when % (str "entity/" (name %))))
       (update :visibility_type #(when % (name %)))))
 
@@ -244,7 +244,6 @@
                                                  :database_position 1})]
                                 :segments     []
                                 :metrics      []
-                                :rows         nil
                                 :id           (mt/id :categories)
                                 :db_id        (mt/id)})]})
            (let [resp (mt/derecordize ((mt/user->client :rasta) :get 200 (format "database/%d/metadata" (mt/id))))]

test/metabase/api/field_test.clj

@@ -45,7 +45,6 @@
                                      :schema                  "PUBLIC"
                                      :name                    "USERS"
                                      :display_name            "Users"
-                                     :rows                    nil
                                      :entity_name             nil
                                      :active                  true
                                      :id                      (mt/id :users)

test/metabase/api/permissions_test.clj

 (ns metabase.api.permissions-test
   "Tests for `/api/permissions` endpoints."
   (:require [clojure.test :refer :all]
-            [expectations :refer [expect]]
+            [metabase
+             [test :as mt]
+             [util :as u]]
+            [metabase.api.permissions :as permissions-api]
             [metabase.models
              [database :refer [Database]]
              [permissions :as perms]
              [permissions-group :as group :refer [PermissionsGroup]]
              [table :refer [Table]]]
-            [metabase.test
-             [data :as data]
-             [fixtures :as fixtures]]
-            [metabase.test.data.users :as test-users]
-            [metabase.util :as u]
-            [toucan.util.test :as tt]))
+            [metabase.test.fixtures :as fixtures]
+            [metabase.util.schema :as su]
+            [schema.core :as s]))
 
+;; there are some issues where it doesn't look like the hydrate function for `member_count` is being added (?)
+(comment permissions-api/keep-me)
+
+;; make sure test users are created first, otherwise we're possibly going to have some WEIRD results
 (use-fixtures :once (fixtures/initialize :test-users))
 
 ;; GET /permissions/group
 ;; Should *not* include inactive users in the counts.
 ;; It should also *not* include the MetaBot group because MetaBot should *not* be enabled
 (defn- fetch-groups []
-  (set ((test-users/user->client :crowberto) :get 200 "permissions/group")))
-
-(expect
-  #{{:id (u/get-id (group/all-users)), :name "All Users",      :member_count 3}
-    {:id (u/get-id (group/admin)),     :name "Administrators", :member_count 1}}
-  ;; make sure test users are created first, otherwise we're possibly going to have some WEIRD results
-  (fetch-groups))
+  (set ((mt/user->client :crowberto) :get 200 "permissions/group")))
 
-;; The endpoint should however return empty groups!
-(tt/expect-with-temp [PermissionsGroup [group]]
-  #{{:id (u/get-id (group/all-users)), :name "All Users",      :member_count 3}
-    {:id (u/get-id (group/admin)),     :name "Administrators", :member_count 1}
-    (assoc (into {} group) :member_count 0)}
-  (fetch-groups))
+(deftest fetch-groups-test
+  (testing "GET /api/permissions/group"
+    (letfn [(check-default-groups-returned [id->group]
+              (testing "All Users Group should be returned"
+                (is (schema= {:id           (s/eq (:id (group/all-users)))
+                              :name         (s/eq "All Users")
+                              :member_count su/IntGreaterThanZero}
+                             (get id->group (:id (group/all-users))))))
+              (testing "Administrators Group should be returned"
+                (is (schema= {:id           (s/eq (:id (group/admin)))
+                              :name         (s/eq "Administrators")
+                              :member_count su/IntGreaterThanZero}
+                       (get id->group (:id (group/admin)))))))]
+      (let [id->group (u/key-by :id (fetch-groups))]
+        (check-default-groups-returned id->group))
 
+      (testing "should return empty groups"
+        (mt/with-temp PermissionsGroup [group]
+          (let [id->group (u/key-by :id (fetch-groups))]
+            (check-default-groups-returned id->group)
+            (testing "empty group should be returned"
+              (is (schema= {:id           su/IntGreaterThanZero
+                            :name         su/NonBlankString
+                            :member_count (s/eq 0)}
+                           (get id->group (:id group)))))))))))
 
-;; GET /permissions/group/:id
-;; Should *not* include inactive users
-(expect
-  #{{:first_name "Crowberto", :last_name "Corv",   :email "crowberto@metabase.com", :user_id (test-users/user->id :crowberto), :membership_id true}
-    {:first_name "Lucky",     :last_name "Pigeon", :email "lucky@metabase.com",     :user_id (test-users/user->id :lucky),     :membership_id true}
-    {:first_name "Rasta",     :last_name "Toucan", :email "rasta@metabase.com",     :user_id (test-users/user->id :rasta),     :membership_id true}}
-  (set
-   (for [member (:members ((test-users/user->client :crowberto) :get 200 (str "permissions/group/" (u/get-id (group/all-users)))))]
-     (update member :membership_id some?))))
+(deftest fetch-group-test
+  (testing "GET /permissions/group/:id"
+    (let [{:keys [members]} ((mt/user->client :crowberto) :get 200 (format "permissions/group/%d" (:id (group/all-users))))
+          id->member        (u/key-by :user_id members)]
+      (is (schema= {:first_name    (s/eq "Crowberto")
+                    :last_name     (s/eq "Corv")
+                    :email         (s/eq "crowberto@metabase.com")
+                    :user_id       (s/eq (mt/user->id :crowberto))
+                    :membership_id su/IntGreaterThanZero}
+                   (get id->member (mt/user->id :crowberto))))
+      (is (schema= {:first_name    (s/eq "Lucky")
+                    :last_name     (s/eq "Pigeon")
+                    :email         (s/eq "lucky@metabase.com")
+                    :user_id       (s/eq (mt/user->id :lucky))
+                    :membership_id su/IntGreaterThanZero}
+                   (get id->member (mt/user->id :lucky))))
+      (is (schema= {:first_name    (s/eq "Rasta")
+                    :last_name     (s/eq "Toucan")
+                    :email         (s/eq "rasta@metabase.com")
+                    :user_id       (s/eq (mt/user->id :rasta))
+                    :membership_id su/IntGreaterThanZero}
+                   (get id->member (mt/user->id :rasta))))
+      (testing "Should *not* include inactive users"
+        (is (= nil
+               (get id->member :trashbird)))))))
 
+(deftest update-perms-graph-test
+  (testing "PUT /api/permissions/graph"
+    (testing "make sure we can update the perms graph from the API"
+      (mt/with-temp PermissionsGroup [group]
+        ((mt/user->client :crowberto) :put 200 "permissions/graph"
+         (assoc-in (perms/graph)
+                   [:groups (u/get-id group) (mt/id) :schemas]
+                   {"PUBLIC" {(mt/id :venues) :all}}))
+        (is (= {(mt/id :venues) :all}
+               (get-in (perms/graph) [:groups (u/get-id group) (mt/id) :schemas "PUBLIC"]))))
 
-;; make sure we can update the perms graph from the API
-(expect
-  {(data/id :venues) :all}
-  (tt/with-temp PermissionsGroup [group]
-    ((test-users/user->client :crowberto) :put 200 "permissions/graph"
-     (assoc-in (perms/graph)
-               [:groups (u/get-id group) (data/id) :schemas]
-               {"PUBLIC" {(data/id :venues) :all}}))
-    (get-in (perms/graph) [:groups (u/get-id group) (data/id) :schemas "PUBLIC"])))
+      (testing "Table-specific perms"
+        (mt/with-temp PermissionsGroup [group]
+          ((mt/user->client :crowberto) :put 200 "permissions/graph"
+           (assoc-in (perms/graph)
+                     [:groups (u/get-id group) (mt/id) :schemas]
+                     {"PUBLIC" {(mt/id :venues) {:read :all, :query :segmented}}}))
+          (is (= {(mt/id :venues) {:read  :all
+                                   :query :segmented}}
+                 (get-in (perms/graph) [:groups (u/get-id group) (mt/id) :schemas "PUBLIC"]))))))
 
-(expect
-  {(data/id :venues) {:read  :all
-                      :query :segmented}}
-  (tt/with-temp PermissionsGroup [group]
-    ((test-users/user->client :crowberto) :put 200 "permissions/graph"
-     (assoc-in (perms/graph)
-               [:groups (u/get-id group) (data/id) :schemas]
-               {"PUBLIC" {(data/id :venues) {:read :all, :query :segmented}}}))
-    (get-in (perms/graph) [:groups (u/get-id group) (data/id) :schemas "PUBLIC"])))
+    (testing "permissions for new db"
+      (let [new-id (inc (mt/id))]
+        (mt/with-temp* [PermissionsGroup [group]
+                        Database         [{db-id :id}]
+                        Table            [_ {:db_id db-id}]]
+          ((mt/user->client :crowberto) :put 200 "permissions/graph"
+           (assoc-in (perms/graph)
+                     [:groups (u/get-id group) db-id :schemas]
+                     :all))
+          (is (= :all
+                 (get-in (perms/graph) [:groups (u/get-id group) db-id :schemas]))))))
 
-;; permissions for new db
-(expect
-  :all
-  (let [new-id (inc (data/id))]
-    (tt/with-temp* [PermissionsGroup [group]
-                    Database         [{db-id :id}]
-                    Table            [_ {:db_id db-id}]]
-      ((test-users/user->client :crowberto) :put 200 "permissions/graph"
-       (assoc-in (perms/graph)
-                 [:groups (u/get-id group) db-id :schemas]
-                 :all))
-      (get-in (perms/graph) [:groups (u/get-id group) db-id :schemas]))))
+    (testing "permissions for new db with no tables"
+      (let [new-id (inc (mt/id))]
+        (mt/with-temp* [PermissionsGroup [group]
+                        Database         [{db-id :id}]]
+          ((mt/user->client :crowberto) :put 200 "permissions/graph"
+           (assoc-in (perms/graph)
+                     [:groups (u/get-id group) db-id :schemas]
+                     :all))
+          (is (= :all
+                 (get-in (perms/graph) [:groups (u/get-id group) db-id :schemas]))))))))
 
-;; permissions for new db with no tables
-(expect
-  :all
-  (let [new-id (inc (data/id))]
-    (tt/with-temp* [PermissionsGroup [group]
-                    Database         [{db-id :id}]]
-      ((test-users/user->client :crowberto) :put 200 "permissions/graph"
-       (assoc-in (perms/graph)
-                 [:groups (u/get-id group) db-id :schemas]
-                 :all))
-      (get-in (perms/graph) [:groups (u/get-id group) db-id :schemas]))))
+;;

test/metabase/models/field_values_test.clj

 (ns metabase.models.field-values-test
   "Tests for specific behavior related to FieldValues and functions in the `metabase.models.field-values` namespace."
-  (:require [clojure.java.jdbc :as jdbc]
-            [expectations :refer :all]
+  (:require [clojure
+             [string :as str]
+             [test :refer :all]]
+            [clojure.java.jdbc :as jdbc]
             [metabase
              [db :as mdb]
              [sync :as sync]
+             [test :as mt]
              [util :as u]]
+            [metabase.db.metadata-queries :as metadata-queries]
             [metabase.models
              [database :refer [Database]]
              [field :refer [Field]]
-             [field-values :refer :all]
+             [field-values :as field-values :refer :all]
              [table :refer [Table]]]
-            [toucan.db :as db]
-            [toucan.util.test :as tt]))
-
-;;; ---------------------------------------- field-should-have-field-values? -----------------------------------------
-
-(expect (field-should-have-field-values? {:has_field_values :list
-                                          :visibility_type  :normal
-                                          :base_type        :type/Text}))
-
-(expect false (field-should-have-field-values? {:has_field_values :list
-                                                :visibility_type  :sensitive
-                                                :base_type        :type/Text}))
-
-(expect false (field-should-have-field-values? {:has_field_values :list
-                                                :visibility_type  :hidden
-                                                :base_type        :type/Text}))
-
-(expect false (field-should-have-field-values? {:has_field_values :list
-                                                :visibility_type  :details-only
-                                                :base_type        :type/Text}))
-
-(expect false (field-should-have-field-values? {:has_field_values nil
-                                                :visibility_type :normal
-                                                :base_type       :type/Text}))
-
-(expect (field-should-have-field-values? {:has_field_values :list
-                                          :visibility_type  :normal
-                                          :base_type        :type/Text}))
-
-(expect (field-should-have-field-values? {:has_field_values :list
-                                          :special_type     :type/Category
-                                          :visibility_type  :normal
-                                          :base_type        "type/Boolean"}))
-
-
-;; retired/sensitive/hidden/details-only fields should always be excluded
-(expect false (field-should-have-field-values? {:base_type        :type/Boolean
-                                                :has_field_values :list
-                                                :visibility_type  :retired}))
-
-(expect false (field-should-have-field-values? {:base_type        :type/Boolean
-                                                :has_field_values :list
-                                                :visibility_type  :sensitive}))
-
-(expect false (field-should-have-field-values? {:base_type        :type/Boolean
-                                                :has_field_values :list
-                                                :visibility_type  :hidden}))
-
-(expect false (field-should-have-field-values? {:base_type        :type/Boolean
-                                                :has_field_values :list
-                                                :visibility_type  :details-only}))
-
-;; date/time based fields should always be excluded
-(expect false (field-should-have-field-values? {:base_type        :type/Date
-                                                :has_field_values :list
-                                                :visibility_type  :normal}))
-
-(expect false (field-should-have-field-values? {:base_type        :type/DateTime
-                                                :has_field_values :list
-                                                :visibility_type  :normal}))
-
-(expect false (field-should-have-field-values? {:base_type        :type/Time
-                                                :has_field_values :list
-                                                :visibility_type  :normal}))
-
-
-;;; ------------------------------------------------ everything else -------------------------------------------------
-
-(expect
-  [[1 2 3]
-   nil]
-  (tt/with-temp* [Database    [{database-id :id}]
+            [toucan.db :as db]))
+
+(deftest field-should-have-field-values?-test
+  (doseq [[group input->expected] {"Text and Category Fields"
+                                   {{:has_field_values :list
+                                     :visibility_type  :normal
+                                     :base_type        :type/Text}
+                                    true
+
+                                    {:has_field_values nil
+                                     :visibility_type  :normal
+                                     :base_type        :type/Text}
+                                    false
+
+                                    {:has_field_values :list
+                                     :special_type     :type/Category
+                                     :visibility_type  :normal
+                                     :base_type        "type/Boolean"}
+                                    true}
+
+                                   "retired/sensitive/hidden/details-only fields should always be excluded"
+                                   {{:base_type        :type/Boolean
+                                     :has_field_values :list
+                                     :visibility_type  :retired}
+                                    false
+
+                                    {:base_type        :type/Boolean
+                                     :has_field_values :list
+                                     :visibility_type  :sensitive}
+                                    false
+
+                                    {:has_field_values :list
+                                     :visibility_type  :sensitive
+                                     :base_type        :type/Text}
+                                    false
+
+                                    {:base_type        :type/Boolean
+                                     :has_field_values :list
+                                     :visibility_type  :hidden}
+                                    false
+
+                                    {:has_field_values :list
+                                     :visibility_type  :hidden
+                                     :base_type        :type/Text}
+                                    false
+
+                                    {:base_type        :type/Boolean
+                                     :has_field_values :list
+                                     :visibility_type  :details-only}
+                                    false
+
+                                    {:has_field_values :list
+                                     :visibility_type  :details-only
+                                     :base_type        :type/Text}
+                                    false}
+
+                                   "date/time based fields should always be excluded"
+                                   {{:base_type        :type/Date
+                                     :has_field_values :list
+                                     :visibility_type  :normal}
+                                    false
+
+                                    {:base_type        :type/DateTime
+                                     :has_field_values :list
+                                     :visibility_type  :normal}
+                                    false
+
+                                    {:base_type        :type/Time
+                                     :has_field_values :list
+                                     :visibility_type  :normal}
+                                    false}}
+          [input expected] input->expected]
+    (testing (str group "\n")
+      (testing (pr-str (list 'field-should-have-field-values? input))
+        (is (= expected
+               (#'field-values/field-should-have-field-values? input)))))))
+
+(deftest distinct-values-test
+  (with-redefs [metadata-queries/field-distinct-values (constantly [1 2 3 4])]
+    (is (= [1 2 3 4]
+           (#'field-values/distinct-values {}))))
+
+  (testing "(#2332) check that if field values are long we skip over them"
+    (with-redefs [metadata-queries/field-distinct-values (constantly [(str/join (repeat 50000 "A"))])]
+      (is (= nil
+             (#'field-values/distinct-values {}))))))
+
+(deftest clear-field-values!-test
+  (mt/with-temp* [Database    [{database-id :id}]
                   Table       [{table-id :id} {:db_id database-id}]
                   Field       [{field-id :id} {:table_id table-id}]
                   FieldValues [_              {:field_id field-id, :values "[1,2,3]"}]]
-    [(db/select-one-field :values FieldValues, :field_id field-id)
-     (do
-       (clear-field-values! field-id)
-       (db/select-one-field :values FieldValues, :field_id field-id))]))
+    (is (= [1 2 3]
+           (db/select-one-field :values FieldValues, :field_id field-id)))
+    (#'field-values/clear-field-values! field-id)
+    (is (= nil
+           (db/select-one-field :values FieldValues, :field_id field-id)))))
 
 (defn- find-values [field-values-id]
   (-> (db/select-one FieldValues :id field-values-id)
@@ -99,48 +121,52 @@
   (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))]))))))
+(deftest update-human-readable-values-test
+  (testing "Test \"fixing\" of human readable values when field values change"
+    (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
+        (mt/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
+          (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\"]"})
+            (let [expected-original-values {:values                [1 2 3]
+                                            :human_readable_values ["a" "b" "c"]}
+                  expected-updated-values  {:values                [-2 -1 0 1 2 3]
+                                            :human_readable_values ["-2" "-1" "0" "a" "b" "c"]}]
+              (is (= expected-original-values
+                     (find-values field-values-id)))
+
+              (testing "There should be no changes to human_readable_values when resync'd"
+                (is (= expected-original-values
+                       (sync-and-find-values db field-values-id))))
+
+              (testing "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"}])
+                (testing "Sync to pickup the new field values and rebuild the human_readable_values"
+                  (is (= expected-updated-values
+                         (sync-and-find-values db field-values-id)))))
+
+              (testing "Resyncing this (with the new field values) should result in the same human_readable_values"
+                (is (= expected-updated-values
+                       (sync-and-find-values db field-values-id))))
+
+              (testing "Test that field values can be removed and the corresponding human_readable_values are removed as well"
+                (jdbc/delete! conn :foo ["id in (?,?,?)" 1 2 3])
+                (is (= {:values [-2 -1 0] :human_readable_values ["-2" "-1" "0"]}
+                       (sync-and-find-values db field-values-id)))))))))))

test/metabase/sync/analyze/table_row_count_test.clj

-(ns metabase.sync.analyze.table-row-count-test
-  "Tests for the sync logic that updates a Table's row count."
-  (:require [metabase.models.table :refer [Table]]
-            [metabase.sync.analyze.table-row-count :as table-row-count]
-            [metabase.test :as mt]
-            [metabase.test
-             [data :as data]
-             [util :as tu]]
-            [metabase.test.data.datasets :as datasets]
-            [toucan.db :as db]))
-
-;; test that syncing table row counts works
-;; TODO - write a Druid version of this test. Works slightly differently since Druid doesn't have a 'venues' table
-;; TODO - not sure why this doesn't work on Oracle. Seems to be an issue with the test rather than with the Oracle driver
-(datasets/expect-with-drivers (mt/normal-drivers-except #{:oracle})
-  100
-  (tu/with-temp-vals-in-db Table (data/id :venues) {:rows 0}
-      (table-row-count/update-row-count! (Table (data/id :venues)))
-      (db/select-one-field :rows Table :id (data/id :venues))))

test/metabase/sync_database/analyze_test.clj

 (ns metabase.sync-database.analyze-test
   "TODO - this namespace follows the old pattern of sync namespaces. Tests should be moved to appropriate new homes at
   some point"
-  (:require [clojure.string :as str]
-            [expectations :refer :all]
-            [metabase.db.metadata-queries :as metadata-queries]
+  (:require [clojure.test :refer :all]
+            [metabase
+             [test :as mt]
+             [util :as u]]
             [metabase.models
              [database :refer [Database]]
              [field :as field :refer [Field]]
-             [field-values :as field-values]
              [table :as table :refer [Table]]]
             [metabase.sync.analyze :as analyze]
             [metabase.sync.analyze.classifiers.text-fingerprint :as classify-text-fingerprint]
             [metabase.sync.analyze.fingerprint.fingerprinters :as fingerprinters]
-            [metabase.test.data :as data]
             [metabase.test.data.users :refer :all]
-            [metabase.util :as u]
-            [toucan.db :as db]
-            [toucan.util.test :as tt]))
+            [toucan.db :as db]))
 
-;; distinct-values
-;; (#2332) check that if field values are long we skip over them
-;; TODO - the next two should probably be moved into field-values-test
-(expect
-  nil
-  (with-redefs [metadata-queries/field-distinct-values (constantly [(str/join (repeat 50000 "A"))])]
-    (#'field-values/distinct-values {})))
-
-(expect
-  [1 2 3 4]
-  (with-redefs [metadata-queries/field-distinct-values (constantly [1 2 3 4])]
-    (#'field-values/distinct-values {})))
-
-
-;;; ## mark-json-field!
-
-(defn- values-are-valid-json? [values]
+(defn- classified-special-type [values]
   (let [field (field/map->FieldInstance {:base_type :type/Text})]
-    (= (:special_type (classify-text-fingerprint/infer-special-type field (transduce identity (fingerprinters/fingerprinter field) values)))
-       :type/SerializedJSON)))
-
-;; When all the values are valid JSON dicts they're valid JSON
-(expect
-  (values-are-valid-json? ["{\"this\":\"is\",\"valid\":\"json\"}"
-                           "{\"this\":\"is\",\"valid\":\"json\"}"
-                           "{\"this\":\"is\",\"valid\":\"json\"}"]))
-
-;; When all the values are valid JSON arrays they're valid JSON
-(expect
-  (values-are-valid-json? ["[1, 2, 3, 4]"
-                           "[1, 2, 3, 4]"
-                           "[1, 2, 3, 4]"]))
-
-;; Some combo of both can still be marked as JSON
-(expect
-  (values-are-valid-json? ["{\"this\":\"is\",\"valid\":\"json\"}"
-                           "[1, 2, 3, 4]"
-                           "[1, 2, 3, 4]"]))
-
-;; Check that things that aren't dictionaries or arrays aren't marked as JSON
-(expect false (values-are-valid-json? ["\"A JSON string should not cause a Field to be marked as JSON\""]))
-(expect false (values-are-valid-json? ["100"]))
-(expect false (values-are-valid-json? ["true"]))
-(expect false (values-are-valid-json? ["false"]))
-
-;; Check that things that are valid emails are marked as Emails
-
-(defn- values-are-valid-emails? [values]
-  (let [field (field/map->FieldInstance {:base_type :type/Text})]
-    (= (:special_type (classify-text-fingerprint/infer-special-type field (transduce identity (fingerprinters/fingerprinter field) values)))
-       :type/Email)))
-
-(expect true (values-are-valid-emails? ["helper@metabase.com"]))
-(expect true (values-are-valid-emails? ["helper@metabase.com", "someone@here.com", "help@nope.com"]))
-
-(expect false (values-are-valid-emails? ["helper@metabase.com", "1111IsNot!An....email", "help@nope.com"]))
-(expect false (values-are-valid-emails? ["\"A string should not cause a Field to be marked as email\""]))
-(expect false (values-are-valid-emails? ["true"]))
-(expect false (values-are-valid-emails? ["false"]))
+    (println "RESULT =" (classify-text-fingerprint/infer-special-type
+                         field
+                         (transduce identity (fingerprinters/fingerprinter field) values)))
+    (:special_type (classify-text-fingerprint/infer-special-type
+                    field
+                    (transduce identity (fingerprinters/fingerprinter field) values)))))
+
+(deftest classify-json-test
+  (doseq [[group values->expected] {"When all the values are valid JSON dicts they're valid JSON"
+                                    {["{\"this\":\"is\",\"valid\":\"json\"}"
+                                      "{\"this\":\"is\",\"valid\":\"json\"}"
+                                      "{\"this\":\"is\",\"valid\":\"json\"}"] true}
+
+                                    "When all the values are valid JSON arrays they're valid JSON"
+                                    {["[1, 2, 3, 4]"
+                                      "[1, 2, 3, 4]"
+                                      "[1, 2, 3, 4]"] true}
+
+                                    "Some combo of both can still be marked as JSON"
+                                    {["{\"this\":\"is\",\"valid\":\"json\"}"
+                                      "[1, 2, 3, 4]"
+                                      "[1, 2, 3, 4]"] true}
+
+                                    "Check that things that aren't dictionaries or arrays aren't marked as JSON"
+                                    {["\"A JSON string should not cause a Field to be marked as JSON\""] false
+                                     ["100"]                                                             false
+                                     ["true"]                                                            false
+                                     ["false"]                                                           false}}
+          [values expected] values->expected]
+    (testing (str group "\n")
+      (testing (pr-str values)
+        (is (= (when expected :type/SerializedJSON)
+               (classified-special-type values)))))))
+
+(deftest classify-emails-test
+  (testing "Check that things that are valid emails are marked as Emails")
+  (doseq [[values expected] {["helper@metabase.com"]                                           true
+                             ["helper@metabase.com", "someone@here.com", "help@nope.com"]      true
+                             ["helper@metabase.com", "1111IsNot!An....email", "help@nope.com"] false
+                             ["\"A string should not cause a Field to be marked as email\""]   false
+                             ["true"]                                                          false
+                             ["false"]                                                         false}]
+    (testing (pr-str values)
+      (is (= (when expected :type/Email)
+             (classified-special-type values))))))
 
 
 ;;; +----------------------------------------------------------------------------------------------------------------+
@@ -96,84 +81,85 @@
     {:order-by [[:last_analyzed :desc]]}))
 
 (defn- set-table-visibility-type-via-api!
-  "Change the VISIBILITY-TYPE of TABLE via an API call.
-   (This is done via the API so we can see which, if any, side effects (e.g. analysis) get triggered.)"
+  "Change the `visibility-type` of `table` via an API call. (This is done via the API so we can see which, if any, side
+  effects (e.g. analysis) get triggered.)"
   [table visibility-type]
   ((user->client :crowberto) :put 200 (format "table/%d" (:id table)) {:display_name    "hiddentable"
                                                                        :visibility_type visibility-type
                                                                        :description     "What a nice table!"}))
 
 (defn- api-sync!
-  "Trigger a sync of TABLE via the API."
+  "Trigger a sync of `table` via the API."
   [table]
   ((user->client :crowberto) :post 200 (format "database/%d/sync" (:db_id table))))
 
 ;; use these functions to create fake Tables & Fields that are actually backed by something real in the database.
 ;; Otherwise when we go to resync them the logic will figure out Table/Field doesn't exist and mark it as inactive
 (defn- fake-table [& {:as additional-options}]
-  (merge {:rows 15, :db_id (data/id), :name "VENUES"}
+  (merge {:db_id (mt/id), :name "VENUES"}
          additional-options))
 
 (defn- fake-field [table & {:as additional-options}]
   (merge {:table_id (u/get-id table), :name "PRICE", :base_type "type/Integer"}
          additional-options))
 
-;; expect all the kinds of hidden tables to stay un-analyzed through transitions and repeated syncing
-(expect
-  false
-  (tt/with-temp* [Table [table (fake-table)]
-                  Field [field (fake-field table)]]
-    (set-table-visibility-type-via-api! table "hidden")
-    (api-sync! table)
-    (set-table-visibility-type-via-api! table "cruft")
-    (set-table-visibility-type-via-api! table "cruft")
-    (api-sync! table)
-    (set-table-visibility-type-via-api! table "technical")
-    (api-sync! table)
-    (set-table-visibility-type-via-api! table "technical")
-    (api-sync! table)
-    (api-sync! table)
-    (fake-field-was-analyzed? field)))
-
-;; same test not coming through the api
 (defn- analyze-table! [table]
   ;; we're calling `analyze-db!` instead of `analyze-table!` because the latter doesn't care if you try to sync a
   ;; hidden table and will allow that. TODO - Does that behavior make sense?
   (analyze/analyze-db! (Database (:db_id table))))
 
-(expect
-  false
-  (tt/with-temp* [Table [table (fake-table)]
-                  Field [field (fake-field table)]]
-    (set-table-visibility-type-via-api! table "hidden")
-    (analyze-table! table)
-    (set-table-visibility-type-via-api! table "cruft")
-    (set-table-visibility-type-via-api! table "cruft")
-    (analyze-table! table)
-    (set-table-visibility-type-via-api! table "technical")
-    (analyze-table! table)
-    (set-table-visibility-type-via-api! table "technical")
-    (analyze-table! table)
-    (analyze-table! table)
-    (fake-field-was-analyzed? field)))
-
-;; un-hiding a table should cause it to be analyzed
-(expect
-  (tt/with-temp* [Table [table (fake-table)]
-                  Field [field (fake-field table)]]
-    (set-table-visibility-type-via-api! table "hidden")
-    (set-table-visibility-type-via-api! table nil)
-    (fake-field-was-analyzed? field)))
-
-;; re-hiding a table should not cause it to be analyzed
-(expect
-  ;; create an initially hidden table
-  (tt/with-temp* [Table [table (fake-table :visibility_type "hidden")]
-                  Field [field (fake-field table)]]
-    ;; switch the table to visible (triggering a sync) and get the last sync time
-    (let [last-sync-time (do (set-table-visibility-type-via-api! table nil)
-                             (latest-sync-time table))]
-      ;; now make it hidden again
+(deftest dont-analyze-hidden-tables-test
+  (testing "expect all the kinds of hidden tables to stay un-analyzed through transitions and repeated syncing"
+    (mt/with-temp* [Table [table (fake-table)]
+                    Field [field (fake-field table)]]
+      (set-table-visibility-type-via-api! table "hidden")
+      (api-sync! table)
+      (set-table-visibility-type-via-api! table "cruft")
+      (set-table-visibility-type-via-api! table "cruft")
+      (api-sync! table)
+      (set-table-visibility-type-via-api! table "technical")
+      (api-sync! table)
+      (set-table-visibility-type-via-api! table "technical")
+      (api-sync! table)
+      (api-sync! table)
+      (is (= false
+             (fake-field-was-analyzed? field))))
+
+    (testing "same test not coming through the api"
+      (mt/with-temp* [Table [table (fake-table)]
+                      Field [field (fake-field table)]]
+        (set-table-visibility-type-via-api! table "hidden")
+        (analyze-table! table)
+        (set-table-visibility-type-via-api! table "cruft")
+        (set-table-visibility-type-via-api! table "cruft")
+        (analyze-table! table)
+        (set-table-visibility-type-via-api! table "technical")
+        (analyze-table! table)
+        (set-table-visibility-type-via-api! table "technical")
+        (analyze-table! table)
+        (analyze-table! table)
+        (is (= false
+               (fake-field-was-analyzed? field)))))))
+
+(deftest analyze-unhidden-tables-test
+  (testing "un-hiding a table should cause it to be analyzed"
+    (mt/with-temp* [Table [table (fake-table)]
+                    Field [field (fake-field table)]]
       (set-table-visibility-type-via-api! table "hidden")
-      ;; sync time shouldn't change
-      (= last-sync-time (latest-sync-time table)))))
+      (set-table-visibility-type-via-api! table nil)
+      (is (= true
+             (fake-field-was-analyzed? field))))))
+
+(deftest dont-analyze-rehidden-table-test
+  (testing "re-hiding a table should not cause it to be analyzed"
+    ;; create an initially hidden table
+    (mt/with-temp* [Table [table (fake-table :visibility_type "hidden")]
+                    Field [field (fake-field table)]]
+      ;; switch the table to visible (triggering a sync) and get the last sync time
+      (let [last-sync-time (do (set-table-visibility-type-via-api! table nil)
+                               (latest-sync-time table))]
+        ;; now make it hidden again
+        (set-table-visibility-type-via-api! table "hidden")
+        (is (= last-sync-time
+               (latest-sync-time table))
+            "sync time shouldn't change")))))

test/metabase/sync_database_test.clj

@@ -93,56 +93,39 @@
                                              (update :fingerprint_version (complement zero?))))))
                tu/boolean-ids-and-timestamps)))
 
-(def ^:private table-defaults
-  {:active                  true
-   :caveats                 nil
-   :created_at              true
-   :db_id                   true
-   :description             nil
-   :entity_name             nil
-   :entity_type             :entity/GenericTable
-   :id                      true
-   :points_of_interest      nil
-   :rows                    nil
-   :schema                  nil
-   :show_in_getting_started false
-   :updated_at              true
-   :visibility_type         nil
-   :fields_hash             true
-   :field_order             :database})
-
-(def ^:private field-defaults
-  {:active              true
-   :caveats             nil
-   :created_at          true
-   :description         nil
-   :fingerprint         false
-   :fingerprint_version false
-   :fk_target_field_id  false
-   :has_field_values    nil
-   :id                  true
-   :last_analyzed       false
-   :parent_id           false
-   :points_of_interest  nil
-   :position            0
-   :database_position   0
-   :custom_position     0
-   :preview_display     true
-   :special_type        nil
-   :table_id            true
-   :updated_at          true
-   :visibility_type     :normal
-   :settings            nil})
-
-(def ^:private field-defaults-with-fingerprint
-  (assoc field-defaults
+(defn- table-defaults []
+  (merge
+   (mt/object-defaults Table)
+   {:created_at  true
+    :db_id       true
+    :entity_type :entity/GenericTable
+    :id          true
+    :updated_at  true
+    :fields_hash true}))
+
+(defn- field-defaults []
+  (merge
+   (mt/object-defaults Field)
+   {:created_at          true
+    :fingerprint         false
+    :fingerprint_version false
+    :fk_target_field_id  false
+    :id                  true
+    :last_analyzed       false
+    :parent_id           false
+    :position            0
+    :table_id            true
+    :updated_at          true}))
+
+(defn- field-defaults-with-fingerprint []
+  (assoc (field-defaults)
     :last_analyzed       true
     :fingerprint_version true
     :fingerprint         true))
 
 (def ^:private field:movie-id
   (merge
-   field-defaults
+   (field-defaults)
    {:name              "id"
     :display_name      "ID"
     :database_type     "SERIAL"
@@ -151,9 +134,9 @@
     :database_position 0
     :position          0}))
 
-(def ^:private field:movie-studio
+(defn- field:movie-studio []
   (merge
-   field-defaults-with-fingerprint
+   (field-defaults-with-fingerprint)
    {:name               "studio"
     :display_name       "Studio"
     :database_type      "VARCHAR"
@@ -163,9 +146,9 @@
     :database_position  2
     :position           2}))
 
-(def ^:private field:movie-title
+(defn- field:movie-title []
   (merge
-   field-defaults-with-fingerprint
+   (field-defaults-with-fingerprint)
    {:name              "title"
     :display_name      "Title"
     :database_type     "VARCHAR"
@@ -174,9 +157,9 @@
     :database_position 1
     :position          1}))
 
-(def ^:private field:studio-name
+(defn- field:studio-name []
   (merge
-   field-defaults-with-fingerprint
+   (field-defaults-with-fingerprint)
    {:name              "name"
     :display_name      "Name"
     :database_type     "VARCHAR"
@@ -186,9 +169,9 @@
     :position          1}))
 
 ;; `studio.studio`? huh?
-(def ^:private field:studio-studio
+(defn- field:studio-studio []
   (merge
-   field-defaults
+   (field-defaults)
    {:name              "studio"
     :display_name      "Studio"
     :database_type     "VARCHAR"
@@ -202,29 +185,34 @@
     (sync/sync-database! db)
     (sync/sync-database! db)
     (let [[movie studio] (mapv table-details (db/select Table :db_id (u/get-id db) {:order-by [:name]}))]
-      (is (= (merge table-defaults {:schema       "default"
-                                    :name         "movie"
-                                    :display_name "Movie"
-                                    :fields       [field:movie-id field:movie-studio field:movie-title]})
-             movie))
-      (is (= (merge table-defaults {:name         "studio"
-                                    :display_name "Studio"
-                                    :fields       [field:studio-name field:studio-studio]})
-             studio)))))
-
+      (testing "`movie` Table"
+        (is (= (merge
+                (table-defaults)
+                {:schema       "default"
+                 :name         "movie"
+                 :display_name "Movie"
+                 :fields       [field:movie-id (field:movie-studio) (field:movie-title)]})
+               movie)))
+      (testing "`studio` Table"
+        (is (= (merge
+                (table-defaults)
+                {:name         "studio"
+                 :display_name "Studio"
+                 :fields       [(field:studio-name) (field:studio-studio)]})
+               studio))))))
 
 (deftest sync-table-test
   (mt/with-temp* [Database [db {:engine :metabase.sync-database-test/sync-test}]
                   Table    [table {:name "movie", :schema "default", :db_id (u/get-id db)}]]
     (sync/sync-table! table)
     (is (= (merge
-            table-defaults
+            (table-defaults)
             {:schema       "default"
              :name         "movie"
              :display_name "Movie"
              :fields       [field:movie-id
-                            (assoc field:movie-studio :fk_target_field_id false :special_type nil)
-                            field:movie-title]})
+                            (assoc (field:movie-studio) :fk_target_field_id false :special_type nil)
+                            (field:movie-title)]})
            (table-details (Table (:id table)))))))