diff --git a/src/metabase/sync/sync_metadata/fields.clj b/src/metabase/sync/sync_metadata/fields.clj
index f0edc4e45400a83dd0753497284d002a7cf9cec1..7e8ee65511bc72f52835f45cf1165d517e1bad79 100644
--- a/src/metabase/sync/sync_metadata/fields.clj
+++ b/src/metabase/sync/sync_metadata/fields.clj
@@ -37,9 +37,17 @@
(s/optional-key :nested-fields) #{(s/recursive #'TableMetadataFieldWithOptionalID)}))
-;;; +------------------------------------------------------------------------------------------------------------------------+
-;;; | CREATING / REACTIVATING FIELDS |
-;;; +------------------------------------------------------------------------------------------------------------------------+
+(s/defn ^:private field-metadata-name-for-logging :- s/Str
+ "Return a 'name for logging' for a map that conforms to the `TableMetadataField` schema.
+
+ (field-metadata-name-for-logging table field-metadata) ; -> \"Table 'venues' Field 'name'\""
+ [table :- i/TableInstance, field-metadata :- TableMetadataFieldWithOptionalID]
+ (format "%s Field '%s'" (sync-util/name-for-logging table) (:name field-metadata)))
+
+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; | CREATING / REACTIVATING FIELDS |
+;;; +----------------------------------------------------------------------------------------------------------------+
(s/defn ^:private matching-inactive-field :- (s/maybe i/FieldInstance)
"Return an inactive metabase Field that matches NEW-FIELD-METADATA, if any such Field existis."
@@ -83,14 +91,37 @@
(create-or-reactivate-field! table nested-field (u/get-id metabase-field)))))
-;;; +------------------------------------------------------------------------------------------------------------------------+
-;;; | "RETIRING" INACTIVE FIELDS |
-;;; +------------------------------------------------------------------------------------------------------------------------+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; | UPDATING FIELD TYPE INFO |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
+(s/defn ^:private update-field-metadata-if-needed!
+ "Update the metadata for a Metabase Field as needed if any of the info coming back from the DB has changed."
+ [table :- i/TableInstance, metabase-field :- TableMetadataFieldWithID, field-metadata :- i/TableMetadataField]
+ (let [{old-database-type :database-type, old-base-type :base-type} metabase-field
+ {new-database-type :database-type, new-base-type :base-type} field-metadata]
+ ;; If the driver is reporting a different `database-type` than what we have recorded in the DB, update it
+ (when-not (= old-database-type new-database-type)
+ (log/info (format "Database type of %s has changed from '%s' to '%s'."
+ (field-metadata-name-for-logging table metabase-field)
+ old-database-type new-database-type))
+ (db/update! Field (u/get-id metabase-field), :database_type new-database-type))
+ ;; Now do the same for `base-type`
+ (when-not (= old-base-type new-base-type)
+ (log/info (format "Base type of %s has changed from '%s' to '%s'."
+ (field-metadata-name-for-logging table metabase-field)
+ old-base-type new-base-type))
+ (db/update! Field (u/get-id metabase-field), :base_type new-base-type))))
+
+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; | "RETIRING" INACTIVE FIELDS |
+;;; +----------------------------------------------------------------------------------------------------------------+
(s/defn ^:private retire-field!
"Mark an OLD-FIELD belonging to TABLE as inactive if corresponding Field object exists."
[table :- i/TableInstance, old-field :- TableMetadataFieldWithID]
- (log/info (format "Marking %s Field '%s' as inactive." (sync-util/name-for-logging table) (:name old-field)))
+ (log/info (format "Marking %s as inactive." (field-metadata-name-for-logging table old-field)))
(db/update! Field (:id old-field)
:active false)
;; Now recursively mark and nested fields as inactive
@@ -98,9 +129,9 @@
(retire-field! table nested-field)))
-;;; +------------------------------------------------------------------------------------------------------------------------+
-;;; | SYNCING FIELDS IN DB (CREATING, REACTIVATING, OR RETIRING) |
-;;; +------------------------------------------------------------------------------------------------------------------------+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; | SYNCING FIELDS IN DB (CREATING, REACTIVATING, OR RETIRING) |
+;;; +----------------------------------------------------------------------------------------------------------------+
(s/defn ^:private matching-field-metadata :- (s/maybe TableMetadataFieldWithOptionalID)
"Find Metadata that matches FIELD-METADATA from a set of OTHER-METADATA, if any exists."
@@ -113,14 +144,20 @@
(s/defn ^:private sync-field-instances!
"Make sure the instances of Metabase Field are in-sync with the DB-METADATA."
- [table :- i/TableInstance, db-metadata :- #{i/TableMetadataField}, our-metadata :- #{TableMetadataFieldWithID}, parent-id :- ParentID]
+ [table :- i/TableInstance, db-metadata :- #{i/TableMetadataField}, our-metadata :- #{TableMetadataFieldWithID}
+ parent-id :- ParentID]
;; Loop thru fields in DB-METADATA. Create/reactivate any fields that don't exist in OUR-METADATA.
(doseq [db-field db-metadata]
- (sync-util/with-error-handling (format "Error checking if Field '%s' needs to be created or reactivated" (:name db-field))
+ (sync-util/with-error-handling (format "Error checking if Field '%s' needs to be created or reactivated"
+ (:name db-field))
(if-let [our-field (matching-field-metadata db-field our-metadata)]
- ;; if field exists in both metadata sets then recursively check the nested fields
- (when-let [db-nested-fields (seq (:nested-fields db-field))]
- (sync-field-instances! table (set db-nested-fields) (:nested-fields our-field) (:id our-field)))
+ ;; if field exists in both metadata sets then...
+ (do
+ ;; ...make sure the data recorded about the Field such as database_type is up-to-date...
+ (update-field-metadata-if-needed! table our-field db-field)
+ ;; ...and then recursively check the nested fields.
+ (when-let [db-nested-fields (seq (:nested-fields db-field))]
+ (sync-field-instances! table (set db-nested-fields) (:nested-fields our-field) (:id our-field))))
;; otherwise if field doesn't exist, create or reactivate it
(create-or-reactivate-field! table db-field parent-id))))
;; ok, loop thru Fields in OUR-METADATA. Mark Fields as inactive if they don't exist in DB-METADATA.
@@ -134,9 +171,9 @@
(retire-field! table our-field)))))
-;;; +------------------------------------------------------------------------------------------------------------------------+
-;;; | UPDATING FIELD METADATA |
-;;; +------------------------------------------------------------------------------------------------------------------------+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; | UPDATING FIELD METADATA |
+;;; +----------------------------------------------------------------------------------------------------------------+
(s/defn ^:private update-metadata!
"Make sure things like PK status and base-type are in sync with what has come back from the DB."
@@ -160,9 +197,9 @@
(update-metadata! table (set db-nested-fields) (u/get-id field)))))))
-;;; +------------------------------------------------------------------------------------------------------------------------+
-;;; | FETCHING OUR CURRENT METADATA |
-;;; +------------------------------------------------------------------------------------------------------------------------+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; | FETCHING OUR CURRENT METADATA |
+;;; +----------------------------------------------------------------------------------------------------------------+
(s/defn ^:private add-nested-fields :- TableMetadataFieldWithID
"Recursively add entries for any nested-fields to FIELD."
@@ -205,9 +242,9 @@
(add-nested-fields field parent-id->fields)))))
-;;; +------------------------------------------------------------------------------------------------------------------------+
-;;; | FETCHING METADATA FROM CONNECTED DB |
-;;; +------------------------------------------------------------------------------------------------------------------------+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; | FETCHING METADATA FROM CONNECTED DB |
+;;; +----------------------------------------------------------------------------------------------------------------+
(s/defn ^:private db-metadata :- #{i/TableMetadataField}
"Fetch metadata about Fields belonging to a given TABLE directly from an external database by calling its
@@ -216,9 +253,9 @@
(:fields (fetch-metadata/table-metadata database table)))
-;;; +------------------------------------------------------------------------------------------------------------------------+
-;;; | PUTTING IT ALL TOGETHER |
-;;; +------------------------------------------------------------------------------------------------------------------------+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; | PUTTING IT ALL TOGETHER |
+;;; +----------------------------------------------------------------------------------------------------------------+
(s/defn sync-fields-for-table!
"Sync the Fields in the Metabase application database for a specific TABLE."
diff --git a/test/metabase/driver/postgres_test.clj b/test/metabase/driver/postgres_test.clj
index e2adf5e1881604a85eba25c041ded3aeeeab55ee..6f8b194794cca96f48f8bfa35d02a0a0d013923b 100644
--- a/test/metabase/driver/postgres_test.clj
+++ b/test/metabase/driver/postgres_test.clj
@@ -281,13 +281,13 @@
;; Make sure we're able to fingerprint TIME fields (#5911)
(expect-with-engine :postgres
- [#metabase.models.field.FieldInstance{:name "start_time", :fingerprint {:global {:distinct-count 1}}}
- #metabase.models.field.FieldInstance{:name "end_time", :fingerprint {:global {:distinct-count 1}}}
- #metabase.models.field.FieldInstance{:name "reason", :fingerprint {:global {:distinct-count 1}
- :type {:type/Text {:percent-json 0.0
- :percent-url 0.0
- :percent-email 0.0
- :average-length 12.0}}}}]
+ #{#metabase.models.field.FieldInstance{:name "start_time", :fingerprint {:global {:distinct-count 1}}}
+ #metabase.models.field.FieldInstance{:name "end_time", :fingerprint {:global {:distinct-count 1}}}
+ #metabase.models.field.FieldInstance{:name "reason", :fingerprint {:global {:distinct-count 1}
+ :type {:type/Text {:percent-json 0.0
+ :percent-url 0.0
+ :percent-email 0.0
+ :average-length 12.0}}}}}
(do
(drop-if-exists-and-create-db! "time_field_test")
(let [details (i/database->connection-details pg-driver :db {:database-name "time_field_test"})]
@@ -301,4 +301,4 @@
" VALUES ('22:00'::time, '9:00'::time, 'Beauty Sleep');")])
(tt/with-temp Database [database {:engine :postgres, :details (assoc details :dbname "time_field_test")}]
(sync/sync-database! database)
- (db/select [Field :name :fingerprint] :table_id (db/select-one-id Table :db_id (u/get-id database)))))))
+ (set (db/select [Field :name :fingerprint] :table_id (db/select-one-id Table :db_id (u/get-id database))))))))
diff --git a/test/metabase/driver/presto_test.clj b/test/metabase/driver/presto_test.clj
index cc794deb8a49fd6bd186e0e15e16adf40736b4ac..288d91a46e09b371e3d1fffd89194cfcdcbea801 100644
--- a/test/metabase/driver/presto_test.clj
+++ b/test/metabase/driver/presto_test.clj
@@ -86,18 +86,24 @@
(datasets/expect-with-engine :presto
{:name "venues"
:schema "default"
- :fields #{{:name "name",
- :base-type :type/Text}
- {:name "latitude"
- :base-type :type/Float}
- {:name "longitude"
- :base-type :type/Float}
- {:name "price"
- :base-type :type/Integer}
- {:name "category_id"
- :base-type :type/Integer}
- {:name "id"
- :base-type :type/Integer}}}
+ :fields #{{:name "name",
+ :database-type "varchar(255)"
+ :base-type :type/Text}
+ {:name "latitude"
+ :database-type "double"
+ :base-type :type/Float}
+ {:name "longitude"
+ :database-type "double"
+ :base-type :type/Float}
+ {:name "price"
+ :database-type "integer"
+ :base-type :type/Integer}
+ {:name "category_id"
+ :database-type "integer"
+ :base-type :type/Integer}
+ {:name "id"
+ :database-type "integer"
+ :base-type :type/Integer}}}
(driver/describe-table (PrestoDriver.) (data/db) (db/select-one 'Table :id (data/id :venues))))
;;; TABLE-ROWS-SAMPLE
diff --git a/test/metabase/sync/sync_metadata/sync_database_type_test.clj b/test/metabase/sync/sync_metadata/sync_database_type_test.clj
new file mode 100644
index 0000000000000000000000000000000000000000..f5ab5d93398784f6c0db357acdca72b97555a4b2
--- /dev/null
+++ b/test/metabase/sync/sync_metadata/sync_database_type_test.clj
@@ -0,0 +1,55 @@
+(ns metabase.sync.sync-metadata.sync-database-type-test
+ "Tests to make sure the newly added Field.database_type field gets populated, even for existing Fields."
+ (:require [expectations :refer :all]
+ [metabase
+ [sync :as sync]
+ [util :as u]]
+ [metabase.models
+ [database :refer [Database]]
+ [field :refer [Field]]
+ [table :refer [Table]]]
+ [metabase.test.data :as data]
+ metabase.test.util ; to make sure defaults for with-temp are registered
+ [toucan.db :as db]
+ [toucan.util.test :as tt]))
+
+;; make sure that if a driver reports back a different database-type the Field gets updated accordingly
+(expect
+ #{{:name "NAME", :database_type "VARCHAR"}
+ {:name "LATITUDE", :database_type "DOUBLE"}
+ {:name "LONGITUDE", :database_type "DOUBLE"}
+ {:name "ID", :database_type "BIGINT"}
+ {:name "PRICE", :database_type "INTEGER"}
+ {:name "CATEGORY_ID", :database_type "INTEGER"}}
+ ;; create a copy of the sample dataset :D
+ (tt/with-temp Database [db (select-keys (data/db) [:details :engine])]
+ (sync/sync-database! db)
+ (let [venues-table (Table :db_id (u/get-id db), :display_name "Venues")]
+ ;; ok, now give all the Fields `?` as their `database_type`. (This is what the DB migration does for existing
+ ;; Fields)
+ (db/update-where! Field {:table_id (u/get-id venues-table)}, :database_type "?")
+ ;; now sync the DB again
+ (sync/sync-database! db)
+ ;; The database_type of these Fields should get set to the correct types. Let's see...
+ (set (map (partial into {})
+ (db/select [Field :name :database_type] :table_id (u/get-id venues-table)))))))
+
+;; make sure that if a driver reports back a different base-type the Field gets updated accordingly
+(expect
+ #{{:name "NAME", :base_type :type/Text}
+ {:name "LATITUDE", :base_type :type/Float}
+ {:name "PRICE", :base_type :type/Integer}
+ {:name "ID", :base_type :type/BigInteger}
+ {:name "LONGITUDE", :base_type :type/Float}
+ {:name "CATEGORY_ID", :base_type :type/Integer}}
+ ;; create a copy of the sample dataset :D
+ (tt/with-temp Database [db (select-keys (data/db) [:details :engine])]
+ (sync/sync-database! db)
+ (let [venues-table (Table :db_id (u/get-id db), :display_name "Venues")]
+ ;; ok, now give all the Fields `:type/*` as their `base_type`
+ (db/update-where! Field {:table_id (u/get-id venues-table)}, :base_type "type/*")
+ ;; now sync the DB again
+ (sync/sync-database! db)
+ ;; The database_type of these Fields should get set to the correct types. Let's see...
+ (set (map (partial into {})
+ (db/select [Field :name :base_type] :table_id (u/get-id venues-table)))))))