mirror of
https://github.com/penpot/penpot.git
synced 2026-10-29 05:50:35 +00:00
🐛 Fix the defects the review of the branch commands found
The source notes on files_branch.clj listed 32 review findings. This commit fixes the 21 that are defects of the code; each fix carries a regression test that failed before it and passes after. Concurrency: merge, update from main, rename, archive and delete now re-read the branch row under the advisory lock and work off that row, so a concurrent merge or update can no longer leave a ten-year base pin behind or release a superseded base twice. Delete and archive take the branch file lock, create and reopen of a pull request refuse a branch that a concurrent merge closed, and create checks the branch quota under the lock. Merge, update and delete publish their msgbus messages only after the transaction commits. Merge, update and materialize re-validate after the repair: merge and update refuse a result that stays invalid, materialize logs it. The change log row carries the repair changes, and the three paths drop the cached file summary under :redis-cache. Contracts: a :conflict refusal answers 409 instead of 500, resolution values must be :main or :branch, and update from main accepts the expected-main-revn token the frontend now sends from a loaded diff. Smaller fixes: create refuses a blank name, writes no file_data row, and a rename reaches the branch file's own name. Deleting a kept merged branch keeps its merged status. get-branch-diff reports the creation time of the current base snapshot. Rename, archive and delete report their duration and outcome. An image used only by a library colour is copied into the target file. Dead fallbacks and dead deleted-at checks are gone. AI-assisted-by: claude-opus-5-5
This commit is contained in:
Álvaro Tejero Cantero
parent
ca07aa6e8a
commit
863c7cab1b
@ -992,6 +992,32 @@
|
||||
(fdata/upsert! cfg file-data-params)
|
||||
nil))
|
||||
|
||||
(defn insert-file-row!
|
||||
"Insert only the `file` row of a new file, without writing the
|
||||
`file_data` payload row. Used for branch creation: a branch stores no
|
||||
data, so the create step must skip `encode-file` (its pointer-map
|
||||
transform would write `file_data` rows for the branch) and the data
|
||||
upsert. Returns nil."
|
||||
[{:keys [::db/conn]} file & {:as opts}]
|
||||
(when (:migrations file)
|
||||
(fmigr/upsert-migrations! conn file))
|
||||
|
||||
(let [file-params (-> file
|
||||
(d/update-when :features into-array)
|
||||
(file->params))]
|
||||
(try
|
||||
(db/insert! conn :file
|
||||
file-params
|
||||
(assoc opts ::db/return-keys false))
|
||||
(catch org.postgresql.util.PSQLException cause
|
||||
(if (db/duplicate-key-error? cause)
|
||||
(ex/raise :type :not-found
|
||||
:code :object-not-found
|
||||
:hint "file already exists"
|
||||
:cause cause)
|
||||
(throw cause)))))
|
||||
nil)
|
||||
|
||||
(defn update-file-row!
|
||||
"Update only the `file` row of an existing file, without touching
|
||||
`file_data`. Used for branch file saves: a branch stores no data, so
|
||||
|
||||
@ -164,6 +164,11 @@
|
||||
{::yres/status 404
|
||||
::yres/body (ex-data err)})
|
||||
|
||||
(defmethod handle-error :conflict
|
||||
[err _ _]
|
||||
{::yres/status 409
|
||||
::yres/body (ex-data err)})
|
||||
|
||||
(defmethod handle-error :nitrate-unavailable
|
||||
[err request _]
|
||||
(binding [l/*context* (request->context request)]
|
||||
|
||||
@ -29,6 +29,7 @@
|
||||
[app.common.uuid :as uuid]
|
||||
[app.config :as cf]
|
||||
[app.db :as db]
|
||||
[app.db.sql :as-alias sql]
|
||||
[app.features.fdata :as fdata]
|
||||
[app.features.file-snapshots :as fsnap]
|
||||
[app.features.logical-deletion :as ldel]
|
||||
@ -224,9 +225,16 @@
|
||||
(defn- unpaired-media-rows
|
||||
"Full file_media_object rows of `file-id` that are used by `data` but have
|
||||
no counterpart in `pairs` — media added on this side since branching.
|
||||
They must be copied into the target file when the merge/update applies."
|
||||
They must be copied into the target file when the merge/update applies.
|
||||
|
||||
The used set is `cfh/collect-used-media` plus the library-color `:image`
|
||||
ids it does not see but `bm/remap-refs` rewrites."
|
||||
[cfg file-id data pairs]
|
||||
(let [used (into #{} (remove pairs) (cfh/collect-used-media data))]
|
||||
(let [used (into #{}
|
||||
(remove pairs)
|
||||
(into (cfh/collect-used-media data)
|
||||
(keep #(get-in % [:image :id]))
|
||||
(vals (:colors data))))]
|
||||
(if (seq used)
|
||||
(->> (db/exec! cfg [sql:media-rows-by-id
|
||||
(db/create-array (db/get-connection cfg) "uuid" used)])
|
||||
@ -328,9 +336,11 @@
|
||||
|
||||
(defn- delete-branch!
|
||||
"Logically delete a branch and its branch file (with the team's deletion
|
||||
delay), release the base snapshot, schedule the file object GC and notify
|
||||
the branch file's open clients. Must run inside a transaction."
|
||||
[{:keys [::mbus/msgbus ::db/conn] :as cfg}
|
||||
delay), release the base snapshot, schedule the file object GC and
|
||||
return the msgbus notification for the branch file's open clients —
|
||||
the caller publishes it, and only once the transaction has committed
|
||||
(`tx-run-notify!`). Must run inside a transaction."
|
||||
[{:keys [::db/conn] :as cfg}
|
||||
{:keys [id branch-file-id] :as branch}
|
||||
{:keys [profile-id session-id status]}]
|
||||
(let [team (teams/get-team conn :profile-id profile-id :file-id branch-file-id)
|
||||
@ -351,16 +361,23 @@
|
||||
(wrk/submit! {::db/conn conn
|
||||
::wrk/task :delete-object
|
||||
::wrk/params {:object :file :deleted-at dt :id branch-file-id}})
|
||||
(mbus/pub! msgbus
|
||||
:topic branch-file-id
|
||||
:message {:type :file-deleted
|
||||
:file-id branch-file-id
|
||||
:profile-id profile-id
|
||||
:session-id session-id})
|
||||
nil))
|
||||
{:topic branch-file-id
|
||||
:message {:type :file-deleted
|
||||
:file-id branch-file-id
|
||||
:profile-id profile-id
|
||||
:session-id session-id}}))
|
||||
|
||||
;; --- COMMAND: create-file-branch
|
||||
|
||||
(defn- check-branch-name!
|
||||
"Refuse a blank branch name. Shared by creation and rename so the API
|
||||
cannot create a branch the rename would refuse."
|
||||
[name]
|
||||
(when (str/blank? name)
|
||||
(ex/raise :type :validation
|
||||
:code :invalid-branch-name
|
||||
:hint "branch name cannot be blank")))
|
||||
|
||||
(def ^:private schema:create-file-branch
|
||||
[:map {:title "create-file-branch"}
|
||||
[:file-id ::sm/uuid]
|
||||
@ -380,6 +397,7 @@
|
||||
[:create-file-branch/global]]}
|
||||
[cfg {:keys [::rpc/profile-id file-id name description]}]
|
||||
(check-branching-enabled!)
|
||||
(check-branch-name! name)
|
||||
(files/check-edition-permissions! cfg profile-id file-id)
|
||||
|
||||
(let [tpoint (ct/tpoint)
|
||||
@ -392,14 +410,6 @@
|
||||
:hint "branches of branches are not supported"
|
||||
:file-id file-id))
|
||||
|
||||
(-> cfg
|
||||
(assoc ::quotes/profile-id profile-id)
|
||||
(assoc ::quotes/project-id (:project-id file-row))
|
||||
(assoc ::quotes/team-id (:team-id project))
|
||||
(assoc ::quotes/file-id file-id)
|
||||
(quotes/check! {::quotes/id ::quotes/branches-per-file}
|
||||
{::quotes/id ::quotes/branches-per-team}))
|
||||
|
||||
(db/tx-run! cfg
|
||||
(fn [{:keys [::db/conn] :as cfg}]
|
||||
(db/exec-one! conn ["SET CONSTRAINTS ALL DEFERRED"])
|
||||
@ -410,6 +420,20 @@
|
||||
;; base) are taken from the SAME state of main.
|
||||
(db/xact-lock! conn file-id)
|
||||
|
||||
;; The quota check runs INSIDE the transaction and
|
||||
;; AFTER the lock: two creators racing for the last
|
||||
;; branch slot serialize on the lock, and the loser's
|
||||
;; count must see the winner's committed row.
|
||||
;; `quotes/check!` reads through `::db/conn`, so it
|
||||
;; counts on this transaction's connection.
|
||||
(-> cfg
|
||||
(assoc ::quotes/profile-id profile-id)
|
||||
(assoc ::quotes/project-id (:project-id file-row))
|
||||
(assoc ::quotes/team-id (:team-id project))
|
||||
(assoc ::quotes/file-id file-id)
|
||||
(quotes/check! {::quotes/id ::quotes/branches-per-file}
|
||||
{::quotes/id ::quotes/branches-per-team}))
|
||||
|
||||
;; 1. Materialize the merge base: a system snapshot of
|
||||
;; main, kept long-lived so it is not pruned by the
|
||||
;; snapshot GC while the branch is open (it is
|
||||
@ -441,7 +465,7 @@
|
||||
|
||||
meta-id (uuid/next)]
|
||||
|
||||
(bfc/insert-file! cfg branch-file {::db/return-keys false})
|
||||
(bfc/insert-file-row! cfg branch-file {::db/return-keys false})
|
||||
|
||||
;; 3. Mark the new file row as a branch so it is
|
||||
;; hidden from the project/team file listings.
|
||||
@ -543,7 +567,7 @@
|
||||
`base-revn` — the two revns are independent counters and must never be
|
||||
mixed."
|
||||
[{:keys [branch-revn source-revn base-revn base-branch-revn]}]
|
||||
[(max 0 (- branch-revn (or base-branch-revn base-revn)))
|
||||
[(max 0 (- branch-revn base-branch-revn))
|
||||
(max 0 (- source-revn base-revn))])
|
||||
|
||||
(defn- branch-id-map
|
||||
@ -782,7 +806,7 @@
|
||||
[{:keys [::db/conn] :as cfg} {:keys [::rpc/profile-id branch-id direction]}]
|
||||
(check-branching-enabled!)
|
||||
(let [branch (db/get* conn :file-branch {:id branch-id})]
|
||||
(when (or (nil? branch) (some? (:deleted-at branch)))
|
||||
(when (nil? branch)
|
||||
(ex/raise :type :not-found
|
||||
:code :branch-not-found
|
||||
:hint "unable to find branch with the provided id"
|
||||
@ -803,6 +827,13 @@
|
||||
(:data branch-file)
|
||||
(branch-id-map cfg (:branch-file-id branch) (:source-file-id branch)))
|
||||
base-data (branch-base-data branch-file)
|
||||
;; when the CURRENT merge-base snapshot row was created: the
|
||||
;; base moves whenever `update-branch-from-main` repositions it,
|
||||
;; and a pinned base carries a future `deleted-at`
|
||||
base-at (:created-at (db/get* conn :file-change
|
||||
{:id (:base-snapshot-id branch)}
|
||||
{::sql/columns [:created-at]
|
||||
::db/remove-deleted false}))
|
||||
;; 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
|
||||
@ -810,17 +841,36 @@
|
||||
(when-let [pages (affected-pages log)]
|
||||
{:only-pages pages}))]
|
||||
;; `:meta` carries the "when" of each side so the resolution UI can show
|
||||
;; how recent main/branch are (base is pinned at branch creation), plus
|
||||
;; how recent main/branch are (base is the snapshot its
|
||||
;; `base-snapshot-id` currently points at), plus
|
||||
;; `:main-revn` so the client can do optimistic concurrency on merge
|
||||
;; (`expected-main-revn`). The last editor's identity is intentionally
|
||||
;; omitted: files do not store a reliable "modified-by".
|
||||
(-> (bm/compute-merge base-data main-data branch-data dir opts)
|
||||
(assoc :meta {:base-at (:created-at branch)
|
||||
(assoc :meta {:base-at base-at
|
||||
:main-at (:modified-at main-file)
|
||||
:branch-at (:modified-at branch-file)
|
||||
:main-revn (:revn main-file)})
|
||||
(audited tpoint :compare)))))
|
||||
|
||||
;; --- Helpers: post-commit notification
|
||||
;;
|
||||
;; `app.db` offers no post-commit hook and `db/tx-run!` has committed
|
||||
;; when it returns, so the messages a transaction collects in `messages`
|
||||
;; leave here, right after the commit: a client that reloads on one of
|
||||
;; them reads the committed state, and a rolled back transaction
|
||||
;; publishes nothing.
|
||||
|
||||
(defn- tx-run-notify!
|
||||
"Run `f` in a transaction, then publish the `messages` it collected (a
|
||||
volatile of `{:topic .. :message ..}` maps) and return its result."
|
||||
[cfg msgbus messages f]
|
||||
(let [result (db/tx-run! cfg f)]
|
||||
(run! (fn [{:keys [topic message]}]
|
||||
(mbus/pub! msgbus :topic topic :message message))
|
||||
@messages)
|
||||
result))
|
||||
|
||||
;; --- COMMAND: merge-file-branch
|
||||
|
||||
(def ^:private schema:merge-file-branch
|
||||
@ -829,12 +879,12 @@
|
||||
;; conflict resolutions keyed by entity id (uuid) or, for structural
|
||||
;; conflicts, a keyword id like :active-themes. A resolution is either a
|
||||
;; whole-entity choice (:main/:branch) or a per-attr map {attr -> side}.
|
||||
[:resolutions {:optional true} [:map-of :any [:or :keyword [:map-of :keyword :keyword]]]]
|
||||
[:resolutions {:optional true} [:map-of :any [:or [:enum :main :branch] [:map-of :keyword [:enum :main :branch]]]]]
|
||||
[:expected-main-revn {:optional true} ::sm/int]
|
||||
;; when false (the default) the branch and its file are logically
|
||||
;; deleted right after a successful merge, in the SAME transaction (so
|
||||
;; merged copies do not pile up); when true the branch is kept as
|
||||
;; "merged" and can be inspected/restored from the archived list.
|
||||
;; "merged", staying readable with status merged until it is deleted.
|
||||
[:keep-branch {:optional true} ::sm/boolean]])
|
||||
|
||||
(sv/defmethod ::merge-file-branch
|
||||
@ -857,7 +907,7 @@
|
||||
{:keys [::rpc/profile-id ::rpc/session-id branch-id resolutions expected-main-revn keep-branch]}]
|
||||
(check-branching-enabled!)
|
||||
(let [branch (db/get* cfg :file-branch {:id branch-id})]
|
||||
(when (or (nil? branch) (some? (:deleted-at branch)))
|
||||
(when (nil? branch)
|
||||
(ex/raise :type :not-found
|
||||
:code :branch-not-found
|
||||
:branch-id branch-id))
|
||||
@ -868,12 +918,15 @@
|
||||
|
||||
(let [tpoint (ct/tpoint)
|
||||
main-id (:source-file-id branch)
|
||||
branch-file-id (:branch-file-id branch)]
|
||||
branch-file-id (:branch-file-id branch)
|
||||
messages (volatile! [])]
|
||||
;; Only editors of main can integrate (same rule as Figma).
|
||||
(files/check-edition-permissions! cfg profile-id main-id)
|
||||
|
||||
(db/tx-run!
|
||||
(tx-run-notify!
|
||||
cfg
|
||||
msgbus
|
||||
messages
|
||||
(fn [{:keys [::db/conn] :as cfg}]
|
||||
;; Serialize against concurrent edits/merges on BOTH files (same
|
||||
;; advisory lock the normal update-file path takes; stable order
|
||||
@ -882,7 +935,19 @@
|
||||
;; the branch is marked merged (and possibly deleted) below.
|
||||
(run! (partial db/xact-lock! conn) (sort [main-id branch-file-id]))
|
||||
|
||||
(let [main-file (-> (bfc/get-file cfg main-id :realize? true)
|
||||
;; the row read before the lock is stale by now: a competing
|
||||
;; merge, delete or archive may have committed while these
|
||||
;; locks were taken, so everything below works off the row as
|
||||
;; it stands under the lock
|
||||
(let [branch (or (db/get* conn :file-branch {:id branch-id})
|
||||
(ex/raise :type :not-found
|
||||
:code :branch-not-found
|
||||
:branch-id branch-id))
|
||||
_ (when (not= "open" (:status branch))
|
||||
(ex/raise :type :validation
|
||||
:code :branch-not-open
|
||||
:branch-id branch-id))
|
||||
main-file (-> (bfc/get-file cfg main-id :realize? true)
|
||||
(check-file-size-limits! :merge))
|
||||
_ (-> (branch-log cfg branch-file-id)
|
||||
(check-oplog-depth-limit! branch-file-id :merge))
|
||||
@ -942,8 +1007,9 @@
|
||||
:status "merged"
|
||||
:deleted-at (ct/in-future delay)})
|
||||
(when-not keep-branch
|
||||
(delete-branch! cfg branch {:profile-id profile-id
|
||||
:session-id session-id}))))]
|
||||
(vswap! messages conj
|
||||
(delete-branch! cfg branch {:profile-id profile-id
|
||||
:session-id session-id})))))]
|
||||
(cond
|
||||
(seq unresolved)
|
||||
(audited {:status :conflicts :conflicts conflicts} tpoint :merge)
|
||||
@ -990,10 +1056,24 @@
|
||||
(update :data #(cpc/process-changes % changes)))
|
||||
libs (bfc/get-resolved-file-libraries cfg merged)
|
||||
errors (not-empty (cfv/validate-file merged libs))
|
||||
merged (if errors
|
||||
(update merged :data cpc/process-changes
|
||||
(cfr/repair-file merged libs errors))
|
||||
merged)]
|
||||
repair (if errors
|
||||
(cfr/repair-file merged libs errors)
|
||||
[])
|
||||
merged (cond-> merged
|
||||
(seq repair)
|
||||
(update :data cpc/process-changes repair))
|
||||
|
||||
;; the validator, not the merge, decides
|
||||
;; validity: the repaired result goes back
|
||||
;; to it, and whatever stays broken refuses
|
||||
;; the merge and rolls the transaction back
|
||||
errors (when errors
|
||||
(not-empty (cfv/validate-file merged libs)))]
|
||||
(when errors
|
||||
(ex/raise :type :validation
|
||||
:code :merge-result-invalid
|
||||
:hint "the merge result still has validation errors after the repair"
|
||||
:codes (into [] (comp (map :code) (distinct)) errors)))
|
||||
|
||||
;; Change log (xlog), GC-eligible after the delay.
|
||||
(db/insert! conn :file-change
|
||||
@ -1007,19 +1087,22 @@
|
||||
:revn (:revn merged)
|
||||
:version (:version merged)
|
||||
:features (into-array (:features merged))
|
||||
:changes (blob/encode (vec changes))}
|
||||
:changes (blob/encode (into (vec changes) repair))}
|
||||
{::db/return-keys false})
|
||||
|
||||
(fupd/persist-file! (assoc cfg ::fupd/timestamp ts) merged)
|
||||
|
||||
(when (contains? cf/flags :redis-cache)
|
||||
(fupd/invalidate-caches! cfg merged))
|
||||
|
||||
(finish-branch! ts)
|
||||
|
||||
(mbus/pub! msgbus
|
||||
:topic main-id
|
||||
:message {:type :file-merged
|
||||
:file-id main-id
|
||||
:session-id session-id
|
||||
:revn (:revn merged)})
|
||||
(vswap! messages conj
|
||||
{:topic main-id
|
||||
:message {:type :file-merged
|
||||
:file-id main-id
|
||||
:session-id session-id
|
||||
:revn (:revn merged)}})
|
||||
|
||||
(audited {:status :merged :revn (:revn merged) :source-file-id main-id}
|
||||
tpoint :merge))))))))))))))
|
||||
@ -1031,7 +1114,8 @@
|
||||
[:branch-id ::sm/uuid]
|
||||
;; resolutions in UI terms: id -> :main (take main) | :branch (keep
|
||||
;; branch) | {attr -> side} (per-attr)
|
||||
[:resolutions {:optional true} [:map-of :any [:or :keyword [:map-of :keyword :keyword]]]]])
|
||||
[:resolutions {:optional true} [:map-of :any [:or [:enum :main :branch] [:map-of :keyword [:enum :main :branch]]]]]
|
||||
[:expected-main-revn {:optional true} ::sm/int]])
|
||||
|
||||
(defn- persist-branch-update!
|
||||
"Persist the result of an update-from-main integration on a branch
|
||||
@ -1079,27 +1163,47 @@
|
||||
::climit/id [[:update-branch-from-main/by-profile ::rpc/profile-id]
|
||||
[:update-branch-from-main/global]]}
|
||||
[{:keys [::mbus/msgbus] :as cfg}
|
||||
{:keys [::rpc/profile-id ::rpc/session-id branch-id resolutions]}]
|
||||
{:keys [::rpc/profile-id ::rpc/session-id branch-id resolutions expected-main-revn]}]
|
||||
(check-branching-enabled!)
|
||||
(let [branch (db/get* cfg :file-branch {:id branch-id})]
|
||||
(when (or (nil? branch) (some? (:deleted-at branch)))
|
||||
(when (nil? branch)
|
||||
(ex/raise :type :not-found :code :branch-not-found :branch-id branch-id))
|
||||
(when (not= "open" (:status branch))
|
||||
(ex/raise :type :validation :code :branch-not-open :branch-id branch-id))
|
||||
|
||||
(let [tpoint (ct/tpoint)
|
||||
branch-file-id (:branch-file-id branch)
|
||||
main-id (:source-file-id branch)]
|
||||
main-id (:source-file-id branch)
|
||||
messages (volatile! [])]
|
||||
;; Editing the branch -> need edition permissions on the branch file.
|
||||
(files/check-edition-permissions! cfg profile-id branch-file-id)
|
||||
|
||||
(db/tx-run!
|
||||
(tx-run-notify!
|
||||
cfg
|
||||
msgbus
|
||||
messages
|
||||
(fn [{:keys [::db/conn] :as cfg}]
|
||||
(db/xact-lock! conn branch-file-id)
|
||||
|
||||
(let [main-file (-> (bfc/get-file cfg main-id :realize? true)
|
||||
;; the row read before the lock is stale by now: a competing
|
||||
;; merge, delete or archive may have committed while this lock
|
||||
;; was taken, so everything below works off the row as it
|
||||
;; stands under the lock
|
||||
(let [branch (or (db/get* conn :file-branch {:id branch-id})
|
||||
(ex/raise :type :not-found
|
||||
:code :branch-not-found
|
||||
:branch-id branch-id))
|
||||
_ (when (not= "open" (:status branch))
|
||||
(ex/raise :type :validation
|
||||
:code :branch-not-open
|
||||
:branch-id branch-id))
|
||||
main-file (-> (bfc/get-file cfg main-id :realize? true)
|
||||
(check-file-size-limits! :update-from-main))
|
||||
_ (when (and (some? expected-main-revn)
|
||||
(not= expected-main-revn (:revn main-file)))
|
||||
(ex/raise :type :conflict
|
||||
:code :file-modified
|
||||
:hint "main was modified, recompute the diff and retry"))
|
||||
_ (-> (branch-log cfg branch-file-id)
|
||||
(check-oplog-depth-limit! branch-file-id :update-from-main))
|
||||
branch-file (bfc/get-file cfg branch-file-id :realize? true)
|
||||
@ -1146,7 +1250,7 @@
|
||||
;; (revn moved since the base was last positioned)
|
||||
branch-diverged?
|
||||
(pos? (- (:revn branch-file)
|
||||
(or (:base-branch-revn branch) (:base-revn branch))))
|
||||
(:base-branch-revn branch)))
|
||||
|
||||
reposition-base!
|
||||
(fn [ts branch-revn]
|
||||
@ -1246,10 +1350,24 @@
|
||||
(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
|
||||
(update updated :data cpc/process-changes
|
||||
(cfr/repair-file updated libs errors))
|
||||
updated)]
|
||||
repair (if errors
|
||||
(cfr/repair-file updated libs errors)
|
||||
[])
|
||||
updated (cond-> updated
|
||||
(seq repair)
|
||||
(update :data cpc/process-changes repair))
|
||||
|
||||
;; the validator, not the update, decides
|
||||
;; validity: the repaired result goes back to
|
||||
;; it, and whatever stays broken refuses the
|
||||
;; update and rolls the transaction back
|
||||
errors (when errors
|
||||
(not-empty (cfv/validate-file updated libs)))]
|
||||
(when errors
|
||||
(ex/raise :type :validation
|
||||
:code :update-result-invalid
|
||||
:hint "the update result still has validation errors after the repair"
|
||||
:codes (into [] (comp (map :code) (distinct)) errors)))
|
||||
|
||||
(db/insert! conn :file-change
|
||||
{:id (uuid/next)
|
||||
@ -1262,7 +1380,7 @@
|
||||
:revn (:revn updated)
|
||||
:version (:version updated)
|
||||
:features (into-array (:features updated))
|
||||
:changes (blob/encode (vec applied))}
|
||||
:changes (blob/encode (into (vec applied) repair))}
|
||||
{::db/return-keys false})
|
||||
|
||||
;; SQUASH: the new base (main's current state)
|
||||
@ -1282,14 +1400,17 @@
|
||||
:hint "the update produces branch-only changes that cannot be replayed"
|
||||
:kinds (vec unsupported)))
|
||||
(persist-branch-update! cfg updated ts branch-id net-changes))
|
||||
|
||||
(when (contains? cf/flags :redis-cache)
|
||||
(fupd/invalidate-caches! cfg updated))
|
||||
(reposition-base! ts (:revn updated))
|
||||
|
||||
(mbus/pub! msgbus
|
||||
:topic branch-file-id
|
||||
:message {:type :file-merged
|
||||
:file-id branch-file-id
|
||||
:session-id session-id
|
||||
:revn (:revn updated)})
|
||||
(vswap! messages conj
|
||||
{:topic branch-file-id
|
||||
:message {:type :file-merged
|
||||
:file-id branch-file-id
|
||||
:session-id session-id
|
||||
:revn (:revn updated)}})
|
||||
|
||||
(audited {:status :updated :revn (:revn updated)}
|
||||
tpoint :update-from-main)))))))))))))
|
||||
@ -1348,14 +1469,30 @@
|
||||
|
||||
(let [libs (bfc/get-resolved-file-libraries cfg file)
|
||||
errors (not-empty (cfv/validate-file file libs))
|
||||
file (if errors
|
||||
(update file :data cpc/process-changes
|
||||
(cfr/repair-file file libs errors))
|
||||
file)]
|
||||
repair (if errors
|
||||
(cfr/repair-file file libs errors)
|
||||
[])
|
||||
file (cond-> file
|
||||
(seq repair)
|
||||
(update :data cpc/process-changes repair))
|
||||
|
||||
;; the exit door never strands a user: the repaired
|
||||
;; result goes back to the validator, and whatever
|
||||
;; stays broken is logged, never refused
|
||||
errors (when errors
|
||||
(not-empty (cfv/validate-file file libs)))]
|
||||
|
||||
(when errors
|
||||
(l/wrn :hint "materialised file still has validation errors"
|
||||
:file-id (str file-id)
|
||||
:codes (into [] (comp (map :code) (distinct)) errors)))
|
||||
|
||||
;; the derived state becomes the file's own payload
|
||||
(fupd/persist-file! (assoc cfg ::fupd/timestamp ts) file)
|
||||
|
||||
(when (contains? cf/flags :redis-cache)
|
||||
(fupd/invalidate-caches! cfg file))
|
||||
|
||||
;; from here the ordinary paths apply
|
||||
(db/update! conn :file
|
||||
{:is-branch false}
|
||||
@ -1469,23 +1606,34 @@
|
||||
::db/transaction true}
|
||||
[{:keys [::db/conn] :as cfg} {:keys [::rpc/profile-id id name description]}]
|
||||
(check-branching-enabled!)
|
||||
(when (and (some? name) (str/blank? name))
|
||||
(ex/raise :type :validation
|
||||
:code :invalid-branch-name
|
||||
:hint "branch name cannot be blank"))
|
||||
(let [branch (db/get* conn :file-branch {:id id})]
|
||||
(when (or (nil? branch) (some? (:deleted-at branch)))
|
||||
(when (some? name)
|
||||
(check-branch-name! name))
|
||||
(let [tpoint (ct/tpoint)
|
||||
branch (db/get* conn :file-branch {:id id})]
|
||||
(when (nil? branch)
|
||||
(ex/raise :type :not-found :code :branch-not-found :branch-id id))
|
||||
(files/check-edition-permissions! cfg profile-id (:branch-file-id branch))
|
||||
(db/update! conn :file-branch
|
||||
(cond-> {:updated-at (ct/now)}
|
||||
(some? name) (assoc :name name)
|
||||
(some? description) (assoc :description description))
|
||||
{:id id}
|
||||
{::db/return-keys false})
|
||||
{:id id
|
||||
:name (or name (:name branch))
|
||||
:description (or description (:description branch))}))
|
||||
;; Serialize against a merge of the same branch (which locks the
|
||||
;; branch file) and re-read the row under the lock: the branch may
|
||||
;; be merged and gone by now.
|
||||
(db/xact-lock! conn (:branch-file-id branch))
|
||||
(let [branch (db/get* conn :file-branch {:id id})]
|
||||
(when (nil? branch)
|
||||
(ex/raise :type :not-found :code :branch-not-found :branch-id id))
|
||||
(db/update! conn :file-branch
|
||||
(cond-> {:updated-at (ct/now)}
|
||||
(some? name) (assoc :name name)
|
||||
(some? description) (assoc :description description))
|
||||
{:id id}
|
||||
{::db/return-keys false})
|
||||
;; the branch file carries its own name (set from the branch name
|
||||
;; at creation), so a rename has to reach the file row too
|
||||
(when (some? name)
|
||||
(files/rename-file conn {:id (:branch-file-id branch) :name name}))
|
||||
(audited {:id id
|
||||
:name (or name (:name branch))
|
||||
:description (or description (:description branch))}
|
||||
tpoint :rename-branch))))
|
||||
|
||||
;; --- COMMAND: archive-file-branch
|
||||
|
||||
@ -1501,27 +1649,36 @@
|
||||
::db/transaction true}
|
||||
[{:keys [::db/conn] :as cfg} {:keys [::rpc/profile-id id archived]}]
|
||||
(check-branching-enabled!)
|
||||
(let [branch (db/get* conn :file-branch {:id id})]
|
||||
(when (or (nil? branch) (some? (:deleted-at branch)))
|
||||
(let [tpoint (ct/tpoint)
|
||||
branch (db/get* conn :file-branch {:id id})]
|
||||
(when (nil? branch)
|
||||
(ex/raise :type :not-found :code :branch-not-found :branch-id id))
|
||||
(when (= "merged" (:status branch))
|
||||
(ex/raise :type :validation :code :branch-merged :branch-id id))
|
||||
(files/check-edition-permissions! cfg profile-id (:branch-file-id branch))
|
||||
(let [status (if (false? archived) "open" "archived")]
|
||||
(db/update! conn :file-branch
|
||||
{:status status :updated-at (ct/now)}
|
||||
{:id id}
|
||||
{::db/return-keys false})
|
||||
;; archiving hides the branch from the default flow, so its review
|
||||
;; sandbox has no reason to stay open either
|
||||
(when (= "archived" status)
|
||||
(let [team (teams/get-team conn :profile-id profile-id
|
||||
:file-id (:branch-file-id branch))
|
||||
dt (ct/in-future (ldel/get-deletion-delay team))]
|
||||
(close-branch-pull-requests! cfg id {:profile-id profile-id
|
||||
:status "closed"
|
||||
:deleted-at dt})))
|
||||
{:id id :status status})))
|
||||
;; Serialize against a merge of the same branch (which locks the
|
||||
;; branch file) and re-read the row under the lock: the branch may
|
||||
;; be merged and gone by now.
|
||||
(db/xact-lock! conn (:branch-file-id branch))
|
||||
(let [branch (db/get* conn :file-branch {:id id})]
|
||||
(when (nil? branch)
|
||||
(ex/raise :type :not-found :code :branch-not-found :branch-id id))
|
||||
(when (= "merged" (:status branch))
|
||||
(ex/raise :type :validation :code :branch-merged :branch-id id))
|
||||
(let [status (if (false? archived) "open" "archived")]
|
||||
(db/update! conn :file-branch
|
||||
{:status status :updated-at (ct/now)}
|
||||
{:id id}
|
||||
{::db/return-keys false})
|
||||
;; archiving hides the branch from the default flow, so its review
|
||||
;; sandbox has no reason to stay open either
|
||||
(when (= "archived" status)
|
||||
(let [team (teams/get-team conn :profile-id profile-id
|
||||
:file-id (:branch-file-id branch))
|
||||
dt (ct/in-future (ldel/get-deletion-delay team))]
|
||||
(close-branch-pull-requests! cfg id {:profile-id profile-id
|
||||
:status "closed"
|
||||
:deleted-at dt})))
|
||||
(audited {:id id :status status} tpoint
|
||||
(if (false? archived) :restore-branch :archive-branch))))))
|
||||
|
||||
;; --- COMMAND: delete-file-branch
|
||||
|
||||
@ -1536,16 +1693,30 @@
|
||||
{::doc/added "2.16"
|
||||
::webhooks/event? true
|
||||
::sm/params schema:delete-file-branch}
|
||||
[cfg {:keys [::rpc/profile-id ::rpc/session-id id]}]
|
||||
[{:keys [::mbus/msgbus] :as cfg} {:keys [::rpc/profile-id ::rpc/session-id id]}]
|
||||
(check-branching-enabled!)
|
||||
(let [branch (db/get* cfg :file-branch {:id id})]
|
||||
(when (or (nil? branch) (some? (:deleted-at branch)))
|
||||
(let [tpoint (ct/tpoint)
|
||||
branch (db/get* cfg :file-branch {:id id})
|
||||
messages (volatile! [])]
|
||||
(when (nil? branch)
|
||||
(ex/raise :type :not-found :code :branch-not-found :branch-id id))
|
||||
(files/check-edition-permissions! cfg profile-id (:branch-file-id branch))
|
||||
(db/tx-run!
|
||||
(tx-run-notify!
|
||||
cfg
|
||||
(fn [cfg]
|
||||
(delete-branch! cfg branch {:profile-id profile-id
|
||||
:session-id session-id
|
||||
:status "archived"})
|
||||
{:status :deleted}))))
|
||||
msgbus
|
||||
messages
|
||||
(fn [{:keys [::db/conn] :as cfg}]
|
||||
;; Serialize against a merge of the same branch (which locks both
|
||||
;; files): without the lock a delete can commit while the merge
|
||||
;; runs, and the merge then deletes the branch a second time. The
|
||||
;; row is re-read under the lock: the branch may be gone by now.
|
||||
(db/xact-lock! conn (:branch-file-id branch))
|
||||
(let [branch (db/get* conn :file-branch {:id id})]
|
||||
(when (nil? branch)
|
||||
(ex/raise :type :not-found :code :branch-not-found :branch-id id))
|
||||
;; no `:status` here: a delete keeps the status the row carries
|
||||
;; (deleting a kept merged branch must not rewrite "merged")
|
||||
(vswap! messages conj
|
||||
(delete-branch! cfg branch {:profile-id profile-id
|
||||
:session-id session-id}))
|
||||
(audited {:status :deleted} tpoint :delete-branch))))))
|
||||
|
||||
@ -261,7 +261,7 @@
|
||||
:code :invalid-pull-request-title
|
||||
:hint "pull request title cannot be blank"))
|
||||
(let [branch (db/get* cfg :file-branch {:id branch-id})]
|
||||
(when (or (nil? branch) (some? (:deleted-at branch)))
|
||||
(when (nil? branch)
|
||||
(ex/raise :type :not-found
|
||||
:code :branch-not-found
|
||||
:branch-id branch-id))
|
||||
@ -298,64 +298,81 @@
|
||||
;; concurrent create-pull-request on the same branch.
|
||||
(db/xact-lock! conn branch-file-id)
|
||||
|
||||
(when (db/exec-one! conn [sql:get-open-pull-request-for-branch branch-id])
|
||||
(ex/raise :type :validation
|
||||
:code :pull-request-already-exists
|
||||
:hint "the branch already has an open pull request"
|
||||
:branch-id branch-id))
|
||||
;; Serialize against a merge of the same branch (which locks
|
||||
;; the branch file) and re-read the row under the lock: the
|
||||
;; branch may be merged and gone by now, so the stale row read
|
||||
;; before the transaction is dropped and everything below
|
||||
;; derives from the fresh one.
|
||||
(let [branch (db/get* conn :file-branch {:id branch-id})
|
||||
branch-file-id (:branch-file-id branch)
|
||||
target-id (:source-file-id branch)]
|
||||
(when (nil? branch)
|
||||
(ex/raise :type :not-found
|
||||
:code :branch-not-found
|
||||
:branch-id branch-id))
|
||||
(when (not= "open" (:status branch))
|
||||
(ex/raise :type :validation
|
||||
:code :branch-not-open
|
||||
:branch-id branch-id))
|
||||
|
||||
(let [branch-file (bfc/get-file cfg branch-file-id :realize? true)
|
||||
snapshot (pin-review-snapshot! cfg branch-file title profile-id)
|
||||
pr-id (uuid/next)
|
||||
ts (ct/now)]
|
||||
(when (db/exec-one! conn [sql:get-open-pull-request-for-branch branch-id])
|
||||
(ex/raise :type :validation
|
||||
:code :pull-request-already-exists
|
||||
:hint "the branch already has an open pull request"
|
||||
:branch-id branch-id))
|
||||
|
||||
(db/insert! conn :file-pull-request
|
||||
{:id pr-id
|
||||
:file-branch-id branch-id
|
||||
:source-file-id branch-file-id
|
||||
:target-file-id target-id
|
||||
:title title
|
||||
:description description
|
||||
:created-by profile-id
|
||||
:status "open"
|
||||
:review-snapshot-id (:id snapshot)
|
||||
:review-revn (:revn branch-file)
|
||||
:review-updated-at ts}
|
||||
{::db/return-keys false})
|
||||
(let [branch-file (bfc/get-file cfg branch-file-id :realize? true)
|
||||
snapshot (pin-review-snapshot! cfg branch-file title profile-id)
|
||||
pr-id (uuid/next)
|
||||
ts (ct/now)]
|
||||
|
||||
(doseq [reviewer-id reviewers]
|
||||
(db/insert! conn :file-pull-request-review
|
||||
{:pull-request-id pr-id
|
||||
:profile-id reviewer-id}
|
||||
{::db/return-keys false}))
|
||||
(db/insert! conn :file-pull-request
|
||||
{:id pr-id
|
||||
:file-branch-id branch-id
|
||||
:source-file-id branch-file-id
|
||||
:target-file-id target-id
|
||||
:title title
|
||||
:description description
|
||||
:created-by profile-id
|
||||
:status "open"
|
||||
:review-snapshot-id (:id snapshot)
|
||||
:review-revn (:revn branch-file)
|
||||
:review-updated-at ts}
|
||||
{::db/return-keys false})
|
||||
|
||||
(send-review-request-emails! conn
|
||||
{:id pr-id
|
||||
:source-file-id branch-file-id
|
||||
:title title
|
||||
:description description}
|
||||
{:team-id team-id
|
||||
:actor-id profile-id
|
||||
:branch-name (:name branch)
|
||||
:target-name (:name target-row)
|
||||
:reviewers reviewers})
|
||||
(doseq [reviewer-id reviewers]
|
||||
(db/insert! conn :file-pull-request-review
|
||||
{:pull-request-id pr-id
|
||||
:profile-id reviewer-id}
|
||||
{::db/return-keys false}))
|
||||
|
||||
(mbus/pub! msgbus
|
||||
:topic target-id
|
||||
:message {:type :pull-request-created
|
||||
:file-id target-id
|
||||
:pull-request-id pr-id
|
||||
:profile-id profile-id})
|
||||
(send-review-request-emails! conn
|
||||
{:id pr-id
|
||||
:source-file-id branch-file-id
|
||||
:title title
|
||||
:description description}
|
||||
{:team-id team-id
|
||||
:actor-id profile-id
|
||||
:branch-name (:name branch)
|
||||
:target-name (:name target-row)
|
||||
:reviewers reviewers})
|
||||
|
||||
{:id pr-id
|
||||
:file-branch-id branch-id
|
||||
:source-file-id branch-file-id
|
||||
:target-file-id target-id
|
||||
:title title
|
||||
:description description
|
||||
:status "open"
|
||||
:review-revn (:revn branch-file)
|
||||
:reviewers reviewers})))))))
|
||||
(mbus/pub! msgbus
|
||||
:topic target-id
|
||||
:message {:type :pull-request-created
|
||||
:file-id target-id
|
||||
:pull-request-id pr-id
|
||||
:profile-id profile-id})
|
||||
|
||||
{:id pr-id
|
||||
:file-branch-id branch-id
|
||||
:source-file-id branch-file-id
|
||||
:target-file-id target-id
|
||||
:title title
|
||||
:description description
|
||||
:status "open"
|
||||
:review-revn (:revn branch-file)
|
||||
:reviewers reviewers}))))))))
|
||||
|
||||
;; --- COMMAND QUERY: get-file-pull-requests
|
||||
|
||||
@ -813,7 +830,6 @@
|
||||
|
||||
(let [branch (db/get* cfg :file-branch {:id (:file-branch-id pr)})]
|
||||
(when (or (nil? branch)
|
||||
(some? (:deleted-at branch))
|
||||
(not= "open" (:status branch)))
|
||||
(ex/raise :type :validation
|
||||
:code :branch-not-open
|
||||
@ -825,6 +841,17 @@
|
||||
(fn [{:keys [::db/conn] :as cfg}]
|
||||
(db/xact-lock! conn (:source-file-id pr))
|
||||
|
||||
;; Serialize against a merge of the same branch (which locks the
|
||||
;; branch file) and re-read the row under the lock: the branch
|
||||
;; may be merged and gone by now.
|
||||
(let [branch (db/get* conn :file-branch {:id (:file-branch-id pr)})]
|
||||
(when (or (nil? branch)
|
||||
(not= "open" (:status branch)))
|
||||
(ex/raise :type :validation
|
||||
:code :branch-not-open
|
||||
:hint "the pull request's branch is no longer open"
|
||||
:pull-request-id id)))
|
||||
|
||||
(when (db/exec-one! conn [sql:get-open-pull-request-for-branch (:file-branch-id pr)])
|
||||
(ex/raise :type :validation
|
||||
:code :pull-request-already-exists
|
||||
|
||||
@ -45,12 +45,12 @@
|
||||
(declare ^:private update-file*)
|
||||
(declare ^:private process-changes-and-validate)
|
||||
(declare ^:private take-snapshot?)
|
||||
(declare ^:private invalidate-caches!)
|
||||
|
||||
;; PUBLIC API; intended to be used outside of this module
|
||||
(declare update-file!)
|
||||
(declare update-file-data!)
|
||||
(declare persist-file!)
|
||||
(declare invalidate-caches!)
|
||||
(declare ^:private persist-branch-file!)
|
||||
(declare get-file)
|
||||
|
||||
@ -400,7 +400,9 @@
|
||||
nil))
|
||||
|
||||
|
||||
(defn- invalidate-caches!
|
||||
(defn invalidate-caches!
|
||||
"Drop the cached library summary of `file`. Every write path that
|
||||
persists a file calls it under the `:redis-cache` flag."
|
||||
[cfg {:keys [id] :as file}]
|
||||
(rds/run! cfg (fn [{:keys [::rds/conn]}]
|
||||
(let [key (files/file-summary-cache-key id)]
|
||||
|
||||
@ -669,3 +669,18 @@
|
||||
(t/is (= "safe user-facing hint" (:hint body)))
|
||||
(t/is (nil? (:state body)))
|
||||
(t/is (nil? (:path body)))))
|
||||
|
||||
(t/deftest conflict-error-answers-409
|
||||
;; A `:type :conflict` refusal (`:file-modified` on merge/update-from-main,
|
||||
;; `:file-locked` and restore `:file-modified` on snapshots) answers 409
|
||||
;; carrying its own ex-data; without a `:conflict` method it fell to the
|
||||
;; default handler: 500 with `:type :server-error`, logged as unhandled.
|
||||
(let [cause (ex-info "conflict"
|
||||
{:type :conflict
|
||||
:code :file-modified
|
||||
:hint "main was modified, recompute the diff and retry"})
|
||||
response (http-errors/handle cause {})
|
||||
body (::yres/body response)]
|
||||
(t/is (= 409 (::yres/status response)))
|
||||
(t/is (= :conflict (:type body)))
|
||||
(t/is (= :file-modified (:code body)))))
|
||||
|
||||
@ -17,6 +17,7 @@
|
||||
[app.common.types.shape :as cts]
|
||||
[app.common.uuid :as uuid]
|
||||
[app.config :as cf]
|
||||
[app.db :as db]
|
||||
[app.rpc :as-alias rpc]
|
||||
[app.util.blob :as blob]
|
||||
[backend-tests.helpers :as th]
|
||||
@ -64,9 +65,7 @@
|
||||
branch-file-id (:branch-file-id create)]
|
||||
|
||||
(t/testing "the branch file stores no real data"
|
||||
(let [rows (stored-data-rows branch-file-id)]
|
||||
(t/is (= 1 (count rows)))
|
||||
(t/is (empty? (-> rows first :data blob/decode :pages)))))
|
||||
(t/is (empty? (stored-data-rows branch-file-id))))
|
||||
|
||||
(t/testing "the op log starts empty"
|
||||
(t/is (empty? (oplog-rows branch-file-id))))
|
||||
@ -106,9 +105,7 @@
|
||||
(blob/decode (:changes (first rows)))))))
|
||||
|
||||
(t/testing "the stored data payload never grew"
|
||||
(let [rows (stored-data-rows branch-file-id)]
|
||||
(t/is (= 1 (count rows)))
|
||||
(t/is (empty? (-> rows first :data blob/decode :pages)))))
|
||||
(t/is (empty? (stored-data-rows branch-file-id))))
|
||||
|
||||
(t/testing "the derived state carries the branch edit"
|
||||
(let [out (th/command! {::th/type :get-file
|
||||
@ -638,4 +635,118 @@
|
||||
(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)))))))))
|
||||
(t/is (empty? (stored-data-rows branch-file-id))))))))
|
||||
|
||||
;;; --- Regression tests
|
||||
|
||||
(t/deftest create-checks-the-branch-quota-inside-the-lock
|
||||
;; Two designers racing for the LAST branch slot of one file: the
|
||||
;; loser is refused only if its quota count runs inside the
|
||||
;; transaction, after the advisory lock, where it sees the winner's
|
||||
;; committed branch. A count taken outside the lock lets both creates
|
||||
;; read "one slot left" and both insert.
|
||||
(with-redefs [cf/flags (conj cf/flags :branching)]
|
||||
(let [profile1 (th/create-profile* 1 {:is-active true})
|
||||
profile2 (th/create-profile* 2 {:is-active true})
|
||||
proj-id (:default-project-id profile1)
|
||||
file (th/create-file* 1 {:profile-id (:id profile1)
|
||||
:project-id proj-id
|
||||
:is-shared false})
|
||||
file-id (:id file)
|
||||
;; the racer is a second designer: the per-profile climit
|
||||
;; serializes one designer's creates against themselves
|
||||
_ (th/create-file-role* {:file-id file-id
|
||||
:profile-id (:id profile2)
|
||||
:role :editor})
|
||||
real-lock db/xact-lock!
|
||||
raced? (atom false)]
|
||||
|
||||
(with-redefs [cf/get (th/config-get-mock {:quotes-branches-per-file 1})
|
||||
db/xact-lock!
|
||||
(fn [conn n]
|
||||
;; on first entry into the transaction the racer
|
||||
;; runs to completion (its own transaction, its own
|
||||
;; connection, committed) BEFORE the outer create
|
||||
;; takes the advisory lock
|
||||
(when (compare-and-set! raced? false true)
|
||||
(t/is (nil? (:error (th/command!
|
||||
{::th/type :create-file-branch
|
||||
::rpc/profile-id (:id profile2)
|
||||
:file-id file-id
|
||||
:name "racer"})))))
|
||||
(real-lock conn n))]
|
||||
(let [out (th/command! {::th/type :create-file-branch
|
||||
::rpc/profile-id (:id profile1)
|
||||
:file-id file-id
|
||||
:name "outer"})
|
||||
data (ex-data (:error out))
|
||||
rows (th/db-query :file-branch {:source-file-id file-id})]
|
||||
|
||||
(t/testing "the loser is refused with the quota error"
|
||||
(t/is (some? (:error out)))
|
||||
(t/is (= :restriction (:type data)))
|
||||
(t/is (= :max-quote-reached (:code data))))
|
||||
|
||||
(t/testing "exactly the racer's branch exists"
|
||||
(t/is (= 1 (count rows)))
|
||||
(t/is (= (:id profile2) (:created-by (first rows))))))))))
|
||||
|
||||
(t/deftest create-refuses-a-blank-branch-name
|
||||
;; `::update-file-branch` refuses a blank name; creation used to
|
||||
;; accept one, leaving behind a branch the rename would reject.
|
||||
(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})
|
||||
out (th/command! {::th/type :create-file-branch
|
||||
::rpc/profile-id (:id profile)
|
||||
:file-id (:id file)
|
||||
:name " "})
|
||||
data (ex-data (:error out))]
|
||||
(t/is (some? (:error out)))
|
||||
(t/is (= :validation (:type data)))
|
||||
(t/is (= :invalid-branch-name (:code data)))
|
||||
(t/is (empty? (th/db-query :file-branch {:source-file-id (:id file)}))))))
|
||||
|
||||
(t/deftest create-writes-no-file-data-row
|
||||
;; The create docstring promises a branch file with NO data payload,
|
||||
;; and `pp:vcs:eht-derived-branch-never-persists-data` forbids the
|
||||
;; row: a payload on a branch file invites a third writer into a
|
||||
;; derived state that already has two. Only materializing the branch
|
||||
;; into an ordinary file creates it.
|
||||
(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})
|
||||
create (create-branch* profile (:id file) "no-payload")
|
||||
branch-file-id (:branch-file-id create)
|
||||
color-id (uuid/random)]
|
||||
|
||||
(t/testing "creation writes no file_data row at all"
|
||||
(t/is (empty? (th/db-query :file-data {:file-id branch-file-id}))))
|
||||
|
||||
(t/testing "the branch still saves to its op log, without a payload"
|
||||
(apply-change* profile branch-file-id
|
||||
{:type :add-color
|
||||
:color {:id color-id :name "C" :color "#112233" :opacity 1}})
|
||||
(t/is (= 1 (count (oplog-rows branch-file-id))))
|
||||
(t/is (empty? (stored-data-rows branch-file-id))))
|
||||
|
||||
(t/testing "the branch still reads"
|
||||
(let [out (th/command! {::th/type :get-file
|
||||
::rpc/profile-id (:id profile)
|
||||
:id branch-file-id})]
|
||||
(t/is (nil? (:error out)))
|
||||
(t/is (contains? (-> out :result :data :colors) color-id))))
|
||||
|
||||
(t/testing "materialize is what creates the payload row"
|
||||
(let [out (th/command! {::th/type :materialize-file-branch
|
||||
::rpc/profile-id (:id profile)
|
||||
:file-id branch-file-id})]
|
||||
(t/is (nil? (:error out)))
|
||||
(t/is (= :materialized (-> out :result :status))))
|
||||
(t/is (some? (th/db-get :file-data {:file-id branch-file-id :type "main"})))))))
|
||||
|
||||
File diff suppressed because it is too large
Load Diff
@ -11,6 +11,7 @@
|
||||
[app.common.time :as ct]
|
||||
[app.common.uuid :as uuid]
|
||||
[app.config :as cf]
|
||||
[app.db :as db]
|
||||
[app.rpc :as-alias rpc]
|
||||
[backend-tests.helpers :as th]
|
||||
[clojure.string :as str]
|
||||
@ -511,6 +512,152 @@
|
||||
(let [[row] (th/db-query :file-pull-request {:id (:id pr)})]
|
||||
(t/is (= "closed" (:status row)))))))))
|
||||
|
||||
(t/deftest create-serializes-against-a-concurrent-merge
|
||||
;; the create must re-read the branch under the advisory lock: a merge
|
||||
;; that wins the race closes the branch while the create waits for the
|
||||
;; lock, so a create arriving afterwards must refuse instead of opening
|
||||
;; a review over a branch that is already merged and gone.
|
||||
(with-redefs [cf/flags pr-flags]
|
||||
(let [author (th/create-profile* 1 {:is-active true})
|
||||
file (th/create-file* 1 {:profile-id (:id author)
|
||||
:project-id (:default-project-id author)})
|
||||
branch (create-branch* author file "contested")
|
||||
branch-id (:id branch)
|
||||
branch-file-id (:branch-file-id branch)]
|
||||
(add-color* author branch-file-id "Brand")
|
||||
(let [orig-lock db/xact-lock!
|
||||
merging? (atom false)
|
||||
merged (atom nil)]
|
||||
(with-redefs [db/xact-lock!
|
||||
(fn [conn id]
|
||||
;; the first lock the create takes is the signal:
|
||||
;; merge the same branch through the pool in its
|
||||
;; own transaction BEFORE the create locks
|
||||
(when (compare-and-set! merging? false true)
|
||||
(reset! merged (th/command! {::th/type :merge-file-branch
|
||||
::rpc/profile-id (:id author)
|
||||
:branch-id branch-id})))
|
||||
(orig-lock conn id))]
|
||||
(let [out (th/command! {::th/type :create-pull-request
|
||||
::rpc/profile-id (:id author)
|
||||
:branch-id branch-id
|
||||
:title "review me"})]
|
||||
(t/is (= :merged (-> @merged :result :status)))
|
||||
(t/is (= :branch-not-found (-> out :error ex-data :code)))
|
||||
(t/is (= [] (th/db-query :file-pull-request {:file-branch-id branch-id})))))))))
|
||||
|
||||
(t/deftest reopen-serializes-against-a-concurrent-merge
|
||||
;; the reopen must re-read the branch under the advisory lock too: the
|
||||
;; merge that wins the race closes the branch under the closed pull
|
||||
;; request's feet, so a reopen arriving afterwards must refuse instead
|
||||
;; of reviving a review over a branch that is already merged and gone.
|
||||
(with-redefs [cf/flags pr-flags]
|
||||
(let [author (th/create-profile* 1 {:is-active true})
|
||||
file (th/create-file* 1 {:profile-id (:id author)
|
||||
:project-id (:default-project-id author)})
|
||||
branch (create-branch* author file "contested")
|
||||
branch-id (:id branch)
|
||||
branch-file-id (:branch-file-id branch)]
|
||||
(add-color* author branch-file-id "Brand")
|
||||
(let [pr (:result (create-pr* author branch-id {}))]
|
||||
(t/is (nil? (:error (th/command! {::th/type :close-pull-request
|
||||
::rpc/profile-id (:id author)
|
||||
:id (:id pr)}))))
|
||||
(let [orig-lock db/xact-lock!
|
||||
merging? (atom false)
|
||||
merged (atom nil)]
|
||||
(with-redefs [db/xact-lock!
|
||||
(fn [conn id]
|
||||
;; the first lock the reopen takes is the signal:
|
||||
;; merge the same branch through the pool in its
|
||||
;; own transaction BEFORE the reopen locks
|
||||
(when (compare-and-set! merging? false true)
|
||||
(reset! merged (th/command! {::th/type :merge-file-branch
|
||||
::rpc/profile-id (:id author)
|
||||
:branch-id branch-id})))
|
||||
(orig-lock conn id))]
|
||||
(let [out (th/command! {::th/type :reopen-pull-request
|
||||
::rpc/profile-id (:id author)
|
||||
:id (:id pr)})]
|
||||
(t/is (= :merged (-> @merged :result :status)))
|
||||
(t/is (= :branch-not-open (-> out :error ex-data :code)))
|
||||
(t/is (= "closed" (:status (first (th/db-query :file-pull-request {:id (:id pr)}))))))))))))
|
||||
|
||||
(t/deftest create-serializes-against-a-kept-concurrent-merge
|
||||
;; same race, but the merge keeps the branch: its row stays readable
|
||||
;; with status "merged" and its file is not deleted, so without the
|
||||
;; re-read the create sails past the stale status check and INSERTS a
|
||||
;; pull request for a branch the merge already closed.
|
||||
(with-redefs [cf/flags pr-flags]
|
||||
(let [author (th/create-profile* 1 {:is-active true})
|
||||
file (th/create-file* 1 {:profile-id (:id author)
|
||||
:project-id (:default-project-id author)})
|
||||
branch (create-branch* author file "contested")
|
||||
branch-id (:id branch)
|
||||
branch-file-id (:branch-file-id branch)]
|
||||
(add-color* author branch-file-id "Brand")
|
||||
(let [orig-lock db/xact-lock!
|
||||
merging? (atom false)
|
||||
merged (atom nil)]
|
||||
(with-redefs [db/xact-lock!
|
||||
(fn [conn id]
|
||||
;; the first lock the create takes is the signal:
|
||||
;; merge the same branch through the pool in its
|
||||
;; own transaction BEFORE the create locks
|
||||
(when (compare-and-set! merging? false true)
|
||||
(reset! merged (th/command! {::th/type :merge-file-branch
|
||||
::rpc/profile-id (:id author)
|
||||
:branch-id branch-id
|
||||
:keep-branch true})))
|
||||
(orig-lock conn id))]
|
||||
(let [out (th/command! {::th/type :create-pull-request
|
||||
::rpc/profile-id (:id author)
|
||||
:branch-id branch-id
|
||||
:title "review me"})]
|
||||
(t/is (= :merged (-> @merged :result :status)))
|
||||
(t/is (= "merged" (:status (first (th/db-query :file-branch {:id branch-id})))))
|
||||
(t/is (= :branch-not-open (-> out :error ex-data :code)))
|
||||
(t/is (= [] (th/db-query :file-pull-request {:file-branch-id branch-id})))))))))
|
||||
|
||||
(t/deftest reopen-serializes-against-a-kept-concurrent-merge
|
||||
;; same race for the reopen: the kept merge leaves the branch row
|
||||
;; readable with status "merged", so without the re-read the reopen
|
||||
;; sails past the stale status check and REOPENS the closed pull
|
||||
;; request over a branch the merge already closed.
|
||||
(with-redefs [cf/flags pr-flags]
|
||||
(let [author (th/create-profile* 1 {:is-active true})
|
||||
file (th/create-file* 1 {:profile-id (:id author)
|
||||
:project-id (:default-project-id author)})
|
||||
branch (create-branch* author file "contested")
|
||||
branch-id (:id branch)
|
||||
branch-file-id (:branch-file-id branch)]
|
||||
(add-color* author branch-file-id "Brand")
|
||||
(let [pr (:result (create-pr* author branch-id {}))]
|
||||
(t/is (nil? (:error (th/command! {::th/type :close-pull-request
|
||||
::rpc/profile-id (:id author)
|
||||
:id (:id pr)}))))
|
||||
(let [orig-lock db/xact-lock!
|
||||
merging? (atom false)
|
||||
merged (atom nil)]
|
||||
(with-redefs [db/xact-lock!
|
||||
(fn [conn id]
|
||||
;; the first lock the reopen takes is the signal:
|
||||
;; merge the same branch through the pool in its
|
||||
;; own transaction BEFORE the reopen locks
|
||||
(when (compare-and-set! merging? false true)
|
||||
(reset! merged (th/command! {::th/type :merge-file-branch
|
||||
::rpc/profile-id (:id author)
|
||||
:branch-id branch-id
|
||||
:keep-branch true})))
|
||||
(orig-lock conn id))]
|
||||
(let [out (th/command! {::th/type :reopen-pull-request
|
||||
::rpc/profile-id (:id author)
|
||||
:id (:id pr)})]
|
||||
(t/is (= :merged (-> @merged :result :status)))
|
||||
(t/is (= "merged" (:status (first (th/db-query :file-branch {:id branch-id})))))
|
||||
(t/is (= :branch-not-open (-> out :error ex-data :code)))
|
||||
(t/is (= "closed" (:status (first (th/db-query :file-pull-request {:id (:id pr)}))))))))))))
|
||||
|
||||
(t/deftest closed-pull-requests-listing
|
||||
(with-redefs [cf/flags pr-flags]
|
||||
(let [author (th/create-profile* 1 {:is-active true})
|
||||
|
||||
@ -684,10 +684,11 @@
|
||||
|
||||
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."
|
||||
complete on their own. The 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), a library color's `:image` id, 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
|
||||
@ -698,6 +699,9 @@
|
||||
(uuid? (:id (:object change)))
|
||||
(update-in [:object :id] lookup)
|
||||
|
||||
(uuid? (get-in change [:color :image :id]))
|
||||
(update-in [:color :image :id] lookup)
|
||||
|
||||
(seq (:operations change))
|
||||
(update :operations #(mapv (fn [op] (remap-op op id-map)) %))))
|
||||
changes))))
|
||||
|
||||
@ -459,34 +459,54 @@
|
||||
(defn update-branch-from-main
|
||||
"Bring main's changes into the branch (reverse of merge). `branch` is the
|
||||
branch row. With conflicts and no `resolutions`, opens the conflict
|
||||
resolution modal in update mode; with resolutions, applies them."
|
||||
resolution modal in update mode; with resolutions, applies them.
|
||||
|
||||
When the current diff in state is this branch's `:main->branch` diff,
|
||||
its main revn is sent as `expected-main-revn` so the server refuses to
|
||||
apply resolutions computed against a stale diff (`:file-modified`); in
|
||||
that case the diff is re-fetched and the user is asked to review it
|
||||
again (same as `merge-branch`)."
|
||||
([branch] (update-branch-from-main branch nil))
|
||||
([branch resolutions]
|
||||
(ptk/reify ::update-branch-from-main
|
||||
ptk/WatchEvent
|
||||
(watch [_ _ _]
|
||||
(rx/concat
|
||||
(rx/of (ev/event {::ev/name "update-branch-from-main"}))
|
||||
(->> (rp/cmd! :update-branch-from-main (cond-> {:branch-id (:id branch)}
|
||||
(seq resolutions) (assoc :resolutions resolutions)))
|
||||
(rx/mapcat
|
||||
(fn [{:keys [status conflicts]}]
|
||||
(case status
|
||||
;; the open branch file just changed server-side: hard-reload
|
||||
;; it so the pulled changes are shown
|
||||
:updated (rx/of (ntf/success (tr "workspace.branches.update.success"))
|
||||
(reload-file-window))
|
||||
;; the returned conflicts are authoritative: they are the very
|
||||
;; ones the command computes `resolved?` against, and the diff
|
||||
;; the modal could fetch on its own is computed in another id
|
||||
;; frame, which can disagree with them (vcs:tp-update-conflict-frame)
|
||||
:conflicts (rx/of (open-conflict-resolutions {:branch branch
|
||||
:mode :update
|
||||
:conflicts conflicts}))
|
||||
:unsupported (rx/of (ntf/warn (tr "workspace.branches.update.unsupported")))
|
||||
(rx/of (ntf/error (tr "workspace.branches.update.error"))))))
|
||||
(rx/catch (fn [cause]
|
||||
(rx/of (refusal-notification cause (tr "workspace.branches.update.error")))))))))))
|
||||
(watch [_ state _]
|
||||
(let [branch-id (:id branch)
|
||||
diff-st (:workspace-branch-diff state)
|
||||
main-revn (when (and (= branch-id (:branch-id diff-st))
|
||||
(= :main->branch (:direction diff-st)))
|
||||
(get-in diff-st [:diff :meta :main-revn]))]
|
||||
(rx/concat
|
||||
(rx/of (ev/event {::ev/name "update-branch-from-main"}))
|
||||
(->> (rp/cmd! :update-branch-from-main (cond-> {:branch-id branch-id}
|
||||
(seq resolutions) (assoc :resolutions resolutions)
|
||||
(some? main-revn) (assoc :expected-main-revn main-revn)))
|
||||
(rx/mapcat
|
||||
(fn [{:keys [status conflicts]}]
|
||||
(case status
|
||||
;; the open branch file just changed server-side: hard-reload
|
||||
;; it so the pulled changes are shown
|
||||
:updated (rx/of (ntf/success (tr "workspace.branches.update.success"))
|
||||
(reload-file-window))
|
||||
;; the returned conflicts are authoritative: they are the very
|
||||
;; ones the command computes `resolved?` against, and the diff
|
||||
;; the modal could fetch on its own is computed in another id
|
||||
;; frame, which can disagree with them (vcs:tp-update-conflict-frame)
|
||||
:conflicts (rx/of (open-conflict-resolutions {:branch branch
|
||||
:mode :update
|
||||
:conflicts conflicts}))
|
||||
:unsupported (rx/of (ntf/warn (tr "workspace.branches.update.unsupported")))
|
||||
(rx/of (ntf/error (tr "workspace.branches.update.error"))))))
|
||||
(rx/catch
|
||||
(fn [cause]
|
||||
(if (= :file-modified (:code (ex-data cause)))
|
||||
;; main moved under our feet: reload the diff so the
|
||||
;; user resolves against the current state (no
|
||||
;; update-specific "main moved" key exists, so this
|
||||
;; reuses merge's)
|
||||
(rx/of (ntf/warn (tr "workspace.branches.merge.main-moved"))
|
||||
(fetch-branch-diff branch-id :main->branch))
|
||||
(rx/of (refusal-notification cause (tr "workspace.branches.update.error")))))))))))))
|
||||
|
||||
(defn set-conflict-resolution
|
||||
"Choose `:main` or `:branch` for a whole conflicting entity (by id). This
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user