Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Revert "Replace inline lambdas with named methods" and "Don't wrap op…

…erations 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."

This reverts commits df63c99 and b17fd25.

The change had unintended side effects, please see #2337.

Conflicts:

	activerecord/test/cases/associations/has_many_associations_test.rb
  • Loading branch information...
commit 048215a193e396516b6601b10e8ca9602314853c 1 parent 61c5c3d
@jonleighton jonleighton authored
View
67 activerecord/lib/active_record/associations/collection_association.rb
@@ -114,13 +114,19 @@ 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?
- if owner.new_record?
- concat_records(records)
- else
- transaction { concat_records(records) }
+ transaction 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
end
+
+ result && records
end
# Starts a transaction in the association class's database connection.
@@ -289,10 +295,14 @@ def replace(other_array)
other_array.each { |val| raise_on_type_mismatch(val) }
original_target = load_target.dup
- if owner.new_record?
- replace_records(other_array, original_target)
- else
- transaction { replace_records(other_array, original_target) }
+ transaction 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
end
end
@@ -428,20 +438,14 @@ def delete_or_destroy(records, method)
records.each { |record| raise_on_type_mismatch(record) }
existing_records = records.reject { |r| r.new_record? }
- if existing_records.empty?
- remove_records(existing_records, records, method)
- else
- transaction { remove_records(existing_records, records, method) }
- end
- end
-
- def remove_records(existing_records, records, method)
- records.each { |record| callback(:before_remove, record) }
+ transaction do
+ records.each { |record| callback(:before_remove, record) }
- delete_records(existing_records, method) if existing_records.any?
- records.each { |record| target.delete(record) }
+ delete_records(existing_records, method) if existing_records.any?
+ records.each { |record| target.delete(record) }
- records.each { |record| callback(:after_remove, record) }
+ records.each { |record| callback(:after_remove, record) }
+ end
end
# Delete the given records from the association, using one of the methods :destroy,
@@ -450,29 +454,6 @@ 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
View
78 activerecord/test/cases/associations/has_many_associations_test.rb
@@ -537,30 +537,6 @@ def test_adding_a_collection
assert_equal 3, companies(:first_firm).clients_of_firm(true).size
end
- def test_transactions_when_adding_to_persisted
- good = Client.new(:name => "Good")
- bad = Client.new(:name => "Bad", :raise_on_save => true)
-
- begin
- companies(:first_firm).clients_of_firm.concat(good, bad)
- rescue Client::RaisedOnSave
- end
-
- assert !companies(:first_firm).clients_of_firm(true).include?(good)
- end
-
- def test_transactions_when_adding_to_new_record
- prev_ignored_sql = ActiveRecord::SQLCounter.ignored_sql
- ActiveRecord::SQLCounter.ignored_sql = []
-
- assert_no_queries do
- firm = Firm.new
- firm.clients_of_firm.concat(Client.new("name" => "Natural Company"))
- end
- ensure
- ActiveRecord::SQLCounter.ignored_sql = prev_ignored_sql
- end
-
def test_new_aliased_to_build
company = companies(:first_firm)
new_client = assert_no_queries { company.clients_of_firm.new("name" => "Another Client") }
@@ -802,34 +778,6 @@ def test_delete_all_with_not_yet_loaded_association_collection
assert_equal 0, companies(:first_firm).clients_of_firm(true).size
end
- def test_transaction_when_deleting_persisted
- good = Client.new(:name => "Good")
- bad = Client.new(:name => "Bad", :raise_on_destroy => true)
-
- companies(:first_firm).clients_of_firm = [good, bad]
-
- begin
- companies(:first_firm).clients_of_firm.destroy(good, bad)
- rescue Client::RaisedOnDestroy
- end
-
- assert_equal [good, bad], companies(:first_firm).clients_of_firm(true)
- end
-
- def test_transaction_when_deleting_new_record
- prev_ignored_sql = ActiveRecord::SQLCounter.ignored_sql
- ActiveRecord::SQLCounter.ignored_sql = []
-
- assert_no_queries do
- firm = Firm.new
- client = Client.new("name" => "New Client")
- firm.clients_of_firm << client
- firm.clients_of_firm.destroy(client)
- end
- ensure
- ActiveRecord::SQLCounter.ignored_sql = prev_ignored_sql
- end
-
def test_clearing_an_association_collection
firm = companies(:first_firm)
client_id = firm.clients_of_firm.first.id
@@ -1163,32 +1111,6 @@ def test_replace_failure
assert_equal orig_accounts, firm.accounts
end
- def test_transactions_when_replacing_on_persisted
- good = Client.new(:name => "Good")
- bad = Client.new(:name => "Bad", :raise_on_save => true)
-
- companies(:first_firm).clients_of_firm = [good]
-
- begin
- companies(:first_firm).clients_of_firm = [bad]
- rescue Client::RaisedOnSave
- end
-
- assert_equal [good], companies(:first_firm).clients_of_firm(true)
- end
-
- def test_transactions_when_replacing_on_new_record
- prev_ignored_sql = ActiveRecord::SQLCounter.ignored_sql
- ActiveRecord::SQLCounter.ignored_sql = []
-
- assert_no_queries do
- firm = Firm.new
- firm.clients_of_firm = [Client.new("name" => "New Client")]
- end
- ensure
- ActiveRecord::SQLCounter.ignored_sql = prev_ignored_sql
- end
-
def test_get_ids
assert_equal [companies(:first_client).id, companies(:second_client).id], companies(:first_firm).client_ids
end

0 comments on commit 048215a

Please sign in to comment.
Something went wrong with that request. Please try again.