Skip to content
Snippets Groups Projects
Unverified Commit 737691d4 authored by Cam Saul's avatar Cam Saul
Browse files

Make sure DB sync can update Field base-type & database-type

parent f8525b92
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
src/metabase/sync/sync_metadata/fields.clj
+ 64
− 27
View file @ 737691d4
......@@ -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."
......
test/metabase/driver/postgres_test.clj
+ 8
− 8
View file @ 737691d4
......@@ -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))))))))
test/metabase/driver/presto_test.clj
+ 18
− 12
View file @ 737691d4
......@@ -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
......
test/metabase/sync/sync_metadata/sync_database_type_test.clj 0 → 100644
+ 55
− 0
View file @ 737691d4
(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)))))))
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment