From d4cb68bf523c1e3f5beb928af372687b5bda6dce Mon Sep 17 00:00:00 2001 From: Eva Marco Date: Fri, 23 Oct 2026 11:29:27 +0200 Subject: [PATCH] :bug: Coerce child ids in :shapes instead of skipping them Skipping invalid child ids in migration 0033 was not enough: the ids stayed in :shapes, so the file then failed schema validation and still could not load. Migration 0033 now first parses string ids in :shapes into uuids and drops nil and ids that point to no shape. This also removes the duplicate that 0032 adds when a child is listed as a string. Files that already passed 0033 get it again: it is removed from their applied migrations before deploy. Relates to #12343 AI-assisted-by: claude-opus-5-5 --- common/src/app/common/files/migrations.cljc | 41 ++++++++++----- .../common_tests/files_migrations_test.cljc | 51 ++++++++++++------- 2 files changed, 61 insertions(+), 31 deletions(-) diff --git a/common/src/app/common/files/migrations.cljc b/common/src/app/common/files/migrations.cljc index eee303fbe4..21dbf6f0d2 100644 --- a/common/src/app/common/files/migrations.cljc +++ b/common/src/app/common/files/migrations.cljc @@ -2245,16 +2245,32 @@ ;; Here we normalize the names of all main instances, and a new file integrity validation has ;; been added to enforce this from now on. ;; Main instances in variants are a special case because they need to match the variant name. - ;; Old files may list in :shapes ids that are not uuids or that point - ;; to no shape; skip them. - (letfn [(update-shape [container shape-id] - (if-let [shape (when (uuid? shape-id) - (ctn/get-shape container shape-id))] - (update-existing-shape container shape) - container)) + ;; Old files may also list in :shapes child ids stored as strings, nil, or ids that point to + ;; no shape. They fail the shape schema and the uuid assert of get-shape, so first parse the + ;; strings into uuids and drop the rest. This also removes the duplicate that 0032 adds when + ;; a child is listed as a string. + (letfn [(fix-children [objects shapes] + (into [] + (comp (keep uuid/coerce) + (filter #(contains? objects %)) + (distinct)) + shapes)) - (update-existing-shape [container shape] - (let [shape (cond + (fix-object [objects shape] + (if-let [shapes (:shapes shape)] + (let [shapes' (fix-children objects shapes)] + (if (= shapes shapes') + shape + (assoc shape :shapes shapes'))) + shape)) + + (fix-objects [objects] + (d/update-vals objects (partial fix-object objects))) + + (update-shape [container shape-id] + (let [shape (ctn/get-shape container shape-id) + + shape (cond ;; Variant container must have a normalized name (ctk/is-variant-container? shape) (let [name (:name shape) @@ -2269,9 +2285,7 @@ ;; Main instance inside a variant must have the same name as the container (ctk/main-instance? shape) (if (ctk/is-variant? shape) - (let [parent-id (:parent-id shape) - variant-container (when (uuid? parent-id) - (ctn/get-shape container parent-id))] + (let [variant-container (ctn/get-shape container (:parent-id shape))] (if (or (= (:name shape) (:name variant-container)) (nil? variant-container)) shape @@ -2295,7 +2309,8 @@ (update-container [container] - (let [root (ctn/get-container-root container)] + (let [container (d/update-when container :objects fix-objects) + root (ctn/get-container-root container)] (if root (update-shape container (:id root)) container)))] diff --git a/common/test/common_tests/files_migrations_test.cljc b/common/test/common_tests/files_migrations_test.cljc index ac087afa4d..10e2000253 100644 --- a/common/test/common_tests/files_migrations_test.cljc +++ b/common/test/common_tests/files_migrations_test.cljc @@ -809,26 +809,41 @@ (t/is (= "Component" (:name component2'))) (t/is (= "Group / Subgroup/Component" (:name copy2'))))) -(t/deftest migration-0033-skips-invalid-children-ids - ;; Old files may list in :shapes ids that are not uuids or that point - ;; to no shape; the migration must skip them and keep going. +(t/deftest migration-0033-fixes-children-ids + ;; Old files may list in :shapes child ids stored as strings, nil or + ;; ids that point to no shape; strings become uuids, the rest goes. (let [migration-id "0033-normalize-main-instance-names" - missing-id (uuid/next) file (-> (thf/sample-file :file1) - (tho/add-simple-component :component1 :root1 :child1 - :component-params {:path "Group / Subgroup" - :name "Name"} - :root-params {:name "Group/Subgroup / Name"})) + (tho/add-frame-with-child :frame1 :child1) + (tho/add-simple-component :component1 :root1 :child2)) child1-id (thi/id :child1) - file (ths/update-shape file :root1 :shapes [nil "not-a-uuid" missing-id child1-id]) + child2-id (thi/id :child2) + file (-> file + (ths/update-shape :frame1 :shapes + [nil "not-a-uuid" (str child1-id) + (uuid/next) child1-id]) + (ths/update-shape :root1 :shapes [(str child2-id)])) + data' (cfm/migrate-data (ctf/file-data file) migration-id) + file' (assoc file :data data')] - data (ctf/file-data file) - data' (cfm/migrate-data data migration-id) - file' (assoc file :data data') - page-id (first (:pages data')) - objects' (get-in data' [:pages-index page-id :objects]) - root1' (ths/get-shape file' :root1)] + (t/is (= [child1-id] (:shapes (ths/get-shape file' :frame1)))) + (t/is (= [child2-id] (:shapes (ths/get-shape file' :root1)))) + (t/is (= data' (cfm/migrate-data data' migration-id)) + "migration is idempotent"))) - (t/is (= "Group / Subgroup / Name" (:name root1'))) - (t/is (not (contains? objects' nil)) "no shape stored under a nil id") - (t/is (not (contains? objects' missing-id)) "no shape created for a missing id"))) +(t/deftest migration-0033-fixes-children-ids-through-file-migration + ;; 0032 adds the uuid of a child listed as a string, so the child is + ;; listed twice before 0033; the file must still pass validation. + (let [file (-> (thf/sample-file :file1) + (tho/add-frame-with-child :frame1 :child1)) + child1-id (thi/id :child1) + file (-> file + (ths/update-shape :frame1 :shapes [(str child1-id)]) + (assoc :migrations + (disj cfm/available-migrations + "0032-fix-children-not-in-parent" + "0033-normalize-main-instance-names"))) + file' (cfm/migrate-file file {})] + + (t/is (not (cfm/need-migration? file')) "new migrations recorded") + (t/is (= [child1-id] (:shapes (ths/get-shape file' :frame1))))))