From ebf27e555f6c75c3fae29a36e3861642d13a0f10 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Thu, 12 Nov 2026 14:25:18 +0000 Subject: [PATCH] :bug: Address second review of the shared files fix 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 --- backend/src/app/rpc/commands/files.clj | 2 ++ backend/test/backend_tests/rpc_file_test.clj | 36 ++++--------------- .../common_tests/files_migrations_test.cljc | 31 +++++++++++++++- 3 files changed, 38 insertions(+), 31 deletions(-) diff --git a/backend/src/app/rpc/commands/files.clj b/backend/src/app/rpc/commands/files.clj index a980fc3dd5..0f907d0d9d 100644 --- a/backend/src/app/rpc/commands/files.clj +++ b/backend/src/app/rpc/commands/files.clj @@ -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) diff --git a/backend/test/backend_tests/rpc_file_test.clj b/backend/test/backend_tests/rpc_file_test.clj index 70b18e986a..5074d76e8b 100644 --- a/backend/test/backend_tests/rpc_file_test.clj +++ b/backend/test/backend_tests/rpc_file_test.clj @@ -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 diff --git a/common/test/common_tests/files_migrations_test.cljc b/common/test/common_tests/files_migrations_test.cljc index 7d877c671d..bbfb359a7f 100644 --- a/common/test/common_tests/files_migrations_test.cljc +++ b/common/test/common_tests/files_migrations_test.cljc @@ -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.