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]))))))