apply a check for `valid-metadata?` when updating the attributes of a Field so...

apply a check for `valid-metadata?` when updating the attributes of a Field so that we can prevent invalid metadata configurations (#824)

src/metabase/api/field.clj

@@ -39,14 +39,18 @@
   {field_type   FieldType
    special_type FieldSpecialType
    display_name NonEmptyString}
-  (write-check Field id)
-  ;; update the Field.  start with keys that may be set to NULL then conditionally add other keys if they have values
-  (check-500 (m/mapply upd Field id (merge {:description  description
-                                            :special_type special_type}
-                                           (when display_name               {:display_name display_name})
-                                           (when field_type                 {:field_type field_type})
-                                           (when-not (nil? preview_display) {:preview_display preview_display}))))
-  (Field id))
+  (let-404 [field (Field id)]
+    (write-check field)
+    (let [field_type   (or field_type (:field_type field))
+          special_type (or special_type (:special_type field))]
+      (check-400 (field/valid-metadata? (:base_type field) field_type special_type))
+      ;; update the Field.  start with keys that may be set to NULL then conditionally add other keys if they have values
+      (check-500 (m/mapply upd Field id (merge {:description  description
+                                                :field_type   field_type
+                                                :special_type special_type}
+                                               (when display_name               {:display_name display_name})
+                                               (when-not (nil? preview_display) {:preview_display preview_display}))))
+      (Field id))))
 
 (defendpoint GET "/:id/summary"
   "Get the count and distinct count of `Field` with ID."

test/metabase/api/field_test.clj

 (ns metabase.api.field-test
   (:require [expectations :refer :all]
-            [metabase.db :refer :all]
+            [metabase.db :as db]
+            [metabase.models.database :refer [Database]]
             (metabase.models [field :refer [Field]]
                              [field-values :refer [FieldValues]]
                              [table :refer [Table]])
             [metabase.test.data :refer :all]
             [metabase.test.data.users :refer :all]
-            [metabase.test.util :refer [match-$ expect-eval-actual-first]]))
+            [metabase.test.util :as tu]))
 
 ;; Helper Fns
 
 (defn- db-details []
-  (match-$ (db)
+  (tu/match-$ (db)
     {:created_at      $
      :engine          "h2"
      :id              $
@@ -26,10 +27,10 @@
 
 ;; ## GET /api/field/:id
 (expect
-    (match-$ (Field (id :users :name))
+    (tu/match-$ (Field (id :users :name))
       {:description     nil
        :table_id        (id :users)
-       :table           (match-$ (Table (id :users))
+       :table           (tu/match-$ (Table (id :users))
                           {:description     nil
                            :entity_type     nil
                            :visibility_type nil
@@ -68,10 +69,10 @@
 ;; ## PUT /api/field/:id
 ;; Check that we can update a Field
 ;; TODO - this should NOT be modifying a field from our test data, we should create new data to mess with
-(expect-eval-actual-first
-    (match-$ (let [field (Field (id :venues :latitude))]
+(tu/expect-eval-actual-first
+    (tu/match-$ (let [field (Field (id :venues :latitude))]
                ;; this is sketchy. But return the Field back to its unmodified state so it won't affect other unit tests
-               (upd Field (id :venues :latitude) :special_type "latitude")
+               (db/upd Field (id :venues :latitude) :special_type "latitude")
                ;; match against the modified Field
                field)
              {:description     nil
@@ -90,22 +91,43 @@
               :parent_id       nil})
   ((user->client :crowberto) :put 200 (format "field/%d" (id :venues :latitude)) {:special_type :fk}))
 
+(expect
+  ["Invalid Request."
+   nil]
+  (tu/with-temp Database [{database-id :id} {:name      "Field Test"
+                                             :engine    :yeehaw
+                                             :details   {}
+                                             :is_sample false}]
+    (tu/with-temp Table [{table-id :id} {:name   "Field Test"
+                                         :db_id  database-id
+                                         :active true}]
+      (tu/with-temp Field [{field-id :id} {:table_id    table-id
+                                           :name        "Field Test"
+                                           :base_type   :TextField
+                                           :field_type  :info
+                                           :active      true
+                                           :preview_display true
+                                           :position    1}]
+        [((user->client :crowberto) :put 400 (format "field/%d" field-id) {:special_type :timestamp_seconds})
+         (db/sel :one :field [Field :special_type] :id field-id)]))))
+
+
 (defn- field->field-values
   "Fetch the `FieldValues` object that corresponds to a given `Field`."
   [table-kw field-kw]
-  (sel :one FieldValues :field_id (id table-kw field-kw)))
+  (db/sel :one FieldValues :field_id (id table-kw field-kw)))
 
 ;; ## GET /api/field/:id/values
 ;; Should return something useful for a field that has special_type :category
-(expect-eval-actual-first
-    (match-$ (field->field-values :venues :price)
+(tu/expect-eval-actual-first
+    (tu/match-$ (field->field-values :venues :price)
       {:field_id              (id :venues :price)
        :human_readable_values {}
        :values                [1 2 3 4]
        :updated_at            $
        :created_at            $
        :id                    $})
-  (do (upd FieldValues (:id (field->field-values :venues :price)) :human_readable_values nil) ; clear out existing human_readable_values in case they're set
+  (do (db/upd FieldValues (:id (field->field-values :venues :price)) :human_readable_values nil) ; clear out existing human_readable_values in case they're set
       ((user->client :rasta) :get 200 (format "field/%d/values" (id :venues :price)))))
 
 ;; Should return nothing for a field whose special_type is *not* :category
@@ -118,9 +140,9 @@
 ;; ## POST /api/field/:id/value_map_update
 
 ;; Check that we can set values
-(expect-eval-actual-first
+(tu/expect-eval-actual-first
     [{:status "success"}
-     (match-$ (sel :one FieldValues :field_id (id :venues :price))
+     (tu/match-$ (db/sel :one FieldValues :field_id (id :venues :price))
        {:field_id              (id :venues :price)
         :human_readable_values {:1 "$"
                                 :2 "$$"
@@ -137,16 +159,16 @@
    ((user->client :rasta) :get 200 (format "field/%d/values" (id :venues :price)))])
 
 ;; Check that we can unset values
-(expect-eval-actual-first
+(tu/expect-eval-actual-first
     [{:status "success"}
-     (match-$ (sel :one FieldValues :field_id (id :venues :price))
+     (tu/match-$ (db/sel :one FieldValues :field_id (id :venues :price))
        {:field_id              (id :venues :price)
         :human_readable_values {}
         :values                [1 2 3 4]
         :updated_at            $
         :created_at            $
         :id                    $})]
-  [(do (upd FieldValues (:id (field->field-values :venues :price)) :human_readable_values {:1 "$" ; make sure they're set
+  [(do (db/upd FieldValues (:id (field->field-values :venues :price)) :human_readable_values {:1 "$" ; make sure they're set
                                                                                            :2 "$$"
                                                                                            :3 "$$$"
                                                                                            :4 "$$$$"})