Skip to content
Snippets Groups Projects
Unverified Commit 765cef44 authored by Cam Saul's avatar Cam Saul Committed by GitHub
Browse files

Field type validation/error handling (#15991) 

* Port legacy data type migrations -> Liquibase

* Fix migration IDs

* Field type validation/error handling

* Have semantic type fallback to nil

* Fix semantic-type-migrations-test

* Fix migrations

* Revert accidental changes
parent 367b14e1
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
src/metabase/models/field.clj
+ 47
− 22
View file @ 765cef44
(ns metabase.models.field
(:require [clojure.core.memoize :as memoize]
[clojure.string :as str]
[clojure.tools.logging :as log]
[medley.core :as m]
[metabase.models.dimension :refer [Dimension]]
[metabase.models.field-values :as fv :refer [FieldValues]]
......@@ -8,6 +9,7 @@
[metabase.models.interface :as i]
[metabase.models.permissions :as perms]
[metabase.util :as u]
[metabase.util.i18n :refer [trs tru]]
[toucan.db :as db]
[toucan.hydrate :refer [hydrate]]
[toucan.models :as models]))
......@@ -58,27 +60,51 @@
(models/defmodel Field :metabase_field)
(defn- check-valid-types [{base-type :base_type, semantic-type :semantic_type,
coercion-strategy :coercion_strategy}]
(when base-type
(assert (isa? (keyword base-type) :type/*)
(str "Invalid base type: " base-type)))
(when semantic-type
(assert (isa? (keyword semantic-type) :type/*)
(str "Invalid semantic type: " semantic-type)))
(when coercion-strategy
(assert (isa? (keyword coercion-strategy) :Coercion/*)
(str "Invalid coercion strategy: " coercion-strategy))))
(defn- hierarchy-keyword-in [column-name & {:keys [ancestor-types]}]
(fn [k]
(when-let [k (keyword k)]
(when-not (some
(partial isa? k)
ancestor-types)
(let [message (tru "Invalid Field {0} {1}" column-name k)]
(throw (ex-info message
{:status-code 400
:errors {column-name message}
:value k}))))
(u/qualified-name k))))
(defn- hierarchy-keyword-out [column-name & {:keys [fallback-type ancestor-types]}]
(fn [s]
(when (seq s)
(let [k (keyword s)]
(if (some
(partial isa? k)
ancestor-types)
k
(do
(log/warn (trs "Invalid Field {0} {1}: falling back to {2}" column-name k fallback-type))
fallback-type))))))
(models/add-type! ::base-type
:in (hierarchy-keyword-in :base_type :ancestor-types [:type/*])
:out (hierarchy-keyword-out :base_type :ancestor-types [:type/*], :fallback-type :type/*))
(models/add-type! ::effective-type
:in (hierarchy-keyword-in :effective_type :ancestor-types [:type/*])
:out (hierarchy-keyword-out :effective_type :ancestor-types [:type/*], :fallback-type :type/*))
(models/add-type! ::semantic-type
:in (hierarchy-keyword-in :semantic_type :ancestor-types [:type/*])
:out (hierarchy-keyword-out :semantic_type :ancestor-types [:type/*], :fallback-type nil))
(models/add-type! ::coercion-strategy
:in (hierarchy-keyword-in :coercion_strategy :ancestor-types [:Coercion/*])
:out (hierarchy-keyword-out :coercion_strategy :ancestor-types [:Coercion/*], :fallback-type nil))
(defn- pre-insert [field]
(check-valid-types field)
(let [defaults {:display_name (humanization/name->human-readable-name (:name field))}]
(merge defaults field)))
(defn- pre-update [field]
(u/prog1 field
(check-valid-types field)))
;;; Field permissions
;; There are several API endpoints where large instances can return many thousands of Fields. Normally Fields require
;; a DB call to fetch information about their Table, because a Field's permissions set is the same as its parent
......@@ -137,17 +163,16 @@
models/IModel
(merge models/IModelDefaults
{:hydration-keys (constantly [:destination :field :origin :human_readable_field])
:types (constantly {:base_type :keyword
:effective_type :keyword
:coercion_strategy :keyword
:semantic_type :keyword
:types (constantly {:base_type ::base-type
:effective_type ::effective-type
:coercion_strategy ::coercion-strategy
:semantic_type ::semantic-type
:visibility_type :keyword
:has_field_values :keyword
:fingerprint :json-for-fingerprints
:settings :json})
:properties (constantly {:timestamped? true})
:pre-insert pre-insert
:pre-update pre-update})
:pre-insert pre-insert})
i/IObjectPermissions
(merge i/IObjectPermissionsDefaults
......
test/metabase/db/schema_migrations_test.clj
+ 63
− 63
View file @ 765cef44
......@@ -61,99 +61,99 @@
(db/exists? User :email (u/lower-case-en e1)))))))))
(deftest semantic-type-migration-tests
;; have to turn off field validation since the semantic types below are no longer valid but were up to 38
(with-redefs [metabase.models.field/check-valid-types (constantly nil)]
(testing "updates each of the coercion types"
(testing "updates each of the coercion types"
(impl/test-migrations [283 296] [migrate!]
;; by name hoists results into a map by name so diffs are easier to read than sets.
(let [by-name #(into {} (map (juxt :name identity)) %)]
(impl/test-migrations [283 296] [migrate!]
(db/simple-insert! Database {:name "DB", :engine "h2", :created_at :%now, :updated_at :%now})
(db/simple-insert! Table {:name "Table", :db_id 1, :created_at :%now, :updated_at :%now, :active true})
(let [by-name #(into {} (map (juxt :name identity)) %)
db-id (db/simple-insert! Database {:name "DB", :engine "h2", :created_at :%now, :updated_at :%now})
table-id (db/simple-insert! Table {:name "Table", :db_id db-id, :created_at :%now, :updated_at :%now, :active true})]
;; have to turn off field validation since the semantic types below are no longer valid but were up to 38
(with-redefs [isa? (constantly true)]
(db/insert-many! Field
[{:base_type :type/Text
:semantic_type :type/Address
:name "address"
:table_id 1
:table_id table-id
:database_type "VARCHAR"}
{:base_type :type/Text
:semantic_type :type/ISO8601DateTimeString
:name "iso-datetime"
:table_id 1
:table_id table-id
:database_type "VARCHAR"}
{:base_type :type/Text
:semantic_type "timestamp_milliseconds"
:name "iso-datetime-v0.20"
:table_id 1
:table_id table-id
:database_type "VARCHAR"}
{:base_type :type/Text
:semantic_type :type/ISO8601DateString
:name "iso-date"
:table_id 1
:table_id table-id
:database_type "VARCHAR"}
{:base_type :type/Text
:semantic_type :type/ISO8601TimeString
:name "iso-time"
:table_id 1
:table_id table-id
:database_type "VARCHAR"}
{:base_type :type/Integer
:semantic_type :type/UNIXTimestampSeconds
:name "unix-seconds"
:table_id 1
:table_id table-id
:database_type "INT"}
{:base_type :type/Integer
:semantic_type :type/UNIXTimestampMilliseconds
:name "unix-millis"
:table_id 1
:table_id table-id
:database_type "INT"}
{:base_type :type/Integer
:semantic_type :type/UNIXTimestampMicroseconds
:name "unix-micros"
:table_id 1
:database_type "INT"}])
(migrate!)
(is (= (by-name
[{:base_type :type/Text
:effective_type :type/Text
:coercion_strategy nil
:semantic_type :type/Address
:name "address"}
{:base_type :type/Text
:effective_type :type/DateTime
:coercion_strategy :Coercion/ISO8601->DateTime
:semantic_type nil
:name "iso-datetime"}
{:base_type :type/Text
:effective_type :type/Instant
:coercion_strategy :Coercion/UNIXMilliSeconds->DateTime
:semantic_type nil
:name "iso-datetime-v0.20"}
{:base_type :type/Text
:effective_type :type/Date
:coercion_strategy :Coercion/ISO8601->Date
:semantic_type nil
:name "iso-date"}
{:base_type :type/Text
:effective_type :type/Time
:coercion_strategy :Coercion/ISO8601->Time
:semantic_type nil
:name "iso-time"}
{:base_type :type/Integer
:effective_type :type/Instant
:coercion_strategy :Coercion/UNIXSeconds->DateTime
:semantic_type nil
:name "unix-seconds"}
{:base_type :type/Integer
:effective_type :type/Instant
:coercion_strategy :Coercion/UNIXMilliSeconds->DateTime
:semantic_type nil
:name "unix-millis"}
{:base_type :type/Integer
:effective_type :type/Instant
:coercion_strategy :Coercion/UNIXMicroSeconds->DateTime
:semantic_type nil
:name "unix-micros"}])
(by-name
(into #{}
(map #(select-keys % [:base_type :effective_type :coercion_strategy
:semantic_type :name]))
(db/select Field))))))))))
:table_id table-id
:database_type "INT"}]))
(migrate!)
(is (= (by-name
[{:base_type :type/Text
:effective_type :type/Text
:coercion_strategy nil
:semantic_type :type/Address
:name "address"}
{:base_type :type/Text
:effective_type :type/DateTime
:coercion_strategy :Coercion/ISO8601->DateTime
:semantic_type nil
:name "iso-datetime"}
{:base_type :type/Text
:effective_type :type/Instant
:coercion_strategy :Coercion/UNIXMilliSeconds->DateTime
:semantic_type nil
:name "iso-datetime-v0.20"}
{:base_type :type/Text
:effective_type :type/Date
:coercion_strategy :Coercion/ISO8601->Date
:semantic_type nil
:name "iso-date"}
{:base_type :type/Text
:effective_type :type/Time
:coercion_strategy :Coercion/ISO8601->Time
:semantic_type nil
:name "iso-time"}
{:base_type :type/Integer
:effective_type :type/Instant
:coercion_strategy :Coercion/UNIXSeconds->DateTime
:semantic_type nil
:name "unix-seconds"}
{:base_type :type/Integer
:effective_type :type/Instant
:coercion_strategy :Coercion/UNIXMilliSeconds->DateTime
:semantic_type nil
:name "unix-millis"}
{:base_type :type/Integer
:effective_type :type/Instant
:coercion_strategy :Coercion/UNIXMicroSeconds->DateTime
:semantic_type nil
:name "unix-micros"}])
(by-name
(into #{}
(map #(select-keys % [:base_type :effective_type :coercion_strategy
:semantic_type :name]))
(db/select Field :table_id table-id)))))))))
test/metabase/models/field_test.clj
+ 32
− 1
View file @ 765cef44
(ns metabase.models.field-test
"Tests for specific behavior related to the Field model."
(:require [clojure.test :refer :all]
[metabase.sync.analyze.classifiers.name :as name]))
[metabase.models.field :refer [Field]]
[metabase.sync.analyze.classifiers.name :as name]
[metabase.test :as mt]
[metabase.util :as u]
[toucan.db :as db]))
(deftest semantic-type-for-name-and-base-type-test
(doseq [[input expected] {["id" :type/Integer] :type/PK
......@@ -13,3 +17,30 @@
(testing (pr-str (cons 'semantic-type-for-name-and-base-type input))
(is (= expected
(apply #'name/semantic-type-for-name-and-base-type input))))))
(deftest unknown-types-test
(doseq [{:keys [column unknown-type fallback-type]} [{:column :base_type
:unknown-type :type/Amazing
:fallback-type :type/*}
{:column :effective_type
:unknown-type :type/Amazing
:fallback-type :type/*}
{:column :semantic_type
:unknown-type :type/Amazing
:fallback-type nil}
{:column :coercion_strategy
:unknown-type :Coercion/Amazing
:fallback-type nil}]]
(testing (format "Field with unknown %s in DB should fall back to %s" column fallback-type)
(mt/with-temp Field [field]
(db/execute! {:update Field
:set {column (u/qualified-name unknown-type)}
:where [:= :id (u/the-id field)]})
(is (= fallback-type
(db/select-one-field column Field :id (u/the-id field))))))
(testing (format "Should throw an Exception if you attempt to save a Field with an invalid %s" column)
(is (thrown-with-msg?
clojure.lang.ExceptionInfo
(re-pattern (format "Invalid Field %s %s" column unknown-type))
(mt/with-temp Field [field {column unknown-type}]
field))))))
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