diff --git a/resources/migrations/000_migrations.yaml b/resources/migrations/000_migrations.yaml
index cf2875d2aa6233c11ec99f737e676f2dd91ef65c..db651b4e310dcc7f1f04377a87d5b7474347a203 100644
--- a/resources/migrations/000_migrations.yaml
+++ b/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
diff --git a/src/metabase/api/table.clj b/src/metabase/api/table.clj
index 3bb08c04cfe34cda3b647b472c066cb9c8964fbf..9150e85f594b5cc7fc58122870502a167ced393d 100644
--- a/src/metabase/api/table.clj
+++ b/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."
diff --git a/src/metabase/sync/analyze.clj b/src/metabase/sync/analyze.clj
index 0a4d07b4f9416f23f52bc607faaf2f70118044d1..51b1bc53a24b94a25c5b6d9d1373f1b1fcc3471b 100644
--- a/src/metabase/sync/analyze.clj
+++ b/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)
diff --git a/src/metabase/sync/analyze/table_row_count.clj b/src/metabase/sync/analyze/table_row_count.clj
deleted file mode 100644
index 8efc35e9fb962b6808a351f630bc921cdee0aa00..0000000000000000000000000000000000000000
--- a/src/metabase/sync/analyze/table_row_count.clj
+++ /dev/null
@@ -1,26 +0,0 @@
-(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))))
diff --git a/test/expectations.clj b/test/expectations.clj
index 84d31218b628b915281c4f5e6c5d147310062ee7..3e7f112aa92e3ca890d4bd194e2c6368a3fc4684 100644
--- a/test/expectations.clj
+++ b/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."
diff --git a/test/metabase/api/database_test.clj b/test/metabase/api/database_test.clj
index f3f19280bc58aa843c7a96e8f3bd145d3de3ba9b..bc1436629c406fbb51954dc0ba00ff319aef2f0f 100644
--- a/test/metabase/api/database_test.clj
+++ b/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))))]
diff --git a/test/metabase/api/field_test.clj b/test/metabase/api/field_test.clj
index 2d902159a023065c66a56667b58d5ade545b8703..a3d111efd83c7574c361eabc48a7c1766ff64063 100644
--- a/test/metabase/api/field_test.clj
+++ b/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)
diff --git a/test/metabase/api/permissions_test.clj b/test/metabase/api/permissions_test.clj
index 00ba48e6549201e1c63a2aa7d90a06bdbaf8be92..a639afe69ab0c003244df386a46a3bce8621ee0f 100644
--- a/test/metabase/api/permissions_test.clj
+++ b/test/metabase/api/permissions_test.clj
@@ -1,93 +1,125 @@
(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]))))
+;;
diff --git a/test/metabase/api/table_test.clj b/test/metabase/api/table_test.clj
index 1eb1a4083f520b0e8cc97d2d42c10cfb236787ce..9160c3f08e545671ec9410e9e539c7c1395848ad 100644
--- a/test/metabase/api/table_test.clj
+++ b/test/metabase/api/table_test.clj
@@ -4,7 +4,6 @@
[clojure
[test :refer :all]
[walk :as walk]]
- [expectations :refer [expect]]
[medley.core :as m]
[metabase
[http-client :as http]
@@ -22,7 +21,6 @@
[metabase.test
[data :as data]
[util :as tu]]
- [metabase.test.data.users :as test-users]
[metabase.test.mock.util :as mutil]
[metabase.timeseries-query-processor-test.util :as tqpt]
[toucan.db :as db]))
@@ -55,42 +53,27 @@
:auto_run_queries true}))
(defn- table-defaults []
- {:description nil
- :caveats nil
- :points_of_interest nil
- :show_in_getting_started false
- :entity_type "entity/GenericTable"
- :visibility_type nil
- :db (db-details)
- :entity_name nil
- :active true
- :db_id (mt/id)
- :segments []
- :metrics []
- :field_order "database"})
-
-(def ^:private field-defaults
- {:description nil
- :active true
- :position 0
- :database_position 0
- :custom_position 0
- :target nil
- :preview_display true
- :visibility_type "normal"
- :caveats nil
- :points_of_interest nil
- :special_type nil
- :parent_id nil
- :dimensions []
- :dimension_options []
- :has_field_values nil
- :default_dimension_option nil
- :settings nil})
+ (merge
+ (mt/object-defaults Table)
+ {:db (db-details)
+ :entity_type "entity/GenericTable"
+ :field_order "database"
+ :metrics []
+ :segments []}))
+
+(defn- field-defaults []
+ (merge
+ (mt/object-defaults Field)
+ {:default_dimension_option nil
+ :dimension_options []
+ :dimensions []
+ :position 0
+ :target nil
+ :visibility_type "normal"}))
(defn- field-details [field]
(merge
- field-defaults
+ (field-defaults)
(select-keys
field
[:created_at :fingerprint :fingerprint_version :fk_target_field_id :id :last_analyzed :updated_at])))
@@ -100,33 +83,29 @@
(dissoc :dimension_options :default_dimension_option)))
-;; ## GET /api/table
-;; These should come back in alphabetical order and include relevant metadata
-(expect
- #{{:name (mt/format-name "categories")
- :display_name "Categories"
- :rows 0
- :id (mt/id :categories)
- :entity_type "entity/GenericTable"}
- {:name (mt/format-name "checkins")
- :display_name "Checkins"
- :rows 0
- :id (mt/id :checkins)
- :entity_type "entity/EventTable"}
- {:name (mt/format-name "users")
- :display_name "Users"
- :rows 0
- :id (mt/id :users)
- :entity_type "entity/UserTable"}
- {:name (mt/format-name "venues")
- :display_name "Venues"
- :rows 0
- :id (mt/id :venues)
- :entity_type "entity/GenericTable"}}
- (->> ((mt/user->client :rasta) :get 200 "table")
- (filter #(= (:db_id %) (mt/id))) ; prevent stray tables from affecting unit test results
- (map #(select-keys % [:name :display_name :rows :id :entity_type]))
- set))
+(deftest list-table-test
+ (testing "GET /api/table"
+ (testing "These should come back in alphabetical order and include relevant metadata"
+ (is (= #{{:name (mt/format-name "categories")
+ :display_name "Categories"
+ :id (mt/id :categories)
+ :entity_type "entity/GenericTable"}
+ {:name (mt/format-name "checkins")
+ :display_name "Checkins"
+ :id (mt/id :checkins)
+ :entity_type "entity/EventTable"}
+ {:name (mt/format-name "users")
+ :display_name "Users"
+ :id (mt/id :users)
+ :entity_type "entity/UserTable"}
+ {:name (mt/format-name "venues")
+ :display_name "Venues"
+ :id (mt/id :venues)
+ :entity_type "entity/GenericTable"}}
+ (->> ((mt/user->client :rasta) :get 200 "table")
+ (filter #(= (:db_id %) (mt/id))) ; prevent stray tables from affecting unit test results
+ (map #(select-keys % [:name :display_name :id :entity_type]))
+ set))))))
(deftest get-table-test
(testing "GET /api/table/:id"
@@ -136,7 +115,6 @@
{:schema "PUBLIC"
:name "VENUES"
:display_name "Venues"
- :rows nil
:pk_field (table/pk-field-id (Table (mt/id :venues)))
:id (mt/id :venues)
:db_id (mt/id)})
@@ -159,163 +137,128 @@
(-> (table-defaults)
(assoc :dimension_options (default-dimension-options))))
-;; ## GET /api/table/:id/query_metadata
-(expect
- (merge
- (query-metadata-defaults)
- (db/select-one [Table :created_at :updated_at :fields_hash] :id (mt/id :categories))
- {:schema "PUBLIC"
- :name "CATEGORIES"
- :display_name "Categories"
- :fields [(merge
- (field-details (Field (mt/id :categories :id)))
- {:table_id (mt/id :categories)
- :special_type "type/PK"
- :name "ID"
- :display_name "ID"
- :database_type "BIGINT"
- :base_type "type/BigInteger"
- :has_field_values "none"})
- (merge
- (field-details (Field (mt/id :categories :name)))
- {:table_id (mt/id :categories)
- :special_type "type/Name"
- :name "NAME"
- :display_name "Name"
- :database_type "VARCHAR"
- :base_type "type/Text"
- :dimension_options []
- :default_dimension_option nil
- :has_field_values "list"
- :database_position 1
- :position 1})]
- :rows nil
- :id (mt/id :categories)})
- ((mt/user->client :rasta) :get 200 (format "table/%d/query_metadata" (mt/id :categories))))
-
-;;; GET api/table/:id/query_metadata?include_sensitive_fields
-;; Make sure that getting the User table *does* include info about the password field, but not actual values
-;; themselves
(deftest sensitive-fields-included-test
- (testing "Sensitive fields are included"
- (is (= (merge
- (query-metadata-defaults)
- (db/select-one [Table :created_at :updated_at :fields_hash] :id (mt/id :users))
- {:schema "PUBLIC"
- :name "USERS"
- :display_name "Users"
- :entity_type "entity/UserTable"
- :fields [(assoc (field-details (Field (mt/id :users :id)))
- :special_type "type/PK"
- :table_id (mt/id :users)
- :name "ID"
- :display_name "ID"
- :database_type "BIGINT"
- :base_type "type/BigInteger"
- :visibility_type "normal"
- :has_field_values "none")
- (assoc (field-details (Field (mt/id :users :name)))
- :special_type "type/Name"
- :table_id (mt/id :users)
- :name "NAME"
- :display_name "Name"
- :database_type "VARCHAR"
- :base_type "type/Text"
- :visibility_type "normal"
- :dimension_options []
- :default_dimension_option nil
- :has_field_values "list"
- :position 1
- :database_position 1)
- (assoc (field-details (Field (mt/id :users :last_login)))
- :table_id (mt/id :users)
- :name "LAST_LOGIN"
- :display_name "Last Login"
- :database_type "TIMESTAMP"
- :base_type "type/DateTime"
- :visibility_type "normal"
- :dimension_options (var-get #'table-api/datetime-dimension-indexes)
- :default_dimension_option (var-get #'table-api/date-default-index)
- :has_field_values "none"
- :position 2
- :database_position 2)
- (assoc (field-details (Field :table_id (mt/id :users), :name "PASSWORD"))
- :special_type "type/Category"
- :table_id (mt/id :users)
- :name "PASSWORD"
- :display_name "Password"
- :database_type "VARCHAR"
- :base_type "type/Text"
- :visibility_type "sensitive"
- :has_field_values "list"
- :position 3
- :database_position 3)]
- :rows nil
- :id (mt/id :users)})
- ((test-users/user->client :rasta) :get 200 (format "table/%d/query_metadata?include_sensitive_fields=true" (mt/id :users)))))))
-
-;;; GET api/table/:id/query_metadata
-;;; Make sure that getting the User table does *not* include password info
+ (testing "GET api/table/:id/query_metadata?include_sensitive_fields"
+ (testing "Sensitive fields are included"
+ (is (= (merge
+ (query-metadata-defaults)
+ (db/select-one [Table :created_at :updated_at :fields_hash] :id (mt/id :users))
+ {:schema "PUBLIC"
+ :name "USERS"
+ :display_name "Users"
+ :entity_type "entity/UserTable"
+ :fields [(assoc (field-details (Field (mt/id :users :id)))
+ :special_type "type/PK"
+ :table_id (mt/id :users)
+ :name "ID"
+ :display_name "ID"
+ :database_type "BIGINT"
+ :base_type "type/BigInteger"
+ :visibility_type "normal"
+ :has_field_values "none")
+ (assoc (field-details (Field (mt/id :users :name)))
+ :special_type "type/Name"
+ :table_id (mt/id :users)
+ :name "NAME"
+ :display_name "Name"
+ :database_type "VARCHAR"
+ :base_type "type/Text"
+ :visibility_type "normal"
+ :dimension_options []
+ :default_dimension_option nil
+ :has_field_values "list"
+ :position 1
+ :database_position 1)
+ (assoc (field-details (Field (mt/id :users :last_login)))
+ :table_id (mt/id :users)
+ :name "LAST_LOGIN"
+ :display_name "Last Login"
+ :database_type "TIMESTAMP"
+ :base_type "type/DateTime"
+ :visibility_type "normal"
+ :dimension_options (var-get #'table-api/datetime-dimension-indexes)
+ :default_dimension_option (var-get #'table-api/date-default-index)
+ :has_field_values "none"
+ :position 2
+ :database_position 2)
+ (assoc (field-details (Field :table_id (mt/id :users), :name "PASSWORD"))
+ :special_type "type/Category"
+ :table_id (mt/id :users)
+ :name "PASSWORD"
+ :display_name "Password"
+ :database_type "VARCHAR"
+ :base_type "type/Text"
+ :visibility_type "sensitive"
+ :has_field_values "list"
+ :position 3
+ :database_position 3)]
+ :id (mt/id :users)})
+ ((mt/user->client :rasta) :get 200 (format "table/%d/query_metadata?include_sensitive_fields=true" (mt/id :users))))
+ "Make sure that getting the User table *does* include info about the password field, but not actual values themselves"))))
+
(deftest sensitive-fields-not-included-test
- (testing "Sensitive fields should not be included"
- (is (= (merge
- (query-metadata-defaults)
- (db/select-one [Table :created_at :updated_at :fields_hash] :id (mt/id :users))
- {:schema "PUBLIC"
- :name "USERS"
- :display_name "Users"
- :entity_type "entity/UserTable"
- :fields [(assoc (field-details (Field (mt/id :users :id)))
- :table_id (mt/id :users)
- :special_type "type/PK"
- :name "ID"
- :display_name "ID"
- :database_type "BIGINT"
- :base_type "type/BigInteger"
- :has_field_values "none")
- (assoc (field-details (Field (mt/id :users :name)))
- :table_id (mt/id :users)
- :special_type "type/Name"
- :name "NAME"
- :display_name "Name"
- :database_type "VARCHAR"
- :base_type "type/Text"
- :has_field_values "list"
- :position 1
- :database_position 1)
- (assoc (field-details (Field (mt/id :users :last_login)))
- :table_id (mt/id :users)
- :name "LAST_LOGIN"
- :display_name "Last Login"
- :database_type "TIMESTAMP"
- :base_type "type/DateTime"
- :dimension_options (var-get #'table-api/datetime-dimension-indexes)
- :default_dimension_option (var-get #'table-api/date-default-index)
- :has_field_values "none"
- :position 2
- :database_position 2)]
- :rows nil
- :id (mt/id :users)})
- ((test-users/user->client :rasta) :get 200 (format "table/%d/query_metadata" (mt/id :users)))))))
-
-;; Check that FK fields belonging to Tables we don't have permissions for don't come back as hydrated `:target`(#3867)
-(expect
- #{{:name "id", :target false}
- {:name "fk", :target false}}
- ;; create a temp DB with two tables; table-2 has an FK to table-1
- (mt/with-temp* [Database [db]
- Table [table-1 {:db_id (u/get-id db)}]
- Table [table-2 {:db_id (u/get-id db)}]
- Field [table-1-id {:table_id (u/get-id table-1), :name "id", :base_type :type/Integer, :special_type :type/PK}]
- Field [table-2-id {:table_id (u/get-id table-2), :name "id", :base_type :type/Integer, :special_type :type/PK}]
- Field [table-2-fk {:table_id (u/get-id table-2), :name "fk", :base_type :type/Integer, :special_type :type/FK, :fk_target_field_id (u/get-id table-1-id)}]]
- ;; grant permissions only to table-2
- (perms/revoke-permissions! (perms-group/all-users) (u/get-id db))
- (perms/grant-permissions! (perms-group/all-users) (u/get-id db) (:schema table-2) (u/get-id table-2))
- ;; metadata for table-2 should show all fields for table-2, but the FK target info shouldn't be hydrated
- (set (for [field (:fields ((mt/user->client :rasta) :get 200 (format "table/%d/query_metadata" (u/get-id table-2))))]
- (-> (select-keys field [:name :target])
- (update :target boolean))))))
+ (testing "GET api/table/:id/query_metadata"
+ (testing "Sensitive fields should not be included"
+ (is (= (merge
+ (query-metadata-defaults)
+ (db/select-one [Table :created_at :updated_at :fields_hash] :id (mt/id :users))
+ {:schema "PUBLIC"
+ :name "USERS"
+ :display_name "Users"
+ :entity_type "entity/UserTable"
+ :fields [(assoc (field-details (Field (mt/id :users :id)))
+ :table_id (mt/id :users)
+ :special_type "type/PK"
+ :name "ID"
+ :display_name "ID"
+ :database_type "BIGINT"
+ :base_type "type/BigInteger"
+ :has_field_values "none")
+ (assoc (field-details (Field (mt/id :users :name)))
+ :table_id (mt/id :users)
+ :special_type "type/Name"
+ :name "NAME"
+ :display_name "Name"
+ :database_type "VARCHAR"
+ :base_type "type/Text"
+ :has_field_values "list"
+ :position 1
+ :database_position 1)
+ (assoc (field-details (Field (mt/id :users :last_login)))
+ :table_id (mt/id :users)
+ :name "LAST_LOGIN"
+ :display_name "Last Login"
+ :database_type "TIMESTAMP"
+ :base_type "type/DateTime"
+ :dimension_options (var-get #'table-api/datetime-dimension-indexes)
+ :default_dimension_option (var-get #'table-api/date-default-index)
+ :has_field_values "none"
+ :position 2
+ :database_position 2)]
+ :id (mt/id :users)})
+ ((mt/user->client :rasta) :get 200 (format "table/%d/query_metadata" (mt/id :users))))
+ "Make sure that getting the User table does *not* include password info"))))
+
+(deftest fk-target-permissions-test
+ (testing "GET /api/table/:id/query_metadata"
+ (testing (str "Check that FK fields belonging to Tables we don't have permissions for don't come back as hydrated "
+ "`:target`(#3867)")
+ ;; create a temp DB with two tables; table-2 has an FK to table-1
+ (mt/with-temp* [Database [db]
+ Table [table-1 {:db_id (u/get-id db)}]
+ Table [table-2 {:db_id (u/get-id db)}]
+ Field [table-1-id {:table_id (u/get-id table-1), :name "id", :base_type :type/Integer, :special_type :type/PK}]
+ Field [table-2-id {:table_id (u/get-id table-2), :name "id", :base_type :type/Integer, :special_type :type/PK}]
+ Field [table-2-fk {:table_id (u/get-id table-2), :name "fk", :base_type :type/Integer, :special_type :type/FK, :fk_target_field_id (u/get-id table-1-id)}]]
+ ;; grant permissions only to table-2
+ (perms/revoke-permissions! (perms-group/all-users) (u/get-id db))
+ (perms/grant-permissions! (perms-group/all-users) (u/get-id db) (:schema table-2) (u/get-id table-2))
+ ;; metadata for table-2 should show all fields for table-2, but the FK target info shouldn't be hydrated
+ (is (= #{{:name "id", :target false}
+ {:name "fk", :target false}}
+ (set (for [field (:fields ((mt/user->client :rasta) :get 200 (format "table/%d/query_metadata" (u/get-id table-2))))]
+ (-> (select-keys field [:name :target])
+ (update :target boolean))))))))))
(deftest update-table-test
(testing "PUT /api/table/:id"
@@ -332,7 +275,6 @@
{:description "What a nice table!"
:entity_type nil
:visibility_type "hidden"
- :rows nil
:display_name "Userz"
:pk_field (table/pk-field-id table)})
(dissoc ((mt/user->client :crowberto) :get 200 (format "table/%d" (u/get-id table)))
@@ -367,52 +309,87 @@
(is (= 2
@called)))))))
-;; ## GET /api/table/:id/fks
-;; We expect a single FK from CHECKINS.USER_ID -> USERS.ID
-(expect
- (let [checkins-user-field (Field (mt/id :checkins :user_id))
- users-id-field (Field (mt/id :users :id))
- fk-field-defaults (dissoc field-defaults :target :dimension_options :default_dimension_option)]
- [{:origin_id (:id checkins-user-field)
- :destination_id (:id users-id-field)
- :relationship "Mt1"
- :origin (-> (fk-field-details checkins-user-field)
- (dissoc :target :dimensions :values)
- (assoc :table_id (mt/id :checkins)
- :name "USER_ID"
- :display_name "User ID"
- :database_type "INTEGER"
- :base_type "type/Integer"
- :special_type "type/FK"
- :database_position 2
- :position 2
- :table (merge
- (dissoc (table-defaults) :segments :field_values :metrics)
- (db/select-one [Table :id :created_at :updated_at :fields_hash]
- :id (mt/id :checkins))
- {:schema "PUBLIC"
- :name "CHECKINS"
- :display_name "Checkins"
- :entity_type "entity/EventTable"
- :rows nil})))
- :destination (-> (fk-field-details users-id-field)
- (dissoc :target :dimensions :values)
- (assoc :table_id (mt/id :users)
- :name "ID"
- :display_name "ID"
- :base_type "type/BigInteger"
- :database_type "BIGINT"
- :special_type "type/PK"
- :table (merge
- (dissoc (table-defaults) :db :segments :field_values :metrics)
- (db/select-one [Table :id :created_at :updated_at :fields_hash]
- :id (mt/id :users))
- {:schema "PUBLIC"
- :name "USERS"
- :display_name "Users"
- :entity_type "entity/UserTable"
- :rows nil})))}])
- ((mt/user->client :rasta) :get 200 (format "table/%d/fks" (mt/id :users))))
+(deftest get-fks-test
+ (testing "GET /api/table/:id/fks"
+ (testing "We expect a single FK from CHECKINS.USER_ID -> USERS.ID"
+ (let [checkins-user-field (Field (mt/id :checkins :user_id))
+ users-id-field (Field (mt/id :users :id))
+ fk-field-defaults (dissoc (field-defaults) :target :dimension_options :default_dimension_option)]
+ (is (= [{:origin_id (:id checkins-user-field)
+ :destination_id (:id users-id-field)
+ :relationship "Mt1"
+ :origin (-> (fk-field-details checkins-user-field)
+ (dissoc :target :dimensions :values)
+ (assoc :table_id (mt/id :checkins)
+ :name "USER_ID"
+ :display_name "User ID"
+ :database_type "INTEGER"
+ :base_type "type/Integer"
+ :special_type "type/FK"
+ :database_position 2
+ :position 2
+ :table (merge
+ (dissoc (table-defaults) :segments :field_values :metrics)
+ (db/select-one [Table :id :created_at :updated_at :fields_hash]
+ :id (mt/id :checkins))
+ {:schema "PUBLIC"
+ :name "CHECKINS"
+ :display_name "Checkins"
+ :entity_type "entity/EventTable"})))
+ :destination (-> (fk-field-details users-id-field)
+ (dissoc :target :dimensions :values)
+ (assoc :table_id (mt/id :users)
+ :name "ID"
+ :display_name "ID"
+ :base_type "type/BigInteger"
+ :database_type "BIGINT"
+ :special_type "type/PK"
+ :table (merge
+ (dissoc (table-defaults) :db :segments :field_values :metrics)
+ (db/select-one [Table :id :created_at :updated_at :fields_hash]
+ :id (mt/id :users))
+ {:schema "PUBLIC"
+ :name "USERS"
+ :display_name "Users"
+ :entity_type "entity/UserTable"})))}]
+ ((mt/user->client :rasta) :get 200 (format "table/%d/fks" (mt/id :users)))))))
+
+ (testing "should just return nothing for 'virtual' tables"
+ (is (= []
+ ((mt/user->client :crowberto) :get 200 "table/card__1000/fks"))))))
+
+(deftest basic-query-metadata-test
+ (testing "GET /api/table/:id/query_metadata"
+ (is (= (merge
+ (query-metadata-defaults)
+ (db/select-one [Table :created_at :updated_at :fields_hash] :id (mt/id :categories))
+ {:schema "PUBLIC"
+ :name "CATEGORIES"
+ :display_name "Categories"
+ :fields [(merge
+ (field-details (Field (mt/id :categories :id)))
+ {:table_id (mt/id :categories)
+ :special_type "type/PK"
+ :name "ID"
+ :display_name "ID"
+ :database_type "BIGINT"
+ :base_type "type/BigInteger"
+ :has_field_values "none"})
+ (merge
+ (field-details (Field (mt/id :categories :name)))
+ {:table_id (mt/id :categories)
+ :special_type "type/Name"
+ :name "NAME"
+ :display_name "Name"
+ :database_type "VARCHAR"
+ :base_type "type/Text"
+ :dimension_options []
+ :default_dimension_option nil
+ :has_field_values "list"
+ :database_position 1
+ :position 1})]
+ :id (mt/id :categories)})
+ ((mt/user->client :rasta) :get 200 (format "table/%d/query_metadata" (mt/id :categories)))))))
(defn- with-field-literal-id [{field-name :name, base-type :base_type :as field}]
(assoc field :id ["field-literal" field-name base-type]))
@@ -484,49 +461,44 @@
(mt/round-all-decimals 2))))))))
(deftest include-date-dimensions-in-nested-query-test
- (testing "Test date dimensions being included with a nested query"
- (mt/with-temp Card [card {:name "Users"
- :database_id (mt/id)
- :dataset_query {:database (mt/id)
- :type :native
- :native {:query (format "SELECT NAME, LAST_LOGIN FROM USERS")}}}]
- (let [card-virtual-table-id (str "card__" (u/get-id card))]
- ;; run the Card which will populate its result_metadata column
- ((mt/user->client :crowberto) :post 202 (format "card/%d/query" (u/get-id card)))
- ;; Now fetch the metadata for this "table" via the API
- (let [[name-metadata last-login-metadata] (db/select-one-field :result_metadata Card :id (u/get-id card))]
- (is (= {:display_name "Users"
- :schema "Everything else"
- :db_id (:database_id card)
- :id card-virtual-table-id
- :description nil
- :dimension_options (default-dimension-options)
- :fields [{:name "NAME"
- :display_name "NAME"
- :base_type "type/Text"
- :table_id card-virtual-table-id
- :id ["field-literal" "NAME" "type/Text"]
- :special_type "type/Name"
- :default_dimension_option nil
- :dimension_options []
- :fingerprint (:fingerprint name-metadata)}
- {:name "LAST_LOGIN"
- :display_name "LAST_LOGIN"
- :base_type "type/DateTime"
- :table_id card-virtual-table-id
- :id ["field-literal" "LAST_LOGIN" "type/DateTime"]
- :special_type nil
- :default_dimension_option (var-get #'table-api/date-default-index)
- :dimension_options (var-get #'table-api/datetime-dimension-indexes)
- :fingerprint (:fingerprint last-login-metadata)}]}
- ((mt/user->client :crowberto) :get 200
- (format "table/card__%d/query_metadata" (u/get-id card))))))))))
-
-
-;; make sure GET /api/table/:id/fks just returns nothing for 'virtual' tables
-(expect
- []
- ((mt/user->client :crowberto) :get 200 "table/card__1000/fks"))
+ (testing "GET /api/table/:id/query_metadata"
+ (testing "Test date dimensions being included with a nested query"
+ (mt/with-temp Card [card {:name "Users"
+ :database_id (mt/id)
+ :dataset_query {:database (mt/id)
+ :type :native
+ :native {:query (format "SELECT NAME, LAST_LOGIN FROM USERS")}}}]
+ (let [card-virtual-table-id (str "card__" (u/get-id card))]
+ ;; run the Card which will populate its result_metadata column
+ ((mt/user->client :crowberto) :post 202 (format "card/%d/query" (u/get-id card)))
+ ;; Now fetch the metadata for this "table" via the API
+ (let [[name-metadata last-login-metadata] (db/select-one-field :result_metadata Card :id (u/get-id card))]
+ (is (= {:display_name "Users"
+ :schema "Everything else"
+ :db_id (:database_id card)
+ :id card-virtual-table-id
+ :description nil
+ :dimension_options (default-dimension-options)
+ :fields [{:name "NAME"
+ :display_name "NAME"
+ :base_type "type/Text"
+ :table_id card-virtual-table-id
+ :id ["field-literal" "NAME" "type/Text"]
+ :special_type "type/Name"
+ :default_dimension_option nil
+ :dimension_options []
+ :fingerprint (:fingerprint name-metadata)}
+ {:name "LAST_LOGIN"
+ :display_name "LAST_LOGIN"
+ :base_type "type/DateTime"
+ :table_id card-virtual-table-id
+ :id ["field-literal" "LAST_LOGIN" "type/DateTime"]
+ :special_type nil
+ :default_dimension_option (var-get #'table-api/date-default-index)
+ :dimension_options (var-get #'table-api/datetime-dimension-indexes)
+ :fingerprint (:fingerprint last-login-metadata)}]}
+ ((mt/user->client :crowberto) :get 200
+ (format "table/card__%d/query_metadata" (u/get-id card)))))))))))
(defn- narrow-fields [category-names api-response]
(for [field (:fields api-response)
@@ -545,84 +517,69 @@
(mt/with-temp-vals-in-db Field (mt/id :venues :category_id) {:special_type special-type}
(f)))
-;; ## GET /api/table/:id/query_metadata
-;; Ensure internal remapped dimensions and human_readable_values are returned
-(expect
- [{:table_id (mt/id :venues)
- :id (mt/id :venues :category_id)
- :name "CATEGORY_ID"
- :dimensions {:name "Foo", :field_id (mt/id :venues :category_id), :human_readable_field_id nil, :type "internal"}}
- {:id (mt/id :venues :price)
- :table_id (mt/id :venues)
- :name "PRICE"
- :dimensions []}]
- (mt/with-temp-objects
- (data/create-venue-category-remapping "Foo")
- (category-id-special-type
- :type/Category
- (fn []
- (narrow-fields ["PRICE" "CATEGORY_ID"]
- ((test-users/user->client :rasta) :get 200 (format "table/%d/query_metadata" (mt/id :venues))))))))
-
-;; ## GET /api/table/:id/query_metadata
-;; Ensure internal remapped dimensions and human_readable_values are returned when type is enum
-(expect
- [{:table_id (mt/id :venues)
- :id (mt/id :venues :category_id)
- :name "CATEGORY_ID"
- :dimensions {:name "Foo", :field_id (mt/id :venues :category_id), :human_readable_field_id nil, :type "internal"}}
- {:id (mt/id :venues :price)
- :table_id (mt/id :venues)
- :name "PRICE"
- :dimensions []}]
- (mt/with-temp-objects
- (data/create-venue-category-remapping "Foo")
- (category-id-special-type
- :type/Enum
- (fn []
- (narrow-fields ["PRICE" "CATEGORY_ID"]
- ((mt/user->client :rasta) :get 200 (format "table/%d/query_metadata" (mt/id :venues))))))))
-
-;; ## GET /api/table/:id/query_metadata
-;; Ensure FK remappings are returned
-(expect
- [{:table_id (mt/id :venues)
- :id (mt/id :venues :category_id)
- :name "CATEGORY_ID"
- :dimensions {:name "Foo", :field_id (mt/id :venues :category_id), :human_readable_field_id (mt/id :categories :name), :type "external"}}
- {:id (mt/id :venues :price)
- :table_id (mt/id :venues)
- :name "PRICE"
- :dimensions []}]
- (mt/with-temp-objects
- (data/create-venue-category-fk-remapping "Foo")
- (category-id-special-type
- :type/Category
- (fn []
- (narrow-fields ["PRICE" "CATEGORY_ID"]
- ((mt/user->client :rasta) :get 200 (format "table/%d/query_metadata" (mt/id :venues))))))))
-
-;; Ensure dimensions options are sorted numerically, but returned as strings
-(expect
- (map str (sort (map #(Long/parseLong %) (var-get #'table-api/datetime-dimension-indexes))))
- (var-get #'table-api/datetime-dimension-indexes))
-
-(expect
- (map str (sort (map #(Long/parseLong %) (var-get #'table-api/numeric-dimension-indexes))))
- (var-get #'table-api/numeric-dimension-indexes))
-
-;; Numeric fields without min/max values should not have binning strategies
-(expect
- []
- (let [fingerprint (db/select-one-field :fingerprint Field {:id (mt/id :venues :latitude)})
- temp-fingerprint (-> fingerprint
- (assoc-in [:type :type/Number :max] nil)
- (assoc-in [:type :type/Number :min] nil))]
- (mt/with-temp-vals-in-db Field (mt/id :venues :latitude) {:fingerprint temp-fingerprint}
- (-> ((mt/user->client :rasta) :get 200 (format "table/%d/query_metadata" (mt/id :categories)))
- (get-in [:fields])
- first
- :dimension_options))))
+(deftest query-metadata-remappings-test
+ (testing "GET /api/table/:id/query_metadata"
+ (mt/with-temp-objects
+ (data/create-venue-category-remapping "Foo")
+ (testing "Ensure internal remapped dimensions and human_readable_values are returned"
+ (is (= [{:table_id (mt/id :venues)
+ :id (mt/id :venues :category_id)
+ :name "CATEGORY_ID"
+ :dimensions {:name "Foo", :field_id (mt/id :venues :category_id), :human_readable_field_id nil, :type "internal"}}
+ {:id (mt/id :venues :price)
+ :table_id (mt/id :venues)
+ :name "PRICE"
+ :dimensions []}]
+ (category-id-special-type
+ :type/Category
+ (fn []
+ (narrow-fields ["PRICE" "CATEGORY_ID"]
+ ((mt/user->client :rasta) :get 200 (format "table/%d/query_metadata" (mt/id :venues)))))))))
+
+ (testing "Ensure internal remapped dimensions and human_readable_values are returned when type is enum"
+ (is (= [{:table_id (mt/id :venues)
+ :id (mt/id :venues :category_id)
+ :name "CATEGORY_ID"
+ :dimensions {:name "Foo", :field_id (mt/id :venues :category_id), :human_readable_field_id nil, :type "internal"}}
+ {:id (mt/id :venues :price)
+ :table_id (mt/id :venues)
+ :name "PRICE"
+ :dimensions []}]
+ (category-id-special-type
+ :type/Enum
+ (fn []
+ (narrow-fields ["PRICE" "CATEGORY_ID"]
+ ((mt/user->client :rasta) :get 200 (format "table/%d/query_metadata" (mt/id :venues))))))))))
+
+ (mt/with-temp-objects
+ (data/create-venue-category-fk-remapping "Foo")
+ (testing "Ensure FK remappings are returned"
+ (is (= [{:table_id (mt/id :venues)
+ :id (mt/id :venues :category_id)
+ :name "CATEGORY_ID"
+ :dimensions {:name "Foo"
+ :field_id (mt/id :venues :category_id)
+ :human_readable_field_id (mt/id :categories :name)
+ :type "external"}}
+ {:id (mt/id :venues :price)
+ :table_id (mt/id :venues)
+ :name "PRICE"
+ :dimensions []}]
+ (category-id-special-type
+ :type/Category
+ (fn []
+ (narrow-fields ["PRICE" "CATEGORY_ID"]
+ ((mt/user->client :rasta) :get 200 (format "table/%d/query_metadata" (mt/id :venues))))))))))))
+
+(deftest dimension-options-sort-test
+ (testing "Ensure dimensions options are sorted numerically, but returned as strings"
+ (testing "datetime indexes"
+ (is (= (map str (sort (map #(Long/parseLong %) (var-get #'table-api/datetime-dimension-indexes))))
+ (var-get #'table-api/datetime-dimension-indexes))))
+
+ (testing "numeric indexes"
+ (is (= (map str (sort (map #(Long/parseLong %) (var-get #'table-api/numeric-dimension-indexes))))
+ (var-get #'table-api/numeric-dimension-indexes))))))
(defn- dimension-options-for-field [response, ^String field-name]
(->> response
@@ -638,22 +595,31 @@
:let [{[_ _ strategy _] :mbql} (get-in response [:dimension_options (keyword dim-index)])]]
strategy)))
-;; Lat/Long fields should use bin-width rather than num-bins
-(expect
- (if (data/binning-supported?)
- #{nil "bin-width" "default"}
- #{})
- (let [response ((mt/user->client :rasta) :get 200 (format "table/%d/query_metadata" (mt/id :venues)))]
- (extract-dimension-options response "latitude")))
-
-;; Number columns without a special type should use "num-bins"
-(expect
- (if (data/binning-supported?)
- #{nil "num-bins" "default"}
- #{})
- (mt/with-temp-vals-in-db Field (mt/id :venues :price) {:special_type nil}
- (let [response ((mt/user->client :rasta) :get 200 (format "table/%d/query_metadata" (mt/id :venues)))]
- (extract-dimension-options response "price"))))
+(deftest numeric-binning-options-test
+ (testing "GET /api/table/:id/query_metadata"
+ (testing "binning options for numeric fields"
+ (testing "Lat/Long fields should use bin-width rather than num-bins"
+ (let [response ((mt/user->client :rasta) :get 200 (format "table/%d/query_metadata" (mt/id :venues)))]
+ (is (= #{nil "bin-width" "default"}
+ (extract-dimension-options response "latitude")))))
+
+ (testing "Number columns without a special type should use \"num-bins\""
+ (mt/with-temp-vals-in-db Field (mt/id :venues :price) {:special_type nil}
+ (let [response ((mt/user->client :rasta) :get 200 (format "table/%d/query_metadata" (mt/id :venues)))]
+ (is (= #{nil "num-bins" "default"}
+ (extract-dimension-options response "price"))))))
+
+ (testing "Numeric fields without min/max values should not have binning options"
+ (let [fingerprint (db/select-one-field :fingerprint Field {:id (mt/id :venues :latitude)})
+ temp-fingerprint (-> fingerprint
+ (assoc-in [:type :type/Number :max] nil)
+ (assoc-in [:type :type/Number :min] nil))]
+ (mt/with-temp-vals-in-db Field (mt/id :venues :latitude) {:fingerprint temp-fingerprint}
+ (is (= []
+ (-> ((mt/user->client :rasta) :get 200 (format "table/%d/query_metadata" (mt/id :categories)))
+ (get-in [:fields])
+ first
+ :dimension_options)))))))))
(deftest datetime-binning-options-test
(testing "GET /api/table/:id/query_metadata"
diff --git a/test/metabase/models/field_values_test.clj b/test/metabase/models/field_values_test.clj
index e00c0525bee4701b18649340dee82185f4dbd876..2cf8f538921714c2e03dbef6ed639dd0d9320689 100644
--- a/test/metabase/models/field_values_test.clj
+++ b/test/metabase/models/field_values_test.clj
@@ -1,95 +1,117 @@
(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)))))))))))
diff --git a/test/metabase/sync/analyze/table_row_count_test.clj b/test/metabase/sync/analyze/table_row_count_test.clj
deleted file mode 100644
index 14b879f3a43874bb52389ea42098d2a0f4a8c959..0000000000000000000000000000000000000000
--- a/test/metabase/sync/analyze/table_row_count_test.clj
+++ /dev/null
@@ -1,19 +0,0 @@
-(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))))
diff --git a/test/metabase/sync_database/analyze_test.clj b/test/metabase/sync_database/analyze_test.clj
index 275886a3fc4b091c0dbf96eca4922b27f0d3e693..ea4529cdbc7f2db777534e156645e9c71af9a9cc 100644
--- a/test/metabase/sync_database/analyze_test.clj
+++ b/test/metabase/sync_database/analyze_test.clj
@@ -1,82 +1,67 @@
(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")))))
diff --git a/test/metabase/sync_database_test.clj b/test/metabase/sync_database_test.clj
index 5e921c3ed9e7e6a0c1e8d385ce41782eaeb24781..0b9e8a8fc1cf5b42aaeb193bebb64fa652b7de9e 100644
--- a/test/metabase/sync_database_test.clj
+++ b/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)))))))