From 317a731ddadf209b69e256be458bf63a9661500a Mon Sep 17 00:00:00 2001 From: Eva Marco Date: Tue, 13 Oct 2026 11:17:01 +0200 Subject: [PATCH] :bug: Detect and repair children missing from their parent's list (#12216) * :bug: Detect and repair children missing from their parent's list A shape could point to an existing parent that did not list it in its :shapes. The validator missed it, since it only walks the tree from the root plus the shapes whose parent does not exist. Operations that walk :shapes missed it too: deleting a variant also deleted its container, which looked empty, and left the other variants without a parent, so the backend rejected the change and the file stopped saving. - get-orphan-shapes also returns shapes not listed by their parent, so :child-not-in-parent is reported. - The :child-not-in-parent repair adds every unlisted child of the parent. All repairs share the same file data and each one sets the whole :shapes, so with several children only the last one was kept. - New migration 0032-fix-children-not-in-parent repairs the affected files when they are opened. Closes #11758 AI-assisted-by: claude-opus-5-5 * :bug: List shapes moved to root by the parent-not-found repair The :parent-not-found repair set the shape's parent to the root but did not add it to the root's :shapes, so it left the shape in the child-not-in-parent state that the validator now reports. It now adds every shape without parent to the root at once, since all repairs are built from the same file data and each one sets the whole :shapes. Also cover the missing parent case in the repair tests. AI-assisted-by: claude-opus-5-5 * :recycle: Simplify unlisted children validation and repair Address review comments on the validation and repair of shapes that their parent does not list. - get-orphan-shapes no longer rebuilds *children-sets*: both callers bind it. The lookup moves to a listed-in-parent? helper shared with check-parent-children, which scans :shapes when the var is unbound. - The :child-not-in-parent and :parent-not-found repairs no longer walk every shape of the page. Each one moves only its own shape with pcb/change-parent. The :mov-objects change adds the shape to the parent's :shapes when applied, so several repairs on one parent no longer overwrite each other. - The :child-not-in-parent repair test uses plain shapes instead of variants. AI-assisted-by: claude-opus-5-5 --- common/src/app/common/files/migrations.cljc | 41 ++++++++++++++- common/src/app/common/files/repair.cljc | 36 ++++++------- common/src/app/common/files/validate.cljc | 32 ++++++++---- .../test/common_tests/files/repair_test.cljc | 47 +++++++++++++++++ .../common_tests/files_migrations_test.cljc | 51 +++++++++++++++++++ 5 files changed, 175 insertions(+), 32 deletions(-) diff --git a/common/src/app/common/files/migrations.cljc b/common/src/app/common/files/migrations.cljc index c0e97bc3d7..dbe0b0abd7 100644 --- a/common/src/app/common/files/migrations.cljc +++ b/common/src/app/common/files/migrations.cljc @@ -2179,6 +2179,44 @@ (update :pages-index d/update-vals repair-container) (d/update-when :components d/update-vals repair-container)))) +;; A shape can point to an existing parent that does not list it in its +;; `:shapes`. Such shapes are unreachable from the root, and operations +;; that walk the tree through `:shapes` miss them; e.g. deleting a variant +;; also deleted its container, which looked empty, and left the other +;; variants pointing to a deleted parent. Add them back to their parent. +(defmethod migrate-data "0032-fix-children-not-in-parent" + [data _] + (letfn [(get-unlisted-children [objects] + ;; Returns a {parent-id -> [child-id]} map + (let [listed (reduce-kv (fn [result id shape] + (if-let [shapes (not-empty (:shapes shape))] + (assoc result id (set shapes)) + result)) + {} + objects)] + (reduce-kv (fn [result id shape] + (let [parent-id (:parent-id shape)] + (if (and (not (cfh/root? shape)) + (contains? objects parent-id) + (not (contains? (get listed parent-id) id))) + (update result parent-id (fnil conj []) id) + result))) + {} + objects))) + + (fix-objects [objects] + (reduce-kv (fn [objects parent-id child-ids] + (update-in objects [parent-id :shapes] (fnil into []) child-ids)) + objects + (get-unlisted-children objects))) + + (update-container [container] + (d/update-when container :objects fix-objects))] + + (-> data + (update :pages-index d/update-vals update-container) + (d/update-when :components d/update-vals update-container)))) + (def available-migrations (into (d/ordered-set) ["legacy-2" @@ -2267,4 +2305,5 @@ "0028-normalize-constrained-values" "0029-move-background-blur-out-of-blur" "0030-remove-stroke-per-side-attr" - "0031-migrate-stroke-width-token-attr"])) + "0031-migrate-stroke-width-token-attr" + "0032-fix-children-not-in-parent"])) diff --git a/common/src/app/common/files/repair.cljc b/common/src/app/common/files/repair.cljc index 9fb37c5e37..a8d98a2d87 100644 --- a/common/src/app/common/files/repair.cljc +++ b/common/src/app/common/files/repair.cljc @@ -45,29 +45,25 @@ (defmethod repair-error :parent-not-found [_ {:keys [shape page-id] :as error} file-data _] - (let [repair-shape - (fn [shape] - ;; Set parent to root frame. - (log/debug :hint " -> set to " :parent-id uuid/zero) - (assoc shape :parent-id uuid/zero))] - - (log/debug :hint "repairing shape :parent-not-found" :id (:id shape) :name (:name shape) :page-id page-id) - (-> (pcb/empty-changes nil page-id) - (pcb/with-file-data file-data) - (pcb/update-shapes [(:id shape)] repair-shape)))) + ;; Move the shape to the root frame. `:mov-objects` adds it to the + ;; root's children list when applied, so several repairs on the same + ;; root do not overwrite each other + (log/debug :hint "repairing shape :parent-not-found" :id (:id shape) :name (:name shape) :page-id page-id) + (log/debug :hint " -> set to " :parent-id uuid/zero) + (-> (pcb/empty-changes nil page-id) + (pcb/with-file-data file-data) + (pcb/change-parent uuid/zero [shape] nil {:allow-altering-copies true}))) (defmethod repair-error :child-not-in-parent [_ {:keys [shape page-id] :as error} file-data _] - (let [repair-shape - (fn [parent-shape] - ;; Add shape to parent's children list - (log/debug :hint " -> add children to" :parent-id (:id parent-shape)) - (update parent-shape :shapes conj (:id shape)))] - - (log/debug :hint "repairing shape :child-not-in-parent" :id (:id shape) :name (:name shape) :page-id page-id) - (-> (pcb/empty-changes nil page-id) - (pcb/with-file-data file-data) - (pcb/update-shapes [(:parent-id shape)] repair-shape)))) + ;; Add the shape to its parent's children list. `:mov-objects` adds it + ;; when applied, so several repairs on the same parent do not + ;; overwrite each other + (log/debug :hint "repairing shape :child-not-in-parent" :id (:id shape) :name (:name shape) :page-id page-id) + (log/debug :hint " -> add children to" :parent-id (:parent-id shape)) + (-> (pcb/empty-changes nil page-id) + (pcb/with-file-data file-data) + (pcb/change-parent (:parent-id shape) [shape] nil {:allow-altering-copies true}))) (defmethod repair-error :duplicated-children [_ {:keys [shape page-id] :as error} file-data _] diff --git a/common/src/app/common/files/validate.cljc b/common/src/app/common/files/validate.cljc index 280f468d10..72a1e112ff 100644 --- a/common/src/app/common/files/validate.cljc +++ b/common/src/app/common/files/validate.cljc @@ -118,6 +118,19 @@ {} objects)) +(defn- listed-in-parent? + "Check if `parent` lists `shape-id` in its `:shapes`. A `nil` parent + lists nothing. + + Fast path: O(1) set lookup when `*children-sets*` is pre-computed for + the current page. Falls back to an O(k) linear scan via `some` when + the var is unbound (e.g. in isolated single-shape validation calls + outside a full page pass)." + [parent shape-id] + (if-let [cs *children-sets*] + (contains? (get cs (:id parent)) shape-id) + (some #(= shape-id %) (:shapes parent)))) + (defn- find-ref-shape* "Cached wrapper around `ctf/find-ref-shape` with `:include-deleted? true`. When `*ref-shape-cache*` is bound, each shape-id is resolved at most once @@ -195,13 +208,7 @@ shape file page) (do (when-not (cfh/root? shape) - ;; Fast path: O(1) set lookup when `*children-sets*` is pre-computed - ;; for the current page. Falls back to an O(k) linear scan via - ;; `some` when the var is unbound (e.g. in isolated single-shape - ;; validation calls outside a full page pass). - (when-not (if-let [cs *children-sets*] - (contains? (get cs (:id parent)) shape-id) - (some #(= shape-id %) (:shapes parent))) + (when-not (listed-in-parent? parent shape-id) (report-error :child-not-in-parent (str/ffmt "Shape % not in parent's children list" shape-id) shape file page))) @@ -797,9 +804,10 @@ (check-variant-component component file))) (defn- get-orphan-shapes - "Return the ids of shapes whose parent does not exist in the objects - map (i.e. shapes unreachable from the root traversal). The root - shape itself is excluded since it is always validated separately. + "Return the ids of shapes unreachable from the root traversal: shapes + whose parent does not exist in the objects map, or whose parent exists + but does not list them in its `:shapes`. The root shape itself is + excluded since it is always validated separately. Implemented with `reduce-kv` rather than a `map`/`filter` pipeline. The previous implementation mapped over `objects` and returned a @@ -810,8 +818,10 @@ [{:keys [objects] :as _page}] (persistent! (reduce-kv (fn [result id shape] + ;; A missing parent is `nil`, which lists nothing, so this + ;; also covers shapes whose parent does not exist (if (and (not (cfh/root? shape)) - (not (contains? objects (:parent-id shape)))) + (not (listed-in-parent? (get objects (:parent-id shape)) id))) (conj! result id) result)) (transient []) diff --git a/common/test/common_tests/files/repair_test.cljc b/common/test/common_tests/files/repair_test.cljc index ad9532e497..d5aba47c98 100644 --- a/common/test/common_tests/files/repair_test.cljc +++ b/common/test/common_tests/files/repair_test.cljc @@ -228,3 +228,50 @@ (t/is (nil? errors')) (t/is (= "Group / Subgroup" (:path comp1'))) (t/is (= "Component" (:name comp1')))))) + +(t/deftest repair-child-not-in-parent + (t/testing "detect and repair shapes whose parent exists but does not list them" + (let [file (-> (thf/sample-file :file1 :page-label :page1) + (ths/add-sample-shape :frame1 :type :frame) + (ths/add-sample-shape :shape1 :parent-label :frame1) + (ths/add-sample-shape :shape2 :parent-label :frame1) + (ths/update-shape :frame1 :shapes [])) + + errors (cfv/validate-file file {}) + changes (cfr/repair-file file {} errors) + file' (thf/apply-changes file {:redo-changes changes} :validate? false) + errors' (cfv/validate-file file' {}) + + frame1' (ths/get-shape file' :frame1 :page-label :page1)] + + (t/is (= #{:child-not-in-parent} (into #{} (map :code) errors))) + (t/is (= #{(thi/id :shape1) (thi/id :shape2)} (into #{} (map :shape-id) errors))) + + (t/is (nil? errors')) + (t/is (= #{(thi/id :shape1) (thi/id :shape2)} (set (:shapes frame1'))))))) + +(t/deftest repair-parent-not-found + (t/testing "detect and repair shapes whose parent does not exist" + (let [file (-> (thf/sample-file :file1 :page-label :page1) + (ths/add-sample-shape :shape1) + (ths/add-sample-shape :shape2) + (ths/update-shape :shape1 :parent-id (uuid/next)) + (ths/update-shape :shape2 :parent-id (uuid/next)) + (ths/update-shape-by-id uuid/zero :shapes [])) + + errors (cfv/validate-file file {}) + changes (cfr/repair-file file {} errors) + file' (thf/apply-changes file {:redo-changes changes} :validate? false) + errors' (cfv/validate-file file' {}) + + shape1' (ths/get-shape file' :shape1 :page-label :page1) + shape2' (ths/get-shape file' :shape2 :page-label :page1) + root' (ths/get-shape-by-id file' uuid/zero :page-label :page1)] + + (t/is (= #{:parent-not-found} (into #{} (map :code) errors))) + (t/is (= #{(thi/id :shape1) (thi/id :shape2)} (into #{} (map :shape-id) errors))) + + (t/is (nil? errors')) + (t/is (= uuid/zero (:parent-id shape1'))) + (t/is (= uuid/zero (:parent-id shape2'))) + (t/is (= #{(thi/id :shape1) (thi/id :shape2)} (set (:shapes root'))))))) diff --git a/common/test/common_tests/files_migrations_test.cljc b/common/test/common_tests/files_migrations_test.cljc index 02c4a441da..0ac8084708 100644 --- a/common/test/common_tests/files_migrations_test.cljc +++ b/common/test/common_tests/files_migrations_test.cljc @@ -554,3 +554,54 @@ (t/is (contains? (:migrations file') migration-id) "migration id persisted") (t/is (= "30" (:stroke-width-top applied')) "legacy token applied to the sides"))) + +(t/deftest migration-0032-adds-unlisted-children-to-their-parent + (let [migration-id "0032-fix-children-not-in-parent" + page-id (uuid/next) + parent-id (uuid/next) + listed-id (uuid/next) + unlisted1-id (uuid/next) + unlisted2-id (uuid/next) + orphan-id (uuid/next) + root (cts/setup-shape {:id uuid/zero :type :frame :shapes [parent-id orphan-id]}) + parent (cts/setup-shape {:id parent-id :type :frame :parent-id uuid/zero + :shapes [listed-id]}) + child (fn [id parent-id] + (cts/setup-shape {:id id :type :rect :parent-id parent-id})) + data {:pages-index + {page-id + {:objects {uuid/zero root + parent-id parent + listed-id (child listed-id parent-id) + unlisted1-id (child unlisted1-id parent-id) + unlisted2-id (child unlisted2-id parent-id) + orphan-id (child orphan-id (uuid/next))}}}} + data' (cfm/migrate-data data migration-id) + objects' (get-in data' [:pages-index page-id :objects]) + shapes' (get-in objects' [parent-id :shapes])] + + (t/is (= listed-id (first shapes')) "already listed children keep their place") + (t/is (= #{listed-id unlisted1-id unlisted2-id} (set shapes')) + "unlisted children added to the parent") + (t/is (= 3 (count shapes')) "no duplicated children") + (t/is (= [parent-id orphan-id] (get-in objects' [uuid/zero :shapes])) + "root unchanged") + (t/is (= (get-in data [:pages-index page-id :objects orphan-id]) + (get objects' orphan-id)) + "shapes whose parent does not exist are left untouched") + (t/is (= data' (cfm/migrate-data data' migration-id)) + "migration is idempotent"))) + +(t/deftest migration-0032-adds-unlisted-children-in-components + (let [migration-id "0032-fix-children-not-in-parent" + component-id (uuid/next) + main-id (uuid/next) + child-id (uuid/next) + data {:components + {component-id + {:objects {main-id (cts/setup-shape {:id main-id :type :frame :shapes []}) + child-id (cts/setup-shape {:id child-id :type :rect + :parent-id main-id})}}}} + data' (cfm/migrate-data data migration-id)] + + (t/is (= [child-id] (get-in data' [:components component-id :objects main-id :shapes])))))