From a4872a2589d728707eacb6c1b6c534bc9b1b9e4b Mon Sep 17 00:00:00 2001 From: Eva Marco Date: Thu, 22 Oct 2026 09:40:48 +0200 Subject: [PATCH] :bug: Fix shared files 500 on shapes with invalid child ids Old files can list in :shapes ids that are not uuids or that point to no shape. Migration 0033 walked them and hit the uuid assert in get-shape, which throws an AssertionError. The shared files list only caught Exception, so that Error broke the whole list with a 500. Make migration 0033 skip those ids, and catch Throwable when loading each shared file, so a broken file is skipped and logged. Closes #12343 AI-assisted-by: claude-opus-5-5 --- backend/src/app/rpc/commands/files.clj | 5 ++-- backend/test/backend_tests/rpc_file_test.clj | 28 +++++++++++-------- common/src/app/common/files/migrations.cljc | 14 ++++++++-- .../common_tests/files_migrations_test.cljc | 24 ++++++++++++++++ 4 files changed, 55 insertions(+), 16 deletions(-) diff --git a/backend/src/app/rpc/commands/files.clj b/backend/src/app/rpc/commands/files.clj index 5c17effec4..3f72eb3fdf 100644 --- a/backend/src/app/rpc/commands/files.clj +++ b/backend/src/app/rpc/commands/files.clj @@ -609,11 +609,12 @@ (fn [{:keys [id library-file-ids]}] ;; One shared file that fails to load (for example, a migration ;; error on old data) must not break the whole list; skip it - ;; and report it with its id. + ;; and report it with its id. Old data can also fail with a JVM + ;; Error (an assert or a deep recursion), so catch Throwable. (try (let [file (get-file-with-summary cfg id)] (assoc file :library-file-ids (db/decode-pgarray library-file-ids #{}))) - (catch Exception cause + (catch Throwable cause (l/error :hint "unable to load shared file" :file-id (str id) :team-id (str team-id) diff --git a/backend/test/backend_tests/rpc_file_test.clj b/backend/test/backend_tests/rpc_file_test.clj index c885219fc2..282fa801ba 100644 --- a/backend/test/backend_tests/rpc_file_test.clj +++ b/backend/test/backend_tests/rpc_file_test.clj @@ -2916,14 +2916,20 @@ file2 (th/create-file* 2 {:profile-id (:id profile) :project-id (:default-project-id profile) :is-shared true}) - get-file (deref #'files/get-file-with-summary) - broken (fn [cfg id] - (if (= id (:id file1)) - (throw (ex-info "broken file" {:type :assertion})) - (get-file cfg id))) - out (with-redefs-fn {#'files/get-file-with-summary broken} - #(th/command! {::th/type :get-team-shared-files - ::rpc/profile-id (:id profile) - :team-id (:default-team-id profile)}))] - (t/is (th/success? out)) - (t/is (= #{(:id file2)} (into #{} (map :id) (:result out)))))) + get-file (deref #'files/get-file-with-summary)] + + ;; Old file data can fail with a JVM Error (an assert or a deep + ;; recursion), not only with an exception. + (doseq [cause [(ex-info "broken file" {:type :assertion}) + (AssertionError. "point instance expected") + (StackOverflowError.)]] + (let [broken (fn [cfg id] + (if (= id (:id file1)) + (throw cause) + (get-file cfg id))) + out (with-redefs-fn {#'files/get-file-with-summary broken} + #(th/command! {::th/type :get-team-shared-files + ::rpc/profile-id (:id profile) + :team-id (:default-team-id profile)}))] + (t/is (th/success? out) (str "skips file failing with " (class cause))) + (t/is (= #{(:id file2)} (into #{} (map :id) (:result out)))))))) diff --git a/common/src/app/common/files/migrations.cljc b/common/src/app/common/files/migrations.cljc index de82f16f9e..eee303fbe4 100644 --- a/common/src/app/common/files/migrations.cljc +++ b/common/src/app/common/files/migrations.cljc @@ -2245,10 +2245,16 @@ ;; 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] - (let [shape (ctn/get-shape container shape-id) + (if-let [shape (when (uuid? shape-id) + (ctn/get-shape container shape-id))] + (update-existing-shape container shape) + container)) - shape (cond + (update-existing-shape [container shape] + (let [shape (cond ;; Variant container must have a normalized name (ctk/is-variant-container? shape) (let [name (:name shape) @@ -2263,7 +2269,9 @@ ;; Main instance inside a variant must have the same name as the container (ctk/main-instance? shape) (if (ctk/is-variant? shape) - (let [variant-container (ctn/get-shape container (:parent-id shape))] + (let [parent-id (:parent-id shape) + variant-container (when (uuid? parent-id) + (ctn/get-shape container parent-id))] (if (or (= (:name shape) (:name variant-container)) (nil? variant-container)) shape diff --git a/common/test/common_tests/files_migrations_test.cljc b/common/test/common_tests/files_migrations_test.cljc index cb27492b6a..ac087afa4d 100644 --- a/common/test/common_tests/files_migrations_test.cljc +++ b/common/test/common_tests/files_migrations_test.cljc @@ -808,3 +808,27 @@ (t/is (= "Component" (:name component1'))) (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. + (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"})) + child1-id (thi/id :child1) + file (ths/update-shape file :root1 :shapes [nil "not-a-uuid" missing-id child1-id]) + + 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 (= "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")))