diff --git a/backend/src/app/binfile/common.clj b/backend/src/app/binfile/common.clj index 712f87df3f..6b3ecde538 100644 --- a/backend/src/app/binfile/common.clj +++ b/backend/src/app/binfile/common.clj @@ -325,8 +325,14 @@ same numbers travel on the returned file's metadata as `::audit/props`, so the RPC command that paid for the derive reports them on its own audit event. An ordinary file read never reaches this function, so an - ordinary read neither times nor logs anything." - [{:keys [::db/conn] :as cfg} {:keys [id] :as file} decode?] + ordinary read neither times nor logs anything. + + With `include-base-data?` true the returned file also carries the + decoded merge-base document under `::base-data`, which is the value + the replay starts from. A caller that is about to compare the branch + with its base reads the branch file with that flag and takes the + value, and it never decodes the same snapshot a second time." + [{:keys [::db/conn] :as cfg} {:keys [id] :as file} decode? include-base-data?] (let [tpoint (ct/tpoint) branch (db/get* conn :file-branch {:branch-file-id id}) @@ -367,6 +373,14 @@ rows (db/exec! conn [sql:get-branch-changes id]) + ;; the decoded merge-base document. It is what the replay starts + ;; from, and `process-changes` builds its result with persistent + ;; updates and never writes into its input, so this value stays + ;; the pre-replay state while `data` becomes the branch. A caller + ;; that needs the merge base takes this value instead of decoding + ;; the same snapshot a second time. + base-data (:data base) + ;; the derive replays the log batch by batch, so each batch is ;; decoded exactly once. `depth` is the flattened op count, the ;; quantity `files_branch.clj::check-oplog-depth-limit!` limits, @@ -376,7 +390,7 @@ (let [changes (blob/decode changes)] [(cpc/process-changes data changes) (+ depth (count changes))])) - [(:data base) 0] + [base-data 0] rows) data (if decode? data (blob/encode data)) @@ -402,6 +416,12 @@ (assoc :version (or (:version base) (:version file))) (cond-> (some? (:migrations base)) (assoc :migrations (:migrations base))) + (cond-> include-base-data? + ;; the comparison's `base` is the value the replay started from, + ;; which is the merge base as decoded. The replay returns the + ;; branch's own state in `data`, so a comparison handed `data` + ;; as its base would compare the branch with itself. + (assoc ::base-data base-data)) ;; the numbers travel with the value, because the RPC audit event ;; of the read that paid for this derive reads `::audit/props` off ;; the result metadata @@ -422,7 +442,8 @@ load-data? throw-if-not-exists? lock-for-update? - lock-for-share?] + lock-for-share? + include-base-data?] :or {lock-for-update? false lock-for-share? false load-data? true @@ -430,7 +451,8 @@ decode? true include-deleted? false throw-if-not-exists? true - realize? false} + realize? false + include-base-data? false} :as options}] (assert (db/connection? conn) "expected cfg with valid connection") @@ -475,7 +497,7 @@ file (if branch? - (branch-file-data cfg file decode?) + (branch-file-data cfg file decode? include-base-data?) (->> file (fmigr/resolve-applied-migrations cfg) (fdata/resolve-file-data cfg))) diff --git a/backend/src/app/rpc/commands/files_branch.clj b/backend/src/app/rpc/commands/files_branch.clj index eff6c48db7..27f7221927 100644 --- a/backend/src/app/rpc/commands/files_branch.clj +++ b/backend/src/app/rpc/commands/files_branch.clj @@ -243,7 +243,12 @@ (defn- get-base-data "The merge-base `:data` from the branch's base snapshot. Raises when the snapshot cannot be resolved: silently falling back to main would degrade - the three-way merge into \"branch overwrites main\" without conflicts." + the three-way merge into \"branch overwrites main\" without conflicts. + + Prefer `branch-base-data` where the branch file was read with + `:include-base-data? true`: the derive decoded the merge base to replay + the op log over it, so this function would decode the same snapshot a + second time." [cfg {:keys [source-file-id base-snapshot-id] :as branch}] (when-not base-snapshot-id (ex/raise :type :not-found @@ -252,6 +257,20 @@ :branch-id (:id branch))) (:data (fsnap/get-snapshot cfg source-file-id base-snapshot-id))) +(defn- branch-base-data + "The comparison's `base`: the merge-base document that a branch file + read with `:include-base-data? true` carries under `::bfc/base-data`. + That document is the state the branch's op log was replayed over, that + is the merge base before the replay. The file's `:data` holds the + branch's own state, so a comparison against `:data` would compare the + branch with itself. Raises when the key is absent, so a read that + forgot the flag fails loudly instead of comparing against nil." + [branch-file] + (or (::bfc/base-data branch-file) + (ex/raise :type :assertion + :code :branch-base-data-missing + :hint "the branch file was read without `:include-base-data? true`"))) + (defn- release-base-snapshot! "Reschedule a branch's base snapshot for normal deletion: it is pinned ~10 years while the branch is open, and must be released when the branch @@ -593,14 +612,19 @@ Raises on failure instead of degrading: the caller decides whether the result is worth caching, and a value computed from an error must never enter the summary cache." - [cfg main-data {:keys [source-file-id branch-file-id ahead-revn behind-revn] :as branch}] + [cfg main-data {:keys [source-file-id branch-file-id ahead-revn behind-revn]}] (if (and (zero? ahead-revn) (zero? behind-revn)) [0 0 0] (let [main-data (or main-data (:data (bfc/get-file cfg source-file-id :realize? true))) + ;; the derive hands back the merge base it replayed the log over, + ;; so the comparison's `base` costs one decode of the snapshot + ;; rather than two + branch-file (bfc/get-file cfg branch-file-id :realize? true + :include-base-data? true) branch-data (bm/remap-refs - (:data (bfc/get-file cfg branch-file-id :realize? true)) + (:data branch-file) (branch-id-map cfg branch-file-id source-file-id)) - base-data (get-base-data cfg branch) + base-data (branch-base-data branch-file) clean-count (fn [m] (let [s (:stats m)] (+ (:added s) (:modified s) (:deleted s)))) fwd (when (pos? ahead-revn) @@ -646,8 +670,9 @@ (defn cached-diff-counts "`branch-diff-counts!` behind the summary cache, degrading to zeros with a warning when the base snapshot cannot be resolved: a listing is - read-only, and merge/update DO refuse loudly in that situation - (`get-base-data`). A value computed from an error never enters the cache. + read-only, and merge/update DO refuse loudly in that situation (reading + the branch file raises `:base-snapshot-missing`). A value computed from + an error never enters the cache. `branch` carries the key tuple and the pair being compared, with `source-file-id` naming main and `branch-file-id` the branch, whichever @@ -759,14 +784,15 @@ dir (or direction :branch->main) main-file (-> (bfc/get-file cfg (:source-file-id branch) :realize? true) (check-file-size-limits! :compare)) - branch-file (bfc/get-file cfg (:branch-file-id branch) :realize? true) + branch-file (bfc/get-file cfg (:branch-file-id branch) + :realize? true :include-base-data? true) log (-> (branch-log cfg (:branch-file-id branch)) (check-oplog-depth-limit! (:branch-file-id branch) :compare)) main-data (:data main-file) branch-data (bm/remap-refs (:data branch-file) (branch-id-map cfg (:branch-file-id branch) (:source-file-id branch))) - base-data (get-base-data cfg branch) + base-data (branch-base-data branch-file) ;; only the branch->main direction may be bounded to the pages ;; the branch touched: the other direction is main's changes and ;; nothing here knows which pages those are @@ -849,8 +875,9 @@ (check-file-size-limits! :merge)) _ (-> (branch-log cfg branch-file-id) (check-oplog-depth-limit! branch-file-id :merge)) - branch-file (bfc/get-file cfg branch-file-id :realize? true) - base-data (get-base-data cfg branch) + branch-file (bfc/get-file cfg branch-file-id + :realize? true :include-base-data? true) + base-data (branch-base-data branch-file) ;; canonicalize the branch's local refs to main's ids: the ;; branch file id (components, library color/typography @@ -1070,19 +1097,39 @@ team (teams/get-team conn :profile-id profile-id :file-id branch-file-id) delay (ldel/get-deletion-delay team) - ;; update applies main's changes INTO the branch, so the - ;; canonical local ids are the branch's: re-point main/base - ;; local refs (file id + paired media) to the branch's ids; - ;; media ADDED on main gets fresh ids pre-allocated here and - ;; its rows copied into the branch only if the update applies. + ;; Two id maps, one per direction of the crossing. + ;; + ;; The COMPARISON runs in main's frame, the frame the merge + ;; base was captured in, the frame the compare view uses + ;; (`files_branch.clj::branch-id-map` in + ;; `files_branch.clj::get-branch-diff`), and the only frame + ;; the branch's inherited content agrees with. A branch stores + ;; no data, so `binfile.clj::branch-file-data` replays its op + ;; log over that base and everything it inherited carries the + ;; base's ids. Comparing in the branch's frame instead + ;; rewrites main's and the base's local references to the + ;; branch file id, and a self reference in main is then + ;; unequal to the branch's own copy of it, so every shape + ;; main touched that carries one reads as an edit on both + ;; sides. + base-data base-raw + main-raw (:data main-file) + branch-cmp (bm/remap-refs (:data branch-file) + (branch-id-map cfg branch-file-id main-id)) + + ;; The WRITE runs in the branch's frame: a reference is only a + ;; reference in the file it names, so main's file id and + ;; main's media rows are re-pointed at the branch's ids before + ;; the change reaches the branch. Media ADDED on main has no + ;; branch row yet, so it gets a fresh id here and its row is + ;; copied below only if the update applies. pairs (media-pairs cfg main-id branch-file-id) - new-media (unpaired-media-rows cfg main-id (:data main-file) pairs) + new-media (unpaired-media-rows cfg main-id main-raw pairs) fresh-map (into {} (map (fn [row] [(:id row) (uuid/next)])) new-media) - id-map (-> pairs + apply-map (-> pairs (merge fresh-map) (assoc main-id branch-file-id)) - base-data (bm/remap-refs base-raw id-map) - main-data (bm/remap-refs (:data main-file) id-map) + main-data (bm/remap-refs main-raw apply-map) ;; did the branch carry its OWN changes before this update? ;; (revn moved since the base was last positioned) @@ -1116,8 +1163,7 @@ {::db/return-keys false}))) merge-summary - (bm/compute-merge base-data main-data - (:data branch-file) :main->branch) + (bm/compute-merge base-data main-raw branch-cmp :main->branch) conflicts (:conflicts merge-summary) resolved? (fn [c] (bm/conflict-resolved? c (get resolutions (:id c)))) @@ -1145,7 +1191,7 @@ ;; assignment compute-changes uses internally here, so it can ;; be reused for the unsupported-kinds gate without re-diffing. (let [{:keys [changes unsupported]} - (bm/compute-changes base-data (:data branch-file) main-data + (bm/compute-changes base-data branch-cmp main-raw (or inverted {}) merge-summary)] (cond @@ -1178,9 +1224,10 @@ ;; under the pre-allocated ids the changes reference (copy-media-rows! conn branch-file-id fresh-map new-media) - (let [updated (-> branch-file + (let [applied (bm/remap-changes changes apply-map) + updated (-> branch-file (update :revn inc) - (update :data #(cpc/process-changes % changes))) + (update :data #(cpc/process-changes % applied))) libs (bfc/get-resolved-file-libraries cfg updated) errors (not-empty (cfv/validate-file updated libs)) updated (if errors @@ -1199,7 +1246,7 @@ :revn (:revn updated) :version (:version updated) :features (into-array (:features updated)) - :changes (blob/encode (vec changes))} + :changes (blob/encode (vec applied))} {::db/return-keys false}) ;; SQUASH: the new base (main's current state) diff --git a/backend/test/backend_tests/rpc_file_branch_oplog_test.clj b/backend/test/backend_tests/rpc_file_branch_oplog_test.clj index 39c4f0b226..ec37dfdd8e 100644 --- a/backend/test/backend_tests/rpc_file_branch_oplog_test.clj +++ b/backend/test/backend_tests/rpc_file_branch_oplog_test.clj @@ -14,6 +14,7 @@ (:require [app.common.features :as cfeat] [app.common.time :as ct] + [app.common.types.shape :as cts] [app.common.uuid :as uuid] [app.config :as cf] [app.rpc :as-alias rpc] @@ -208,3 +209,101 @@ (or (nil? deleted-at) (ct/is-after? deleted-at (ct/in-future {:days 400})))) rows))))))) + +(t/deftest update-from-main-compares-in-main-frame + ;; A branch stores no data: its document is the base snapshot plus the op + ;; log, so every reference it inherited names MAIN. A local reference is a + ;; self reference in both files, because a branch keeps main's colours and + ;; typographies under the same entity ids, so the comparison must run in + ;; main's frame and the change that lands in the branch must still name the + ;; branch. + (with-redefs [cf/flags (conj cf/flags :branching)] + (let [profile (th/create-profile* 1 {:is-active true}) + proj-id (:default-project-id profile) + file (th/create-file* 1 {:profile-id (:id profile) + :project-id proj-id + :is-shared false}) + main-id (:id file) + page-id (-> (th/command! {::th/type :get-file + ::rpc/profile-id (:id profile) + :id main-id}) + :result :data :pages first) + color-id (uuid/random) + shape-id (uuid/random) + fills (fn [ref-file color] + [{:fill-color color + :fill-opacity 1 + :fill-color-ref-id color-id + :fill-color-ref-file ref-file}])] + + ;; main publishes a colour of its own and one of its shapes uses it, so + ;; the shape carries main's file id in a self reference + (apply-change* profile main-id + {:type :add-color + :color {:id color-id :name "Brand" :color "#ff0000" :opacity 1}}) + (apply-change* profile main-id + {:type :add-obj + :page-id page-id + :id shape-id + :parent-id uuid/zero + :frame-id uuid/zero + :obj (-> (cts/setup-shape + {:id shape-id :name "Referenced" :type :rect + :parent-id uuid/zero :frame-id uuid/zero}) + (assoc :fills (fills main-id "#ff0000")))}) + + (let [create (create-branch* profile main-id "main-frame") + branch-id (:id create) + branch-file-id (:branch-file-id create)] + + (t/testing "main's repaint is not a conflict, and is not logged as a branch change" + (apply-change* profile main-id + {:type :mod-obj + :page-id page-id + :id shape-id + :operations [{:type :set :attr :fills :val (fills main-id "#00ff00")}]}) + (let [out (th/command! {::th/type :update-branch-from-main + ::rpc/profile-id (:id profile) + :branch-id branch-id})] + (t/is (nil? (:error out))) + (t/is (= :updated (-> out :result :status)))) + (let [out (th/command! {::th/type :get-file + ::rpc/profile-id (:id profile) + :id branch-file-id})] + (t/is (= "#00ff00" (get-in out [:result :data :pages-index page-id + :objects shape-id :fills 0 :fill-color])))) + ;; main's own change arrives through the repositioned base, so the + ;; net the update writes is empty. A ref left in main's frame would + ;; make this shape differ from the new base and put it in the log. + (t/is (empty? (oplog-rows branch-file-id)))) + + (t/testing "a genuine conflict still reports" + (apply-change* profile main-id + {:type :mod-obj + :page-id page-id + :id shape-id + :operations [{:type :set :attr :fills :val (fills main-id "#0000ff")}]}) + (apply-change* profile branch-file-id + {:type :mod-obj + :page-id page-id + :id shape-id + :operations [{:type :set :attr :fills :val (fills branch-file-id "#ffffff")}]}) + (let [out (th/command! {::th/type :update-branch-from-main + ::rpc/profile-id (:id profile) + :branch-id branch-id})] + (t/is (= :conflicts (-> out :result :status))) + (t/is (= 1 (count (-> out :result :conflicts)))) + (t/is (= shape-id (-> out :result :conflicts first :id))))) + + (t/testing "taking main keeps the branch writeable" + (let [out (th/command! {::th/type :update-branch-from-main + ::rpc/profile-id (:id profile) + :branch-id branch-id + :resolutions {shape-id :main}})] + (t/is (nil? (:error out))) + (t/is (= :updated (-> out :result :status)))) + (let [out (th/command! {::th/type :get-file + ::rpc/profile-id (:id profile) + :id branch-file-id})] + (t/is (= "#0000ff" (get-in out [:result :data :pages-index page-id + :objects shape-id :fills 0 :fill-color]))))))))) diff --git a/common/src/app/common/files/branch_merge.cljc b/common/src/app/common/files/branch_merge.cljc index be0f5a96d9..f2c11fbe2b 100644 --- a/common/src/app/common/files/branch_merge.cljc +++ b/common/src/app/common/files/branch_merge.cljc @@ -644,6 +644,57 @@ colors colors))))))) +(def ^:private reference-attrs + "Shape attrs whose value IS a reference to the file that owns the entity. + `relink-refs` finds them inside a shape map, where the attr and the value + travel together. An operation that sets one of them carries the id alone." + #{:component-file :fill-color-ref-file :stroke-color-ref-file :typography-ref-file}) + +(defn- remap-op + "Rewrite the reference of one `:mod-obj` operation through `id-map`. + `shape-set-ops` emits one `:set` per differing attr, so a reference can + arrive either as the attr's own value or, for an image shape, as the media + map under `:metadata`." + [op id-map] + (let [attr (:attr op) + val (:val op)] + (cond + (and (contains? reference-attrs attr) (uuid? val)) + (assoc op :val (get id-map val val)) + + (and (= :metadata attr) (uuid? (:id val))) + (assoc op :val (update val :id #(get id-map % %))) + + :else op))) + +(defn remap-changes + "Rewrite the cross-file references of a change vector through `id-map`, + the counterpart of `remap-refs` for the changes `compute-changes` emits. + A change computed in the source file's frame has to be applied in the + target's, because a reference is only a reference in the file it names: a + fill whose `:fill-color-ref-file` names another file loses its link, and so + does an instance whose `:component-file` does. + + Every payload `compute-changes` can emit is covered. `relink-refs` walks a + whole map, so `:obj`, `:page`, `:params`, and a component's `:objects` are + complete on their own. The two payloads it cannot see are the values that + ARE the reference: a media object's `:id` (its `:media-id` is a storage key + and must not be touched), and an operation value that is the reference + itself. Token changes are left alone: no id in `id-map` is a token id." + [changes id-map] + (if (empty? id-map) + changes + (let [lookup #(get id-map % %) + relink #(cfh/relink-refs % lookup)] + (mapv (fn [change] + (cond-> (relink change) + (uuid? (:id (:object change))) + (update-in [:object :id] lookup) + + (seq (:operations change)) + (update :operations #(mapv (fn [op] (remap-op op id-map)) %)))) + changes)))) + (defn- strip-modified-at "Remove the `:modified-at` bookkeeping timestamp from every entry of an indexed collection (components, colors, typographies).