Merge pull request #4633 from penpot/niwinz-objects-gc-locking

✨ Improve object deletion flow

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)
+               (let [file (-> (absorb-library! cfg id)
-                              (check-version!)
+                              (assoc :is-shared false))]
-                              (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)
-
                  (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)))
@@ -912,13 +920,19 @@
 
 ;; --- MUTATION COMMAND: delete-file
 
-(defn- mark-file-deleted!
+(defn- mark-file-deleted
   [conn file-id]
-  (db/update! conn :file
+  (let [file (db/update! conn :file
-              {:deleted-at (dt/now)}
+                         {:deleted-at (dt/now)}
-              {:id file-id}
+                         {:id file-id}
-              {::db/return-keys [:id :name :is-shared :deleted-at
+                         {::db/return-keys [:id :name :is-shared :deleted-at
-                                 :project-id :created-at :modified-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,36 +943,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)]
+  (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})
-
-    ;; 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)

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
+    (let [project (delete-project conn id)]
-                              {: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})
-
       (rph/with-meta (rph/wrap)
         {::audit/props {:team-id (:team-id project)
                         :name (:name project)

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)
+    (let [perms (get-permissions conn profile-id id)]
-          deleted-at (dt/now)]
-
       (when-not (:is-owner perms)
         (ex/raise :type :validation
                   :code :only-owner-can-delete-team))
 
-      (db/update! conn :team
+      (delete-team conn id)
-                  {: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})
       nil)))
 
-
 ;; --- Mutation: Team Update Role
 
 (s/def ::team-id ::us/uuid)

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,107 @@
 
 
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
-;; 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})
+
+  :restored)
+
+
+
+(defn- restore-project*
+  [{:keys [::db/conn] :as cfg} 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* cfg id))
+
+  :restored)
+
+(defn- restore-team*
+  [{:keys [::db/conn] :as cfg} 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* cfg id))
+
+  :restored)
+
 (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
+    (db/tx-run! main/system restore-team* team-id)))
-                (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)})))))))
-
 
 (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
+    (db/tx-run! main/system restore-project* project-id)))
-                (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})]
 
-                    ;; Fragments are not handled here because they use
+(defn restore-deleted-file!
-                    ;; the database cascade operation and they are not
+  "Mark a file and all related objects as not deleted"
-                    ;; marked for deletion with objects-gc task
+  [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
+(defn delete-team!
-                                {:deleted-at nil}
+  "Mark a team for deletion"
-                                {:file-id (:id file)})
+  [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
+(defn delete-project!
-                    (db/update! conn :file-thumbnail
+  "Mark a project for deletion"
-                                {:deleted-at nil}
+  [project-id]
-                                {:file-id (:id file)})
+  (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
+(defn delete-file!
-                                {:deleted-at nil}
+  "Mark a project for deletion"
-                                {:file-id (:id file)}))))))
+  [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

backend/src/app/tasks/delete_object.clj

@@ -9,32 +9,46 @@
   (: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 id)
+  (l/trc :hint "marking for deletion" :rel "file" :id (str id))
-  ;; Mark file media objects to be deleted
+  (when-let [file (db/get* conn :file {:id id} {::db/remove-deleted false})]
-  (db/update! conn :file-media-object
+    (when (and (:is-shared file)
-              {:deleted-at deleted-at}
+               (not *team-deletion*))
-              {:file-id id})
+      ;; 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
+    ;; Mark file media objects to be deleted
-  (db/update! conn :file-thumbnail
+    (db/update! conn :file-media-object
-              {:deleted-at deleted-at}
+                {:deleted-at deleted-at}
-              {:file-id id})
+                {:file-id id})
 
-  (db/update! conn :file-tagged-object-thumbnail
+    ;; Mark thumbnails to be deleted
-              {:deleted-at deleted-at}
+    (db/update! conn :file-thumbnail
-              {:file-id id}))
+                {: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]}]
-  (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,17 +58,18 @@
 
 (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})
 
-  (doseq [project (db/update! conn :project
+  (binding [*team-deletion* true]
-                              {:deleted-at deleted-at}
+    (doseq [project (db/update! conn :project
-                              {:team-id id}
+                                {:deleted-at deleted-at}
-                              {::db/return-keys [:id :deleted-at]
+                                {:team-id id}
-                               ::db/many true})]
+                                {::db/return-keys [:id :deleted-at]
-    (delete-object cfg (assoc project :object :project))))
+                                 ::db/many true})]
+      (delete-object cfg (assoc project :object :project)))))
 
 (defmethod delete-object :default
   [_cfg props]

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)
+  (let [deleted-at (dt/now)]
-       (map :id)
+    (->> (db/cursor conn sql:get-orphan-teams)
-       (reduce (fn [total team-id]
+         (map :id)
-                 (l/trc :hint "mark orphan team for deletion" :id (str team-id))
+         (reduce (fn [total team-id]
-                 (db/update! conn :team
+                   (l/trc :hint "mark orphan team for deletion" :id (str team-id))
-                             {:deleted-at (dt/now)}
+
-                             {:id team-id})
+                   (db/update! conn :team
-                 (inc total))
+                               {:deleted-at deleted-at}
-               0)))
+                               {: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})))))

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))