diff --git a/backend/src/app/rpc/commands/files_branch.clj b/backend/src/app/rpc/commands/files_branch.clj index 9c26a5faf5..d5653f05b4 100644 --- a/backend/src/app/rpc/commands/files_branch.clj +++ b/backend/src/app/rpc/commands/files_branch.clj @@ -1230,71 +1230,75 @@ [cfg {:keys [::rpc/profile-id file-id]}] (check-branching-enabled!) (files/check-edition-permissions! cfg profile-id file-id) - (db/tx-run! - cfg - (fn [{:keys [::db/conn] :as cfg}] - ;; the advisory lock every save on this file takes: a concurrent - ;; branch save must not land between the derive and the persist, or - ;; its op would be dropped together with the log - (db/xact-lock! conn file-id) + (let [tpoint (ct/tpoint)] + (db/tx-run! + cfg + (fn [{:keys [::db/conn] :as cfg}] + ;; the advisory lock every save on this file takes: a concurrent + ;; branch save must not land between the derive and the persist, or + ;; its op would be dropped together with the log + (db/xact-lock! conn file-id) - (let [branch (db/get* conn :file-branch {:branch-file-id file-id})] - (if (nil? branch) - {:status :materialized :file-id file-id :changed false} - (let [file (bfc/get-file cfg file-id :realize? true) - team (teams/get-team conn :profile-id profile-id :file-id file-id) - delay (ldel/get-deletion-delay team) - ts (ct/now)] + (let [branch (db/get* conn :file-branch {:branch-file-id file-id})] + (if (nil? branch) + (audited {:status :materialized :file-id file-id :changed false} + tpoint :materialize) + (let [file (bfc/get-file cfg file-id :realize? true) + team (teams/get-team conn :profile-id profile-id :file-id file-id) + delay (ldel/get-deletion-delay team) + ts (ct/now)] - (binding [pmap/*tracked* (pmap/create-tracked) - pmap/*load-fn* (partial fdata/load-pointer cfg file-id) - cfeat/*current* (:features file) - cfeat/*previous* (:features file)] + (binding [pmap/*tracked* (pmap/create-tracked) + pmap/*load-fn* (partial fdata/load-pointer cfg file-id) + cfeat/*current* (:features file) + cfeat/*previous* (:features file)] - (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)] + (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)] - ;; the derived state becomes the file's own payload - (fupd/persist-file! (assoc cfg ::fupd/timestamp ts) file) + ;; the derived state becomes the file's own payload + (fupd/persist-file! (assoc cfg ::fupd/timestamp ts) file) - ;; from here the ordinary paths apply - (db/update! conn :file - {:is-branch false} - {:id file-id} - {::db/return-keys false}) + ;; from here the ordinary paths apply + (db/update! conn :file + {:is-branch false} + {:id file-id} + {::db/return-keys false}) - ;; the payload replaced the log - (db/delete! conn :file-branch-change {:branch-id (:id branch)}) + ;; the payload replaced the log + (db/delete! conn :file-branch-change {:branch-id (:id branch)}) - ;; a review sandbox over a file that is no longer a branch - ;; has nothing left to review - (close-branch-pull-requests! cfg (:id branch) - {:profile-id profile-id - :status "closed" - :deleted-at (ct/in-future delay)}) + ;; a review sandbox over a file that is no longer a branch + ;; has nothing left to review + (close-branch-pull-requests! cfg (:id branch) + {:profile-id profile-id + :status "closed" + :deleted-at (ct/in-future delay)}) - ;; nothing derives from the base any more, so the pin goes - (release-base-snapshot! cfg branch (ct/in-future delay)) + ;; nothing derives from the base any more, so the pin goes + (release-base-snapshot! cfg branch (ct/in-future delay)) - ;; The branch row is archived rather than deleted: its pull - ;; requests reference it with ON DELETE CASCADE, so deleting - ;; it would take the review history along, and `archived` is - ;; a status the schema already allows. - (db/update! conn :file-branch - {:status "archived" - :deleted-at (ct/in-future delay) - :updated-at ts} - {:id (:id branch)} - {::db/return-keys false}) + ;; The branch row is archived rather than deleted: its pull + ;; requests reference it with ON DELETE CASCADE, so deleting + ;; it would take the review history along, and `archived` is + ;; a status the schema already allows. + (db/update! conn :file-branch + {:status "archived" + :deleted-at (ct/in-future delay) + :updated-at ts} + {:id (:id branch)} + {::db/return-keys false}) - {:status :materialized - :file-id file-id - :changed true - :revn (:revn file)})))))))) + (audited + {:status :materialized + :file-id file-id + :changed true + :revn (:revn file)} + tpoint :materialize)))))))))) ;; --- COMMAND QUERY: get-file-branch-info diff --git a/backend/src/app/rpc/commands/files_update.clj b/backend/src/app/rpc/commands/files_update.clj index 438e3dc38d..a6ed200c83 100644 --- a/backend/src/app/rpc/commands/files_update.clj +++ b/backend/src/app/rpc/commands/files_update.clj @@ -210,7 +210,7 @@ (binding [l/*context* (some-> (meta params) (get :app.http/request) (errors/request->context))] - (-> (update-file* cfg params) + (-> (update-file* cfg params tpoint) (rph/with-defer #(let [elapsed (tpoint)] (l/trace :hint "update-file" :time (ct/format-duration elapsed)))))))) @@ -221,9 +221,14 @@ Follow the inner implementation to `update-file-data!` function. + `tpoint` is the request's own timer, threaded in so that a save routed + to a branch's op log can report its duration on the audit event the way + the other branch commands do. + Only intended for internal use on this module." [{:keys [::db/conn ::timestamp] :as cfg} - {:keys [profile-id file team features changes session-id skip-validate] :as params}] + {:keys [profile-id file team features changes session-id skip-validate] :as params} + tpoint] (binding [pmap/*tracked* (pmap/create-tracked) pmap/*load-fn* (partial fdata/load-pointer cfg (:id file))] @@ -286,11 +291,21 @@ (with-meta {:revn revn :lagged (get-lagged-changes conn params)} {::audit/replace-props - {:id (:id file) - :name (:name file) - :features (:features file) - :project-id (:project-id file) - :team-id (:team-id file)}})))) + (cond-> {:id (:id file) + :name (:name file) + :features (:features file) + :project-id (:project-id file) + :team-id (:team-id file)} + + ;; A save routed to a branch's op log is the preview's most + ;; frequent operation, so it reports its duration and outcome on + ;; its own audit event like every other branch command. The + ;; ordinary save path is untouched: the props above are the ones + ;; it has always carried. + (:is-branch file) + (assoc :branch-operation "save-branch" + :branch-outcome "saved" + :branch-duration-ms (inst-ms (tpoint))))})))) (defn get-file "Get not-decoded file, only decodes the features set." diff --git a/backend/test/backend_tests/rpc_file_branch_test.clj b/backend/test/backend_tests/rpc_file_branch_test.clj index 2c34bf7961..c8db98b875 100644 --- a/backend/test/backend_tests/rpc_file_branch_test.clj +++ b/backend/test/backend_tests/rpc_file_branch_test.clj @@ -1249,4 +1249,46 @@ (t/is (nil? (:error out))) (t/is (= "merge" (:branch-operation props))) (t/is (= "merged" (:branch-outcome props))) + (t/is (int? (:branch-duration-ms props))))) + + (t/testing "a save routed to the branch reports the same three" + ;; the ordinary save path replaces its props, so the branch keys + ;; travel in `::audit/replace-props` and not in `::audit/props` + (let [created (th/command! {::th/type :create-file-branch + ::rpc/profile-id (:id profile) + :file-id (:id file) + :name "audited-save"}) + bfid (-> created :result :branch-file-id) + file-row (th/db-get :file {:id bfid}) + out (th/command! {::th/type :update-file + ::rpc/profile-id (:id profile) + :id bfid + :session-id (uuid/random) + :revn (:revn file-row) + :vern (:vern file-row) + :features cfeat/supported-features + :changes [{:type :add-color + :color {:id (uuid/random) + :name "audited" + :color "#112233" + :opacity 1}}]}) + props (-> out :result meta :app.loggers.audit/replace-props)] + (t/is (nil? (:error out))) + (t/is (= "save-branch" (:branch-operation props))) + (t/is (= "saved" (:branch-outcome props))) + (t/is (int? (:branch-duration-ms props))) + (t/is (= bfid (:id props))))) + + (t/testing "materialising reports its own operation and outcome" + (let [created (th/command! {::th/type :create-file-branch + ::rpc/profile-id (:id profile) + :file-id (:id file) + :name "audited-exit"}) + out (th/command! {::th/type :materialize-file-branch + ::rpc/profile-id (:id profile) + :file-id (-> created :result :branch-file-id)}) + props (-> out :result meta :app.loggers.audit/props)] + (t/is (nil? (:error out))) + (t/is (= "materialize" (:branch-operation props))) + (t/is (= "materialized" (:branch-outcome props))) (t/is (int? (:branch-duration-ms props))))))))