github/penpot
1
0
Fork 0
mirror of https://github.com/penpot/penpot.git synced 2026-11-14 22:24:56 +00:00
Code Issues Packages Projects Releases Wiki Activity

🐛 Address second review of the shared files fix

Browse Source 
Drop the log assertion from the skip test: it relied on racing the
logger thread and log lines are not asserted in this suite. Keep the
functional assertions (list contains only the healthy file).

Add the cases missing from the second review: a shape without a
:shapes key survives 0033 and 0036 untouched, and the untouched
objects test now wraps the page objects in the ObjectsMap the
backend uses on the unrealized save path.

Restore the thread interrupt flag in the per-file catch when the
cause is an InterruptedException.

AI-assisted-by: inkling-2-small-free
This commit is contained in:
Andrey Antukh 2026-11-12 14:25:18 +00:00
parent 031d81f74b
commit ebf27e555f
3 changed files with 38 additions and 31 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -616,6 +616,8 @@
(let [file (get-file-with-summary cfg id)]
(assoc file :library-file-ids (db/decode-pgarray library-file-ids #{})))
(catch Throwable cause
(when (instance? InterruptedException cause)
(.interrupt (Thread/currentThread)))
(l/error :hint "unable to load shared file"
:file-id (str id)
:team-id (str team-id)

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

@ -11,7 +11,6 @@
[app.common.files.changes-builder :as pcb]
[app.common.files.migrations :as fmg]
[app.common.files.tokens :as cfo]
[app.common.logging :as l]
[app.common.pprint :as pp]
[app.common.thumbnails :as thc]
[app.common.time :as ct]
@ -3255,19 +3254,6 @@
(t/is (= (:id team)
(get-in (meta (:result out)) [:app.loggers.audit/props :team-id])))))
(defn- wait-for-log-record
"The first captured log record whose message hint is `hint`, waiting up
to two seconds because logging is asynchronous."
[records hint]
(loop [tries 0]
(if-let [record (->> @records
(filter #(= hint (get-in % [::l/props :hint])))
(first))]
record
(when (< tries 200)
(Thread/sleep 10)
(recur (inc tries))))))
(t/deftest get-team-shared-files-skips-files-that-fail-to-load
(let [profile (th/create-profile* 1)
file1 (th/create-file* 1 {:profile-id (:id profile)
@ -3283,26 +3269,16 @@
(doseq [cause [(ex-info "broken file" {:type :assertion})
(AssertionError. "point instance expected")
(StackOverflowError.)]]
(let [records (atom [])
broken (fn [cfg id]
(let [broken (fn [cfg id]
(if (= id (:id file1))
(throw cause)
(get-file cfg id)))
out (try
(add-watch l/log-record ::capture
(fn [_ _ _ record] (swap! records conj record)))
(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)}))
(finally
(remove-watch l/log-record ::capture)))]
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))))
(t/is (= (str (:id file1))
(some-> (wait-for-log-record records "unable to load shared file")
(get-in [::l/props :file-id])))
(str "reports file failing with " (class cause)))))))
(t/is (= #{(:id file2)} (into #{} (map :id) (:result out))))))))
(t/deftest get-team-shared-files-repairs-invalid-children-ids
;; A stored shared file that lists nil and a dangling id in :shapes and

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

@ -17,6 +17,7 @@
[app.common.test-helpers.shapes :as ths]
[app.common.test-helpers.variants :as thv]
[app.common.types.file :as ctf]
[app.common.types.objects-map :as omap]
[app.common.types.path :as path]
[app.common.types.shape :as cts]
[app.common.types.token :as ctt]
@ -888,7 +889,20 @@
data' (cfm/migrate-data data migration-id)]
(t/is (identical? (get-in data [:pages-index page-id :objects])
(get-in data' [:pages-index page-id :objects])))))
(get-in data' [:pages-index page-id :objects]))))
;; Same property through the objects-map wrapper the backend uses on the
;; unrealized save path, whose `assoc` marks the page as modified.
(let [migration-id "0036-fix-shape-children-ids"
file (-> (thf/sample-file :file1)
(tho/add-frame-with-child :frame1 :child1))
page-id (thf/current-page-id file)
objects (omap/wrap (get-in (ctf/file-data file) [:pages-index page-id :objects]))
data (assoc-in (ctf/file-data file) [:pages-index page-id :objects] objects)
data' (cfm/migrate-data data migration-id)]
(t/is (omap/objects-map? objects) "wrapped in an ObjectsMap")
(t/is (identical? objects (get-in data' [:pages-index page-id :objects])))))
(t/deftest migration-0036-fixes-children-ids-of-files-that-applied-0033
;; Files that applied 0033 before it repaired children never run it
@ -906,6 +920,21 @@
(t/is (not (cfm/need-migration? file')) "new migration recorded")
(t/is (= [child1-id] (:shapes (ths/get-shape file' :frame1))))))
(t/deftest migrations-leave-shapes-without-the-shapes-key-untouched
;; Only containers have a :shapes key; other shapes come out of the
;; repair unchanged, without a :shapes key being added.
(doseq [migration-id ["0033-normalize-main-instance-names"
"0036-fix-shape-children-ids"]]
(let [file (-> (thf/sample-file :file1)
(tho/add-frame-with-child :frame1 :child1))
child (ths/get-shape file :child1)
data' (cfm/migrate-data (ctf/file-data file) migration-id)
child' (ths/get-shape (assoc file :data data') :child1)]
(t/is (not (contains? child :shapes)) "the fixture shape has no :shapes key")
(t/is (not (contains? child' :shapes)) (str migration-id))
(t/is (= child child') migration-id))))
(t/deftest migration-0033-fixes-children-ids-through-file-migration
;; 0032 adds the uuid of a child listed as a string, so the child is
;; listed twice before 0033; the file must still pass validation.