From 291577035de287b73b4e20f2ebac9dc8122683f6 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Tue, 13 Oct 2026 10:17:18 +0000 Subject: [PATCH] :bug: 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 --- backend/src/app/binfile/common.clj | 21 +++++- backend/src/app/binfile/v3.clj | 58 ++++++++------- backend/src/app/rpc/commands/files.clj | 18 ++--- backend/test/backend_tests/binfile_test.clj | 78 ++++++++++++++++++++- 4 files changed, 137 insertions(+), 38 deletions(-) diff --git a/backend/src/app/binfile/common.clj b/backend/src/app/binfile/common.clj index fe85844dc2..3e4ff243c1 100644 --- a/backend/src/app/binfile/common.clj +++ b/backend/src/app/binfile/common.clj @@ -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] diff --git a/backend/src/app/binfile/v3.clj b/backend/src/app/binfile/v3.clj index e8f56f3779..09bcdefebe 100644 --- a/backend/src/app/binfile/v3.clj +++ b/backend/src/app/binfile/v3.clj @@ -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}] diff --git a/backend/src/app/rpc/commands/files.clj b/backend/src/app/rpc/commands/files.clj index b71d425040..e1b556850a 100644 --- a/backend/src/app/rpc/commands/files.clj +++ b/backend/src/app/rpc/commands/files.clj @@ -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])) diff --git a/backend/test/backend_tests/binfile_test.clj b/backend/test/backend_tests/binfile_test.clj index e2c7fa17e5..0c431f7a64 100644 --- a/backend/test/backend_tests/binfile_test.clj +++ b/backend/test/backend_tests/binfile_test.clj @@ -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))))))))