github/penpot
1
0
Fork 0
mirror of https://github.com/penpot/penpot.git synced 2026-10-29 14:00:37 +00:00
Code Issues Packages Projects Releases Wiki Activity

🐛 Address review findings on circular import guard

Browse Source 
F1 extracts the cycle check into bfc/library-link-status and reuses
it in link-file-to-library and v3 import, keeping UI errors intact.

F3 orders get-files-rels and manifest relations so the first edge
wins deterministically, and documents it.

F2 adds a three-node transitive cycle test; F4 asserts file_library_sync
has no orphan rows for skipped links and matches rels otherwise.

AI-assisted-by: muse-spark-1.3-contributor
This commit is contained in:
Andrey Antukh 2026-10-13 10:17:18 +00:00
parent 48ab90b1ec
commit 291577035d
4 changed files with 137 additions and 38 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

21
backend/src/app/binfile/common.clj
View File

@ -473,7 +473,9 @@
:deleted-at nil}))
(defn get-files-rels
"Given a set of file-id's, return all matching relations with the libraries"
"Given a set of file-id's, return all matching relations with the libraries.
Rows come back in stable `(file-id, library-file-id)` order so export
and import resolve cycles deterministically."
[cfg ids]
(assert (set? ids) "expected a set of uuids")
@ -490,7 +492,8 @@
" ON fls.file_id = flr.file_id"
" AND fls.library_file_id = flr.library_file_id"
" WHERE flr.file_id = ANY(?)"
" AND l.deleted_at IS NULL;")]
" AND l.deleted_at IS NULL"
" ORDER BY flr.file_id, flr.library_file_id;")]
(db/exec! conn [sql ids])))))
(def ^:private sql:upsert-file-library-sync
@ -526,6 +529,20 @@
(->> (db/exec! conn [sql:get-libraries ids'])
(into #{} xf-map-id))))))
(defn library-link-status
"Check whether linking `file-id` to `library-id` is safe. Returns
`:ok` when the link can be created, `:self-link` when both ids are
the same, and `:circular` when the link would close a dependency
cycle (i.e. `file-id` is already among the transitive libraries of
`library-id`). Shared by the UI command (`link-file-to-library`,
which raises) and the v3 importer (`import-file-relations`, which
skips and logs), so both stay consistent."
[cfg file-id library-id]
(cond
(= file-id library-id) :self-link
(contains? (get-libraries cfg [library-id]) file-id) :circular
:else :ok))
(defn get-file-object-thumbnails
"Return all file object thumbnails for a given file."
[cfg file-id]

58
backend/src/app/binfile/v3.clj
View File

@ -986,37 +986,45 @@
file-id')))
(defn- import-file-relations
"Import library relations from the manifest, skipping edges that would
close a dependency cycle or link a file to itself (same check as
`link-file-to-library`, see `bfc/library-link-status`). Relations are
processed in sorted manifest order and the first edge wins: an edge
that would close a cycle with previously imported relations is skipped
and logged."
[{:keys [::db/conn ::manifest ::bfc/timestamp] :as cfg}]
(events/tap :progress {:section :relations})
(doseq [[file-id libr-id] (:relations manifest)]
(let [relations (sort-by (fn [[file-id libr-id]] [(str file-id) (str libr-id)])
(:relations manifest))]
(doseq [[file-id libr-id] relations]
(let [file-id (bfc/lookup-index file-id)
libr-id (bfc/lookup-index libr-id)]
(let [file-id (bfc/lookup-index file-id)
libr-id (bfc/lookup-index libr-id)]
(when (and file-id libr-id)
(cond
(= file-id libr-id)
(l/warn :hint "ignoring self-referencing library link"
:file-id (str file-id)
:lib-id (str libr-id)
::l/sync? true)
(when (and file-id libr-id)
(case (bfc/library-link-status cfg file-id libr-id)
:self-link
(l/warn :hint "ignoring self-referencing library link"
:file-id (str file-id)
:lib-id (str libr-id)
::l/sync? true)
(contains? (bfc/get-libraries cfg [libr-id]) file-id)
(l/warn :hint "ignoring circular library link"
:file-id (str file-id)
:lib-id (str libr-id)
::l/sync? true)
:circular
(l/warn :hint "ignoring circular library link"
:file-id (str file-id)
:lib-id (str libr-id)
::l/sync? true)
:else
(do
(l/dbg :hint "create file library link"
:file-id (str file-id)
:lib-id (str libr-id)
::l/sync? true)
(let [rel-params {:file-id file-id
:library-file-id libr-id}]
(db/insert! conn :file-library-rel rel-params)
(bfc/upsert-file-library-sync! conn (assoc rel-params :synced-at timestamp)))))))))
:ok
(do
(l/dbg :hint "create file library link"
:file-id (str file-id)
:lib-id (str libr-id)
::l/sync? true)
(let [rel-params {:file-id file-id
:library-file-id libr-id}]
(db/insert! conn :file-library-rel rel-params)
(bfc/upsert-file-library-sync! conn (assoc rel-params :synced-at timestamp))))))))))
(defn- import-storage-objects
[{:keys [::bfc/input ::entries-index ::bfc/timestamp] :as cfg}]

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

@ -1173,21 +1173,23 @@
::db/transaction true}
[{:keys [::db/conn] :as cfg} {:keys [::rpc/profile-id file-id library-id] :as params}]
(when (= file-id library-id)
(case (bfc/library-link-status cfg file-id library-id)
:self-link
(ex/raise :type :validation
:code :invalid-library
:hint "A file cannot be linked to itself"))
:hint "A file cannot be linked to itself")
:circular
(ex/raise :type :validation
:code :circular-library-reference
:hint "linking this library would create a circular dependency")
:ok nil)
(check-edition-permissions! conn profile-id file-id)
(check-edition-permissions! conn profile-id library-id)
(check-library-team-ownership! conn file-id library-id)
(let [transitive-deps (bfc/get-libraries cfg [library-id])]
(when (contains? transitive-deps file-id)
(ex/raise :type :validation
:code :circular-library-reference
:hint "linking this library would create a circular dependency")))
(link-file-to-library conn params)
(bfc/get-libraries cfg [library-id]))

78
backend/test/backend_tests/binfile_test.clj
View File

@ -2900,7 +2900,8 @@
;; each other (possible before the 2.17.1 guard) export both relations in
;; the manifest; the import must skip the one closing the cycle instead
;; of recreating it. The cycle is built with direct inserts because the
;; UI command `link-file-to-library` already rejects it.
;; UI command `link-file-to-library` already rejects it. Relations are
;; processed in sorted manifest order and the first edge wins.
(let [profile (th/create-profile* 1)
lib-a (th/create-file* 1 {:profile-id (:id profile)
:project-id (:default-project-id profile)
@ -2930,10 +2931,71 @@
(assoc ::bfc/input output)
(v3/import-files!)))
rels (mapcat #(db/query th/*system* :file-library-rel {:file-id %})
result)
syncs (mapcat #(db/query th/*system* :file-library-sync {:file-id %})
result)]
(t/is (= 2 (count result)))
;; only one of the two cycle edges survives the import
(t/is (= 1 (count rels)))
;; the skipped edge leaves no sync row behind
(t/is (= 1 (count syncs)))
(t/is (= (set (map (juxt :file-id :library-file-id) syncs))
(set (map (juxt :file-id :library-file-id) rels))))
;; no imported file lists itself among its transitive libraries
(doseq [file-id result]
(t/is (not (contains? (bfc/get-libraries th/*system* [file-id])
file-id))))))))
(t/deftest import-skips-transitive-cycle
;; A three-node cycle (A -> B -> C -> A) only breaks when the check uses
;; the transitive closure: a direct-edge-only check would keep all three
;; edges. Built with direct inserts because the UI command rejects it.
(let [profile (th/create-profile* 1)
lib-a (th/create-file* 1 {:profile-id (:id profile)
:project-id (:default-project-id profile)
:is-shared true
:name "Library A"})
lib-b (th/create-file* 2 {:profile-id (:id profile)
:project-id (:default-project-id profile)
:is-shared true
:name "Library B"})
lib-c (th/create-file* 3 {:profile-id (:id profile)
:project-id (:default-project-id profile)
:is-shared true
:name "Library C"})]
(db/insert! th/*system* :file-library-rel
{:file-id (:id lib-a)
:library-file-id (:id lib-b)})
(db/insert! th/*system* :file-library-rel
{:file-id (:id lib-b)
:library-file-id (:id lib-c)})
(db/insert! th/*system* :file-library-rel
{:file-id (:id lib-c)
:library-file-id (:id lib-a)})
(let [output (tmp/tempfile :suffix ".zip")]
(v3/export-files!
(-> th/*system*
(assoc ::bfc/ids #{(:id lib-a) (:id lib-b) (:id lib-c)})
(assoc ::bfc/export-type :include-libraries))
(io/output-stream output))
(let [result (:file-ids (-> th/*system*
(assoc ::bfc/project-id (:default-project-id profile))
(assoc ::bfc/profile-id (:id profile))
(assoc ::bfc/input output)
(v3/import-files!)))
rels (mapcat #(db/query th/*system* :file-library-rel {:file-id %})
result)
syncs (mapcat #(db/query th/*system* :file-library-sync {:file-id %})
result)]
(t/is (= 3 (count result)))
;; only two of the three cycle edges survive the import
(t/is (= 2 (count rels)))
;; the skipped edge leaves no sync row behind
(t/is (= 2 (count syncs)))
(t/is (= (set (map (juxt :file-id :library-file-id) syncs))
(set (map (juxt :file-id :library-file-id) rels))))
;; no imported file lists itself among its transitive libraries
(doseq [file-id result]
(t/is (not (contains? (bfc/get-libraries th/*system* [file-id])
@ -2964,9 +3026,13 @@
(assoc ::bfc/input output)
(v3/import-files!)))
rels (mapcat #(db/query th/*system* :file-library-rel {:file-id %})
result)
syncs (mapcat #(db/query th/*system* :file-library-sync {:file-id %})
result)]
(t/is (= 1 (count result)))
(t/is (= 0 (count rels)))))))
(t/is (= 0 (count rels)))
;; the skipped self-link leaves no sync row behind
(t/is (= 0 (count syncs)))))))
(t/deftest import-keeps-acyclic-library-relations
;; Control test: a chain without cycles must be imported untouched, so
@ -3004,6 +3070,12 @@
(assoc ::bfc/input output)
(v3/import-files!)))
rels (mapcat #(db/query th/*system* :file-library-rel {:file-id %})
result)
syncs (mapcat #(db/query th/*system* :file-library-sync {:file-id %})
result)]
(t/is (= 3 (count result)))
(t/is (= 2 (count rels)))))))
(t/is (= 2 (count rels)))
;; every kept relation has its sync row
(t/is (= 2 (count syncs)))
(t/is (= (set (map (juxt :file-id :library-file-id) syncs))
(set (map (juxt :file-id :library-file-id) rels))))))))