From 11a1ac2a09f31a52dff85e61898c8c667a0db937 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Wed, 11 Mar 2026 13:16:57 +0000 Subject: [PATCH] :bug: Fix RangeError (stack overflow) in find-component-main Refactor find-component-main to use an iterative loop/recur pattern instead of direct recursion and added cycle detection for malformed data structures. Signed-off-by: Andrey Antukh --- common/src/app/common/types/container.cljc | 46 ++++-- common/test/common_tests/runner.cljc | 2 + .../common_tests/types/container_test.cljc | 156 ++++++++++++++++++ 3 files changed, 188 insertions(+), 16 deletions(-) create mode 100644 common/test/common_tests/types/container_test.cljc diff --git a/common/src/app/common/types/container.cljc b/common/src/app/common/types/container.cljc index fa27ce5f07..324528854b 100644 --- a/common/src/app/common/types/container.cljc +++ b/common/src/app/common/types/container.cljc @@ -212,25 +212,39 @@ (get-instance-root objects (get objects (:parent-id shape))))) (defn find-component-main - "If the shape is a component main instance or is inside one, return that instance" + "If the shape is a component main instance or is inside one, return that instance. + Uses an iterative loop with cycle detection to prevent stack overflow on circular + parent references or malformed data structures." ([objects shape] (find-component-main objects shape true)) ([objects shape only-direct-child?] - (cond - (or (nil? shape) (cfh/root? shape)) - nil - (nil? (:parent-id shape)) ; This occurs in the root of components v1 - shape - (ctk/main-instance? shape) - shape - (and only-direct-child? ;; If we are asking only for direct childs of a component-main, - (ctk/instance-head? shape)) ;; stop when we found a instance-head that isn't main-instance - nil - (and (not only-direct-child?) - (ctk/instance-root? shape)) - nil - :else - (find-component-main objects (get objects (:parent-id shape)))))) + (loop [shape shape + visited #{}] + (cond + (or (nil? shape) (cfh/root? shape)) + nil + + ;; Cycle detected: we have already visited this shape id + (contains? visited (:id shape)) + nil + + (nil? (:parent-id shape)) ; This occurs in the root of components v1 + shape + + (ctk/main-instance? shape) + shape + + (and only-direct-child? ;; If we are asking only for direct childs of a component-main, + (ctk/instance-head? shape)) ;; stop when we found a instance-head that isn't main-instance + nil + + (and (not only-direct-child?) + (ctk/instance-root? shape)) + nil + + :else + (recur (get objects (:parent-id shape)) + (conj visited (:id shape))))))) (defn inside-component-main? "Check if the shape is a component main instance or is inside one." diff --git a/common/test/common_tests/runner.cljc b/common/test/common_tests/runner.cljc index 1fa9d63c14..58419240f3 100644 --- a/common/test/common_tests/runner.cljc +++ b/common/test/common_tests/runner.cljc @@ -39,6 +39,7 @@ [common-tests.time-test] [common-tests.types.absorb-assets-test] [common-tests.types.components-test] + [common-tests.types.container-test] [common-tests.types.fill-test] [common-tests.types.modifiers-test] [common-tests.types.objects-map-test] @@ -92,6 +93,7 @@ 'common-tests.time-test 'common-tests.types.absorb-assets-test 'common-tests.types.components-test + 'common-tests.types.container-test 'common-tests.types.fill-test 'common-tests.types.modifiers-test 'common-tests.types.objects-map-test diff --git a/common/test/common_tests/types/container_test.cljc b/common/test/common_tests/types/container_test.cljc new file mode 100644 index 0000000000..e4498db9b7 --- /dev/null +++ b/common/test/common_tests/types/container_test.cljc @@ -0,0 +1,156 @@ +;; This Source Code Form is subject to the terms of the Mozilla Public +;; License, v. 2.0. If a copy of the MPL was not distributed with this +;; file, You can obtain one at http://mozilla.org/MPL/2.0/. +;; +;; Copyright (c) KALEIDOS INC + +(ns common-tests.types.container-test + (:require + [app.common.types.container :as ctc] + [app.common.types.shape :as cts] + [app.common.uuid :as uuid] + [clojure.test :as t])) + +;; --------------------------------------------------------------------------- +;; Helpers +;; --------------------------------------------------------------------------- + +(defn- make-shape + "Build a realistic shape using setup-shape, so it has proper geometric + data (selrect, points, transform, …) and follows project data standards." + [id & {:as attrs}] + (cts/setup-shape (merge {:type :rect + :x 0 + :y 0 + :width 100 + :height 100} + attrs + {:id id}))) + +(defn- objects-map + "Build an objects map from a seq of shapes." + [& shapes] + (into {} (map (juxt :id identity) shapes))) + +;; The sentinel root shape (uuid/zero) recognised by cfh/root? +(def root-id uuid/zero) + +(defn- root-shape + "Create the page-root frame shape (id = uuid/zero, type :frame)." + [] + (cts/setup-shape {:id root-id + :type :frame + :x 0 + :y 0 + :width 100 + :height 100})) + +;; --------------------------------------------------------------------------- +;; Tests – base cases +;; --------------------------------------------------------------------------- + +(t/deftest find-component-main-nil-shape + (t/testing "returns nil when shape is nil" + (t/is (nil? (ctc/find-component-main {} nil))))) + +(t/deftest find-component-main-root-shape + (t/testing "returns nil when shape is the page root (uuid/zero)" + (let [root (root-shape) + objects (objects-map root)] + (t/is (nil? (ctc/find-component-main objects root)))))) + +(t/deftest find-component-main-no-parent-id + (t/testing "returns the shape itself when parent-id is nil (v1 component root)" + (let [id (uuid/next) + ;; Simulate a v1 component root: setup-shape produces a full shape, + ;; then we explicitly clear :parent-id to nil, which is how legacy + ;; component roots appear in deserialized data. + shape (assoc (make-shape id) :parent-id nil) + objects (objects-map shape)] + (t/is (= shape (ctc/find-component-main objects shape)))))) + +(t/deftest find-component-main-main-instance + (t/testing "returns the shape when it is a main-instance" + (let [parent-id (uuid/next) + id (uuid/next) + parent (make-shape parent-id) + shape (make-shape id :parent-id parent-id :main-instance true) + objects (objects-map parent shape)] + (t/is (= shape (ctc/find-component-main objects shape)))))) + +(t/deftest find-component-main-instance-head-stops-when-only-direct-child + (t/testing "returns nil when hitting an instance-head that is not main (only-direct-child? true)" + (let [parent-id (uuid/next) + id (uuid/next) + ;; instance-head? ← has :component-id but NOT :main-instance + shape (make-shape id + :parent-id parent-id + :component-id (uuid/next)) + parent (make-shape parent-id) + objects (objects-map parent shape)] + (t/is (nil? (ctc/find-component-main objects shape true)))))) + +(t/deftest find-component-main-instance-root-stops-when-not-only-direct-child + (t/testing "returns nil when hitting an instance-root and only-direct-child? is false" + (let [parent-id (uuid/next) + id (uuid/next) + ;; instance-root? ← has :component-root true + shape (make-shape id + :parent-id parent-id + :component-id (uuid/next) + :component-root true) + parent (make-shape parent-id) + objects (objects-map parent shape)] + (t/is (nil? (ctc/find-component-main objects shape false)))))) + +(t/deftest find-component-main-walks-to-main-ancestor + (t/testing "traverses ancestors and returns the first main-instance found" + (let [gp-id (uuid/next) + p-id (uuid/next) + child-id (uuid/next) + grandparent (make-shape gp-id :parent-id nil :main-instance true) + parent (make-shape p-id :parent-id gp-id) + child (make-shape child-id :parent-id p-id) + objects (objects-map grandparent parent child)] + (t/is (= grandparent (ctc/find-component-main objects child)))))) + +;; --------------------------------------------------------------------------- +;; Tests – cycle detection (the bug fix) +;; --------------------------------------------------------------------------- + +(t/deftest find-component-main-direct-self-loop + (t/testing "returns nil (no crash) when a shape's parent-id points to itself" + (let [id (uuid/next) + ;; deliberately malformed: parent-id == id (self-loop) + shape (make-shape id :parent-id id) + objects (objects-map shape)] + (t/is (nil? (ctc/find-component-main objects shape)))))) + +(t/deftest find-component-main-two-node-cycle + (t/testing "returns nil (no crash) for a two-node circular reference A→B→A" + (let [id-a (uuid/next) + id-b (uuid/next) + shape-a (make-shape id-a :parent-id id-b) + shape-b (make-shape id-b :parent-id id-a) + objects (objects-map shape-a shape-b)] + (t/is (nil? (ctc/find-component-main objects shape-a)))))) + +(t/deftest find-component-main-multi-node-cycle + (t/testing "returns nil (no crash) for a longer cycle A→B→C→A" + (let [id-a (uuid/next) + id-b (uuid/next) + id-c (uuid/next) + shape-a (make-shape id-a :parent-id id-b) + shape-b (make-shape id-b :parent-id id-c) + shape-c (make-shape id-c :parent-id id-a) + objects (objects-map shape-a shape-b shape-c)] + (t/is (nil? (ctc/find-component-main objects shape-a)))))) + +(t/deftest find-component-main-only-direct-child-with-cycle + (t/testing "cycle detection works correctly with only-direct-child? false as well" + (let [id-a (uuid/next) + id-b (uuid/next) + shape-a (make-shape id-a :parent-id id-b) + shape-b (make-shape id-b :parent-id id-a) + objects (objects-map shape-a shape-b)] + (t/is (nil? (ctc/find-component-main objects shape-a false))))))