Simplify FastDestroyAll module

app/models/ci/build_trace_chunk.rb

@@ -6,7 +6,6 @@ module Ci
     belongs_to :build, class_name: "Ci::Build", foreign_key: :build_id
 
     default_value_for :data_store, :redis
-    fast_destroy_all_with :redis_delete_data, :redis_data_keys
 
     WriteError = Class.new(StandardError)
 
@@ -39,6 +38,18 @@ module Ci
           redis.del(keys)
         end
       end
+
+      ##
+      # FastDestroyAll concerns
+      def begin_fast_destroy
+        redis_data_keys
+      end
+
+      ##
+      # FastDestroyAll concerns
+      def finalize_fast_destroy(keys)
+        redis_delete_data(keys)
+      end
     end
 
     ##

app/models/concerns/fast_destroy_all.rb

@@ -4,23 +4,33 @@
 # In general, `destroy_all` is inefficient because it calls each callback with `DELETE` queries i.e. O(n), whereas,
 # `delete_all` is efficient as it deletes all rows with a single `DELETE` query.
 #
-# It's better to use `delete_all` as much as possible, however, in the real cases, it's hard to adopt because
-# in some cases, external data (in ObjectStorage/FileStorage/Redis) is assosiated with rows.
+# It's better to use `delete_all` as our best practice, however,
+# if external data (e.g. ObjectStorage, FileStorage or Redis) are assosiated with database records,
+# it is difficult to accomplish it.
 #
-# This module introduces a protocol to support the adoption with easy way.
-# You can use in the following scnenes
-# - When calling `destroy_all` explicitly
-#   e.g. `job.trace_chunks.fast_destroy_all``
-# - When a parent record is deleted and all children are deleted subsequently (cascade delete)
-#   e.g. `before_destroy -> { run_after_commit(&build_trace_chunks.after_commit_cleanup_proc) }``
+# This module defines a format to use `delete_all` and delete associated external data.
+# Here is an exmaple
+#
+# Situation
+# - `Project` has many `Ci::BuildTraceChunk` through `Ci::Build`
+# - `Ci::BuildTraceChunk` stores associated data in Redis, so it relies on `dependent: :destroy` and `before_destroy` for the deletion
+#
+# How to use
+# - Define `use_fast_destroy :build_trace_chunks` in `Project` model.
+# - Define `begin_fast_destroy` and `finalize_fast_destroy(params)` in `Ci::BuildTraceChunk` model.
+# - Use `fast_destroy_all` instead of `destroy` and `destroy_all`
+# - Remove `dependent: :destroy` and `before_destroy` as it's no longer need
+#
+# Expectation
+# - When a project is `destroy`ed, the associated trace_chunks will be deleted by `delete_all`,
+#   and the associated data will be removed, too.
+# - When `fast_destroy_all` is called, it also performns as same.
 module FastDestroyAll
   extend ActiveSupport::Concern
 
   ForbiddenActionError = Class.new(StandardError)
 
   included do
-    class_attribute :_delete_method, :_delete_params_generator
-
     before_destroy do
       raise ForbiddenActionError, '`destroy` and `destroy_all` are forbbiden. Please use `fast_destroy_all`'
     end
@@ -28,40 +38,31 @@ module FastDestroyAll
 
   class_methods do
     ##
-    # This method is for registering :delete_method and :delete_params_generator
-    # You have to define the method if you want to use `FastDestroyAll`` module.
+    # This method delete rows and associated external data efficiently
     #
-    # e.g. fast_destroy_all_with :delete_all_redis_data, :redis_all_data_keys
-    def fast_destroy_all_with(delete_method, delete_params_generator)
-      self._delete_method = delete_method
-      self._delete_params_generator = delete_params_generator
+    # This method can replace `destroy` and `destroy_all` withtout having `after_destroy` hook
+    def fast_destroy_all
+      params = begin_fast_destroy
+
+      delete_all
+
+      finalize_fast_destroy(params)
     end
 
     ##
-    # This method generates a proc to delete external data.
-    # It's useful especially when you want to hook parent record's deletion event.
+    # This method returns identifiers to delete associated external data (e.g. file paths, redis keys)
     #
-    # e.g. before_destroy -> { run_after_commit(&build_trace_chunks.after_commit_cleanup_proc) }
-    def after_commit_cleanup_proc
-      params = send self._delete_params_generator # rubocop:disable GitlabSecurity/PublicSend
-      subject = self # Preserve the subject class, otherwise `proc` uses a different class
-
-      proc do
-        subject.send subject._delete_method, params # rubocop:disable GitlabSecurity/PublicSend
-
-        true
-      end
+    # This method must be defined in fast destroyable model
+    def begin_fast_destroy
+      raise NotImplementedError
     end
 
     ##
-    # This method deletes all rows at first and delete all external data at second.
-    # Before deleting the rows, it generates a proc to delete external data.
-    # So it won't lose the track of deleting external data, even if it happened after rows had been deleted.
-    def fast_destroy_all
-      after_commit_cleanup_proc.tap do |delete_all_external_data|
-        delete_all
-        delete_all_external_data.call
-      end
+    # This method deletes associated external data with the identifiers returned by `begin_fast_destroy`
+    #
+    # This method must be defined in fast destroyable model
+    def finalize_fast_destroy(params)
+      raise NotImplementedError
     end
   end
 
@@ -70,20 +71,21 @@ module FastDestroyAll
 
     class_methods do
       ##
-      # This is a helper method for performaning fast_destroy_all when parent relations are deleted
-      # Their children must include `FastDestroyAll` module.
-      #
-      # `use_fast_destroy` must be defined **before** `has_many` and `has_one`, such as `has_many :relation, dependent: :destroy`
-      # Otherwise `use_fast_destroy` performs against **deleted** rows, which fails to get identifiers of external data
-      #
-      # e.g. use_fast_destroy :build_trace_chunks
+      # This method is to be defined on models which have fast destroyable models as children,
+      # and let us avoid to use `dependent: :destroy` hook
       def use_fast_destroy(relation)
         before_destroy do
-          subject = public_send(relation) # rubocop:disable GitlabSecurity/PublicSend
-          after_commit_proc = subject.after_commit_cleanup_proc
-          run_after_commit(&after_commit_proc)
+          perform_fast_destroy(public_send(relation)) # rubocop:disable GitlabSecurity/PublicSend
         end
       end
     end
+
+    def perform_fast_destroy(subject)
+      params = subject.begin_fast_destroy
+
+      run_after_commit do
+        subject.finalize_fast_destroy(params)
+      end
+    end
   end
 end