Refactor and fix some edge cases

#2

.github/workflows/tests.yml

@@ -24,7 +24,7 @@ jobs:
       CODECOV_TOKEN: '69b1a286-6f1c-4599-b715-74ad2db7728d'
 
     steps:
-      - uses: actions/checkout@v2
+      - uses: actions/checkout@v3
       - name: Set up Ruby ${{ matrix.ruby }}
         uses: ruby/setup-ruby@v1
         with:

Appraisals

@@ -7,8 +7,9 @@
   appraise "activerecord-#{version}" do
     gem 'activerecord', version
 
-    group :development do
+    group :development, :test do
       gem 'rails', version
+      remove_gem 'appraisal'
     end
   end
 end

Gemfile

@@ -1,4 +1,10 @@
 source 'https://rubygems.org'
+git_source(:github) { |repo| "https://github.com/#{repo}.git" }
 
 # Specify your gem's dependencies in delete_recursively.gemspec
 gemspec
+
+group :development, :test do
+  # until fix for https://github.com/thoughtbot/appraisal/issues/199 is released
+  gem 'appraisal', github: "thoughtbot/appraisal"
+end

README.md

@@ -1,4 +1,3 @@
-
 # DeleteRecursively
 
 [![Gem Version](https://badge.fury.io/rb/delete_recursively.svg)](http://badge.fury.io/rb/delete_recursively)

delete_recursively.gemspec

@@ -18,14 +18,13 @@ Gem::Specification.new do |s|
   s.email       = 'janosch84@gmail.com'
   s.homepage    = 'https://github.com/jaynetics/delete_recursively'
 
-  s.files       = ['lib/delete_recursively.rb',
-                   'lib/delete_recursively/version.rb']
+  s.files       = Dir['lib/**/*.rb']
 
   s.required_ruby_version = '>= 2.1.1'
 
   s.add_dependency 'activerecord', '>= 4.1.14', '< 8.0.0'
 
-  s.add_development_dependency 'appraisal', '~> 2.3'
+  s.add_development_dependency 'appraisal', '~> 2.4'
   s.add_development_dependency 'codecov', '~> 0.2'
   s.add_development_dependency 'rails', '>= 4.1.14', '< 8.0.0'
   s.add_development_dependency 'rake', '~> 13.0'

gemfiles/activerecord_5.2.6.gemfile

@@ -4,7 +4,7 @@ source "https://rubygems.org"
 
 gem "activerecord", "5.2.6"
 
-group :development do
+group :development, :test do
   gem "rails", "5.2.6"
 end
 

gemfiles/activerecord_6.1.4.1.gemfile

@@ -4,7 +4,7 @@ source "https://rubygems.org"
 
 gem "activerecord", "6.1.4.1"
 
-group :development do
+group :development, :test do
   gem "rails", "6.1.4.1"
 end
 

gemfiles/activerecord_7.0.1.gemfile

@@ -4,7 +4,7 @@ source "https://rubygems.org"
 
 gem "activerecord", "7.0.1"
 
-group :development do
+group :development, :test do
   gem "rails", "7.0.1"
 end
 

lib/delete_recursively.rb

@@ -6,200 +6,101 @@
 # Adds a new dependent: option to ActiveRecord associations.
 #
 module DeleteRecursively
-  require_relative File.join('delete_recursively', 'version')
-
   NEW_DEPENDENT_OPTION = :delete_recursively
 
-  # override ::valid_dependent_options to make the new
-  # dependent option available to Association::Builder classes
-  module OptionPermission
-    def valid_dependent_options
-      super + [NEW_DEPENDENT_OPTION]
-    end
-  end
-
-  # override Association#handle_dependency to apply the new option if it is set
-  module DependencyHandling
-    def handle_dependency
-      if DeleteRecursively.enabled_for?(self)
-        delete_dependencies_recursively
-      else
-        super
-      end
-    end
-
-    def delete_dependencies_recursively(force: false)
-      if reflection.belongs_to?
-        # Special case. The owner is already destroyed at this point,
-        # so we cannot use the efficient ::dependent_ids lookup. Note that this
-        # only happens for a single entry-record on #destroy, though.
-        return unless target = load_target
-
-        DeleteRecursively.delete_records_recursively(target.class, target.id, force: force)
-      else
-        DeleteRecursively.delete_recursively(reflection, owner.class, owner.id, force: force)
-      end
-    end
-  end
+  require_relative File.join('delete_recursively', 'active_record_extensions')
+  require_relative File.join('delete_recursively', 'associated_class_finder')
+  require_relative File.join('delete_recursively', 'dependent_id_finder')
+  require_relative File.join('delete_recursively', 'railtie') if defined?(::Rails::Railtie)
+  require_relative File.join('delete_recursively', 'version')
 
   class << self
-    def delete_recursively(reflection, owner_class, owner_ids, seen: [], force: false)
+    def delete_recursively(reflection, _legacy_arg, owner_ids, seen: [], force: false)
+      owner_ids = Array(owner_ids)
+      return if owner_ids.empty?
+
       # Dependent deletion can be bi-directional, so we need to avoid a loop.
-      return if seen.include?(reflection)
+      # Note, however, that an association could be reached multiple times, from
+      # different starting points within the association tree, and having
+      # different owner_ids. In this case, we do need to go through it again.
+      recursion_identifier = [reflection, owner_ids]
+      return if seen.include?(recursion_identifier)
 
-      seen << reflection
+      seen << recursion_identifier
 
-      associated_classes(reflection).each do |record_class|
+      AssociatedClassFinder.call(reflection).each do |assoc_class|
         record_ids = nil # fetched only when needed for recursion, deletion, or both
 
         if recurse_on?(reflection)
-          record_ids = dependent_ids(owner_class, owner_ids, reflection, record_class)
-          record_class.reflect_on_all_associations.each do |subref|
-            delete_recursively(subref, record_class, record_ids, seen: seen, force: force)
+          record_ids = DependentIdFinder.call(owner_ids, reflection, assoc_class)
+          assoc_class.reflect_on_all_associations.each do |subref|
+            delete_recursively(subref, nil, record_ids, seen: seen, force: force)
           end
         end
 
-        if dest_method = destructive_method(reflection, record_class, record_ids, force: force)
-          record_ids ||= dependent_ids(owner_class, owner_ids, reflection, record_class)
-          record_class.send(dest_method, record_ids)
+        if dest_method = destructive_method(reflection, force: force)
+          record_ids ||= DependentIdFinder.call(owner_ids, reflection, assoc_class)
+          assoc_class.send(dest_method, record_ids)
         end
       end
     end
 
-    def associated_classes(reflection)
-      if reflection.polymorphic?
-        # This ignores relatives where the inverse relation is not defined.
-        # The alternative to this approach would be to expensively select
-        # all distinct values from the *_type column:
-        # reflection.active_record.distinct.pluck(reflection.foreign_type)
-        ActiveRecord::Base.descendants.select do |klass|
-          klass.reflect_on_all_associations
-               .any? { |ref| ref.inverse_of == reflection }
-        end
-      else
-        [reflection.klass]
-      end
-    end
-
     def delete_records_recursively(record_class, record_ids, force: false)
       record_class.reflect_on_all_associations.each do |ref|
-        delete_recursively(ref, record_class, record_ids, force: force)
+        delete_recursively(ref, nil, record_ids, force: force)
       end
       record_class.delete(record_ids)
     end
 
-    def destructive_method(reflection, record_class, record_ids, force: false)
-      if deleting?(reflection) || force && destructive?(reflection)
-        :delete
-      elsif destructive?(reflection)
-        :destroy
-      end
-    end
-
-    def recurse_on?(reflection)
-      enabled_for?(reflection) || destructive?(reflection)
-    end
-
-    def enabled_for?(reflection)
-      reflection.options[:dependent] == NEW_DEPENDENT_OPTION
-    end
-
-    def destructive?(reflection)
-      %i[destroy destroy_all].include?(reflection.options[:dependent])
-    end
-
-    def deleting?(reflection)
-      [:delete, :delete_all, NEW_DEPENDENT_OPTION].include?(reflection.options[:dependent])
-    end
-
-    def dependent_ids(owner_class, owner_ids, reflection, assoc_class = nil)
-      if reflection.belongs_to?
-        owners = owner_class.where(owner_class.primary_key => owner_ids)
-        if reflection.polymorphic?
-          owners = owners.where(reflection.foreign_type => assoc_class.to_s)
-        end
-        owners.pluck(reflection.foreign_key).compact
-      elsif reflection.through_reflection
-        dependent_ids_with_through_option(owner_class, owner_ids, reflection)
-      else # plain `has_many` or `has_one`
-        owner_foreign_key = foreign_key(owner_class, reflection)
-        reflection.klass.where(owner_foreign_key => owner_ids).ids
-      end
-    end
-
-    def dependent_ids_with_through_option(owner_class, owner_ids, reflection)
-      through_reflection = reflection.through_reflection
-      owner_foreign_key = foreign_key(owner_class, through_reflection)
-
-      dependent_class = reflection.klass
-      dependent_through_reflection = inverse_through_reflection(reflection)
-      dependent_foreign_key =
-        foreign_key(dependent_class, dependent_through_reflection)
-
-      through_reflection.klass
-                        .where(owner_foreign_key => owner_ids)
-                        .pluck(dependent_foreign_key)
-    end
-
-    def inverse_through_reflection(reflection)
-      through_class = reflection.through_reflection.klass
-      reflection.klass.reflect_on_all_associations
-                .map(&:through_reflection)
-                .find { |thr_ref| thr_ref && thr_ref.klass == through_class }
-    end
-
-    def foreign_key(owner_class, reflection)
-      reflection && reflection.foreign_key || owner_class.to_s.foreign_key
-    end
-
     def all(record_class, criteria = {}, seen = [])
       return if seen.include?(record_class)
 
       seen << record_class
 
       record_class.reflect_on_all_associations.each do |reflection|
-        associated_classes(reflection).each do |assoc_class|
+        AssociatedClassFinder.call(reflection).each do |assoc_class|
           if recurse_on?(reflection)
             all(assoc_class, criteria, seen)
           elsif deleting?(reflection)
             delete_with_applicable_criteria(assoc_class, criteria)
           end
         end
       end
+
       delete_with_applicable_criteria(record_class, criteria)
     end
 
+    def enabled_for?(reflection)
+      reflection.options[:dependent] == NEW_DEPENDENT_OPTION
+    end
+
+    private
+
     def delete_with_applicable_criteria(record_class, criteria)
       applicable_criteria = criteria.select do |column_name, _value|
         record_class.column_names.include?(column_name.to_s)
       end
       record_class.where(applicable_criteria).delete_all
     end
-  end
-end
-
-require 'active_record'
 
-module ActiveRecord
-  module Associations
-    %w[BelongsTo HasMany HasOne].each do |assoc_name|
-      assoc_builder = Builder.const_get(assoc_name)
-      assoc_builder.singleton_class.prepend(DeleteRecursively::OptionPermission)
+    def recurse_on?(reflection)
+      enabled_for?(reflection) || destructive?(reflection)
+    end
 
-      assoc_class = const_get("#{assoc_name}Association")
-      assoc_class.prepend(DeleteRecursively::DependencyHandling)
+    def destructive?(reflection)
+      %i[destroy destroy_all].include?(reflection.options[:dependent])
     end
-  end
 
-  class Base
-    def delete_recursively(force: false)
-      DeleteRecursively.delete_records_recursively(self.class, id, force: force)
+    def deleting?(reflection)
+      [:delete, :delete_all, NEW_DEPENDENT_OPTION].include?(reflection.options[:dependent])
     end
-  end
 
-  class Relation
-    def delete_all_recursively(force: false)
-      DeleteRecursively.delete_records_recursively(klass, ids, force: force)
+    def destructive_method(reflection, force: false)
+      if deleting?(reflection) || force && destructive?(reflection)
+        :delete
+      elsif destructive?(reflection)
+        :destroy
+      end
     end
   end
 end

lib/delete_recursively/active_record_extensions.rb

@@ -0,0 +1,57 @@
+# override ::valid_dependent_options to make the new
+# dependent option available to Association::Builder classes
+module DeleteRecursively::OptionPermission
+  def valid_dependent_options
+    super + [DeleteRecursively::NEW_DEPENDENT_OPTION]
+  end
+end
+
+# override Association#handle_dependency to apply the new option if it is set
+module DeleteRecursively::DependencyHandling
+  def handle_dependency
+    if DeleteRecursively.enabled_for?(self)
+      delete_dependencies_recursively
+    else
+      super
+    end
+  end
+
+  def delete_dependencies_recursively(force: false)
+    if reflection.belongs_to?
+      # Special case. The owner is already destroyed at this point,
+      # so we cannot use the efficient ::dependent_ids lookup. Note that this
+      # only happens for a single entry-record on #destroy, though.
+      return unless target = load_target
+
+      DeleteRecursively.delete_records_recursively(target.class, target.id, force: force)
+    else
+      DeleteRecursively.delete_recursively(reflection, nil, owner.id, force: force)
+    end
+  end
+end
+
+require 'active_record'
+
+module ActiveRecord
+  module Associations
+    %w[BelongsTo HasMany HasOne].each do |assoc_name|
+      assoc_builder = Builder.const_get(assoc_name)
+      assoc_builder.singleton_class.prepend(DeleteRecursively::OptionPermission)
+
+      assoc_class = const_get("#{assoc_name}Association")
+      assoc_class.prepend(DeleteRecursively::DependencyHandling)
+    end
+  end
+
+  class Base
+    def delete_recursively(force: false)
+      DeleteRecursively.delete_records_recursively(self.class, id, force: force)
+    end
+  end
+
+  class Relation
+    def delete_all_recursively(force: false)
+      DeleteRecursively.delete_records_recursively(klass, ids, force: force)
+    end
+  end
+end