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

🐛 Fix shared files 500 on shapes with invalid child ids

Browse Source 
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
This commit is contained in:
Eva Marco 2026-10-22 09:40:48 +02:00
parent 452d751dff
commit a4872a2589
4 changed files with 55 additions and 16 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
backend/src/app/rpc/commands/files.clj
View File

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

28
backend/test/backend_tests/rpc_file_test.clj
View File

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

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

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

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

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