Don't wrap operations on collection associations in transactions when they are not needed, so the connection adapter does not send empty BEGIN COMMIT transactions blocks to the database.

2 files changed, 54 insertions, 3 deletions

diff --git a/activerecord/lib/active_record/associations/collection_association.rb b/activerecord/lib/active_record/associations/collection_association.rb
index 7e1a41e84d..7a8c0bf59f 100644
--- a/activerecord/lib/active_record/associations/collection_association.rb
+++ b/activerecord/lib/active_record/associations/collection_association.rb
@@ -117,7 +117,7 @@ module ActiveRecord
         result = true
         load_target if owner.new_record?
 
-        transaction do
+        block = lambda do
           records.flatten.each do |record|
             raise_on_type_mismatch(record)
             add_to_target(record) do |r|
@@ -126,6 +126,8 @@ module ActiveRecord
           end
         end
 
+        owner.new_record? ? block.call : transaction(&block)
+
         result && records
       end
 
@@ -295,7 +297,7 @@ module ActiveRecord
         other_array.each { |val| raise_on_type_mismatch(val) }
         original_target = load_target.dup
 
-        transaction do
+        block = lambda do
           delete(target - other_array)
 
           unless concat(other_array - target)
@@ -304,6 +306,8 @@ module ActiveRecord
                                   "new records could not be saved."
           end
         end
+
+        owner.new_record? ? block.call : transaction(&block)
       end
 
       def include?(record)
@@ -444,7 +448,7 @@ module ActiveRecord
           records.each { |record| raise_on_type_mismatch(record) }
           existing_records = records.reject { |r| r.new_record? }
 
-          transaction do
+          block = lambda do
             records.each { |record| callback(:before_remove, record) }
 
             delete_records(existing_records, method) if existing_records.any?
@@ -452,6 +456,8 @@ module ActiveRecord
 
             records.each { |record| callback(:after_remove, record) }
           end
+
+          existing_records.any? ? transaction(&block) : block.call
         end
 
         # Delete the given records from the association, using one of the methods :destroy,
diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb
index 49999630b6..79b56d81df 100644
--- a/activerecord/test/cases/associations/has_many_associations_test.rb
+++ b/activerecord/test/cases/associations/has_many_associations_test.rb
@@ -537,6 +537,18 @@ class HasManyAssociationsTest < ActiveRecord::TestCase
     assert_equal 3, companies(:first_firm).clients_of_firm(true).size
   end
 
+  def test_transactions_when_adding_to_persisted
+    force_signal37_to_load_all_clients_of_firm
+    Client.expects(:transaction)
+    companies(:first_firm).clients_of_firm.concat(Client.new("name" => "Natural Company"))
+  end
+
+  def test_transactions_when_adding_to_new_record
+    Client.expects(:transaction).never
+    firm = Firm.new
+    firm.clients_of_firm.concat(Client.new("name" => "Natural Company"))
+  end
+
   def test_new_aliased_to_build
     company = companies(:first_firm)
     new_client = assert_no_queries { company.clients_of_firm.new("name" => "Another Client") }
@@ -778,6 +790,21 @@ class HasManyAssociationsTest < ActiveRecord::TestCase
     assert_equal 0, companies(:first_firm).clients_of_firm(true).size
   end
 
+  def test_transaction_when_deleting_persisted
+    force_signal37_to_load_all_clients_of_firm
+    client = companies(:first_firm).clients_of_firm.create("name" => "Another Client")
+    Client.expects(:transaction)
+    companies(:first_firm).clients_of_firm.delete(client)
+  end
+
+  def test_transaction_when_deleting_new_record
+    client = Client.new("name" => "New Client")
+    firm = Firm.new
+    firm.clients_of_firm << client
+    Client.expects(:transaction).never
+    firm.clients_of_firm.delete(client)
+  end
+
   def test_clearing_an_association_collection
     firm = companies(:first_firm)
     client_id = firm.clients_of_firm.first.id
@@ -1111,6 +1138,24 @@ class HasManyAssociationsTest < ActiveRecord::TestCase
     assert_equal orig_accounts, firm.accounts
   end
 
+  def test_transactions_when_replacing_on_persisted
+    firm = Firm.find(:first, :order => "id")
+    firm.clients = [companies(:first_client)]
+    assert firm.save, "Could not save firm"
+    firm.reload
+
+    Client.expects(:transaction)
+    firm.clients_of_firm = [Client.new("name" => "Natural Company")]
+  end
+
+  def test_transactions_when_replacing_on_new_record
+    firm = Firm.new
+    firm.clients_of_firm << Client.new("name" => "Natural Company")
+
+    Client.expects(:transaction).never
+    firm.clients_of_firm = [Client.new("name" => "New Client")]
+  end
+
   def test_get_ids
     assert_equal [companies(:first_client).id, companies(:second_client).id], companies(:first_firm).client_ids
   end