diff --git a/activerecord/lib/active_record/associations/collection_association.rb b/activerecord/lib/active_record/associations/collection_association.rb index 0835aa63087c2..1c174b51f7199 100644 --- a/activerecord/lib/active_record/associations/collection_association.rb +++ b/activerecord/lib/active_record/associations/collection_association.rb @@ -114,21 +114,13 @@ def create!(attributes = {}, options = {}, &block) # Add +records+ to this association. Returns +self+ so method calls may be chained. # Since << flattens its argument list and inserts each record, +push+ and +concat+ behave identically. def concat(*records) - result = true load_target if owner.new_record? - block = lambda do - records.flatten.each do |record| - raise_on_type_mismatch(record) - add_to_target(record) do |r| - result &&= insert_record(record) unless owner.new_record? - end - end + if owner.new_record? + concat_records(records) + else + transaction { concat_records(records) } end - - owner.new_record? ? block.call : transaction(&block) - - result && records end # Starts a transaction in the association class's database connection. @@ -297,17 +289,11 @@ def replace(other_array) other_array.each { |val| raise_on_type_mismatch(val) } original_target = load_target.dup - block = lambda do - delete(target - other_array) - - unless concat(other_array - target) - @target = original_target - raise RecordNotSaved, "Failed to replace #{reflection.name} because one or more of the " \ - "new records could not be saved." - end + if owner.new_record? + replace_records(other_array, original_target) + else + transaction { replace_records(other_array, original_target) } end - - owner.new_record? ? block.call : transaction(&block) end def include?(record) @@ -448,16 +434,20 @@ def delete_or_destroy(records, method) records.each { |record| raise_on_type_mismatch(record) } existing_records = records.reject { |r| r.new_record? } - block = lambda do - records.each { |record| callback(:before_remove, record) } + if existing_records.empty? + remove_records(existing_records, records, method) + else + transaction { remove_records(existing_records, records, method) } + end + end - delete_records(existing_records, method) if existing_records.any? - records.each { |record| target.delete(record) } + def remove_records(existing_records, records, method) + records.each { |record| callback(:before_remove, record) } - records.each { |record| callback(:after_remove, record) } - end + delete_records(existing_records, method) if existing_records.any? + records.each { |record| target.delete(record) } - existing_records.any? ? transaction(&block) : block.call + records.each { |record| callback(:after_remove, record) } end # Delete the given records from the association, using one of the methods :destroy, @@ -466,6 +456,29 @@ def delete_records(records, method) raise NotImplementedError end + def replace_records(new_target, original_target) + delete(target - new_target) + + unless concat(new_target - target) + @target = original_target + raise RecordNotSaved, "Failed to replace #{reflection.name} because one or more of the " \ + "new records could not be saved." + end + end + + def concat_records(records) + result = true + + records.flatten.each do |record| + raise_on_type_mismatch(record) + add_to_target(record) do |r| + result &&= insert_record(record) unless owner.new_record? + end + end + + result && records + end + def callback(method, record) callbacks_for(method).each do |callback| case callback