Merge pull request #3285 from metabase/fix-modifying-fields-with-sensitive-field-type

Fix modifying fields with legacy "sensitive" field_type 

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)."

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,45 @@
          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))
+    (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"

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)

test/metabase/api/field_test.clj

 (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`."

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?