From d8ae2764d08ef6474eee978ce03ffb83ab49c6e1 Mon Sep 17 00:00:00 2001 From: eileencodes Date: Sun, 11 May 2014 20:49:14 -0400 Subject: begin refactoring delete_records method Refactor by creating two methods delete_all_records and delete_records to be called by delete_all and delete (or destroy) respectively. This reduces the number of conditionals required to handle _how_ records get deleted. The new delete_count method handles how scope is applied to which delete action. A delete_all_records method also has to be called in has_many_through association because of how the methods are chained. This will be refactored later on. --- .../associations/collection_association.rb | 2 +- .../associations/has_many_association.rb | 37 +++++++++++++++------- .../associations/has_many_through_association.rb | 4 +++ 3 files changed, 30 insertions(+), 13 deletions(-) (limited to 'activerecord/lib') diff --git a/activerecord/lib/active_record/associations/collection_association.rb b/activerecord/lib/active_record/associations/collection_association.rb index 48628230c7..d83019a920 100644 --- a/activerecord/lib/active_record/associations/collection_association.rb +++ b/activerecord/lib/active_record/associations/collection_association.rb @@ -194,7 +194,7 @@ module ActiveRecord options[:dependent] end - delete_records(:all, dependent).tap do + delete_all_records(dependent).tap do reset loaded! end diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb index aac85a36c8..eed6b0d952 100644 --- a/activerecord/lib/active_record/associations/has_many_association.rb +++ b/activerecord/lib/active_record/associations/has_many_association.rb @@ -105,23 +105,36 @@ module ActiveRecord } end + def delete_count(records, method, scope) + case method + when :destroy + records.length + when :delete_all + scope.delete_all + else + scope.update_all(reflection.foreign_key => nil) + end + end + + def delete_all_records(method) + scope = self.scope + + count = delete_count(:all, method, scope) + + update_counter(-count) + end + # Deletes the records according to the :dependent option. def delete_records(records, method) + scope = self.scope.where(reflection.klass.primary_key => records) + + count = delete_count(records, method, scope) + if method == :destroy records.each(&:destroy!) - update_counter(-records.length) unless inverse_updates_counter_cache? + update_counter(-count) unless inverse_updates_counter_cache? else - if records == :all || !reflection.klass.primary_key - scope = self.scope - else - scope = self.scope.where(reflection.klass.primary_key => records) - end - - if method == :delete_all - update_counter(-scope.delete_all) - else - update_counter(-scope.update_all(reflection.foreign_key => nil)) - end + update_counter(-count) end end diff --git a/activerecord/lib/active_record/associations/has_many_through_association.rb b/activerecord/lib/active_record/associations/has_many_through_association.rb index aeb77e2753..5a45016cfa 100644 --- a/activerecord/lib/active_record/associations/has_many_through_association.rb +++ b/activerecord/lib/active_record/associations/has_many_through_association.rb @@ -130,6 +130,10 @@ module ActiveRecord end end + def delete_all_records(method) + delete_records(:all, method) + end + def delete_records(records, method) ensure_not_nested -- cgit v1.2.3 From 1c851b8cdfba73066b4d6c045d9d228647635f98 Mon Sep 17 00:00:00 2001 From: eileencodes Date: Mon, 12 May 2014 14:01:19 -0400 Subject: remove need for :all symbol Refactor delete_count method to only handle delete_all or nullify/nil cases and not destroy and switch to if/else rather than case statement. This refactoring allows removal of :all symbol usage. --- .../associations/has_many_association.rb | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) (limited to 'activerecord/lib') diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb index eed6b0d952..8da543f3ff 100644 --- a/activerecord/lib/active_record/associations/has_many_association.rb +++ b/activerecord/lib/active_record/associations/has_many_association.rb @@ -105,11 +105,8 @@ module ActiveRecord } end - def delete_count(records, method, scope) - case method - when :destroy - records.length - when :delete_all + def delete_count(method, scope) + if method == :delete_all scope.delete_all else scope.update_all(reflection.foreign_key => nil) @@ -117,23 +114,19 @@ module ActiveRecord end def delete_all_records(method) - scope = self.scope - - count = delete_count(:all, method, scope) - + count = delete_count(method, self.scope) update_counter(-count) end # Deletes the records according to the :dependent option. def delete_records(records, method) - scope = self.scope.where(reflection.klass.primary_key => records) - - count = delete_count(records, method, scope) - if method == :destroy + count = records.length records.each(&:destroy!) update_counter(-count) unless inverse_updates_counter_cache? else + scope = self.scope.where(reflection.klass.primary_key => records) + count = delete_count(method, scope) update_counter(-count) end end -- cgit v1.2.3 From 05a90c36c55fe0e43d60c2d0aa09d7a16ddd34ee Mon Sep 17 00:00:00 2001 From: eileencodes Date: Mon, 12 May 2014 14:11:15 -0400 Subject: rename delete_all_records to delete_or_nullify_all_records Rename delete_all_records because this name better describes what the method is doing. We can then remove :all from the hm:t version and pull out the unoptimized call to load_target in delete_records and pass it directly. --- .../lib/active_record/associations/collection_association.rb | 2 +- .../lib/active_record/associations/has_many_association.rb | 2 +- .../active_record/associations/has_many_through_association.rb | 8 ++------ 3 files changed, 4 insertions(+), 8 deletions(-) (limited to 'activerecord/lib') diff --git a/activerecord/lib/active_record/associations/collection_association.rb b/activerecord/lib/active_record/associations/collection_association.rb index d83019a920..caf4e612f9 100644 --- a/activerecord/lib/active_record/associations/collection_association.rb +++ b/activerecord/lib/active_record/associations/collection_association.rb @@ -194,7 +194,7 @@ module ActiveRecord options[:dependent] end - delete_all_records(dependent).tap do + delete_or_nullify_all_records(dependent).tap do reset loaded! end diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb index 8da543f3ff..4a7d8ed74a 100644 --- a/activerecord/lib/active_record/associations/has_many_association.rb +++ b/activerecord/lib/active_record/associations/has_many_association.rb @@ -113,7 +113,7 @@ module ActiveRecord end end - def delete_all_records(method) + def delete_or_nullify_all_records(method) count = delete_count(method, self.scope) update_counter(-count) end diff --git a/activerecord/lib/active_record/associations/has_many_through_association.rb b/activerecord/lib/active_record/associations/has_many_through_association.rb index 5a45016cfa..35ad512537 100644 --- a/activerecord/lib/active_record/associations/has_many_through_association.rb +++ b/activerecord/lib/active_record/associations/has_many_through_association.rb @@ -130,17 +130,13 @@ module ActiveRecord end end - def delete_all_records(method) - delete_records(:all, method) + def delete_or_nullify_all_records(method) + delete_records(load_target, method) end def delete_records(records, method) ensure_not_nested - # This is unoptimised; it will load all the target records - # even when we just want to delete everything. - records = load_target if records == :all - scope = through_association.scope scope.where! construct_join_attributes(*records) -- cgit v1.2.3 From 34db2b7caacefac2dd808b52a7f77d58ff6a8b69 Mon Sep 17 00:00:00 2001 From: eileencodes Date: Tue, 13 May 2014 20:55:36 -0400 Subject: remove count var this change was unneccsary as nothing was gained from it --- activerecord/lib/active_record/associations/has_many_association.rb | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) (limited to 'activerecord/lib') diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb index 4a7d8ed74a..f5e911c739 100644 --- a/activerecord/lib/active_record/associations/has_many_association.rb +++ b/activerecord/lib/active_record/associations/has_many_association.rb @@ -121,13 +121,11 @@ module ActiveRecord # Deletes the records according to the :dependent option. def delete_records(records, method) if method == :destroy - count = records.length records.each(&:destroy!) - update_counter(-count) unless inverse_updates_counter_cache? + update_counter(-records.length) unless inverse_updates_counter_cache? else scope = self.scope.where(reflection.klass.primary_key => records) - count = delete_count(method, scope) - update_counter(-count) + update_counter(-delete_count(method, scope)) end end -- cgit v1.2.3