mirror of
https://github.com/penpot/penpot.git
synced 2026-10-29 14:00:37 +00:00
🐛 Stop an update from main discarding a diverged branch's own work
Data loss, reproduced twice rather than inferred. A branch stores no data of its own, so its whole work lives in `file_branch_change`, and `update-branch-from-main` had a fast path that deleted every one of those rows whenever the update found nothing of main's to apply. The comment there assumed the branch was in sync with main, which holds only when the branch has no changes of its own, and `branch-diverged?` was already computed two lines above and consulted only inside `reposition-base!`. A branch with one fill edit of `#bb0000` and an untouched main, updated with no resolutions, returned `:updated` and came back `#000000` with an empty log. The user asked to catch up with a main that had not moved and lost their edit. Through the modal the same defect made the resolution that says "keep mine" the one that discards it. The fast path is now guarded by `(not branch-diverged?)`, so a diverged branch falls through to the squash that re-derives its net against the new base. The predicate is the right one: it compares the branch file's own revision counter against the revision the base was last positioned at, and a log row and a revision bump are one event, because `persist-branch-file!` is the log's only appender. The comment at the guard states the invariant, which is that the log may be emptied only when what it encodes already sits in the base. One correction to how the fast path was described while this was being chased: `(empty? changes)` does not mean the branch has nothing of its own. In the update direction the change set is MAIN's contribution, so a branch that is behind main in content already took the squash path and still does. The fast path is reached when main has nothing to contribute, which is what the third new test pins. The three sibling paths that also empty or replace a log were each judged and are sound, because each one writes the state elsewhere inside the same transaction before emptying. Three regression tests, one per reproduction plus the case the fast path exists for, asserting through the derived document rather than through log row counts, because a legitimate squash rewrites the log while preserving the state. Verified: the whole backend suite, 923 tests and 5,550 assertions with 0 failures. The two live demo branches were checked read-only and still hold their one op-log row each, so the hand-test fixture was never damaged.
This commit is contained in:
Álvaro Tejero Cantero
parent
a033a8e1e3
commit
5cb615ea88
@ -1198,11 +1198,16 @@
|
||||
(seq unsupported)
|
||||
(audited {:status :unsupported :kinds (vec unsupported)} tpoint :update-from-main)
|
||||
|
||||
(empty? changes)
|
||||
(and (empty? changes) (not branch-diverged?))
|
||||
(let [ts (ct/now)]
|
||||
;; the branch is in sync with main: the repositioned
|
||||
;; base already represents the branch, so the op log
|
||||
;; is emptied (old ops were built against the old base)
|
||||
;; is emptied (old ops were built against the old base).
|
||||
;; The log is a branch's only durable record, so it may
|
||||
;; be emptied only when what it encodes already sits in
|
||||
;; the base. A diverged branch holds its own work in
|
||||
;; that log, so it falls through to the squash below,
|
||||
;; which re-derives the net against the new base.
|
||||
(db/delete! conn :file-branch-change {:branch-id branch-id})
|
||||
(reposition-base! ts (:revn branch-file))
|
||||
(audited {:status :updated :revn (:revn branch-file)} tpoint :update-from-main))
|
||||
|
||||
@ -307,3 +307,335 @@
|
||||
:id branch-file-id})]
|
||||
(t/is (= "#0000ff" (get-in out [:result :data :pages-index page-id
|
||||
:objects shape-id :fills 0 :fill-color])))))))))
|
||||
|
||||
(t/deftest update-resolutions-are-document-keyed
|
||||
;; A resolution names a DOCUMENT, not the role the merge engine happened to
|
||||
;; hand it. In an update from main the engine runs with the branch as
|
||||
;; `theirs` and main as `ours` (`branch_merge.cljc::compute-merge*`), so
|
||||
;; `branch_merge.cljc::three-way-entities` writes the branch's value under
|
||||
;; the key named `:main` and main's value under the key named `:branch`.
|
||||
;; The client re-keys the conflicts it displays and sends resolutions in
|
||||
;; document terms; the command inverts them before applying
|
||||
;; (`files_branch.clj::update-branch-from-main`). This test pins that
|
||||
;; contract at the command boundary: `:main` brings main's value into the
|
||||
;; branch, `:branch` keeps the branch's value, and a per-attribute map
|
||||
;; obeys the same vocabulary for the attribute it names.
|
||||
;;
|
||||
;; Each fixture also gives main one non-conflicting change, and it has to:
|
||||
;; a resolution that keeps the branch's own side leaves the engine nothing
|
||||
;; to apply for that entity, and an update with an empty change set takes
|
||||
;; the command's no-op path (`files_branch.clj::update-branch-from-main`
|
||||
;; discards the branch's op log there and moves the merge base to main).
|
||||
;; Without a main-side change, every resolution would leave the branch
|
||||
;; carrying main's value, so the two `:main` cases below could pass while
|
||||
;; the resolution was ignored. Main's extra change keeps the write path
|
||||
;; exercised, and every case below fails if the mapping is inverted.
|
||||
(with-redefs [cf/flags (conj cf/flags :branching)]
|
||||
(let [profile (th/create-profile* 1 {:is-active true})
|
||||
proj-id (:default-project-id profile)
|
||||
branch-fill "#bb0000"
|
||||
main-fill "#00bb00"
|
||||
fills (fn [color] [{:fill-color color :fill-opacity 1}])
|
||||
fixture (fn [idx name]
|
||||
(let [file (th/create-file* idx {: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)
|
||||
shape-id (uuid/random)
|
||||
color-id (uuid/random)]
|
||||
|
||||
;; the shape enters the merge base before the fork
|
||||
(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 "Coded" :type :rect
|
||||
:parent-id uuid/zero :frame-id uuid/zero})
|
||||
(assoc :fills (fills "#000000")))})
|
||||
|
||||
(let [create (create-branch* profile main-id name)]
|
||||
;; the same attribute diverges on the two sides
|
||||
(apply-change* profile (:branch-file-id create)
|
||||
{:type :mod-obj
|
||||
:page-id page-id
|
||||
:id shape-id
|
||||
:operations [{:type :set :attr :fills
|
||||
:val (fills branch-fill)}]})
|
||||
(apply-change* profile main-id
|
||||
{:type :mod-obj
|
||||
:page-id page-id
|
||||
:id shape-id
|
||||
:operations [{:type :set :attr :fills
|
||||
:val (fills main-fill)}]})
|
||||
;; main also gains a change the branch never touched,
|
||||
;; so the update has something of main's to apply
|
||||
(apply-change* profile main-id
|
||||
{:type :add-color
|
||||
:color {:id color-id :name "Main-only"
|
||||
:color "#336699" :opacity 1}})
|
||||
{:branch-id (:id create)
|
||||
:branch-file-id (:branch-file-id create)
|
||||
:page-id page-id
|
||||
:shape-id shape-id})))
|
||||
fill-of (fn [{:keys [branch-file-id page-id shape-id]}]
|
||||
(get-in (th/command! {::th/type :get-file
|
||||
::rpc/profile-id (:id profile)
|
||||
:id branch-file-id})
|
||||
[:result :data :pages-index page-id :objects shape-id
|
||||
:fills 0 :fill-color]))
|
||||
update! (fn [{:keys [branch-id]} resolutions]
|
||||
(th/command! {::th/type :update-branch-from-main
|
||||
::rpc/profile-id (:id profile)
|
||||
:branch-id branch-id
|
||||
:resolutions resolutions}))]
|
||||
|
||||
(t/testing "a whole-entity :main resolution applies main's value"
|
||||
(let [fx (fixture 1 "res-doc-main")
|
||||
out (update! fx {(:shape-id fx) :main})]
|
||||
(t/is (nil? (:error out)))
|
||||
(t/is (= :updated (-> out :result :status)))
|
||||
(t/is (= main-fill (fill-of fx)))))
|
||||
|
||||
(t/testing "a whole-entity :branch resolution keeps the branch's value"
|
||||
(let [fx (fixture 2 "res-doc-branch")
|
||||
out (update! fx {(:shape-id fx) :branch})]
|
||||
(t/is (nil? (:error out)))
|
||||
(t/is (= :updated (-> out :result :status)))
|
||||
(t/is (= branch-fill (fill-of fx)))))
|
||||
|
||||
(t/testing "a per-attribute :main resolution applies main's value to that attribute"
|
||||
(let [fx (fixture 3 "res-doc-attr")
|
||||
out (update! fx {(:shape-id fx) {:fills :main}})]
|
||||
(t/is (nil? (:error out)))
|
||||
(t/is (= :updated (-> out :result :status)))
|
||||
(t/is (= main-fill (fill-of fx))))))))
|
||||
|
||||
;; A branch stores no data payload, so its op log IS its work: an update
|
||||
;; from main that discards that log without having folded the work into
|
||||
;; the new base destroys it. `files_branch.clj::update-branch-from-main`
|
||||
;; has a cheap path for a branch that is already in sync with main, and
|
||||
;; the three tests below pin both directions of it. Two of them reproduce
|
||||
;; an observed loss, and the third pins the case the cheap path is for.
|
||||
|
||||
(t/deftest update-from-main-keeps-a-branch-edit-when-main-has-none
|
||||
;; Main holds nothing of its own here, so
|
||||
;; `branch_merge.cljc::compute-changes` returns an empty change set and
|
||||
;; the command reached its cheap path on that result alone. The branch's
|
||||
;; own edit lived only in the op log, so discarding the log reverted the
|
||||
;; shape to the base's fill. Removing this test lets that loss come back
|
||||
;; through the "main has nothing to say" door.
|
||||
(with-redefs [cf/flags (conj cf/flags :branching)]
|
||||
(let [profile (th/create-profile* 1 {:is-active true})
|
||||
proj-id (:default-project-id profile)
|
||||
branch-fill "#bb0000"
|
||||
fills (fn [color] [{:fill-color color :fill-opacity 1}])
|
||||
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)
|
||||
shape-id (uuid/random)]
|
||||
|
||||
;; the shape enters the base before the fork, so the branch inherits it
|
||||
(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 "Branch-only" :type :rect
|
||||
:parent-id uuid/zero :frame-id uuid/zero})
|
||||
(assoc :fills (fills "#000000")))})
|
||||
|
||||
(let [create (create-branch* profile main-id "fast-path-own-edit")
|
||||
branch-id (:id create)
|
||||
branch-file-id (:branch-file-id create)
|
||||
read-fill (fn []
|
||||
(get-in (th/command! {::th/type :get-file
|
||||
::rpc/profile-id (:id profile)
|
||||
:id branch-file-id})
|
||||
[:result :data :pages-index page-id :objects shape-id
|
||||
:fills 0 :fill-color]))]
|
||||
|
||||
;; the branch's only edit, and main never moves after the fork
|
||||
(apply-change* profile branch-file-id
|
||||
{:type :mod-obj
|
||||
:page-id page-id
|
||||
:id shape-id
|
||||
:operations [{:type :set :attr :fills :val (fills branch-fill)}]})
|
||||
(t/is (= branch-fill (read-fill)))
|
||||
|
||||
(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))))
|
||||
|
||||
(t/testing "the branch still derives its own edit from its log"
|
||||
(t/is (= branch-fill (read-fill))))))))
|
||||
|
||||
(t/deftest update-from-main-keeps-the-branch-on-a-branch-resolution
|
||||
;; A conflict resolved to the branch removes the entity from the change
|
||||
;; set, because the branch's value is already the one that must survive.
|
||||
;; Main's repaint is the only change in play, so the change set came out
|
||||
;; empty and the cheap path discarded the log that held the branch's own
|
||||
;; value. Removing this test lets the resolution path lose the branch's
|
||||
;; side while every other resolution case stays green.
|
||||
(with-redefs [cf/flags (conj cf/flags :branching)]
|
||||
(let [profile (th/create-profile* 1 {:is-active true})
|
||||
proj-id (:default-project-id profile)
|
||||
branch-fill "#bb0000"
|
||||
main-fill "#00bb00"
|
||||
fills (fn [color] [{:fill-color color :fill-opacity 1}])
|
||||
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)
|
||||
shape-id (uuid/random)]
|
||||
|
||||
(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 "Contested" :type :rect
|
||||
:parent-id uuid/zero :frame-id uuid/zero})
|
||||
(assoc :fills (fills "#000000")))})
|
||||
|
||||
(let [create (create-branch* profile main-id "fast-path-conflict")
|
||||
branch-id (:id create)
|
||||
branch-file-id (:branch-file-id create)
|
||||
read-fill (fn []
|
||||
(get-in (th/command! {::th/type :get-file
|
||||
::rpc/profile-id (:id profile)
|
||||
:id branch-file-id})
|
||||
[:result :data :pages-index page-id :objects shape-id
|
||||
:fills 0 :fill-color]))]
|
||||
|
||||
;; both sides repaint the same shape, and nothing else changes
|
||||
(apply-change* profile branch-file-id
|
||||
{:type :mod-obj
|
||||
:page-id page-id
|
||||
:id shape-id
|
||||
:operations [{:type :set :attr :fills :val (fills branch-fill)}]})
|
||||
(apply-change* profile main-id
|
||||
{:type :mod-obj
|
||||
:page-id page-id
|
||||
:id shape-id
|
||||
:operations [{:type :set :attr :fills :val (fills main-fill)}]})
|
||||
|
||||
(let [out (th/command! {::th/type :update-branch-from-main
|
||||
::rpc/profile-id (:id profile)
|
||||
:branch-id branch-id
|
||||
:resolutions {shape-id :branch}})]
|
||||
(t/is (nil? (:error out)))
|
||||
(t/is (= :updated (-> out :result :status))))
|
||||
|
||||
(t/testing "the resolution keeps the branch's value after the update"
|
||||
(t/is (= branch-fill (read-fill))))))))
|
||||
|
||||
(t/deftest update-from-main-catches-a-branch-with-no-work-up-cheaply
|
||||
;; The cheap path's own case: main advanced its revision, the branch
|
||||
;; holds no work of its own, and the engine finds nothing of main's to
|
||||
;; apply, so the repositioned base alone reproduces the branch's state.
|
||||
;; A branch that is behind main in CONTENT does not reach this path, for
|
||||
;; `branch_merge.cljc::compute-changes` reports main's own changes as the
|
||||
;; set the update must apply, and the command then takes the squash
|
||||
;; below. Removing this test lets the cheap path
|
||||
;; (`files_branch.clj::update-branch-from-main`) be widened or dropped
|
||||
;; until every such update pays for a branch snapshot, a branch revision,
|
||||
;; and a rewrite of an empty log.
|
||||
(with-redefs [cf/flags (conj cf/flags :branching)]
|
||||
(let [profile (th/create-profile* 1 {:is-active true})
|
||||
proj-id (:default-project-id profile)
|
||||
fills (fn [color] [{:fill-color color :fill-opacity 1}])
|
||||
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)
|
||||
shape-id (uuid/random)
|
||||
;; an edit that changes nothing: main's revision moves, its
|
||||
;; content does not, so the update has nothing of main's to apply
|
||||
bump-main! (fn []
|
||||
(let [f (th/db-get :file {:id main-id})]
|
||||
(th/command! {::th/type :update-file
|
||||
::rpc/profile-id (:id profile)
|
||||
:id main-id
|
||||
:session-id (uuid/random)
|
||||
:revn (:revn f)
|
||||
:vern (:vern f)
|
||||
:features cfeat/supported-features
|
||||
:changes []})))]
|
||||
|
||||
(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 "Inherited" :type :rect
|
||||
:parent-id uuid/zero :frame-id uuid/zero})
|
||||
(assoc :fills (fills "#000000")))})
|
||||
|
||||
(let [create (create-branch* profile main-id "fast-path-no-work")
|
||||
branch-id (:id create)
|
||||
branch-file-id (:branch-file-id create)
|
||||
revn-before (:revn (th/db-get :file {:id branch-file-id}))
|
||||
xlog-before (count (th/db-query :file-change {:file-id branch-file-id}))
|
||||
base-before (th/db-get :file-branch {:id branch-id})
|
||||
derive (fn [file-id]
|
||||
(-> (th/command! {::th/type :get-file
|
||||
::rpc/profile-id (:id profile)
|
||||
:id file-id})
|
||||
:result :data :pages-index))]
|
||||
|
||||
;; main moves on in revision terms, and the branch still holds no
|
||||
;; work of its own
|
||||
(bump-main!)
|
||||
|
||||
(t/testing "the branch is behind main before the update"
|
||||
(t/is (pos? (- (:revn (th/db-get :file {:id main-id}))
|
||||
(:base-revn base-before)))))
|
||||
|
||||
(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))))
|
||||
|
||||
(t/testing "the branch's derived state is main's"
|
||||
(t/is (= (derive main-id) (derive branch-file-id))))
|
||||
|
||||
(t/testing "the merge base moved to main's current revision"
|
||||
(let [base-after (th/db-get :file-branch {:id branch-id})
|
||||
main-file (th/db-get :file {:id main-id})]
|
||||
(t/is (= (:revn main-file) (:base-revn base-after)))
|
||||
(t/is (not= (:base-snapshot-id base-before) (:base-snapshot-id base-after)))))
|
||||
|
||||
(t/testing "the branch was not rewritten to catch up"
|
||||
(t/is (= revn-before (:revn (th/db-get :file {:id branch-file-id}))))
|
||||
(t/is (= xlog-before (count (th/db-query :file-change {:file-id branch-file-id})))))
|
||||
(t/testing "the branch still stores no data payload"
|
||||
(t/is (= 1 (count (stored-data-rows branch-file-id)))))))))
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user