From 2e6f107fd968702b8adfcdfbc52566a4be943aa5 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Thu, 29 Oct 2026 16:09:20 +0000 Subject: [PATCH] :bug: Apply pending migrations when previewing a version Automatic snapshots are attached before the save applies pending file migrations, so a version can store the old format. The preview RPC handed that data to the UI as stored, and the viewport renders it with the current code. Realize the snapshot with the live file id (fragments are keyed by file, not by snapshot), normalize its migrations to a set (the decoder keeps the pgarray as a vector, and `set/difference` would compare indexes) and apply the pending migrations with the pure `fmg/migrate-file` and the live libraries. Nothing is persisted: the stored version keeps its data and restoring it re-migrates on the next read. The response now carries the snapshot's migrations. Use `:features` the way `get-file` does for `fdata/objects-map`: a client that does not declare it gets plain objects. The historical feature set is not checked against the enabled one on purpose: a version may predate a feature change. Cover the preview of a snapshot with a pending migration and the objects realization; both pin that the stored row is untouched. Update the file-data memory with the preview contract and the vector/set trap. AI-assisted-by: deepseek-v4.1-flash --- .serena/memories/backend/subtleties.md | 2 +- .../src/app/rpc/commands/files_snapshot.clj | 39 ++++- .../backend_tests/rpc_file_snapshot_test.clj | 137 ++++++++++++++++++ 3 files changed, 171 insertions(+), 7 deletions(-) diff --git a/.serena/memories/backend/subtleties.md b/.serena/memories/backend/subtleties.md index e0b6f20636..c4f797e66a 100644 --- a/.serena/memories/backend/subtleties.md +++ b/.serena/memories/backend/subtleties.md @@ -94,7 +94,7 @@ - Pointer-map fragments are persisted separately as type `fragment`, and only modified pointer maps are written. A pointer keeps its id while nothing changes; content (`assoc`/`without`) and metadata (`with-meta`) changes produce a fresh id, so a fragment row is never rewritten after it is written. Re-assoc with an identical value returns the same map in array and hash maps alike, so a save keeps the fragment id of every page it does not change. - `enable-objects-map` skips unloaded pointer pages: their fragment was already wrapped when it was written, and loading it only to wrap it again costs one page read per untouched page on every save. A page with a plain fragment is wrapped when it is first loaded or edited; a data migration that enables `fdata/objects-map` must rewrite the data itself. - Snapshots share fragment rows with the live file (they are stored with pointers, not materialized). `file-gc` marks every fragment referenced neither by the current main file nor by an unpurged snapshot; the file is read unrealized and unchanged pages keep their ids via reattach, so a pass over an unchanged file writes no new fragments. -- A save attaches its automatic snapshot before applying pending file migrations: the snapshot keeps the pre-migration pointers, data and migration set, and restoring it leaves the next read to migrate the file. This also holds for files without the feature; `get-file-snapshot` returns that data as stored and takes `:migrations` from the live file, so an unmigrated automatic snapshot previews in the old format. The window is narrow: the `get-file` RPC reads with `migrate?` on and its ETag hashes `available-migrations`, so a post-deploy read cannot be a 304 from the pre-deploy state and migrates before any save in the normal flow; the pre-migration snapshot only appears when the save is the first request to touch the file after the deploy (an open tab that saves without re-reading, or a non-UI writer). `create-file-snapshot` and the restore backup read with `migrate?` on, so those versions are stored already migrated. +- A save attaches its automatic snapshot before applying pending file migrations: the snapshot keeps the pre-migration pointers, data and migration set, and restoring it leaves the next read to migrate the file. This also holds for files without the feature. The preview (`get-file-snapshot`) applies the pending migrations in memory with the live file's libraries and never persists them: the stored version keeps its data, the payload carries the snapshot's `:migrations` (normalized to a set; the decoder keeps the pgarray as a vector and `set/difference` would compare indexes) and `fdata/objects-map` is realized for clients that do not declare it. The historical feature set is deliberately not checked against the enabled one: a version may predate a feature change. The window is narrow: the `get-file` RPC reads with `migrate?` on and its ETag hashes `available-migrations`, so a post-deploy read cannot be a 304 from the pre-deploy state and migrates before any save in the normal flow; the pre-migration snapshot only appears when the save is the first request to touch the file after the deploy (an open tab that saves without re-reading, or a non-UI writer). `create-file-snapshot` and the restore backup read with `migrate?` on, so those versions are stored already migrated. - The migration read path (`binfile.common/migrate-file`) captures `fdata/original-pointers` before realizing, reattaches unchanged positions after applying the migrations, and persists the reattached file; it returns the realized file because reads expose plain data. A migration that adds `fdata/objects-map` skips the reattach so the pages get wrapped. - `fdata/load-pointer` reads rows that a previous `file-gc` pass marked on purpose (explicit `::db/remove-deleted false`): the fragment is only gone once `objects-gc` purges it. The `get-file-fragment` RPC keeps rejecting marked rows. - Bind the fragment loader through `fdata/with-pointers` (reads), `fdata/with-tracked-pointers` (writes with a loader) or `fdata/with-tracked` (writes with nothing to load yet, e.g. file creation), and `fdata/with-untracked` (validation passes that must track nothing) instead of hand-rolled `binding` forms. Two direct bindings remain on purpose: the fine-grained tracked toggles of `app.rpc.commands.files-update` (validation/repair turn tracking off, restore the existing atom and read it before repairing), and `binfile.common/encode-file` (a fresh tracked set inheriting the current one). Converge them only if atom-scoped macros (`with-tracking`, `with-inherited-tracking`) are added; today the extra indirection buys nothing. diff --git a/backend/src/app/rpc/commands/files_snapshot.clj b/backend/src/app/rpc/commands/files_snapshot.clj index af3494c935..80a867bcee 100644 --- a/backend/src/app/rpc/commands/files_snapshot.clj +++ b/backend/src/app/rpc/commands/files_snapshot.clj @@ -9,6 +9,7 @@ [app.binfile.common :as bfc] [app.common.exceptions :as ex] [app.common.features :as-alias cfeat] + [app.common.files.migrations :as fmg] [app.common.schema :as sm] [app.common.time :as ct] [app.db :as db] @@ -54,7 +55,7 @@ ::sm/params schema:get-file-snapshot ::sm/result files/schema:file-with-permissions ::db/transaction true} - [cfg {:keys [::rpc/profile-id file-id id] :as params}] + [cfg {:keys [::rpc/profile-id file-id id features] :as params}] (let [perms (perms/get-file-read-permissions cfg profile-id file-id)] (files/check-read-permissions! perms) (let [snapshot (fsnap/get-snapshot cfg file-id id)] @@ -71,13 +72,39 @@ ;; file, so materialize them here: the preview UI applies the ;; response straight into state and cannot resolve pointers. ;; NOTE: the loader binds the live file-id, not the snapshot id. + ;; + ;; A snapshot taken while a migration was pending stores the old + ;; format. Apply the pending migrations in memory, the same pure + ;; step a normal read runs, and never persist them: the stored + ;; version keeps its data and restoring it re-migrates on the + ;; next read. `:migrations` is normalized to a set because the + ;; snapshot decoder keeps the pgarray as a vector and + ;; `set/difference` would compare indexes. (let [base-file (bfc/get-file cfg file-id :load-data? false) - data (fdata/realize-pointers cfg file-id (:data snapshot))] + snapshot (-> snapshot + (assoc :id file-id) + (update :migrations #(into #{} %))) + file (if (fmg/need-migration? snapshot) + (let [file (fdata/realize cfg snapshot)] + (fmg/migrate-file + file + (delay (bfc/get-resolved-file-libraries cfg file)))) + (update snapshot :data + #(fdata/realize-pointers cfg file-id %))) + ;; Works like in `get-file`: a client that does not declare + ;; `fdata/objects-map` gets plain objects. The historical + ;; feature set is deliberately not checked against the + ;; enabled one: a version may predate a feature change. + file (if (and (contains? (:features file) "fdata/objects-map") + (not (contains? features "fdata/objects-map"))) + (fdata/realize-objects cfg file) + file)] (-> base-file - (assoc :data data) - (assoc :version (:version snapshot)) - (assoc :features (:features snapshot)) - (assoc :revn (:revn snapshot)) + (assoc :data (:data file)) + (assoc :version (:version file)) + (assoc :features (:features file)) + (assoc :migrations (:migrations file)) + (assoc :revn (:revn file)) (assoc :vern (rand-int 100000)) (assoc :permissions perms)))))) diff --git a/backend/test/backend_tests/rpc_file_snapshot_test.clj b/backend/test/backend_tests/rpc_file_snapshot_test.clj index 025c1dc290..9f6e20b6b1 100644 --- a/backend/test/backend_tests/rpc_file_snapshot_test.clj +++ b/backend/test/backend_tests/rpc_file_snapshot_test.clj @@ -11,6 +11,7 @@ [app.common.pprint :as pp] [app.common.thumbnails :as thc] [app.common.time :as ct] + [app.common.types.objects-map :as omap] [app.common.types.shape :as cts] [app.common.uuid :as uuid] [app.config :as cf] @@ -549,3 +550,139 @@ (t/is (nil? (:error out))) (t/is (empty? (fdata/get-used-pointer-ids (get-in out [:result :data]))))))))) + +(t/deftest snapshot-preview-applies-pending-migrations + ;; A snapshot taken while a migration was pending stores the old + ;; format. The preview must apply the pending migration in memory + ;; (the same pure step a normal read runs) instead of handing the old + ;; format to the UI, and must not persist anything: the stored + ;; version keeps its data so restoring it re-migrates on next read. + (let [profile (th/create-profile* 1) + file (th/create-file* 1 {:profile-id (:id profile) + :project-id (:default-project-id profile) + :is-shared false}) + file-id (:id file) + page-id (first (get-in file [:data :pages])) + shape-id (uuid/random)] + + (update-file! :profile-id (:id profile) + :file-id file-id + :revn 0 + :changes [{:type :add-obj + :page-id page-id + :id shape-id + :parent-id uuid/zero + :frame-id uuid/zero + :components-v2 true + :obj (-> (cts/setup-shape {:id shape-id + :name "rect" + :frame-id uuid/zero + :parent-id uuid/zero + :type :rect}) + (assoc :applied-tokens {:stroke-width "token"}))}]) + + ;; Force the 0031 migration on the next save. + (th/db-delete! :file-migration {:file-id file-id + :name "0031-migrate-stroke-width-token-attr"}) + + (update-file! :profile-id (:id profile) + :file-id file-id + :revn 1 + :changes [{:type :add-page :name "b" :id (uuid/random)}]) + + (let [snapshot (->> (th/db-query :file-change {:file-id file-id}) + (filter #(and (some? (:label %)) + (= 1 (:revn %)))) + (first)) + snapshot-id (:id snapshot) + stored (fdata/realize th/*system* + (assoc (fsnap/get-snapshot th/*system* file-id snapshot-id) + :id file-id)) + out (th/command! {::th/type :get-file-snapshot + ::rpc/profile-id (:id profile) + :file-id file-id + :id snapshot-id + :features cfeat/supported-features})] + + (t/is (uuid? snapshot-id)) + + ;; The stored version keeps the old format and the pending + ;; migration: the preview must not touch the stored row. + (t/is (not (contains? (set (:migrations stored)) + "0031-migrate-stroke-width-token-attr"))) + (t/is (= {:stroke-width "token"} + (get-in stored [:data :pages-index page-id + :objects shape-id :applied-tokens]))) + + ;; The preview migrates it in memory. + (t/is (nil? (:error out))) + (t/is (= file-id (get-in out [:result :data :id]))) + (let [tokens (get-in out [:result :data :pages-index page-id + :objects shape-id :applied-tokens])] + (t/is (not (contains? tokens :stroke-width))) + (t/is (contains? tokens :stroke-width-top)) + (t/is (contains? tokens :stroke-width-bottom))) + + ;; The preview declared the migrated set in the payload. + (t/is (contains? (set (get-in out [:result :migrations])) + "0031-migrate-stroke-width-token-attr"))) + + ;; Nothing was persisted by the preview. + (let [snapshot-id (:id (->> (th/db-query :file-change {:file-id file-id}) + (filter #(and (some? (:label %)) + (= 1 (:revn %)))) + (first))) + stored (fdata/realize th/*system* + (assoc (fsnap/get-snapshot th/*system* file-id snapshot-id) + :id file-id))] + (t/is (= {:stroke-width "token"} + (get-in stored [:data :pages-index page-id + :objects shape-id :applied-tokens])))))) + +(t/deftest snapshot-preview-realizes-objects-for-clients-without-the-feature + ;; `:features` works like in `get-file`: a client that does not + ;; declare `fdata/objects-map` receives plain objects instead of the + ;; storage structure of the fragment. + (let [profile (th/create-profile* 1) + file (th/create-file* 1 {:profile-id (:id profile) + :project-id (:default-project-id profile) + :is-shared false}) + file-id (:id file) + page-id (first (get-in file [:data :pages])) + shape-id (uuid/random) + _ (update-file! :profile-id (:id profile) + :file-id file-id + :revn 0 + :changes [{:type :add-obj + :page-id page-id + :id shape-id + :parent-id uuid/zero + :frame-id uuid/zero + :components-v2 true + :obj (cts/setup-shape + {:id shape-id + :name "rect" + :frame-id uuid/zero + :parent-id uuid/zero + :type :rect})}]) + + snapshot-id (:id (:result (th/command! {::th/type :create-file-snapshot + ::rpc/profile-id (:id profile) + :file-id file-id + :label "v1"}))) + preview (fn [features] + (th/command! {::th/type :get-file-snapshot + ::rpc/profile-id (:id profile) + :file-id file-id + :id snapshot-id + :features features})) + with-feature (preview cfeat/supported-features) + without-feature (preview (disj cfeat/supported-features + "fdata/objects-map"))] + + (t/is (nil? (:error with-feature))) + (t/is (nil? (:error without-feature))) + (t/is (omap/objects-map? + (get-in with-feature [:result :data :pages-index page-id :objects]))) + (t/is (not (omap/objects-map? + (get-in without-feature [:result :data :pages-index page-id :objects]))))))