From 0920f1d13a07c97a73dce606ce20cdd3f0b5e5da Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cam=20Sa=C3=BCl?= <cammsaul@gmail.com>
Date: Tue, 6 Sep 2016 16:09:39 -0700
Subject: [PATCH] Cleanup in prepartion for new hierarchical field types
:shower:
---
frontend/src/metabase/lib/core.js | 14 ----
frontend/src/metabase/lib/query.js | 1 -
.../migrations/041_drop_field_field_type.yaml | 24 ++++++
resources/migrations/liquibase.json | 3 +-
.../metabase/sample_dataset/generate.clj | 3 +-
src/metabase/db/migrations.clj | 5 --
src/metabase/models/field.clj | 55 ++++++-------
src/metabase/query_processor/expand.clj | 10 +--
src/metabase/query_processor/interface.clj | 63 +++++++--------
src/metabase/sync_database/interface.clj | 1 -
src/metabase/util/schema.clj | 16 ++++
test/metabase/api/database_test.clj | 2 -
test/metabase/api/field_test.clj | 8 --
test/metabase/api/table_test.clj | 13 ----
test/metabase/models/field_test.clj | 8 +-
.../sync_database/sync_dynamic_test.clj | 2 +-
test/metabase/sync_database/sync_test.clj | 2 +-
test/metabase/sync_database_test.clj | 8 --
test/metabase/test/data.clj | 5 +-
test/metabase/test/data/interface.clj | 78 ++++++++-----------
test/metabase/test/mock/moviedb.clj | 13 ----
.../test/mock/schema_per_customer.clj | 29 -------
test/metabase/test/mock/toucanery.clj | 12 ---
test/metabase/test/util.clj | 11 +--
24 files changed, 150 insertions(+), 236 deletions(-)
create mode 100644 resources/migrations/041_drop_field_field_type.yaml
create mode 100644 src/metabase/util/schema.clj
diff --git a/frontend/src/metabase/lib/core.js b/frontend/src/metabase/lib/core.js
index 3249404b8d8..06cedb96b4c 100644
--- a/frontend/src/metabase/lib/core.js
+++ b/frontend/src/metabase/lib/core.js
@@ -88,20 +88,6 @@ export const field_special_types = [{
export const field_special_types_map = field_special_types
.reduce((map, type) => Object.assign({}, map, {[type.id]: type}), {});
-export const field_field_types = [{
- 'id': 'info',
- 'name': 'Information',
- 'description': 'Non-numerical value that is not meant to be used.'
-}, {
- 'id': 'metric',
- 'name': 'Metric',
- 'description': 'A number that can be added, graphed, etc.'
-}, {
- 'id': 'dimension',
- 'name': 'Dimension',
- 'description': 'A high or low-cardinality numerical string value that is meant to be used as a grouping.'
-}];
-
export const field_visibility_types = [{
'id': 'normal',
'name': 'Everywhere',
diff --git a/frontend/src/metabase/lib/query.js b/frontend/src/metabase/lib/query.js
index c70be0e9a1d..7e4ed6eb86c 100644
--- a/frontend/src/metabase/lib/query.js
+++ b/frontend/src/metabase/lib/query.js
@@ -521,7 +521,6 @@ var Query = {
operators_lookup: {},
valid_operators: [],
active: true,
- field_type: "info",
fk_target_field_id: null,
parent_id: null,
preview_display: true,
diff --git a/resources/migrations/041_drop_field_field_type.yaml b/resources/migrations/041_drop_field_field_type.yaml
new file mode 100644
index 00000000000..8b40de6dc74
--- /dev/null
+++ b/resources/migrations/041_drop_field_field_type.yaml
@@ -0,0 +1,24 @@
+databaseChangeLog:
+ - changeSet:
+ id: 41
+ author: camsaul
+ changes:
+ - dropColumn:
+ tableName: metabase_field
+ columnName: field_type
+ - addDefaultValue:
+ tableName: metabase_field
+ columnName: active
+ defaultValueBoolean: true
+ - addDefaultValue:
+ tableName: metabase_field
+ columnName: preview_display
+ defaultValueBoolean: true
+ - addDefaultValue:
+ tableName: metabase_field
+ columnName: position
+ defaultValueNumeric: 0
+ - addDefaultValue:
+ tableName: metabase_field
+ columnName: visibility_type
+ defaultValue: "normal"
diff --git a/resources/migrations/liquibase.json b/resources/migrations/liquibase.json
index c79840f6275..6cfa2675026 100644
--- a/resources/migrations/liquibase.json
+++ b/resources/migrations/liquibase.json
@@ -37,6 +37,7 @@
{"include": {"file": "migrations/036_add_dashboard_filters_columns.yaml"}},
{"include": {"file": "migrations/037_add_query_hash_and_indexes.yaml"}},
{"include": {"file": "migrations/038_getting_started_guide.yaml"}},
- {"include": {"file": "migrations/039_add_user_google_auth_column.yaml"}}
+ {"include": {"file": "migrations/039_add_user_google_auth_column.yaml"}},
+ {"include": {"file": "migrations/041_drop_field_field_type.yaml"}}
]
}
diff --git a/sample_dataset/metabase/sample_dataset/generate.clj b/sample_dataset/metabase/sample_dataset/generate.clj
index c39c6c6fe60..eb5614e7a6f 100644
--- a/sample_dataset/metabase/sample_dataset/generate.clj
+++ b/sample_dataset/metabase/sample_dataset/generate.clj
@@ -337,8 +337,7 @@
:latitude {:description "This is the latitude of the user on sign-up. It might be updated in the future to the last seen location."}
:longitude {:description "This is the longitude of the user on sign-up. It might be updated in the future to the last seen location."}
:name {:description "The name of the user who owns an account"}
- :password {:description "This is the salted password of the user. It should not be visible"
- :field_type :sensitive}
+ :password {:description "This is the salted password of the user. It should not be visible"}
:source {:description "The channel through which we acquired this user. Valid values include: Affiliate, Facebook, Google, Organic and Twitter"}
:state {:description "The state or province of the account’s billing address"}
:zip {:description "The postal code of the account’s billing address"
diff --git a/src/metabase/db/migrations.clj b/src/metabase/db/migrations.clj
index 24a69821af8..1272e2f1d8d 100644
--- a/src/metabase/db/migrations.clj
+++ b/src/metabase/db/migrations.clj
@@ -135,11 +135,6 @@
(db/update-where! Field {:visibility_type "unset"
:active false}
:visibility_type "retired")
- ;; anything that is active with field_type = :sensitive gets visibility_type :sensitive
- (db/update-where! Field {:visibility_type "unset"
- :active true
- :field_type "sensitive"}
- :visibility_type "sensitive")
;; if field is active but preview_display = false then it becomes :details-only
(db/update-where! Field {:visibility_type "unset"
:active true
diff --git a/src/metabase/models/field.clj b/src/metabase/models/field.clj
index 43a788fce4a..958c6ac992c 100644
--- a/src/metabase/models/field.clj
+++ b/src/metabase/models/field.clj
@@ -8,6 +8,9 @@
[interface :as i])
[metabase.util :as u]))
+
+;;; ------------------------------------------------------------ Type Mappings ------------------------------------------------------------
+
(def ^:const special-types
"Possible values for `Field.special_type`."
#{:avatar
@@ -46,13 +49,6 @@
:UUIDField ; e.g. a Postgres 'UUID' column
:UnknownField})
-(def ^:const ^:deprecated field-types
- "Possible values for `Field.field_type`.
- `field_type` is deprecated, and issue #2454 is open for removing it entirely. It's no longer displayed in the UI, nor is it possible to change its value from the API."
- #{:metric ; A number that can be added, graphed, etc.
- :dimension ; A high or low-cardinality numerical string value that is meant to be used as a grouping
- :info}) ; Non-numerical value that is not meant to be used
-
(def ^:const visibility-types
"Possible values for `Field.visibility_type`."
#{:normal ; Default setting. field has no visibility restrictions.
@@ -77,15 +73,13 @@
(contains? valid-base-types (keyword base-type)))))
+
+;;; ------------------------------------------------------------ Entity & Lifecycle ------------------------------------------------------------
+
(i/defentity Field :metabase_field)
(defn- pre-insert [field]
- (let [defaults {:active true
- :preview_display true
- :field_type :info
- :visibility_type :normal
- :position 0
- :display_name (humanization/name->human-readable-name (:name field))}]
+ (let [defaults {:display_name (humanization/name->human-readable-name (:name field))}]
(merge defaults field)))
(defn- pre-cascade-delete [{:keys [id]}]
@@ -93,6 +87,22 @@
(db/cascade-delete! 'FieldValues :field_id id)
(db/cascade-delete! 'MetricImportantField :field_id id))
+(u/strict-extend (class Field)
+ i/IEntity (merge i/IEntityDefaults
+ {:hydration-keys (constantly [:destination :field :origin])
+ :types (constantly {:base_type :keyword
+ :special_type :keyword
+ :visibility_type :keyword
+ :description :clob})
+ :timestamped? (constantly true)
+ :can-read? (constantly true)
+ :can-write? i/superuser?
+ :pre-insert pre-insert
+ :pre-cascade-delete pre-cascade-delete}))
+
+
+;;; ------------------------------------------------------------ Hydration / Util Fns ------------------------------------------------------------
+
(defn target
"Return the FK target `Field` that this `Field` points to."
@@ -153,19 +163,8 @@
[{:keys [table_id]}]
(db/select-one 'Table, :id table_id))
-(u/strict-extend (class Field)
- i/IEntity (merge i/IEntityDefaults
- {:hydration-keys (constantly [:destination :field :origin])
- :types (constantly {:base_type :keyword
- :field_type :keyword
- :special_type :keyword
- :visibility_type :keyword
- :description :clob})
- :timestamped? (constantly true)
- :can-read? (constantly true)
- :can-write? i/superuser?
- :pre-insert pre-insert
- :pre-cascade-delete pre-cascade-delete}))
+
+;;; ------------------------------------------------------------ Sync Util Type Inference Fns ------------------------------------------------------------
(def ^:private ^:const pattern+base-types+special-type
"Tuples of [pattern set-of-valid-base-types special-type]
@@ -219,7 +218,7 @@
(assert (every? (partial contains? base-types) base-types))
(assert (contains? special-types special-type)))
-(defn infer-field-special-type
+(defn- infer-field-special-type
"If `name` and `base-type` matches a known pattern, return the `special_type` we should assign to it."
[field-name base_type]
(when (and (string? field-name)
@@ -234,6 +233,8 @@
(last matching-pattern)))))
+;;; ------------------------------------------------------------ Sync Util CRUD Fns ------------------------------------------------------------
+
(defn update-field!
"Update an existing `Field` from the given FIELD-DEF."
[{:keys [id], :as existing-field} {field-name :name, :keys [base-type special-type pk? parent-id]}]
diff --git a/src/metabase/query_processor/expand.clj b/src/metabase/query_processor/expand.clj
index de4a00b814b..e532a5ae149 100644
--- a/src/metabase/query_processor/expand.clj
+++ b/src/metabase/query_processor/expand.clj
@@ -9,8 +9,8 @@
[metabase.db :as db]
[metabase.models.table :refer [Table]]
[metabase.query-processor.interface :refer [*driver*], :as i]
- [metabase.util :as u])
-
+ [metabase.util :as u]
+ [metabase.util.schema :as su])
(:import (metabase.query_processor.interface AgFieldRef
BetweenFilter
ComparisonFilter
@@ -29,7 +29,7 @@
(s/defn ^:private ^:always-validate normalize-token :- s/Keyword
"Convert a string or keyword in various cases (`lisp-case`, `snake_case`, or `SCREAMING_SNAKE_CASE`) to a lisp-cased keyword."
- [token :- (s/named (s/cond-pre s/Keyword s/Str) "Valid token (keyword or string)")]
+ [token :- su/KeywordOrString]
(-> (name token)
str/lower-case
(str/replace #"_" "-")
@@ -48,7 +48,7 @@
(s/defn ^:ql ^:always-validate field-id :- FieldPlaceholder
"Create a generic reference to a `Field` with ID."
- [id :- i/IntGreaterThanZero]
+ [id :- su/IntGreaterThanZero]
(i/map->FieldPlaceholder {:field-id id}))
(s/defn ^:private ^:always-validate field :- i/AnyField
@@ -108,7 +108,7 @@
(s/defn ^:ql ^:always-validate expression :- ExpressionRef
{:added "0.17.0"}
- [expression-name :- (s/cond-pre s/Str s/Keyword)]
+ [expression-name :- su/KeywordOrString]
(i/strict-map->ExpressionRef {:expression-name (name expression-name)}))
diff --git a/src/metabase/query_processor/interface.clj b/src/metabase/query_processor/interface.clj
index 44c49f5ba89..77ae08e6753 100644
--- a/src/metabase/query_processor/interface.clj
+++ b/src/metabase/query_processor/interface.clj
@@ -4,7 +4,8 @@
should go in `metabase.query-processor.expand`."
(:require [schema.core :as s]
[metabase.models.field :as field]
- [metabase.util :as u])
+ [metabase.util :as u]
+ [metabase.util.schema :as su])
(:import clojure.lang.Keyword
java.sql.Timestamp))
@@ -34,21 +35,15 @@
;; These are just used by the QueryExpander to record information about how joins should occur.
-(def IntGreaterThanZero
- "Schema representing an `s/Int` than must also be greater than zero."
- (s/constrained s/Int
- (partial < 0)
- "Integer greater than zero"))
-
-(s/defrecord JoinTableField [field-id :- IntGreaterThanZero
- field-name :- s/Str])
+(s/defrecord JoinTableField [field-id :- su/IntGreaterThanZero
+ field-name :- su/NonBlankString])
(s/defrecord JoinTable [source-field :- JoinTableField
pk-field :- JoinTableField
- table-id :- IntGreaterThanZero
- table-name :- s/Str
- schema :- (s/maybe s/Str)
- join-alias :- s/Str])
+ table-id :- su/IntGreaterThanZero
+ table-name :- su/NonBlankString
+ schema :- (s/maybe su/NonBlankString)
+ join-alias :- su/NonBlankString])
;;; # ------------------------------------------------------------ PROTOCOLS ------------------------------------------------------------
@@ -61,19 +56,19 @@
;;; # ------------------------------------------------------------ "RESOLVED" TYPES: FIELD + VALUE ------------------------------------------------------------
;; Field is the expansion of a Field ID in the standard QL
-(s/defrecord Field [field-id :- IntGreaterThanZero
- field-name :- s/Str
- field-display-name :- s/Str
+(s/defrecord Field [field-id :- su/IntGreaterThanZero
+ field-name :- su/NonBlankString
+ field-display-name :- su/NonBlankString
base-type :- (apply s/enum field/base-types)
special-type :- (s/maybe (apply s/enum field/special-types))
visibility-type :- (apply s/enum field/visibility-types)
- table-id :- IntGreaterThanZero
- schema-name :- (s/maybe s/Str)
- table-name :- (s/maybe s/Str) ; TODO - Why is this `maybe` ?
- position :- (s/maybe s/Int) ; TODO - int >= 0
+ table-id :- su/IntGreaterThanZero
+ schema-name :- (s/maybe su/NonBlankString)
+ table-name :- (s/maybe su/NonBlankString) ; TODO - Why is this `maybe` ?
+ position :- (s/maybe su/IntGreaterThanZero)
fk-field-id :- (s/maybe s/Int)
- description :- (s/maybe s/Str)
- parent-id :- (s/maybe IntGreaterThanZero)
+ description :- (s/maybe su/NonBlankString)
+ parent-id :- (s/maybe su/IntGreaterThanZero)
;; Field once its resolved; FieldPlaceholder before that
parent :- s/Any]
clojure.lang.Named
@@ -116,11 +111,7 @@
clojure.lang.Named
(getName [_] (name field)))
-(def NonEmptyString
- "Schema for a non-empty string."
- (s/constrained s/Str seq "non-empty string")) ; TODO - should this be used elsewhere as well, for things like `Field`?
-
-(s/defrecord ExpressionRef [expression-name :- NonEmptyString]
+(s/defrecord ExpressionRef [expression-name :- su/NonBlankString]
clojure.lang.Named
(getName [_] expression-name)
IField
@@ -130,7 +121,7 @@
;; Value is the expansion of a value within a QL clause
;; Information about the associated Field is included for convenience
-(s/defrecord Value [value :- (s/maybe (s/cond-pre s/Bool s/Num s/Str))
+(s/defrecord Value [value :- (s/maybe (s/cond-pre s/Bool s/Num su/NonBlankString))
field :- (s/named (s/cond-pre Field ExpressionRef) ; TODO - Value doesn't need the whole field, just the relevant type info / units
"field or expression reference")])
@@ -162,8 +153,8 @@
;;; # ------------------------------------------------------------ PLACEHOLDER TYPES: FIELDPLACEHOLDER + VALUEPLACEHOLDER ------------------------------------------------------------
;; Replace Field IDs with these during first pass
-(s/defrecord FieldPlaceholder [field-id :- IntGreaterThanZero
- fk-field-id :- (s/maybe (s/constrained IntGreaterThanZero
+(s/defrecord FieldPlaceholder [field-id :- su/IntGreaterThanZero
+ fk-field-id :- (s/maybe (s/constrained su/IntGreaterThanZero
(fn [_] (or (assert-driver-supports :foreign-keys) true))
"foreign-keys is not supported by this driver."))
datetime-unit :- (s/maybe (apply s/enum datetime-field-units))])
@@ -203,7 +194,7 @@
(def LiteralDatetimeString
"Schema for an MBQL datetime string literal, in ISO-8601 format."
- (s/constrained s/Str u/date-string? "Valid ISO-8601 datetime string literal"))
+ (s/constrained su/NonBlankString u/date-string? "Valid ISO-8601 datetime string literal"))
(def LiteralDatetime
"Schema for an MBQL literal datetime value: and ISO-8601 string or `java.sql.Date`."
@@ -219,7 +210,7 @@
(def AnyValue
"Schema for anything that is a considered a valid value in MBQL - `nil`, a `Boolean`, `Number`, `String`, or relative datetime form."
- (s/named (s/maybe (s/cond-pre s/Bool s/Str OrderableValue)) "Valid value (must be nil, boolean, number, string, or a relative-datetime form)"))
+ (s/named (s/maybe (s/cond-pre s/Bool su/NonBlankString OrderableValue)) "Valid value (must be nil, boolean, number, string, or a relative-datetime form)"))
;; Replace values with these during first pass over Query.
;; Include associated Field ID so appropriate the info can be found during Field resolution
@@ -316,8 +307,8 @@
(def Page
"Schema for the top-level `page` clause in a MBQL query."
- (s/named {:page IntGreaterThanZero
- :items IntGreaterThanZero}
+ (s/named {:page su/IntGreaterThanZero
+ :items su/IntGreaterThanZero}
"Valid page clause"))
(def Query
@@ -326,8 +317,8 @@
(s/optional-key :breakout) [FieldPlaceholderOrExpressionRef]
(s/optional-key :fields) [AnyField]
(s/optional-key :filter) Filter
- (s/optional-key :limit) IntGreaterThanZero
+ (s/optional-key :limit) su/IntGreaterThanZero
(s/optional-key :order-by) [OrderBy]
(s/optional-key :page) Page
(s/optional-key :expressions) {s/Keyword Expression}
- :source-table IntGreaterThanZero})
+ :source-table su/IntGreaterThanZero})
diff --git a/src/metabase/sync_database/interface.clj b/src/metabase/sync_database/interface.clj
index 2b3dffdeeb8..047151cd116 100644
--- a/src/metabase/sync_database/interface.clj
+++ b/src/metabase/sync_database/interface.clj
@@ -20,7 +20,6 @@
"Schema for a given Field as provided in `describe-table` or `analyze-table`."
{:name schema/Str
:base-type (apply schema/enum field/base-types)
- (schema/optional-key :field-type) (apply schema/enum field/field-types)
(schema/optional-key :special-type) (apply schema/enum field/special-types)
(schema/optional-key :pk?) schema/Bool
(schema/optional-key :nested-fields) #{(schema/recursive #'DescribeTableField)}
diff --git a/src/metabase/util/schema.clj b/src/metabase/util/schema.clj
new file mode 100644
index 00000000000..f6b39d289a4
--- /dev/null
+++ b/src/metabase/util/schema.clj
@@ -0,0 +1,16 @@
+(ns metabase.util.schema
+ "Various schemas that are useful throughout the app."
+ (:require [clojure.string :as str]
+ [schema.core :as s]))
+
+(def NonBlankString
+ "Schema for a string that cannot be blank."
+ (s/constrained s/Str (complement str/blank?) "Non-blank string"))
+
+(def IntGreaterThanZero
+ "Schema representing an integer than must also be greater than zero."
+ (s/constrained s/Int (partial < 0) "Integer greater than zero"))
+
+(def KeywordOrString
+ "Schema for something that can be either a `Keyword` or a `String`."
+ (s/named (s/cond-pre s/Keyword s/Str) "Keyword or string"))
diff --git a/test/metabase/api/database_test.clj b/test/metabase/api/database_test.clj
index b74e625f414..affa2cb2da8 100644
--- a/test/metabase/api/database_test.clj
+++ b/test/metabase/api/database_test.clj
@@ -266,7 +266,6 @@
:active true
:id $
:raw_column_id $
- :field_type "info"
:position 0
:target nil
:preview_display true
@@ -289,7 +288,6 @@
:active true
:id $
:raw_column_id $
- :field_type "info"
:position 0
:target nil
:preview_display true
diff --git a/test/metabase/api/field_test.clj b/test/metabase/api/field_test.clj
index 0cebd9f9409..b73e12f0f62 100644
--- a/test/metabase/api/field_test.clj
+++ b/test/metabase/api/field_test.clj
@@ -62,7 +62,6 @@
:last_analyzed $
:active true
:id (id :users :name)
- :field_type "info"
:visibility_type "normal"
:position 0
:preview_display true
@@ -144,13 +143,6 @@
((user->client :crowberto) :put 200 (str "field/" field-id) {:special_type :timestamp_seconds})
(db/select-one-field :special_type Field, :id field-id)))
-;; check that a Field with a :base_type of :sensitive can still be modified as normal.
-;; This was a bug introduced when :visibility-type was added -- see issue #2678
-(expect
- (tu/with-temp* [Field [{field-id :id} {:field_type "sensitive"}]]
- (boolean ((user->client :crowberto) :put 200 (str "field/" field-id) {:special_type :avatar}))))
-
-
(defn- field->field-values
"Fetch the `FieldValues` object that corresponds to a given `Field`."
diff --git a/test/metabase/api/table_test.clj b/test/metabase/api/table_test.clj
index 3ae47aa9c05..a8f32ff2470 100644
--- a/test/metabase/api/table_test.clj
+++ b/test/metabase/api/table_test.clj
@@ -122,7 +122,6 @@
:updated_at $
:active true
:id (id :categories :id)
- :field_type "info"
:position 0
:preview_display true
:created_at $
@@ -145,7 +144,6 @@
:updated_at $
:active true
:id (id :categories :name)
- :field_type "info"
:position 0
:preview_display true
:created_at $
@@ -181,7 +179,6 @@
:updated_at $
:active true
:id $
- :field_type "info"
:position 0
:target nil
:preview_display true
@@ -203,7 +200,6 @@
:updated_at $
:active true
:id $
- :field_type "info"
:position 0
:target nil
:preview_display true
@@ -269,7 +265,6 @@
:updated_at $
:active true
:id $
- :field_type "info"
:position 0
:target nil
:preview_display true
@@ -291,7 +286,6 @@
:updated_at $
:active true
:id $
- :field_type "info"
:position 0
:target nil
:preview_display true
@@ -313,7 +307,6 @@
:updated_at $
:active true
:id $
- :field_type "info"
:position 0
:target nil
:preview_display true
@@ -335,7 +328,6 @@
:updated_at $
:active true
:id $
- :field_type "info"
:position 0
:target nil
:preview_display true
@@ -399,7 +391,6 @@
:updated_at $
:active true
:id $
- :field_type "info"
:position 0
:target nil
:preview_display true
@@ -421,7 +412,6 @@
:updated_at $
:active true
:id $
- :field_type "info"
:position 0
:target nil
:preview_display true
@@ -443,7 +433,6 @@
:updated_at $
:active true
:id $
- :field_type "info"
:position 0
:target nil
:preview_display true
@@ -547,7 +536,6 @@
:visibility_type "normal"
:preview_display $
:position $
- :field_type "info"
:active true
:special_type "fk"
:fk_target_field_id $
@@ -587,7 +575,6 @@
:visibility_type "normal"
:preview_display $
:position $
- :field_type "info"
:active true
:special_type "id"
:fk_target_field_id $
diff --git a/test/metabase/models/field_test.clj b/test/metabase/models/field_test.clj
index 045e556aaed..4bd22bbc545 100644
--- a/test/metabase/models/field_test.clj
+++ b/test/metabase/models/field_test.clj
@@ -2,7 +2,8 @@
(:require [expectations :refer :all]
(metabase.models [field :refer :all]
[field-values :refer :all])
- [metabase.test.data :refer :all]))
+ [metabase.test.data :refer :all]
+ [metabase.test.util :as tu]))
;; valid-special-type-for-base-type?
@@ -80,7 +81,10 @@
:visibility_type :normal}))
-;; infer-field-special-type
+;;; infer-field-special-type
+
+(tu/resolve-private-fns metabase.models.field infer-field-special-type)
+
(expect nil (infer-field-special-type nil nil))
(expect nil (infer-field-special-type "id" nil))
(expect nil (infer-field-special-type nil :IntegerField))
diff --git a/test/metabase/sync_database/sync_dynamic_test.clj b/test/metabase/sync_database/sync_dynamic_test.clj
index 184698c1d28..46e0b3b7253 100644
--- a/test/metabase/sync_database/sync_dynamic_test.clj
+++ b/test/metabase/sync_database/sync_dynamic_test.clj
@@ -287,7 +287,7 @@
(let [get-fields (fn []
(for [field (db/select Field, :table_id table-id, {:order-by [:id]})]
(dissoc (tu/boolean-ids-and-timestamps field)
- :active :field_type :position :preview_display)))]
+ :active :position :preview_display)))]
;; start with no fields
[(get-fields)
;; first sync will add all the fields
diff --git a/test/metabase/sync_database/sync_test.clj b/test/metabase/sync_database/sync_test.clj
index 5983e9a48ce..59ef6f78e24 100644
--- a/test/metabase/sync_database/sync_test.clj
+++ b/test/metabase/sync_database/sync_test.clj
@@ -289,7 +289,7 @@
(let [get-fields #(->> (db/select Field, :table_id table-id, {:order-by [:id]})
(mapv tu/boolean-ids-and-timestamps)
(mapv (fn [m]
- (dissoc m :active :field_type :position :preview_display))))
+ (dissoc m :active :position :preview_display))))
initial-fields (get-fields)
first-sync (do
(save-table-fields! tbl)
diff --git a/test/metabase/sync_database_test.clj b/test/metabase/sync_database_test.clj
index a234865123b..7970b39be0d 100644
--- a/test/metabase/sync_database_test.clj
+++ b/test/metabase/sync_database_test.clj
@@ -91,7 +91,6 @@
:name "id"
:active true
:parent_id false
- :field_type :info
:position 0
:preview_display true
:display_name "ID"
@@ -111,7 +110,6 @@
:name "studio"
:active true
:parent_id false
- :field_type :info
:position 0
:preview_display true
:display_name "Studio"
@@ -131,7 +129,6 @@
:name "title"
:active true
:parent_id false
- :field_type :info
:position 0
:preview_display true
:display_name "Title"
@@ -168,7 +165,6 @@
:name "name"
:active true
:parent_id false
- :field_type :info
:position 0
:preview_display true
:display_name "Name"
@@ -188,7 +184,6 @@
:name "studio"
:active true
:parent_id false
- :field_type :info
:position 0
:preview_display true
:display_name "Studio"
@@ -236,7 +231,6 @@
:name "id"
:active true
:parent_id false
- :field_type :info
:position 0
:preview_display true
:display_name "ID"
@@ -256,7 +250,6 @@
:name "studio"
:active true
:parent_id false
- :field_type :info
:position 0
:preview_display true
:display_name "Studio"
@@ -276,7 +269,6 @@
:name "title"
:active true
:parent_id false
- :field_type :info
:position 0
:preview_display true
:display_name "Title"
diff --git a/test/metabase/test/data.clj b/test/metabase/test/data.clj
index bb944bc9b89..01367ce1f94 100644
--- a/test/metabase/test/data.clj
+++ b/test/metabase/test/data.clj
@@ -196,14 +196,11 @@
table-name
(u/pprint-to-str (dissoc table-definition :rows))
(u/pprint-to-str (db/select [Table :schema :name], :db_id (:id db))))))))]
- (doseq [{:keys [field-name field-type visibility-type special-type], :as field-definition} (:field-definitions table-definition)]
+ (doseq [{:keys [field-name visibility-type special-type], :as field-definition} (:field-definitions table-definition)]
(let [field (delay (or (i/metabase-instance field-definition @table)
(throw (Exception. (format "Field '%s' not loaded from definition:\n"
field-name
(u/pprint-to-str field-definition))))))]
- (when field-type
- (log/debug (format "SET FIELD TYPE %s.%s -> %s" table-name field-name field-type))
- (db/update! Field (:id @field) :field_type (name field-type)))
(when visibility-type
(log/debug (format "SET VISIBILITY TYPE %s.%s -> %s" table-name field-name visibility-type))
(db/update! Field (:id @field) :visibility_type (name visibility-type)))
diff --git a/test/metabase/test/data/interface.clj b/test/metabase/test/data/interface.clj
index 996af1c691a..70fb810a603 100644
--- a/test/metabase/test/data/interface.clj
+++ b/test/metabase/test/data/interface.clj
@@ -3,36 +3,39 @@
Objects that implement `IDatasetLoader` know how to load a `DatabaseDefinition` into an
actual physical RDMS database. This functionality allows us to easily test with multiple datasets."
- (:require [clojure.string :as s]
+ (:require [clojure.string :as str]
+ [schema.core :as s]
(metabase [db :as db]
[driver :as driver])
(metabase.models [database :refer [Database]]
[field :refer [Field] :as field]
[table :refer [Table]])
- [metabase.util :as u])
+ [metabase.util :as u]
+ [metabase.util.schema :as su])
(:import clojure.lang.Keyword))
-(defrecord FieldDefinition [^String field-name
- ^Keyword base-type
- ^Keyword field-type
- ^Keyword special-type
- ^Keyword visibility-type
- ^Keyword fk])
+(s/defrecord FieldDefinition [field-name :- su/NonBlankString
+ base-type :- (s/cond-pre {:native su/NonBlankString}
+ (apply s/enum field/base-types))
+ special-type :- (s/maybe (apply s/enum field/special-types))
+ visibility-type :- (s/maybe (apply s/enum field/visibility-types))
+ fk :- (s/maybe s/Keyword)])
-(defrecord TableDefinition [^String table-name
- field-definitions
- rows])
+(s/defrecord TableDefinition [table-name :- su/NonBlankString
+ field-definitions :- [FieldDefinition]
+ rows :- [[s/Any]]])
-(defrecord DatabaseDefinition [^String database-name
- table-definitions
- ;; Optional. Set this to non-nil to let dataset loaders know that we don't intend to keep it
- ;; for long -- they can adjust connection behavior, e.g. choosing simple connections instead of creating pools.
- ^Boolean short-lived?])
+(s/defrecord DatabaseDefinition [database-name :- su/NonBlankString
+ table-definitions :- [TableDefinition]
+ ;; Optional. Set this to non-nil to let dataset loaders know that we don't intend to keep it
+ ;; for long -- they can adjust connection behavior, e.g. choosing simple connections instead of creating pools.
+ ;; TODO - not sure this is still used now that we create connection pools directly via C3P0; we might be able to remove it
+ short-lived? :- (s/maybe s/Bool)])
(defn escaped-name
"Return escaped version of database name suitable for use as a filename / database name / etc."
^String [^DatabaseDefinition database-definition]
- (s/replace (:database-name database-definition) #"\s+" "_"))
+ (str/replace (:database-name database-definition) #"\s+" "_"))
(defn db-qualified-table-name
"Return a combined table name qualified with the name of its database, suitable for use as an identifier.
@@ -41,7 +44,7 @@
^String [^String database-name, ^String table-name]
{:pre [(string? database-name) (string? table-name)]}
;; take up to last 30 characters because databases like Oracle have limits on the lengths of identifiers
- (apply str (take-last 30 (s/replace (s/lower-case (str database-name \_ table-name)) #"-" "_"))))
+ (apply str (take-last 30 (str/replace (str/lower-case (str database-name \_ table-name)) #"-" "_"))))
(defprotocol IMetabaseInstance
@@ -53,19 +56,19 @@
(extend-protocol IMetabaseInstance
FieldDefinition
(metabase-instance [this table]
- (Field :table_id (:id table), :%lower.name (s/lower-case (:field-name this))))
+ (Field :table_id (:id table), :%lower.name (str/lower-case (:field-name this))))
TableDefinition
(metabase-instance [this database]
;; Look first for an exact table-name match; otherwise allow DB-qualified table names for drivers that need them like Oracle
- (or (Table :db_id (:id database), :%lower.name (s/lower-case (:table-name this)))
+ (or (Table :db_id (:id database), :%lower.name (str/lower-case (:table-name this)))
(Table :db_id (:id database), :%lower.name (db-qualified-table-name (:name database) (:table-name this)))))
DatabaseDefinition
(metabase-instance [{:keys [database-name]} engine-kw]
(assert (string? database-name))
(assert (keyword? engine-kw))
- (db/setup-db-if-needed :auto-migrate true)
+ (db/setup-db-if-needed, :auto-migrate true)
(Database :name database-name, :engine (name engine-kw))))
@@ -129,34 +132,21 @@
(defn create-field-definition
"Create a new `FieldDefinition`; verify its values."
^FieldDefinition [{:keys [field-name base-type field-type special-type visibility-type fk], :as field-definition-map}]
- (assert (or (contains? field/base-types base-type)
- (and (map? base-type)
- (string? (:native base-type))))
- (str (format "Invalid field base type: '%s'\n" base-type)
- "Field base-type should be either a valid base type like :TextField or be some native type wrapped in a map, like {:native \"JSON\"}."))
- (when field-type
- (assert (contains? field/field-types field-type)))
- (when visibility-type
- (assert (contains? field/visibility-types visibility-type)))
- (when special-type
- (assert (contains? field/special-types special-type)))
- (map->FieldDefinition field-definition-map))
+ (s/validate FieldDefinition (map->FieldDefinition field-definition-map)))
(defn create-table-definition
"Convenience for creating a `TableDefinition`."
- ^TableDefinition [^String table-name field-definition-maps rows]
- (map->TableDefinition {:table-name table-name
- :rows rows
- :field-definitions (mapv create-field-definition field-definition-maps)}))
+ ^TableDefinition [^String table-name, field-definition-maps rows]
+ (s/validate TableDefinition (map->TableDefinition {:table-name table-name
+ :rows rows
+ :field-definitions (mapv create-field-definition field-definition-maps)})))
(defn create-database-definition
"Convenience for creating a new `DatabaseDefinition`."
^DatabaseDefinition [^String database-name & table-name+field-definition-maps+rows]
- {:pre [(string? database-name)
- (not (s/blank? database-name))]}
- (map->DatabaseDefinition {:database-name database-name
- :table-definitions (mapv (partial apply create-table-definition)
- table-name+field-definition-maps+rows)}))
+ (s/validate DatabaseDefinition (map->DatabaseDefinition {:database-name database-name
+ :table-definitions (mapv (partial apply create-table-definition)
+ table-name+field-definition-maps+rows)})))
(defmacro def-database-definition
"Convenience for creating a new `DatabaseDefinition` named by the symbol DATASET-NAME."
@@ -221,8 +211,8 @@
field-name
(let [[_ fk-table fk-dest-name] field-name]
(-> fk-table
- (s/replace #"ies$" "y")
- (s/replace #"s$" "")
+ (str/replace #"ies$" "y")
+ (str/replace #"s$" "")
(str \_ (flatten-field-name fk-dest-name))))))
(defn flatten-dbdef
diff --git a/test/metabase/test/mock/moviedb.clj b/test/metabase/test/mock/moviedb.clj
index 9c59a0e1eb5..e2ca8b37994 100644
--- a/test/metabase/test/mock/moviedb.clj
+++ b/test/metabase/test/mock/moviedb.clj
@@ -266,7 +266,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -286,7 +285,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -323,7 +321,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -343,7 +340,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -363,7 +359,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -400,7 +395,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -420,7 +414,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -440,7 +433,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -477,7 +469,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -497,7 +488,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -517,7 +507,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -537,7 +526,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -557,7 +545,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
diff --git a/test/metabase/test/mock/schema_per_customer.clj b/test/metabase/test/mock/schema_per_customer.clj
index cf7e6d917c3..03600191cc7 100644
--- a/test/metabase/test/mock/schema_per_customer.clj
+++ b/test/metabase/test/mock/schema_per_customer.clj
@@ -483,7 +483,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -503,7 +502,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -540,7 +538,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -560,7 +557,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -597,7 +593,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -617,7 +612,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -638,7 +632,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -658,7 +651,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -696,7 +688,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -717,7 +708,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -737,7 +727,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -774,7 +763,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -794,7 +782,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -831,7 +818,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -851,7 +837,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -872,7 +857,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -892,7 +876,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -930,7 +913,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -951,7 +933,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -971,7 +952,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -1008,7 +988,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -1028,7 +1007,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -1065,7 +1043,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -1085,7 +1062,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -1106,7 +1082,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -1126,7 +1101,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -1164,7 +1138,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -1185,7 +1158,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -1205,7 +1177,6 @@
:id true
:raw_column_id true
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
diff --git a/test/metabase/test/mock/toucanery.clj b/test/metabase/test/mock/toucanery.clj
index 2d15914f27c..33f66b846f0 100644
--- a/test/metabase/test/mock/toucanery.clj
+++ b/test/metabase/test/mock/toucanery.clj
@@ -103,7 +103,6 @@
:id true
:raw_column_id false
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -123,7 +122,6 @@
:id true
:raw_column_id false
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -160,7 +158,6 @@
:id true
:raw_column_id false
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -180,7 +177,6 @@
:id true
:raw_column_id false
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -200,7 +196,6 @@
:id true
:raw_column_id false
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -220,7 +215,6 @@
:id true
:raw_column_id false
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -240,7 +234,6 @@
:id true
:raw_column_id false
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -260,7 +253,6 @@
:id true
:raw_column_id false
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -280,7 +272,6 @@
:id true
:raw_column_id false
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -300,7 +291,6 @@
:id true
:raw_column_id false
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -320,7 +310,6 @@
:id true
:raw_column_id false
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
@@ -340,7 +329,6 @@
:id true
:raw_column_id false
:last_analyzed false
- :field_type :info
:position 0
:visibility_type :normal
:preview_display true
diff --git a/test/metabase/test/util.clj b/test/metabase/test/util.clj
index 1a3ce45b302..531f5695503 100644
--- a/test/metabase/test/util.clj
+++ b/test/metabase/test/util.clj
@@ -123,13 +123,10 @@
(u/strict-extend (class Field)
WithTempDefaults
- {:with-temp-defaults (fn [_] {:active true
- :base_type :TextField
- :field_type :info
- :name (random-name)
- :position 1
- :preview_display true
- :table_id (data/id :venues)})})
+ {:with-temp-defaults (fn [_] {:base_type :TextField
+ :name (random-name)
+ :position 1
+ :table_id (data/id :venues)})})
(u/strict-extend (class Metric)
WithTempDefaults
--
GitLab