diff --git a/src/metabase/api/common.clj b/src/metabase/api/common.clj
index 1bcca2b55f397bf2ba632a09cd8dbf44bee10e2b..cd30dc149f15aeff8021864e98676fa595d81efd 100644
--- a/src/metabase/api/common.clj
+++ b/src/metabase/api/common.clj
@@ -123,8 +123,7 @@
-> (check (contains? #{:fav :all :mine} f)
[400 (str \"Invalid value '\" f \"' for 'f': must be one of: #{:fav :all :mine}\")])"
[valid-values-set symb value]
- {:pre [(set? valid-values-set)
- (symbol? symb)]}
+ {:pre [(set? valid-values-set) (symbol? symb)]}
(checkp-with (partial contains? valid-values-set) symb value (str "must be one of: " valid-values-set)))
@@ -362,7 +361,7 @@
(defannotation NonEmptyString
"Param must be a non-empty string (strings that only contain whitespace are considered empty)."
[symb value :nillable]
- (checkp-with (complement clojure.string/blank?) symb value "value must be a non-empty string."))
+ (checkp-with (complement s/blank?) symb value "value must be a non-empty string."))
(defannotation PublicPerms
"Param must be an integer and either `0` (no public permissions), `1` (public may read), or `2` (public may read and write)."
diff --git a/src/metabase/api/field.clj b/src/metabase/api/field.clj
index 87a9287e21e4361228dfeedea59cda0051161d72..3d0adcf2c53e62c861733a62913e060e041d783b 100644
--- a/src/metabase/api/field.clj
+++ b/src/metabase/api/field.clj
@@ -17,10 +17,6 @@
[symb value :nillable]
(checkp-contains? field/visibility-types symb (keyword value)))
-(defannotation FieldType
- "Param must be a valid `Field` base type."
- [symb value :nillable]
- (checkp-contains? field/field-types symb (keyword value)))
(defendpoint GET "/:id"
"Get `Field` with ID."
@@ -29,35 +25,46 @@
read-check
(hydrate [:table :db])))
+
(defendpoint PUT "/:id"
"Update `Field` with ID."
- [id :as {{:keys [special_type visibility_type fk_target_field_id description display_name caveats points_of_interest], :as body} :body}]
- {special_type FieldSpecialType
- visibility_type FieldVisibilityType
- display_name NonEmptyString}
+ [id :as {{:keys [caveats description display_name fk_target_field_id points_of_interest special_type visibility_type], :as body} :body}]
+ {caveats NonEmptyString
+ description NonEmptyString
+ display_name NonEmptyString
+ fk_target_field_id Integer
+ points_of_interest NonEmptyString
+ special_type FieldSpecialType
+ visibility_type FieldVisibilityType}
+
(let-404 [field (Field id)]
(write-check field)
- (let [special_type (if (contains? body :special_type) special_type (:special_type field))
+ (println "special_type:" special_type) ; NOCOMMIT
+ (let [special_type (keyword (get body :special_type (:special_type field)))
visibility_type (or visibility_type (:visibility_type field))
- fk_target_field_id (when (= :fk special_type)
- ;; only let target field be set for :fk type fields,
- ;; and if it's not in the payload then leave the current value
- (if (contains? body :fk_target_field_id)
- fk_target_field_id
- (:fk_target_field_id field)))]
- (check-400 (field/valid-metadata? (:base_type field) (:field_type field) special_type visibility_type))
- ;; validate that fk_target_field_id is a valid Field within the same database as our field
- (when fk_target_field_id
- (checkp (db/exists? Field :id fk_target_field_id)
+ ;; only let target field be set for :fk type fields, and if it's not in the payload then leave the current value
+ fk-target-field-id (when (= :fk special_type)
+ (get body :fk_target_field_id (:fk_target_field_id field)))]
+ ;; make sure that the special type is allowed for the base type
+ (check (field/valid-special-type-for-base-type? special_type (:base_type field))
+ [400 (format "Special type %s cannot be used for fields with base type %s. Base type must be one of: %s"
+ special_type
+ (:base_type field)
+ (field/special-type->valid-base-types special_type))])
+ ;; validate that fk_target_field_id is a valid Field
+ ;; TODO - we should also check that the Field is within the same database as our field
+ (when fk-target-field-id
+ (checkp (db/exists? Field :id fk-target-field-id)
:fk_target_field_id "Invalid target field"))
- ;; update the Field. start with keys that may be set to NULL then conditionally add other keys if they have values
- (check-500 (db/update! Field id (merge {:description description
- :caveats caveats
+ ;; everything checks out, now update the field
+ (check-500 (db/update! Field id (merge {:caveats caveats
+ :description description
+ :fk_target_field_id fk_target_field_id
:points_of_interest points_of_interest
:special_type special_type
- :visibility_type visibility_type
- :fk_target_field_id fk_target_field_id}
+ :visibility_type visibility_type}
(when display_name {:display_name display_name}))))
+ ;; return updated field
(Field id))))
(defendpoint GET "/:id/summary"
diff --git a/src/metabase/models/field.clj b/src/metabase/models/field.clj
index c21d9f1be9e60569aab7b798b2166d31e1f6b236..43a788fce4a1d9c71e60ef0cbe94b1d250218535 100644
--- a/src/metabase/models/field.clj
+++ b/src/metabase/models/field.clj
@@ -46,8 +46,9 @@
:UUIDField ; e.g. a Postgres 'UUID' column
:UnknownField})
-(def ^:const field-types
- "Possible values for `Field.field_type`."
+(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
@@ -60,18 +61,20 @@
:sensitive ; Strict removal of field from all places except data model listing. queries should error if someone attempts to access.
:retired}) ; For fields that no longer exist in the physical db. automatically set by Metabase. QP should error if encountered in a query.
-(defn valid-metadata?
- "Predicate function that checks if the set of metadata types for a field are sensible."
- [base-type field-type special-type visibility-type]
- (and (contains? base-types base-type)
- (contains? field-types field-type)
- (contains? visibility-types visibility-type)
- (or (nil? special-type)
- (contains? special-types special-type))
- ;; this verifies that we have a numeric base-type when trying to use the unix timestamp transform (#824)
- (or (nil? special-type)
- (not (contains? #{:timestamp_milliseconds :timestamp_seconds} special-type))
- (contains? #{:BigIntegerField :DecimalField :FloatField :IntegerField} base-type))))
+
+(def ^:const special-type->valid-base-types
+ "Map of special types to set of base types that are allowed to have that special type.
+ Special types not included in this map can be applied to any base type."
+ (let [numeric-base-types #{:BigIntegerField :DecimalField :FloatField :IntegerField}]
+ {:timestamp_seconds numeric-base-types
+ :timestamp_milliseconds numeric-base-types}))
+
+(defn valid-special-type-for-base-type?
+ "Can SPECIAL-TYPE be used for this BASE-TYPE?"
+ ^Boolean [special-type base-type]
+ (let [valid-base-types (special-type->valid-base-types (keyword special-type))]
+ (or (not valid-base-types)
+ (contains? valid-base-types (keyword base-type)))))
(i/defentity Field :metabase_field)
diff --git a/test/metabase/api/field_test.clj b/test/metabase/api/field_test.clj
index f198bf0f5c4ebf8dff6c0d4a97e1f5e794af458a..0cebd9f94091fd4843934383df9227d0e0aecb0b 100644
--- a/test/metabase/api/field_test.clj
+++ b/test/metabase/api/field_test.clj
@@ -1,6 +1,7 @@
(ns metabase.api.field-test
(:require [expectations :refer :all]
[metabase.db :as db]
+ [metabase.driver :as driver]
(metabase.models [database :refer [Database]]
[field :refer [Field]]
[field-values :refer [FieldValues]]
@@ -24,7 +25,7 @@
:is_full_sync true
:organization_id nil
:description nil
- :features (mapv name (metabase.driver/features (metabase.driver/engine->driver :h2)))}))
+ :features (mapv name (driver/features (driver/engine->driver :h2)))}))
;; ## GET /api/field/:id
@@ -100,23 +101,7 @@
:description nil
:special_type nil
:visibility_type :sensitive}]
- (tu/with-temp* [Database [{database-id :id} {:name "Field Test"
- :engine :yeehaw
- :caveats nil
- :points_of_interest nil
- :details {}
- :is_sample false}]
- Table [{table-id :id} {:name "Field Test"
- :db_id database-id
- :active true}]
- Field [{field-id :id} {:table_id table-id
- :name "Field Test"
- :base_type :TextField
- :field_type :info
- :special_type nil
- :active true
- :preview_display true
- :position 1}]]
+ (tu/with-temp* [Field [{field-id :id} {:name "Field Test"}]]
(let [original-val (simple-field-details (Field field-id))]
;; set it
((user->client :crowberto) :put 200 (format "field/%d" field-id) {:name "something else"
@@ -136,48 +121,36 @@
(expect
[true
nil]
- (tu/with-temp* [Database [{database-id :id} {:name "Field Test"
- :engine :yeehaw
- :caveats nil
- :points_of_interest nil
- :details {}
- :is_sample false}]
- Table [{table-id :id} {:name "Field Test"
- :db_id database-id
- :active true}]
- Field [{field-id1 :id} {:table_id table-id
- :name "Target Field"
- :base_type :TextField
- :field_type :info
- :special_type :id
- :active true
- :preview_display true
- :position 1}]
- Field [{field-id :id} {:table_id table-id
- :name "Field Test"
- :base_type :TextField
- :field_type :info
- :special_type :fk
- :fk_target_field_id field-id1
- :active true
- :preview_display true
- :position 1}]]
+ (tu/with-temp* [Field [{fk-field-id :id}]
+ Field [{field-id :id} {:special_type :fk, :fk_target_field_id fk-field-id}]]
(let [original-val (boolean (db/select-one-field :fk_target_field_id Field, :id field-id))]
;; unset the :fk special-type
((user->client :crowberto) :put 200 (format "field/%d" field-id) {:special_type :name})
[original-val
(db/select-one-field :fk_target_field_id Field, :id field-id)])))
-;; check that you can't set a field to :timestamp_seconds if it's not of a proper base_type
+;; check that you can't set a field to :timestamp_seconds/:timestamp_milliseconds if it's not of a proper base_type
(expect
- ["Invalid Request."
+ ["Special type :timestamp_seconds cannot be used for fields with base type :TextField. Base type must be one of: #{:BigIntegerField :DecimalField :IntegerField :FloatField}"
nil]
- (tu/with-temp* [Database [{database-id :id}]
- Table [{table-id :id} {:db_id database-id}]
- Field [{field-id :id} {:table_id table-id}]]
+ (tu/with-temp* [Field [{field-id :id} {:base_type :TextField, :special_type nil}]]
[((user->client :crowberto) :put 400 (str "field/" field-id) {:special_type :timestamp_seconds})
(db/select-one-field :special_type Field, :id field-id)]))
+;; check that you *can* set it if it *is* the proper base type
+(expect
+ :timestamp_seconds
+ (tu/with-temp* [Field [{field-id :id} {:base_type :IntegerField}]]
+ ((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/models/field_test.clj b/test/metabase/models/field_test.clj
index 8f3b702d3236c3b7163087e33ee38f7006f392a8..045e556aaed3d7f26e78f05836730d98c5a1fa1f 100644
--- a/test/metabase/models/field_test.clj
+++ b/test/metabase/models/field_test.clj
@@ -5,18 +5,14 @@
[metabase.test.data :refer :all]))
-;; valid-metadata?
-(expect false (valid-metadata? nil nil nil nil))
-(expect false (valid-metadata? :IntegerField nil nil nil))
-(expect false (valid-metadata? :IntegerField :metric nil nil))
-(expect true (valid-metadata? :IntegerField :metric nil :normal))
-(expect false (valid-metadata? :foo :metric nil :normal))
-(expect false (valid-metadata? :IntegerField :foo nil :normal))
-(expect false (valid-metadata? :IntegerField :metric nil :foo))
-(expect true (valid-metadata? :IntegerField :metric :timestamp_seconds :normal))
-(expect true (valid-metadata? :IntegerField :metric :timestamp_milliseconds :normal))
-(expect false (valid-metadata? :DateTimeField :metric :timestamp_seconds :normal))
-(expect false (valid-metadata? :DateTimeField :metric :timestamp_milliseconds :normal))
+;; valid-special-type-for-base-type?
+(expect true (valid-special-type-for-base-type? nil nil))
+(expect true (valid-special-type-for-base-type? nil :foo))
+(expect true (valid-special-type-for-base-type? nil :IntegerField))
+(expect true (valid-special-type-for-base-type? :timestamp_seconds :IntegerField))
+(expect true (valid-special-type-for-base-type? :timestamp_milliseconds :IntegerField))
+(expect false (valid-special-type-for-base-type? :timestamp_seconds :DateTimeField))
+(expect false (valid-special-type-for-base-type? :timestamp_milliseconds :DateTimeField))
;; field-should-have-field-values?