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