From 761bbb7334e08928375fe3b6c1c0194229295b9f Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Fri, 24 May 2024 10:54:42 +0200 Subject: [PATCH 1/3] :sparkles: Add srepl helpers for delete/restore teams, projects, and files --- backend/src/app/rpc/commands/files.clj | 29 ++--- backend/src/app/rpc/commands/projects.clj | 38 +++--- backend/src/app/rpc/commands/teams.clj | 42 ++++--- backend/src/app/srepl/main.clj | 139 +++++++++++++--------- backend/src/app/tasks/delete_object.clj | 6 +- backend/test/backend_tests/helpers.clj | 2 +- 6 files changed, 143 insertions(+), 113 deletions(-) diff --git a/backend/src/app/rpc/commands/files.clj b/backend/src/app/rpc/commands/files.clj index e165173f2..ea9653f1f 100644 --- a/backend/src/app/rpc/commands/files.clj +++ b/backend/src/app/rpc/commands/files.clj @@ -912,13 +912,19 @@ ;; --- MUTATION COMMAND: delete-file -(defn- mark-file-deleted! +(defn- mark-file-deleted [conn file-id] - (db/update! conn :file - {:deleted-at (dt/now)} - {:id file-id} - {::db/return-keys [:id :name :is-shared :deleted-at - :project-id :created-at :modified-at]})) + (let [file (db/update! conn :file + {:deleted-at (dt/now)} + {:id file-id} + {::db/return-keys [:id :name :is-shared :deleted-at + :project-id :created-at :modified-at]})] + (wrk/submit! {::wrk/task :delete-object + ::wrk/conn conn + :object :file + :deleted-at (:deleted-at file) + :id file-id}) + file)) (def ^:private schema:delete-file @@ -929,14 +935,7 @@ (defn- delete-file [{:keys [::db/conn] :as cfg} {:keys [profile-id id] :as params}] (check-edition-permissions! conn profile-id id) - (let [file (mark-file-deleted! conn id)] - - (wrk/submit! {::wrk/task :delete-object - ::wrk/delay (dt/duration "1m") - ::wrk/conn conn - :object :file - :deleted-at (:deleted-at file) - :id id}) + (let [file (mark-file-deleted conn id)] ;; NOTE: when a file is a shared library, then we proceed to load ;; the whole file, proceed with feature checking and properly execute @@ -951,8 +950,6 @@ :profile-id profile-id :project-id (:project-id file))] - - (-> (cfeat/get-team-enabled-features cf/flags team) (cfeat/check-client-features! (:features params)) (cfeat/check-file-features! (:features file))) diff --git a/backend/src/app/rpc/commands/projects.clj b/backend/src/app/rpc/commands/projects.clj index 29cbeaf51..a8236008e 100644 --- a/backend/src/app/rpc/commands/projects.clj +++ b/backend/src/app/rpc/commands/projects.clj @@ -7,6 +7,7 @@ (ns app.rpc.commands.projects (:require [app.common.data.macros :as dm] + [app.common.exceptions :as ex] [app.common.spec :as us] [app.db :as db] [app.db.sql :as-alias sql] @@ -245,32 +246,37 @@ ;; --- MUTATION: Delete Project +(defn- delete-project + [conn project-id] + (let [project (db/update! conn :project + {:deleted-at (dt/now)} + {:id project-id} + {::db/return-keys true})] + + (when (:is-default project) + (ex/raise :type :validation + :code :non-deletable-project + :hint "impossible to delete default project")) + + (wrk/submit! {::wrk/task :delete-object + ::wrk/conn conn + :object :project + :deleted-at (:deleted-at project) + :id project-id}) + + project)) + (s/def ::delete-project (s/keys :req [::rpc/profile-id] :req-un [::id])) -;; TODO: right now, we just don't allow delete default projects, in a -;; future we need to ensure raise a correct exception signaling that -;; this is not allowed. - (sv/defmethod ::delete-project {::doc/added "1.18" ::webhooks/event? true} [{:keys [::db/pool] :as cfg} {:keys [::rpc/profile-id id] :as params}] (db/with-atomic [conn pool] (check-edition-permissions! conn profile-id id) - (let [project (db/update! conn :project - {:deleted-at (dt/now)} - {:id id :is-default false} - {::db/return-keys true})] - - (wrk/submit! {::wrk/task :delete-object - ::wrk/delay (dt/duration "1m") - ::wrk/conn conn - :object :project - :deleted-at (:deleted-at project) - :id id}) - + (let [project (delete-project conn id)] (rph/with-meta (rph/wrap) {::audit/props {:team-id (:team-id project) :name (:name project) diff --git a/backend/src/app/rpc/commands/teams.clj b/backend/src/app/rpc/commands/teams.clj index dabf6c848..4cc75de5a 100644 --- a/backend/src/app/rpc/commands/teams.clj +++ b/backend/src/app/rpc/commands/teams.clj @@ -517,38 +517,44 @@ ;; --- Mutation: Delete Team +(defn- delete-team + "Mark a team for deletion" + [conn team-id] + + (let [deleted-at (dt/now) + team (db/update! conn :team + {:deleted-at deleted-at} + {:id team-id} + {::db/return-keys true})] + + (when (:is-default team) + (ex/raise :type :validation + :code :non-deletable-team + :hint "impossible to delete default team")) + + (wrk/submit! {::wrk/task :delete-object + ::wrk/conn conn + :object :team + :deleted-at deleted-at + :id team-id}) + team)) + (s/def ::delete-team (s/keys :req [::rpc/profile-id] :req-un [::id])) -;; TODO: right now just don't allow delete default team, in future it -;; should raise a specific exception for signal that this action is -;; not allowed. - (sv/defmethod ::delete-team {::doc/added "1.17"} [{:keys [::db/pool] :as cfg} {:keys [::rpc/profile-id id] :as params}] (db/with-atomic [conn pool] - (let [perms (get-permissions conn profile-id id) - deleted-at (dt/now)] - + (let [perms (get-permissions conn profile-id id)] (when-not (:is-owner perms) (ex/raise :type :validation :code :only-owner-can-delete-team)) - (db/update! conn :team - {:deleted-at deleted-at} - {:id id :is-default false}) - - (wrk/submit! {::wrk/task :delete-object - ::wrk/delay (dt/duration "1m") - ::wrk/conn conn - :object :team - :deleted-at deleted-at - :id id}) + (delete-team conn id) nil))) - ;; --- Mutation: Team Update Role (s/def ::team-id ::us/uuid) diff --git a/backend/src/app/srepl/main.clj b/backend/src/app/srepl/main.clj index 622dc840e..87ff5cd81 100644 --- a/backend/src/app/srepl/main.clj +++ b/backend/src/app/srepl/main.clj @@ -30,6 +30,8 @@ [app.rpc.commands.files-snapshot :as fsnap] [app.rpc.commands.management :as mgmt] [app.rpc.commands.profile :as profile] + [app.rpc.commands.projects :as projects] + [app.rpc.commands.teams :as teams] [app.srepl.fixes :as fixes] [app.srepl.helpers :as h] [app.util.blob :as blob] @@ -475,80 +477,99 @@ ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; -;; RESTORE DELETED OBJECTS +;; DELETE/RESTORE OBJECTS (WITH CASCADE, SOFT) ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; +(defn- restore-file* + [{:keys [::db/conn]} file-id] + (db/update! conn :file + {:deleted-at nil + :has-media-trimmed false} + {:id file-id}) + + ;; Fragments are not handled here because they + ;; use the database cascade operation and they + ;; are not marked for deletion with objects-gc + ;; task + + (db/update! conn :file-media-object + {:deleted-at nil} + {:file-id file-id}) + + ;; Mark thumbnails to be deleted + (db/update! conn :file-thumbnail + {:deleted-at nil} + {:file-id file-id}) + + (db/update! conn :file-tagged-object-thumbnail + {:deleted-at nil} + {:file-id file-id})) + +(defn- restore-project* + [{:keys [::db/conn]} project-id] + + (db/update! conn :project + {:deleted-at nil} + {:id project-id}) + + (doseq [{:keys [id]} (db/query conn :file + {:project-id project-id} + {::db/columns [:id]})] + (restore-file* conn id))) + +(defn- restore-team* + [{:keys [::db/conn]} team-id] + (db/update! conn :team + {:deleted-at nil} + {:id team-id}) + + (db/update! conn :team-font-variant + {:deleted-at nil} + {:team-id team-id}) + + (doseq [{:keys [id]} (db/query conn :project + {:team-id team-id} + {::db/columns [:id]})] + (restore-project* conn id))) + (defn restore-deleted-team! "Mark a team and all related objects as not deleted" [team-id] (let [team-id (h/parse-uuid team-id)] - (db/tx-run! main/system - (fn [{:keys [::db/conn]}] - (db/update! conn :team-font-variant - {:deleted-at nil} - {:team-id team-id}) - - (doseq [project (db/update! conn :project - {:deleted-at nil} - {:team-id team-id} - {::db/return-keys [:id] - ::db/many true})] - - (doseq [file (db/update! conn :file - {:deleted-at nil - :has-media-trimmed false} - {:project-id (:id project)} - {::db/return-keys [:id] - ::db/many true})] - - ;; Fragments are not handled here because they - ;; use the database cascade operation and they - ;; are not marked for deletion with objects-gc - ;; task - - (db/update! conn :file-media-object - {:deleted-at nil} - {:file-id (:id file)}) - - ;; Mark thumbnails to be deleted - (db/update! conn :file-thumbnail - {:deleted-at nil} - {:file-id (:id file)}) - - (db/update! conn :file-tagged-object-thumbnail - {:deleted-at nil} - {:file-id (:id file)}))))))) - + (db/tx-run! main/system restore-team* team-id))) (defn restore-deleted-project! "Mark a project and all related objects as not deleted" [project-id] (let [project-id (h/parse-uuid project-id)] - (db/tx-run! main/system - (fn [{:keys [::db/conn]}] - (doseq [file (db/update! conn :file - {:deleted-at nil - :has-media-trimmed false} - {:project-id project-id} - {::db/return-keys [:id] - ::db/many true})] + (db/tx-run! main/system restore-project* project-id))) - ;; Fragments are not handled here because they use - ;; the database cascade operation and they are not - ;; marked for deletion with objects-gc task +(defn restore-deleted-file! + "Mark a file and all related objects as not deleted" + [file-id] + (let [file-id (h/parse-uuid file-id)] + (db/tx-run! main/system restore-file* file-id))) - (db/update! conn :file-media-object - {:deleted-at nil} - {:file-id (:id file)}) +(defn delete-team! + "Mark a team for deletion" + [team-id] + (let [team-id (h/parse-uuid team-id)] + (db/tx-run! main/system (fn [{:keys [::db/conn]}] + (#'teams/delete-team conn team-id))))) - ;; Mark thumbnails to be deleted - (db/update! conn :file-thumbnail - {:deleted-at nil} - {:file-id (:id file)}) +(defn delete-project! + "Mark a project for deletion" + [project-id] + (let [project-id (h/parse-uuid project-id)] + (db/tx-run! main/system (fn [{:keys [::db/conn]}] + (#'projects/delete-project conn project-id))))) - (db/update! conn :file-tagged-object-thumbnail - {:deleted-at nil} - {:file-id (:id file)})))))) +(defn delete-file! + "Mark a project for deletion" + [file-id] + (let [file-id (h/parse-uuid file-id)] + (db/tx-run! main/system (fn [{:keys [::db/conn]}] + (#'files/mark-file-deleted conn file-id))))) ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;; MISC diff --git a/backend/src/app/tasks/delete_object.clj b/backend/src/app/tasks/delete_object.clj index f0a60d30a..bd954cd9d 100644 --- a/backend/src/app/tasks/delete_object.clj +++ b/backend/src/app/tasks/delete_object.clj @@ -17,7 +17,7 @@ (defmethod delete-object :file [{:keys [::db/conn]} {:keys [id deleted-at]}] - (l/trc :hint "marking for deletion" :rel "file" :id id) + (l/trc :hint "marking for deletion" :rel "file" :id (str id)) ;; Mark file media objects to be deleted (db/update! conn :file-media-object {:deleted-at deleted-at} @@ -34,7 +34,7 @@ (defmethod delete-object :project [{:keys [::db/conn] :as cfg} {:keys [id deleted-at]}] - (l/trc :hint "marking for deletion" :rel "project" :id id) + (l/trc :hint "marking for deletion" :rel "project" :id (str id)) (doseq [file (db/update! conn :file {:deleted-at deleted-at} {:project-id id} @@ -44,7 +44,7 @@ (defmethod delete-object :team [{:keys [::db/conn] :as cfg} {:keys [id deleted-at]}] - (l/trc :hint "marking for deletion" :rel "team" :id id) + (l/trc :hint "marking for deletion" :rel "team" :id (str id)) (db/update! conn :team-font-variant {:deleted-at deleted-at} {:team-id id}) diff --git a/backend/test/backend_tests/helpers.clj b/backend/test/backend_tests/helpers.clj index 12d76785e..cb399e70f 100644 --- a/backend/test/backend_tests/helpers.clj +++ b/backend/test/backend_tests/helpers.clj @@ -219,7 +219,7 @@ ([params] (mark-file-deleted* *system* params)) ([conn {:keys [id] :as params}] - (#'files/mark-file-deleted! conn id))) + (#'files/mark-file-deleted conn id))) (defn create-team* ([i params] (create-team* *system* i params)) From 39119ac0401cc9b9006b0e3c769f5ded29539335 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Fri, 24 May 2024 11:06:30 +0200 Subject: [PATCH 2/3] :sparkles: Reuse team deletion logic on orphan teams gc task --- backend/src/app/tasks/orphan_teams_gc.clj | 68 +++++++++++++---------- 1 file changed, 38 insertions(+), 30 deletions(-) diff --git a/backend/src/app/tasks/orphan_teams_gc.clj b/backend/src/app/tasks/orphan_teams_gc.clj index c04123a83..8869c72cc 100644 --- a/backend/src/app/tasks/orphan_teams_gc.clj +++ b/backend/src/app/tasks/orphan_teams_gc.clj @@ -10,29 +10,10 @@ [app.common.logging :as l] [app.db :as db] [app.util.time :as dt] + [app.worker :as wrk] [clojure.spec.alpha :as s] [integrant.core :as ig])) -(declare ^:private delete-orphan-teams!) - -(defmethod ig/pre-init-spec ::handler [_] - (s/keys :req [::db/pool])) - -(defmethod ig/init-key ::handler - [_ cfg] - (fn [params] - (db/tx-run! cfg (fn [{:keys [::db/conn] :as cfg}] - (l/inf :hint "gc started" :rollback? (boolean (:rollback? params))) - (let [total (delete-orphan-teams! cfg)] - (l/inf :hint "task finished" - :teams total - :rollback? (boolean (:rollback? params))) - - (when (:rollback? params) - (db/rollback! conn)) - - {:processed total}))))) - (def ^:private sql:get-orphan-teams "SELECT t.id FROM team AS t @@ -44,16 +25,43 @@ FOR UPDATE OF t SKIP LOCKED") -(defn- delete-orphan-teams! +(defn- delete-orphan-teams "Find all orphan teams (with no members) and mark them for deletion (soft delete)." [{:keys [::db/conn] :as cfg}] - (->> (db/cursor conn sql:get-orphan-teams) - (map :id) - (reduce (fn [total team-id] - (l/trc :hint "mark orphan team for deletion" :id (str team-id)) - (db/update! conn :team - {:deleted-at (dt/now)} - {:id team-id}) - (inc total)) - 0))) + (let [deleted-at (dt/now)] + (->> (db/cursor conn sql:get-orphan-teams) + (map :id) + (reduce (fn [total team-id] + (l/trc :hint "mark orphan team for deletion" :id (str team-id)) + + (db/update! conn :team + {:deleted-at deleted-at} + {:id team-id}) + + (wrk/submit! {::wrk/task :delete-object + ::wrk/conn conn + :object :team + :deleted-at deleted-at + :id team-id}) + + (inc total)) + 0)))) + +(defmethod ig/pre-init-spec ::handler [_] + (s/keys :req [::db/pool])) + +(defmethod ig/init-key ::handler + [_ cfg] + (fn [params] + (db/tx-run! cfg (fn [{:keys [::db/conn] :as cfg}] + (l/inf :hint "gc started" :rollback? (boolean (:rollback? params))) + (let [total (delete-orphan-teams cfg)] + (l/inf :hint "task finished" + :teams total + :rollback? (boolean (:rollback? params))) + + (when (:rollback? params) + (db/rollback! conn)) + + {:processed total}))))) From 574c8d17890a89c5d2dea27b8435497241769d10 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Mon, 27 May 2024 10:13:59 +0200 Subject: [PATCH 3/3] :sparkles: Move library-absorb operation to async task And make it not mandatory in case of failure --- backend/src/app/rpc/commands/files.clj | 60 ++++++++++--------------- backend/src/app/srepl/main.clj | 18 +++++--- backend/src/app/tasks/delete_object.clj | 51 +++++++++++++-------- 3 files changed, 70 insertions(+), 59 deletions(-) diff --git a/backend/src/app/rpc/commands/files.clj b/backend/src/app/rpc/commands/files.clj index ea9653f1f..dc48abd6e 100644 --- a/backend/src/app/rpc/commands/files.clj +++ b/backend/src/app/rpc/commands/files.clj @@ -823,7 +823,7 @@ (feat.fdata/persist-pointers! cfg file-id)))) -(defn- absorb-library! +(defn- absorb-library "Find all files using a shared library, and absorb all library assets into the file local libraries" [cfg {:keys [id] :as library}] @@ -841,7 +841,26 @@ :library-id (str id) :files (str/join "," (map str ids))) - (run! (partial absorb-library-by-file! cfg ldata) ids))) + (run! (partial absorb-library-by-file! cfg ldata) ids) + library)) + +(defn absorb-library! + [{:keys [::db/conn] :as cfg} id] + (let [file (-> (get-file cfg id + :lock-for-update? true + :include-deleted? true) + (check-version!)) + + proj (db/get* conn :project {:id (:project-id file)} + {::db/remove-deleted false}) + team (-> (db/get* conn :team {:id (:team-id proj)} + {::db/remove-deleted false}) + (teams/decode-row))] + + (-> (cfeat/get-team-enabled-features cf/flags team) + (cfeat/check-file-features! (:features file))) + + (absorb-library cfg file))) (defn- set-file-shared [{:keys [::db/conn] :as cfg} {:keys [profile-id id] :as params}] @@ -854,25 +873,14 @@ ;; file, we need to perform more complex operation, ;; so in this case we retrieve the complete file and ;; perform all required validations. - (let [file (-> (get-file cfg id :lock-for-update? true) - (check-version!) - (assoc :is-shared false)) - team (teams/get-team conn - :profile-id profile-id - :project-id (:project-id file))] - - (-> (cfeat/get-team-enabled-features cf/flags team) - (cfeat/check-client-features! (:features params)) - (cfeat/check-file-features! (:features file))) - - (absorb-library! cfg file) - + (let [file (-> (absorb-library! cfg id) + (assoc :is-shared false))] (db/delete! conn :file-library-rel {:library-file-id id}) (db/update! conn :file {:is-shared false :modified-at (dt/now)} {:id id}) - file) + (select-keys file [:id :name :is-shared])) (and (false? (:is-shared file)) (true? (:is-shared params))) @@ -936,26 +944,6 @@ [{:keys [::db/conn] :as cfg} {:keys [profile-id id] :as params}] (check-edition-permissions! conn profile-id id) (let [file (mark-file-deleted conn id)] - - ;; NOTE: when a file is a shared library, then we proceed to load - ;; the whole file, proceed with feature checking and properly execute - ;; the absorb-library procedure - (when (:is-shared file) - (let [file (-> (get-file cfg id - :lock-for-update? true - :include-deleted? true) - (check-version!)) - - team (teams/get-team conn - :profile-id profile-id - :project-id (:project-id file))] - - (-> (cfeat/get-team-enabled-features cf/flags team) - (cfeat/check-client-features! (:features params)) - (cfeat/check-file-features! (:features file))) - - (absorb-library! cfg file))) - (rph/with-meta (rph/wrap) {::audit/props {:project-id (:project-id file) :name (:name file) diff --git a/backend/src/app/srepl/main.clj b/backend/src/app/srepl/main.clj index 87ff5cd81..193f72d1c 100644 --- a/backend/src/app/srepl/main.clj +++ b/backend/src/app/srepl/main.clj @@ -503,10 +503,14 @@ (db/update! conn :file-tagged-object-thumbnail {:deleted-at nil} - {:file-id file-id})) + {:file-id file-id}) + + :restored) + + (defn- restore-project* - [{:keys [::db/conn]} project-id] + [{:keys [::db/conn] :as cfg} project-id] (db/update! conn :project {:deleted-at nil} @@ -515,10 +519,12 @@ (doseq [{:keys [id]} (db/query conn :file {:project-id project-id} {::db/columns [:id]})] - (restore-file* conn id))) + (restore-file* cfg id)) + + :restored) (defn- restore-team* - [{:keys [::db/conn]} team-id] + [{:keys [::db/conn] :as cfg} team-id] (db/update! conn :team {:deleted-at nil} {:id team-id}) @@ -530,7 +536,9 @@ (doseq [{:keys [id]} (db/query conn :project {:team-id team-id} {::db/columns [:id]})] - (restore-project* conn id))) + (restore-project* cfg id)) + + :restored) (defn restore-deleted-team! "Mark a team and all related objects as not deleted" diff --git a/backend/src/app/tasks/delete_object.clj b/backend/src/app/tasks/delete_object.clj index bd954cd9d..557e35b59 100644 --- a/backend/src/app/tasks/delete_object.clj +++ b/backend/src/app/tasks/delete_object.clj @@ -9,28 +9,42 @@ (:require [app.common.logging :as l] [app.db :as db] + [app.rpc.commands.files :as files] [clojure.spec.alpha :as s] [integrant.core :as ig])) +(def ^:dynamic *team-deletion* false) + (defmulti delete-object (fn [_ props] (:object props))) (defmethod delete-object :file - [{:keys [::db/conn]} {:keys [id deleted-at]}] + [{:keys [::db/conn] :as cfg} {:keys [id deleted-at]}] (l/trc :hint "marking for deletion" :rel "file" :id (str id)) - ;; Mark file media objects to be deleted - (db/update! conn :file-media-object - {:deleted-at deleted-at} - {:file-id id}) + (when-let [file (db/get* conn :file {:id id} {::db/remove-deleted false})] + (when (and (:is-shared file) + (not *team-deletion*)) + ;; NOTE: we don't prevent file deletion on absorb operation failure + (try + (db/tx-run! cfg files/absorb-library! id) + (catch Throwable cause + (l/warn :hint "error on absorbing library" + :file-id id + :cause cause)))) - ;; Mark thumbnails to be deleted - (db/update! conn :file-thumbnail - {:deleted-at deleted-at} - {:file-id id}) + ;; Mark file media objects to be deleted + (db/update! conn :file-media-object + {:deleted-at deleted-at} + {:file-id id}) - (db/update! conn :file-tagged-object-thumbnail - {:deleted-at deleted-at} - {:file-id id})) + ;; Mark thumbnails to be deleted + (db/update! conn :file-thumbnail + {:deleted-at deleted-at} + {:file-id id}) + + (db/update! conn :file-tagged-object-thumbnail + {:deleted-at deleted-at} + {:file-id id}))) (defmethod delete-object :project [{:keys [::db/conn] :as cfg} {:keys [id deleted-at]}] @@ -49,12 +63,13 @@ {:deleted-at deleted-at} {:team-id id}) - (doseq [project (db/update! conn :project - {:deleted-at deleted-at} - {:team-id id} - {::db/return-keys [:id :deleted-at] - ::db/many true})] - (delete-object cfg (assoc project :object :project)))) + (binding [*team-deletion* true] + (doseq [project (db/update! conn :project + {:deleted-at deleted-at} + {:team-id id} + {::db/return-keys [:id :deleted-at] + ::db/many true})] + (delete-object cfg (assoc project :object :project))))) (defmethod delete-object :default [_cfg props]