Remove external dimensions when FK special_type is removed

This checks field updates to sniff out when a field is going from an
FK field to something else. If it's no longer an FK field, any
external remapping should be removed as well.

src/metabase/api/field.clj

@@ -29,6 +29,16 @@
   (-> (api/read-check Field id)
       (hydrate [:table :db])))
 
+(defn- clear-dimension-on-fk-change! [{{dimension-id :id dimension-type :type} :dimensions :as field}]
+  (when (and dimension-id (= :external dimension-type))
+    (db/delete! Dimension :id dimension-id))
+  true)
+
+(defn- removed-fk-special-type? [old-special-type new-special-type]
+  (and (not= old-special-type new-special-type)
+       (isa? old-special-type :type/FK)
+       (or (nil? new-special-type)
+           (not (isa? new-special-type :type/FK)))))
 
 (api/defendpoint PUT "/:id"
   "Update `Field` with ID."
@@ -40,23 +50,39 @@
    points_of_interest (s/maybe su/NonBlankString)
    special_type       (s/maybe FieldType)
    visibility_type    (s/maybe FieldVisibilityType)}
-  (let [field              (api/write-check Field id)
-        special_type       (keyword (or special_type (:special_type field)))
-        ;; only let target field be set for :type/FK type fields, and if it's not in the payload then leave the current value
-        fk_target_field_id (when (isa? special_type :type/FK)
-                             (or fk_target_field_id (:fk_target_field_id field)))]
-    ;; 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
-      (api/checkp (db/exists? Field :id fk_target_field_id)
-        :fk_target_field_id "Invalid target field"))
-    ;; everything checks out, now update the field
-    (api/check-500 (db/update! Field id
-                     (u/select-keys-when (assoc body :fk_target_field_id fk_target_field_id)
-                       :present #{:caveats :description :fk_target_field_id :points_of_interest :special_type :visibility_type}
-                       :non-nil #{:display_name})))
-    ;; return updated field
-    (Field id)))
+  (try
+    (let [field              (hydrate (api/write-check Field id) :dimensions)
+          new-special-type   (get body :special_type (:special_type field))
+          removed-fk?        (removed-fk-special-type? (:special_type field) new-special-type)
+          fk-target-field-id (get body :fk_target_field_id (:fk_target_field_id field))]
+
+      ;; 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
+        (api/checkp (db/exists? Field :id fk_target_field_id)
+          :fk_target_field_id "Invalid target field"))
+      ;; everything checks out, now update the field
+      (api/check-500
+       (try
+         (db/transaction
+           (and
+            (if removed-fk?
+              (clear-dimension-on-fk-change! field)
+              true)
+            (db/update! Field id
+              (u/select-keys-when (assoc body :fk_target_field_id (when-not removed-fk? fk_target_field_id))
+                :present #{:caveats :description :fk_target_field_id :points_of_interest :special_type :visibility_type}
+                :non-nil #{:display_name}))))
+         (catch Exception e
+           (println "error " (.getMessage e))
+           (.printStackTrace e)
+           (throw e))))
+      ;; return updated field
+      (Field id))
+    (catch Exception e
+      (println "ERROR!" (.getMessage e))
+      (.printStackTrace e)
+      (throw e))))
 
 (api/defendpoint GET "/:id/summary"
   "Get the count and distinct count of `Field` with ID."

test/metabase/api/field_test.clj

@@ -88,25 +88,28 @@
 ;; ## PUT /api/field/:id
 
 (defn- simple-field-details [field]
-  (select-keys field [:name :display_name :description :visibility_type :special_type]))
+  (select-keys field [:name :display_name :description :visibility_type :special_type :fk_target_field_id]))
 
 ;; test that we can do basic field update work, including unsetting some fields such as special-type
 (expect
-  [{:name            "Field Test"
-    :display_name    "Field Test"
-    :description     nil
-    :special_type    nil
-    :visibility_type :normal}
-   {:name            "Field Test"
-    :display_name    "yay"
-    :description     "foobar"
-    :special_type    :type/Name
-    :visibility_type :sensitive}
-   {:name            "Field Test"
-    :display_name    "yay"
-    :description     nil
-    :special_type    nil
-    :visibility_type :sensitive}]
+  [{:name               "Field Test"
+    :display_name       "Field Test"
+    :description        nil
+    :special_type       nil
+    :visibility_type    :normal
+    :fk_target_field_id nil}
+   {:name               "Field Test"
+    :display_name       "yay"
+    :description        "foobar"
+    :special_type       :type/Name
+    :visibility_type    :sensitive
+    :fk_target_field_id nil}
+   {:name               "Field Test"
+    :display_name       "yay"
+    :description        nil
+    :special_type       nil
+    :visibility_type    :sensitive
+    :fk_target_field_id nil}]
   (tt/with-temp* [Field [{field-id :id} {:name "Field Test"}]]
     (let [original-val (simple-field-details (Field field-id))]
       ;; set it
@@ -306,6 +309,7 @@
       [before-creation
        (tu/boolean-ids-and-timestamps new-dim)])))
 
+;; Ensure we can delete a dimension
 (expect
   [{:id true
     :created_at true
@@ -323,3 +327,71 @@
       ((user->client :crowberto) :delete 204 (format "field/%d/dimension" field-id))
       [(tu/boolean-ids-and-timestamps new-dim)
        (dimension-for-field field-id)])))
+
+;; When an FK field gets it's special_type removed, we should clear the external dimension
+(expect
+  [{:id true
+    :created_at true
+    :updated_at true
+    :type :external
+    :name "fk-remove-dimension"
+    :human_readable_field_id true
+    :field_id true}
+   []]
+  (tt/with-temp* [Field [{field-id-1 :id} {:name "Field Test 1"
+                                           :special_type :type/FK}]
+                  Field [{field-id-2 :id} {:name "Field Test 2"}]]
+
+    (dimension-post field-id-1 {:name "fk-remove-dimension", :type "external" :human_readable_field_id field-id-2})
+
+    (let [new-dim          (dimension-for-field field-id-1)
+          _                ((user->client :crowberto) :put 200 (format "field/%d" field-id-1) {:special_type nil})
+          dim-after-update (dimension-for-field field-id-1)]
+      [(tu/boolean-ids-and-timestamps new-dim)
+       (tu/boolean-ids-and-timestamps dim-after-update)])))
+
+;; The dimension should stay as long as the FK didn't change
+(expect
+  (repeat 2 {:id true
+             :created_at true
+             :updated_at true
+             :type :external
+             :name "fk-remove-dimension"
+             :human_readable_field_id true
+             :field_id true})
+  (tt/with-temp* [Field [{field-id-1 :id} {:name "Field Test 1"
+                                           :special_type :type/FK}]
+                  Field [{field-id-2 :id} {:name "Field Test 2"}]]
+
+    (dimension-post field-id-1 {:name "fk-remove-dimension", :type "external" :human_readable_field_id field-id-2})
+
+    (let [new-dim          (dimension-for-field field-id-1)
+          _                ((user->client :crowberto) :put 200 (format "field/%d" field-id-1) {:description "something diffrent"})
+          dim-after-update (dimension-for-field field-id-1)]
+      [(tu/boolean-ids-and-timestamps new-dim)
+       (tu/boolean-ids-and-timestamps dim-after-update)])))
+
+;; When removing the FK special type, the fk_target_field_id should be cleared as well
+(expect
+  [{:name               "Field Test 2",
+    :display_name       "Field Test 2",
+    :description        nil,
+    :visibility_type    :normal,
+    :special_type       :type/FK,
+    :fk_target_field_id true}
+   {:name               "Field Test 2",
+    :display_name       "Field Test 2",
+    :description        nil,
+    :visibility_type    :normal,
+    :special_type       nil,
+    :fk_target_field_id false}]
+  (tt/with-temp* [Field [{field-id-1 :id} {:name "Field Test 1"}]
+                  Field [{field-id-2 :id} {:name               "Field Test 2"
+                                           :special_type       :type/FK
+                                           :fk_target_field_id field-id-1}]]
+
+    (let [before-change (simple-field-details (Field field-id-2))
+          _             ((user->client :crowberto) :put 200 (format "field/%d" field-id-2) {:special_type nil})
+          after-change  (simple-field-details (Field field-id-2))]
+      [(tu/boolean-ids-and-timestamps before-change)
+       (tu/boolean-ids-and-timestamps after-change)])))