github/penpot
1
0
Fork 0
mirror of https://github.com/penpot/penpot.git synced 2026-10-28 21:40:34 +00:00
Code Issues Packages Projects Releases Wiki Activity

🐛 Coerce child ids in :shapes instead of skipping them

Browse Source 
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
This commit is contained in:
Eva Marco 2026-10-23 11:29:27 +02:00
parent a4872a2589
commit d4cb68bf52
2 changed files with 61 additions and 31 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

41
common/src/app/common/files/migrations.cljc
View File

@ -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)))]

51
common/test/common_tests/files_migrations_test.cljc
View File

@ -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))))))