mirror of
https://github.com/penpot/penpot.git
synced 2026-11-12 13:00:37 +00:00
🐛 Skip circular library links on binfile import (#12062)
* 🐛 Skip circular library links on binfile import File import recreated circular library references that the link-file-to-library command already rejects. The v3 importer now skips manifest relations that would close a cycle, or link a file to itself, and logs each skipped link. Closes #12052. AI-assisted-by: muse-spark-1.3-contributor-free 🐛 Address review findings on circular import guard 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 🐛 Pass team-id in binfile v3 circular library tests The four circular library relation tests (import-skips-circular, import-skips-transitive-cycle, import-skips-self-referencing, import-keeps-acyclic) called v3/import-files! without ::bfc/team-id. resolve-and-link-libraries asserts (uuid? team-id), so with enable-backend-asserts the tests errored instead of running. Add (assoc ::bfc/team-id (:default-team-id profile)) to each test, mirroring the production RPC import path and the earlier fix in a36193e54b for the other binfile v3 import tests. Verified: 1338 tests, 6910 assertions, 0 failures. AI-assisted-by: mistral-large-4 * 🐛 Guard the v1 import and restore the RPC check order The version 1 importer recreated the circular library references that the version 3 importer already skips, and a user reaches it through `import-binfile` and the import job. It now asks `bfc/library-link-status` before inserting each relation, so a self link or an edge that would close a cycle is skipped and logged, and it processes the relations in sorted order like v3 does. Closes #12052. The version 2 importer stays out on purpose: only the developer REPL reaches it, and a note in the code records the gap. `link-file-to-library` keeps the shared helper but restores the order of the base commit: the self link guard, then the edition permission and team ownership checks, and only then the cycle check. A caller without permission gets `not-found` again instead of the circular error, and a rejected request no longer runs the recursive library query first. Tests: a v1 import that recreates a cycle, the self link error of the command, and the permission check running before the cycle check. AI-assisted-by: deepseek-v4.1-flash * 🐛 Tolerate repeated file entries in the v1 binfile import The v1 exporter unioned the requested files with their libraries without deduplicating, so a package exported from a file inside a library cycle (see #12052) lists that file twice in the metadata. The v1 import created one file per entry, so the second insert hit the primary key of `file` and aborted the whole import with "file already exists". The importer now reads the pair of objects of every entry to keep the stream in sync but imports the first occurrence only, warns about the repeats, and answers with one id per imported file. The v1 export keeps its behavior: it has no caller in the product, and only the tests and the developer REPL use it. Tests: a package with a repeated entry, built with the v1 export from a cyclic library pair, imports the file once and leaves no duplicated rows. AI-assisted-by: deepseek-v4.1-flash
This commit is contained in:
Andrey Antukh
GitHub
parent
1599e096da
commit
bcac14a4a8
@ -67,7 +67,8 @@ Every background job is a job-def: a plain `(defn execute-X [cfg params] ...)` i
|
||||
- Job resources (`app.jobs.storage`) are the objects a job owns through `job.resource_id`: they live in the `job-resource` bucket, are written with the profile of the job as owner (only that profile can read them back from `/assets/by-id`) and are never deduplicated. They carry no expiry of their own, because `expired-at` is stored as `deleted_at` and an object with it is unreadable from birth: the retention of the artifact is the retention of its job row. The writer marks the object as touched right away, so an artifact that no row references (a crash between the upload and the completion) is reclaimed by `storage-gc-touched`, and a row that does reference it freezes it until the jobs GC deletes the row and touches the object.
|
||||
- The binfile jobs (`:export-binfile` and `:import-binfile`) run the core of `bf.v1`/`bf.v3` through `app.binfile.jobs`, which turns the `:progress` taps of the core into `progress` events of the job with `jobs/heartbeat` (the `modified_at` touch at most once per second, progress events at up to ~200ms). Each event is a self-contained milestone: a `:stage` keyword of one vocabulary shared by the two formats (the same work has a different name in v1 and v3) and an optional `:counters` map, with the counter of the activity named after its own stage and the outer scopes in course as context, which the tap carries itself (the reader puts `::outer-counters` in every tap inside them — today only `:files`, extendable to `:teams`/`:projects` for multi-scope exports — so the adapter remembers nothing between taps and a tap that happens outside any scope has no context); the ids and names the taps carry never reach the event. Every tap is reported, so a tap with no units to count still keeps the job alive, and a milestone the throttle drops loses nothing because the next one carries the same state. The adapter checks that the job is still active before the run, and the beat raises an `:interrupt` right there on the run thread when the job is no longer active, so the work stops at its next event point inside the transaction of its handler, which rolls back. A run whose last tap predates the interruption is discarded by the check once it returns. The adapter passes the job id from the handler context. Their job-def components carry pool, storage, metrics and msgbus, because a job with a `profile_id` cannot store an event without a msgbus on its cfg. Both readers answer with the same shape (`:file-ids` and, in version 3, the resolution of the libraries), because the version 1 reader returns a bare set of file ids.
|
||||
- Both binfile handlers revalidate their permission at run time, not only when the job is created, because it can be revoked while the job waits: the export checks the read permission of every file before reading anything and stores the package in `job-resource` as the resource of the job (answering with the completion envelope), and the import checks the edition permission of the destination and runs the core inside a transaction, which is what makes a cancelled import roll back instead of leaving half of a file. The import releases its package (marks it as deleted and drops the reference from the job row in the same transaction) and deletes its temporary copy once the job is terminal, whatever the outcome. Both handlers check their own result against a schema before returning it (the import job-def declares that same schema as its result schema, which is what types the resolution when the row is read).
|
||||
- The v3 import only materializes a library relation when both ids belong to the package and differ; a relation that points outside the package or a self link is ignored with a warning. Migration `0158-del-forbidden-file-library-rels` deletes the legacy cross-team and self rows from `file_library_rel` and their `file_library_sync` rows, sync rows first.
|
||||
- The v3 and v1 imports only materialize a library relation when both ids belong to the package and the link would neither be a self link nor close a dependency cycle (the same `bfc/library-link-status` check that `link-file-to-library` uses to raise); relations are processed in sorted `(file_id, library_file_id)` order, which `get-files-rels` also returns, so the first edge wins deterministically, and every edge it ignores is logged with a warning. The v2 import stays out of that guard on purpose: it is only reachable from the developer REPL (`app.srepl.binfile`) and keeps inserting the relations as they come. Migration `0158-del-forbidden-file-library-rels` deletes the legacy cross-team and self rows from `file_library_rel` and their `file_library_sync` rows, sync rows first.
|
||||
- A version 1 package lists its files in the metadata and the importer creates one file per entry. The v1 export (gone from the product; only tests and the developer REPL use it) unioned the requested files with their libraries without deduplicating, so a package exported from a file inside a library cycle lists that file twice. The importer reads the pair of objects of every entry to keep the stream in sync, imports the first occurrence only, warns about the repeats, and answers with one id per imported file.
|
||||
- The domain cfg of a binfile request lives in `app.binfile.common` (`export-cfg`, `import-cfg`), so the legacy RPC and the jobs freeze the same ids, resolve the same team features and apply the same anti-zip-bomb limits. `resolve-export-type` is the translation the legacy RPC needs for its deprecated boolean flags; the creation command asks the caller for the type instead of guessing a default.
|
||||
- Two retention knobs, easy to confuse: `:jobs-retention` sweeps terminal internal rows (the ones without a profile), while `:jobs-user-ttl` is the default expiry a user-facing job gets when it is created; the jobs GC deletes the row once `expires_at` passes and touches its resource. The heavy work of a user job runs on its own `:binfile` queue (`:worker-binfile-parallelism`, default 1) so it does not block the default runner.
|
||||
- The quote of a family of jobs (`::export-jobs-per-profile`, `::import-jobs-per-profile`) counts the jobs of the profile that are still on their way (`new`/`scheduled`/`running`/`retry`) whose name the registry gives for that family, so a new job of the family is covered without touching the quote.
|
||||
|
||||
@ -488,10 +488,13 @@
|
||||
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;")
|
||||
|
||||
(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")
|
||||
@ -534,6 +537,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]
|
||||
|
||||
@ -482,9 +482,11 @@
|
||||
(bfm/apply-pending-migrations! cfg)
|
||||
|
||||
;; Knowing that the ids of the created files are in index,
|
||||
;; just lookup them and return them in the same shape as v3.
|
||||
;; just lookup them and return them in the same shape as v3. The
|
||||
;; metadata of an old package may repeat a file id, so the answer
|
||||
;; carries one id per imported file.
|
||||
(let [files (-> bfc/*state* deref :files)]
|
||||
{:file-ids (into [] (keep #(get-in @bfc/*state* [:index %])) files)})))))
|
||||
{:file-ids (into [] (comp (keep #(get-in @bfc/*state* [:index %])) (distinct)) files)})))))
|
||||
|
||||
(defmethod read-import :v1
|
||||
[options]
|
||||
@ -510,89 +512,114 @@
|
||||
|
||||
(defmethod read-section :v1/files
|
||||
[{:keys [::bfc/input ::bfc/project-id ::bfc/name] :as system}]
|
||||
(doseq [[idx expected-file-id] (d/enumerate (-> bfc/*state* deref :files))]
|
||||
(let [file (read-obj! input)
|
||||
media (read-obj! input)
|
||||
(let [imported (volatile! #{})]
|
||||
(doseq [[idx expected-file-id] (d/enumerate (-> bfc/*state* deref :files))]
|
||||
(let [file (read-obj! input)
|
||||
media (read-obj! input)
|
||||
file-id (:id file)]
|
||||
|
||||
file-id (:id file)
|
||||
file-id' (bfc/lookup-index file-id)
|
||||
(when (not= file-id expected-file-id)
|
||||
(ex/raise :type :validation
|
||||
:code :inconsistent-penpot-file
|
||||
:found-id file-id
|
||||
:expected-id expected-file-id
|
||||
:hint "the penpot file seems corrupt, found unexpected uuid (file-id)"))
|
||||
|
||||
file (bfc/clean-file-features file)
|
||||
thumbnails (:thumbnails file)]
|
||||
(if (contains? @imported file-id)
|
||||
;; Old exports could repeat a file id in the metadata (the file was
|
||||
;; also one of its own libraries). The entry is read above to keep
|
||||
;; the stream in sync, but the file is imported only once.
|
||||
(l/warn :hint "ignoring repeated file entry"
|
||||
:file-id (str file-id)
|
||||
::l/sync? true)
|
||||
|
||||
(when (not= file-id expected-file-id)
|
||||
(ex/raise :type :validation
|
||||
:code :inconsistent-penpot-file
|
||||
:found-id file-id
|
||||
:expected-id expected-file-id
|
||||
:hint "the penpot file seems corrupt, found unexpected uuid (file-id)"))
|
||||
(let [file-id' (bfc/lookup-index file-id)
|
||||
file (bfc/clean-file-features file)
|
||||
thumbnails (:thumbnails file)]
|
||||
|
||||
(l/dbg :hint "processing file"
|
||||
:id (str file-id)
|
||||
:features (:features file)
|
||||
:version (-> file :data :version)
|
||||
:media (count media)
|
||||
:thumbnails (count thumbnails)
|
||||
::l/sync? true)
|
||||
(vswap! imported conj file-id)
|
||||
|
||||
(when (seq thumbnails)
|
||||
(let [thumbnails (remap-thumbnails thumbnails file-id')]
|
||||
(l/dbg :hint "updated index with thumbnails"
|
||||
:total (count thumbnails)
|
||||
::l/sync? true)
|
||||
(vswap! bfc/*state* update :thumbnails bfc/into-vec thumbnails)))
|
||||
(l/dbg :hint "processing file"
|
||||
:id (str file-id)
|
||||
:features (:features file)
|
||||
:version (-> file :data :version)
|
||||
:media (count media)
|
||||
:thumbnails (count thumbnails)
|
||||
::l/sync? true)
|
||||
|
||||
(when (seq media)
|
||||
;; Update index with media
|
||||
(l/dbg :hint "update index with media" :total (count media) ::l/sync? true)
|
||||
(vswap! bfc/*state* update :index bfc/update-index (map :id media))
|
||||
(when (seq thumbnails)
|
||||
(let [thumbnails (remap-thumbnails thumbnails file-id')]
|
||||
(l/dbg :hint "updated index with thumbnails"
|
||||
:total (count thumbnails)
|
||||
::l/sync? true)
|
||||
(vswap! bfc/*state* update :thumbnails bfc/into-vec thumbnails)))
|
||||
|
||||
;; Store file media for later insertion
|
||||
(l/dbg :hint "update media references" ::l/sync? true)
|
||||
(vswap! bfc/*state* update :media into (map #(update % :id bfc/lookup-index)) media))
|
||||
(when (seq media)
|
||||
;; Update index with media
|
||||
(l/dbg :hint "update index with media" :total (count media) ::l/sync? true)
|
||||
(vswap! bfc/*state* update :index bfc/update-index (map :id media))
|
||||
|
||||
(let [file (-> file
|
||||
(assoc :id file-id')
|
||||
(cond-> (and (= idx 0) (some? name))
|
||||
(assoc :name name))
|
||||
(assoc :project-id project-id)
|
||||
(dissoc :thumbnails))
|
||||
file (bfc/process-file system file)]
|
||||
;; Store file media for later insertion
|
||||
(l/dbg :hint "update media references" ::l/sync? true)
|
||||
(vswap! bfc/*state* update :media into (map #(update % :id bfc/lookup-index)) media))
|
||||
|
||||
;; All features that are enabled and requires explicit migration are
|
||||
;; added to the state for a posterior migration step.
|
||||
(doseq [feature (-> (::bfc/features system)
|
||||
(set/difference cfeat/no-migration-features)
|
||||
(set/difference (:features file)))]
|
||||
(vswap! bfc/*state* update :pending-to-migrate (fnil conj []) [feature file-id']))
|
||||
(let [file (-> file
|
||||
(assoc :id file-id')
|
||||
(cond-> (and (= idx 0) (some? name))
|
||||
(assoc :name name))
|
||||
(assoc :project-id project-id)
|
||||
(dissoc :thumbnails))
|
||||
file (bfc/process-file system file)]
|
||||
|
||||
(l/dbg :hint "create file" :id (str file-id') ::l/sync? true)
|
||||
(bfc/save-file! system file ::db/return-keys false)
|
||||
;; All features that are enabled and requires explicit migration
|
||||
;; are added to the state for a posterior migration step.
|
||||
(doseq [feature (-> (::bfc/features system)
|
||||
(set/difference cfeat/no-migration-features)
|
||||
(set/difference (:features file)))]
|
||||
(vswap! bfc/*state* update :pending-to-migrate (fnil conj []) [feature file-id']))
|
||||
|
||||
file-id'))))
|
||||
(l/dbg :hint "create file" :id (str file-id') ::l/sync? true)
|
||||
(bfc/save-file! system file ::db/return-keys false)
|
||||
|
||||
file-id')))))))
|
||||
|
||||
(defmethod read-section :v1/rels
|
||||
[{:keys [::db/conn ::bfc/input ::bfc/timestamp]}]
|
||||
(let [rels (read-obj! input)
|
||||
[{:keys [::db/conn ::bfc/input ::bfc/timestamp] :as cfg}]
|
||||
(let [rels (->> (read-obj! input)
|
||||
(sort-by (fn [{:keys [file-id library-file-id]}]
|
||||
[(str file-id) (str library-file-id)])))
|
||||
ids (into #{} (-> bfc/*state* deref :files))]
|
||||
;; Insert all file relations
|
||||
(doseq [{:keys [library-file-id] :as rel} rels]
|
||||
(let [rel (-> rel
|
||||
(update :file-id bfc/lookup-index)
|
||||
(update :library-file-id bfc/lookup-index))]
|
||||
|
||||
(if (contains? ids library-file-id)
|
||||
(do
|
||||
(l/dbg :hint "create file library link"
|
||||
:file-id (:file-id rel)
|
||||
:lib-id (:library-file-id rel)
|
||||
::l/sync? true)
|
||||
(let [rel-params (dissoc rel :synced-at)]
|
||||
(db/insert! conn :file-library-rel rel-params)
|
||||
(bfc/upsert-file-library-sync! conn {:file-id (:file-id rel-params)
|
||||
:library-file-id (:library-file-id rel-params)
|
||||
:synced-at (or (:synced-at rel)
|
||||
timestamp)})))
|
||||
(case (bfc/library-link-status cfg (:file-id rel) (:library-file-id rel))
|
||||
:self-link
|
||||
(l/warn :hint "ignoring self-referencing library link"
|
||||
:file-id (:file-id rel)
|
||||
:lib-id (:library-file-id rel)
|
||||
::l/sync? true)
|
||||
|
||||
:circular
|
||||
(l/warn :hint "ignoring circular library link"
|
||||
:file-id (:file-id rel)
|
||||
:lib-id (:library-file-id rel)
|
||||
::l/sync? true)
|
||||
|
||||
:ok
|
||||
(do
|
||||
(l/dbg :hint "create file library link"
|
||||
:file-id (:file-id rel)
|
||||
:lib-id (:library-file-id rel)
|
||||
::l/sync? true)
|
||||
(let [rel-params (dissoc rel :synced-at)]
|
||||
(db/insert! conn :file-library-rel rel-params)
|
||||
(bfc/upsert-file-library-sync! conn {:file-id (:file-id rel-params)
|
||||
:library-file-id (:library-file-id rel-params)
|
||||
:synced-at (or (:synced-at rel)
|
||||
timestamp)}))))
|
||||
|
||||
(l/warn :hint "ignoring file library link"
|
||||
:file-id (:file-id rel)
|
||||
|
||||
@ -319,6 +319,11 @@
|
||||
(db/insert! conn :file-tagged-object-thumbnail thumbnail
|
||||
{::db/return-keys false})))
|
||||
|
||||
;; NOTE: v2 packages are only produced and imported through the
|
||||
;; developer REPL (`app.srepl.binfile`), never through the product, so
|
||||
;; this path keeps inserting the relations as they come: unlike v1 and
|
||||
;; v3 it does not skip self-referencing or circular links (see
|
||||
;; `bfc/library-link-status`).
|
||||
(doseq [rel (read-obj cfg :file-rels file-id)]
|
||||
(let [rel (-> rel
|
||||
(update :file-id bfc/lookup-index)
|
||||
|
||||
@ -1038,26 +1038,49 @@
|
||||
file-id')))
|
||||
|
||||
(defn- import-file-relations
|
||||
"Import library relations from the manifest, ignoring relations whose
|
||||
endpoints are not both part of the package. Of the rest, skip 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}]
|
||||
(let [relations (:relations manifest)
|
||||
(let [relations (sort-by (fn [[file-id libr-id]] [(str file-id) (str libr-id)])
|
||||
(:relations manifest))
|
||||
file-ids (into #{} (map :id) (:files manifest))]
|
||||
(doseq [[index [file-id libr-id]] (d/enumerate relations)]
|
||||
(tap-progress cfg {:section :relations
|
||||
:current (inc index) :total (count relations)})
|
||||
:current (inc index)
|
||||
:total (count relations)})
|
||||
|
||||
(if (and (not= file-id libr-id)
|
||||
(contains? file-ids file-id)
|
||||
(if (and (contains? file-ids file-id)
|
||||
(contains? file-ids libr-id))
|
||||
(let [file-id (bfc/lookup-index file-id)
|
||||
libr-id (bfc/lookup-index libr-id)]
|
||||
(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))))
|
||||
(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)
|
||||
|
||||
:circular
|
||||
(l/warn :hint "ignoring circular library link"
|
||||
:file-id (str file-id)
|
||||
:lib-id (str libr-id)
|
||||
::l/sync? true)
|
||||
|
||||
: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))))))
|
||||
(l/warn :hint "ignoring invalid file library relation"
|
||||
:file-id (str file-id)
|
||||
:lib-id (str libr-id))))))
|
||||
|
||||
@ -1240,11 +1240,10 @@
|
||||
(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")))
|
||||
(when (= :circular (bfc/library-link-status cfg file-id library-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]))
|
||||
|
||||
@ -3154,7 +3154,8 @@
|
||||
[[(str (:id file)) (str (:id file))]])
|
||||
[file-id :as result] (import-bundle profile evil)]
|
||||
(t/is (= 1 (count result)))
|
||||
(t/is (empty? (th/db-query :file-library-rel {:file-id file-id})))))
|
||||
(t/is (empty? (th/db-query :file-library-rel {:file-id file-id})))
|
||||
(t/is (empty? (th/db-query :file-library-sync {:file-id file-id})))))
|
||||
|
||||
(t/deftest import-v3-keeps-valid-library-relation
|
||||
(let [profile (th/create-profile* 1)
|
||||
@ -3173,6 +3174,251 @@
|
||||
(t/is (= 1 (count (th/db-query :file-library-rel
|
||||
{:file-id (:id new-file)})))))))
|
||||
|
||||
(t/deftest import-skips-circular-library-relations
|
||||
;; https://github.com/penpot/penpot/issues/12052: two libraries that use
|
||||
;; 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. 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)
|
||||
: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"})]
|
||||
(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-a)})
|
||||
|
||||
(let [output (tmp/tempfile :suffix ".zip")]
|
||||
(v3/export-files!
|
||||
(-> th/*system*
|
||||
(assoc ::bfc/file-ids #{(:id lib-a) (:id lib-b)})
|
||||
(assoc ::bfc/export-type :include-libraries)
|
||||
(assoc ::bfc/output (io/output-stream output))))
|
||||
|
||||
(let [result (import-bundle profile output)
|
||||
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/file-ids #{(:id lib-a) (:id lib-b) (:id lib-c)})
|
||||
(assoc ::bfc/export-type :include-libraries)
|
||||
(assoc ::bfc/output (io/output-stream output))))
|
||||
|
||||
(let [result (import-bundle profile output)
|
||||
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])
|
||||
file-id))))))))
|
||||
|
||||
(t/deftest import-keeps-acyclic-library-relations
|
||||
;; Control test: a chain without cycles must be imported untouched, so
|
||||
;; the new guard only skips edges that would close a cycle.
|
||||
(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)})
|
||||
|
||||
(let [output (tmp/tempfile :suffix ".zip")]
|
||||
(v3/export-files!
|
||||
(-> th/*system*
|
||||
(assoc ::bfc/file-ids #{(:id lib-a) (:id lib-b) (:id lib-c)})
|
||||
(assoc ::bfc/export-type :include-libraries)
|
||||
(assoc ::bfc/output (io/output-stream output))))
|
||||
|
||||
(let [result (import-bundle profile output)
|
||||
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)))
|
||||
;; 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))))))))
|
||||
|
||||
(t/deftest import-v1-skips-circular-library-relations
|
||||
;; Issue #12052 also names the version 1 importer: importing a package
|
||||
;; whose libraries use each other must not recreate the cycle. The cycle
|
||||
;; is built with direct inserts because `link-file-to-library` already
|
||||
;; rejects it. The exported file only *uses* one of the libraries, so the
|
||||
;; package carries the cycle the same way a real export does.
|
||||
(let [profile (th/create-profile* 1)
|
||||
file (prepare-simple-file profile)
|
||||
lib-a (th/create-file* 2 {:profile-id (:id profile)
|
||||
:project-id (:default-project-id profile)
|
||||
:is-shared true
|
||||
:name "Library A"})
|
||||
lib-b (th/create-file* 3 {:profile-id (:id profile)
|
||||
:project-id (:default-project-id profile)
|
||||
:is-shared true
|
||||
:name "Library B"})]
|
||||
(th/link-file-to-library* {:file-id (:id file)
|
||||
:library-id (:id lib-a)})
|
||||
(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-a)})
|
||||
|
||||
(let [output (tmp/tempfile :suffix ".penpot")]
|
||||
(v1/export-files!
|
||||
(-> th/*system*
|
||||
(assoc ::bfc/file-ids #{(:id file)})
|
||||
(assoc ::bfc/include-libraries true))
|
||||
(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)
|
||||
(v1/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)))
|
||||
;; the relation of the file to its library, plus one of the two
|
||||
;; cycle edges; the edge that would close the cycle is skipped
|
||||
(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])
|
||||
file-id))))))))
|
||||
|
||||
(t/deftest import-v1-tolerates-repeated-file-entries
|
||||
;; Old v1 exports could list the same file twice in the package metadata:
|
||||
;; the exporter unioned the requested files with their libraries without
|
||||
;; deduplicating, and a file inside a library cycle is one of its own
|
||||
;; libraries. The import must read every entry and import each file once.
|
||||
(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"})]
|
||||
(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-a)})
|
||||
|
||||
(let [output (tmp/tempfile :suffix ".penpot")]
|
||||
;; exporting only lib-a repeats its id in the metadata, because it is
|
||||
;; among its own transitive libraries
|
||||
(v1/export-files!
|
||||
(-> th/*system*
|
||||
(assoc ::bfc/file-ids #{(:id lib-a)})
|
||||
(assoc ::bfc/include-libraries true))
|
||||
(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)
|
||||
(v1/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)]
|
||||
;; two files: the repeated entry is imported once
|
||||
(t/is (= 2 (count result)))
|
||||
;; one of the two cycle edges survives the import
|
||||
(t/is (= 1 (count rels)))
|
||||
(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 export-multi-file-includes-package-relations
|
||||
(let [profile (th/create-profile* 1)
|
||||
file (prepare-simple-file profile)
|
||||
|
||||
@ -2749,6 +2749,43 @@
|
||||
(let [edata (-> out :error ex-data)]
|
||||
(t/is (= :circular-library-reference (:code edata)))))))
|
||||
|
||||
(t/deftest link-file-to-library-self-reference
|
||||
(let [profile (th/create-profile* 1)
|
||||
file (th/create-file* 1 {:profile-id (:id profile)
|
||||
:project-id (:default-project-id profile)})]
|
||||
(let [out (th/command! {::th/type :link-file-to-library
|
||||
::rpc/profile-id (:id profile)
|
||||
:file-id (:id file)
|
||||
:library-id (:id file)})]
|
||||
(t/is (th/ex-info? (:error out)))
|
||||
(t/is (th/ex-of-type? (:error out) :validation))
|
||||
(t/is (= :invalid-library (:code (ex-data (:error out))))))))
|
||||
|
||||
(t/deftest permissions-check-precedes-circular-library-check
|
||||
;; A caller without edition permission must get `:not-found`, not the
|
||||
;; circular error: the permission checks run before the cycle check, so
|
||||
;; the dependency structure of files the caller cannot read never leaks.
|
||||
(let [profile1 (th/create-profile* 1)
|
||||
profile2 (th/create-profile* 2)
|
||||
file1 (th/create-file* 1 {:profile-id (:id profile1)
|
||||
:project-id (:default-project-id profile1)
|
||||
:is-shared true})
|
||||
file2 (th/create-file* 2 {:profile-id (:id profile1)
|
||||
:project-id (:default-project-id profile1)
|
||||
:is-shared true})
|
||||
file3 (th/create-file* 3 {:profile-id (:id profile1)
|
||||
:project-id (:default-project-id profile1)
|
||||
:is-shared true})]
|
||||
;; file3 -> file2 -> file1 makes linking file1 -> file3 circular
|
||||
(th/link-file-to-library* {:file-id (:id file3) :library-id (:id file2)})
|
||||
(th/link-file-to-library* {:file-id (:id file2) :library-id (:id file1)})
|
||||
(let [out (th/command! {::th/type :link-file-to-library
|
||||
::rpc/profile-id (:id profile2)
|
||||
:file-id (:id file1)
|
||||
:library-id (:id file3)})]
|
||||
(t/is (th/ex-info? (:error out)))
|
||||
(t/is (th/ex-of-type? (:error out) :not-found)))))
|
||||
|
||||
(t/deftest get-file-etag-includes-deleted-at
|
||||
(let [profile-id (uuid/random)
|
||||
file1 {:modified-at (ct/now)
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user