From e62b57647258fad34129975c5a264d19af2dbbe8 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Sun, 6 Feb 2011 22:14:16 +0000 Subject: Refactor the implementations of AssociatioCollection#delete and #destroy to be more consistent with each other, and to stop passing blocks around, thus making the execution easier to follow. --- .../associations/association_collection.rb | 26 +++++++++------------- .../has_and_belongs_to_many_association.rb | 2 +- .../associations/has_many_association.rb | 2 +- .../associations/has_many_through_association.rb | 2 +- 4 files changed, 14 insertions(+), 18 deletions(-) (limited to 'activerecord/lib') diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb index acb9fe7ff8..3e3237c348 100644 --- a/activerecord/lib/active_record/associations/association_collection.rb +++ b/activerecord/lib/active_record/associations/association_collection.rb @@ -178,10 +178,7 @@ module ActiveRecord # are actually removed from the database, that depends precisely on # +delete_records+. They are in any case removed from the collection. def delete(*records) - remove_records(records) do |_records, old_records| - delete_records(old_records) if old_records.any? - _records.each { |record| @target.delete(record) } - end + delete_or_destroy(records, @reflection.options[:dependent]) end # Destroy +records+ and remove them from this association calling @@ -190,12 +187,8 @@ module ActiveRecord # Note that this method will _always_ remove records from the database # ignoring the +:dependent+ option. def destroy(*records) - records = find(records) if records.any? {|record| record.kind_of?(Fixnum) || record.kind_of?(String)} - remove_records(records) do |_records, old_records| - delete_records(old_records, :destroy) if old_records.any? - end - - load_target + records = find(records) if records.any? { |record| record.kind_of?(Fixnum) || record.kind_of?(String) } + delete_or_destroy(records, :destroy) end def create(attrs = {}) @@ -450,21 +443,24 @@ module ActiveRecord add_record_to_target_with_callbacks(record, &block) end - def remove_records(*records) + def delete_or_destroy(records, method) records = records.flatten records.each { |record| raise_on_type_mismatch(record) } + existing_records = records.reject { |r| r.new_record? } transaction do records.each { |record| callback(:before_remove, record) } - old_records = records.reject { |r| r.new_record? } - yield(records, old_records) + + delete_records(existing_records, method) if existing_records.any? + records.each { |record| @target.delete(record) } + records.each { |record| callback(:after_remove, record) } end end # Delete the given records from the association, using one of the methods :destroy, - # :delete_all or :nullify. The default method used is given by the :dependent option. - def delete_records(records, method = @reflection.options[:dependent]) + # :delete_all or :nullify (or nil, in which case a default is used). + def delete_records(records, method) raise NotImplementedError end diff --git a/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb b/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb index d70f326e55..b0ccab2de6 100644 --- a/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb +++ b/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb @@ -40,7 +40,7 @@ module ActiveRecord load_target.size end - def delete_records(records, method = nil) + def delete_records(records, method) if sql = @reflection.options[:delete_sql] records.each { |record| @owner.connection.delete(interpolate_sql(sql, record)) } else diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb index 732f63713e..0b246587c0 100644 --- a/activerecord/lib/active_record/associations/has_many_association.rb +++ b/activerecord/lib/active_record/associations/has_many_association.rb @@ -80,7 +80,7 @@ module ActiveRecord end # Deletes the records according to the :dependent option. - def delete_records(records, method = @reflection.options[:dependent]) + def delete_records(records, method) if method == :destroy records.each { |r| r.destroy } update_counter(-records.length) unless inverse_updates_counter_cache? 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 040de7e98f..81dd373f7b 100644 --- a/activerecord/lib/active_record/associations/has_many_through_association.rb +++ b/activerecord/lib/active_record/associations/has_many_through_association.rb @@ -57,7 +57,7 @@ module ActiveRecord end end - def delete_records(records, method = @reflection.options[:dependent]) + def delete_records(records, method) through = @owner.send(:association_proxy, @reflection.through_reflection.name) scope = through.scoped.where(construct_join_attributes(*records)) -- cgit v1.2.3