Skip to content
Snippets Groups Projects
Commit f47d2f63 authored by Ryan Senior's avatar Ryan Senior
Browse files

Fix bug with fk_target_field_id getting removed 

Specifically updating a field attribute that wasn't it's type or it's
fk_target_field_id would cause this issue
parent b77bc8d0
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
src/metabase/api/field.clj
+ 23
− 33
View file @ f47d2f63
......@@ -50,39 +50,29 @@
points_of_interest (s/maybe su/NonBlankString)
special_type (s/maybe FieldType)
visibility_type (s/maybe FieldVisibilityType)}
(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))))
(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
(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})))))
;; return updated field
(Field id)))
(api/defendpoint GET "/:id/summary"
"Get the count and distinct count of `Field` with ID."
......
test/metabase/api/field_test.clj
+ 78
− 0
View file @ f47d2f63
......@@ -395,3 +395,81 @@
after-change (simple-field-details (Field field-id-2))]
[(tu/boolean-ids-and-timestamps before-change)
(tu/boolean-ids-and-timestamps after-change)])))
;; Checking update of the fk_target_field_id
(expect
[{:name "Field Test 3",
:display_name "Field Test 3",
:description nil,
:visibility_type :normal,
:special_type :type/FK,
:fk_target_field_id true}
{:name "Field Test 3",
:display_name "Field Test 3",
:description nil,
:visibility_type :normal,
:special_type :type/FK,
:fk_target_field_id true}
true]
(tt/with-temp* [Field [{field-id-1 :id} {:name "Field Test 1"}]
Field [{field-id-2 :id} {:name "Field Test 2"}]
Field [{field-id-3 :id} {:name "Field Test 3"
:special_type :type/FK
:fk_target_field_id field-id-1}]]
(let [before-change (simple-field-details (Field field-id-3))
_ ((user->client :crowberto) :put 200 (format "field/%d" field-id-3) {:fk_target_field_id field-id-2})
after-change (simple-field-details (Field field-id-3))]
[(tu/boolean-ids-and-timestamps before-change)
(tu/boolean-ids-and-timestamps after-change)
(not= (:fk_target_field_id before-change)
(:fk_target_field_id after-change))])))
;; Checking update of the fk_target_field_id along with an FK change
(expect
[{:name "Field Test 2",
:display_name "Field Test 2",
:description nil,
:visibility_type :normal,
:special_type nil
:fk_target_field_id false}
{:name "Field Test 2",
:display_name "Field Test 2",
:description nil,
:visibility_type :normal,
:special_type :type/FK,
:fk_target_field_id true}]
(tt/with-temp* [Field [{field-id-1 :id} {:name "Field Test 1"}]
Field [{field-id-2 :id} {:name "Field Test 2"}]]
(let [before-change (simple-field-details (Field field-id-2))
_ ((user->client :crowberto) :put 200 (format "field/%d" field-id-2) {:special_type :type/FK
:fk_target_field_id field-id-1})
after-change (simple-field-details (Field field-id-2))]
[(tu/boolean-ids-and-timestamps before-change)
(tu/boolean-ids-and-timestamps after-change)])))
;; Checking update of the fk_target_field_id and FK remain unchanged on updates of other fields
(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 "foo",
:visibility_type :normal,
:special_type :type/FK,
:fk_target_field_id true}]
(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) {:description "foo"})
after-change (simple-field-details (Field field-id-2))]
[(tu/boolean-ids-and-timestamps before-change)
(tu/boolean-ids-and-timestamps after-change)])))
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment