diff options
Diffstat (limited to 'activerecord')
17 files changed, 132 insertions, 87 deletions
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 36a3d59784..dda7d19915 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,7 @@ +* Fix logic on disabling commit callbacks so they are not called unexpectedly when errors occur. + + *Brian Durand* + * Ensure `Associations::CollectionAssociation#size` and `Associations::CollectionAssociation#empty?` use loaded association ids if present. diff --git a/activerecord/lib/active_record/associations/has_one_association.rb b/activerecord/lib/active_record/associations/has_one_association.rb index 090b082cb0..4e8e7ee43b 100644 --- a/activerecord/lib/active_record/associations/has_one_association.rb +++ b/activerecord/lib/active_record/associations/has_one_association.rb @@ -107,6 +107,14 @@ module ActiveRecord yield end end + + def _create_record(attributes, raise_error = false) + unless owner.persisted? + raise ActiveRecord::RecordNotSaved, "You cannot call create unless the parent is saved" + end + + super + end end end end diff --git a/activerecord/lib/active_record/associations/singular_association.rb b/activerecord/lib/active_record/associations/singular_association.rb index 441bd715e4..ead89bfe6c 100644 --- a/activerecord/lib/active_record/associations/singular_association.rb +++ b/activerecord/lib/active_record/associations/singular_association.rb @@ -63,10 +63,6 @@ module ActiveRecord end def _create_record(attributes, raise_error = false) - unless owner.persisted? - raise ActiveRecord::RecordNotSaved, "You cannot call create unless the parent is saved" - end - record = build_record(attributes) yield(record) if block_given? saved = record.save diff --git a/activerecord/lib/active_record/transactions.rb b/activerecord/lib/active_record/transactions.rb index 97cba5d1c7..be4f41050e 100644 --- a/activerecord/lib/active_record/transactions.rb +++ b/activerecord/lib/active_record/transactions.rb @@ -340,7 +340,7 @@ module ActiveRecord # Ensure that it is not called if the object was never persisted (failed create), # but call it after the commit of a destroyed object. def committed!(should_run_callbacks: true) #:nodoc: - if should_run_callbacks && destroyed? || persisted? + if should_run_callbacks && (destroyed? || persisted?) _run_commit_without_transaction_enrollment_callbacks _run_commit_callbacks end diff --git a/activerecord/test/cases/arel/attributes/attribute_test.rb b/activerecord/test/cases/arel/attributes/attribute_test.rb index 52573021a5..671e273543 100644 --- a/activerecord/test/cases/arel/attributes/attribute_test.rb +++ b/activerecord/test/cases/arel/attributes/attribute_test.rb @@ -978,7 +978,7 @@ module Arel table = Table.new(:foo) condition = table["id"].eq("1") - refute table.able_to_type_cast? + assert_not table.able_to_type_cast? condition.to_sql.must_equal %("foo"."id" = '1') end diff --git a/activerecord/test/cases/arel/helper.rb b/activerecord/test/cases/arel/helper.rb index 1f8612f799..f8ce658440 100644 --- a/activerecord/test/cases/arel/helper.rb +++ b/activerecord/test/cases/arel/helper.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -require "rubygems" +require "active_support" require "minitest/autorun" require "arel" @@ -13,7 +13,7 @@ class Object end module Arel - class Test < Minitest::Test + class Test < ActiveSupport::TestCase def setup super @arel_engine = Arel::Table.engine @@ -24,11 +24,6 @@ module Arel Arel::Table.engine = @arel_engine if defined? @arel_engine super end - - def assert_like(expected, actual) - assert_equal expected.gsub(/\s+/, " ").strip, - actual.gsub(/\s+/, " ").strip - end end class Spec < Minitest::Spec @@ -40,5 +35,11 @@ module Arel after do Arel::Table.engine = @arel_engine if defined? @arel_engine end + include ActiveSupport::Testing::Assertions + + # test/unit backwards compatibility methods + alias :assert_no_match :refute_match + alias :assert_not_equal :refute_equal + alias :assert_not_same :refute_same end end diff --git a/activerecord/test/cases/arel/nodes/ascending_test.rb b/activerecord/test/cases/arel/nodes/ascending_test.rb index 1efb16222a..4811e6ff5b 100644 --- a/activerecord/test/cases/arel/nodes/ascending_test.rb +++ b/activerecord/test/cases/arel/nodes/ascending_test.rb @@ -29,7 +29,7 @@ module Arel def test_descending? ascending = Ascending.new "zomg" - assert !ascending.descending? + assert_not ascending.descending? end def test_equality_with_same_ivars diff --git a/activerecord/test/cases/arel/nodes/binary_test.rb b/activerecord/test/cases/arel/nodes/binary_test.rb index 9bc55a155b..d160e7cd9d 100644 --- a/activerecord/test/cases/arel/nodes/binary_test.rb +++ b/activerecord/test/cases/arel/nodes/binary_test.rb @@ -4,22 +4,24 @@ require_relative "../helper" module Arel module Nodes - describe "Binary" do - describe "#hash" do - it "generates a hash based on its value" do - eq = Equality.new("foo", "bar") - eq2 = Equality.new("foo", "bar") - eq3 = Equality.new("bar", "baz") + class NodesTest < Arel::Spec + describe "Binary" do + describe "#hash" do + it "generates a hash based on its value" do + eq = Equality.new("foo", "bar") + eq2 = Equality.new("foo", "bar") + eq3 = Equality.new("bar", "baz") - assert_equal eq.hash, eq2.hash - refute_equal eq.hash, eq3.hash - end + assert_equal eq.hash, eq2.hash + assert_not_equal eq.hash, eq3.hash + end - it "generates a hash specific to its class" do - eq = Equality.new("foo", "bar") - neq = NotEqual.new("foo", "bar") + it "generates a hash specific to its class" do + eq = Equality.new("foo", "bar") + neq = NotEqual.new("foo", "bar") - refute_equal eq.hash, neq.hash + assert_not_equal eq.hash, neq.hash + end end end end diff --git a/activerecord/test/cases/arel/nodes/case_test.rb b/activerecord/test/cases/arel/nodes/case_test.rb index 2c087e624e..89861488df 100644 --- a/activerecord/test/cases/arel/nodes/case_test.rb +++ b/activerecord/test/cases/arel/nodes/case_test.rb @@ -4,79 +4,81 @@ require_relative "../helper" module Arel module Nodes - describe "Case" do - describe "#initialize" do - it "sets case expression from first argument" do - node = Case.new "foo" + class NodesTest < Arel::Spec + describe "Case" do + describe "#initialize" do + it "sets case expression from first argument" do + node = Case.new "foo" - assert_equal "foo", node.case - end + assert_equal "foo", node.case + end - it "sets default case from second argument" do - node = Case.new nil, "bar" + it "sets default case from second argument" do + node = Case.new nil, "bar" - assert_equal "bar", node.default + assert_equal "bar", node.default + end end - end - describe "#clone" do - it "clones case, conditions and default" do - foo = Nodes.build_quoted "foo" + describe "#clone" do + it "clones case, conditions and default" do + foo = Nodes.build_quoted "foo" - node = Case.new - node.case = foo - node.conditions = [When.new(foo, foo)] - node.default = foo + node = Case.new + node.case = foo + node.conditions = [When.new(foo, foo)] + node.default = foo - dolly = node.clone + dolly = node.clone - assert_equal dolly.case, node.case - refute_same dolly.case, node.case + assert_equal dolly.case, node.case + assert_not_same dolly.case, node.case - assert_equal dolly.conditions, node.conditions - refute_same dolly.conditions, node.conditions + assert_equal dolly.conditions, node.conditions + assert_not_same dolly.conditions, node.conditions - assert_equal dolly.default, node.default - refute_same dolly.default, node.default + assert_equal dolly.default, node.default + assert_not_same dolly.default, node.default + end end - end - describe "equality" do - it "is equal with equal ivars" do - foo = Nodes.build_quoted "foo" - one = Nodes.build_quoted 1 - zero = Nodes.build_quoted 0 + describe "equality" do + it "is equal with equal ivars" do + foo = Nodes.build_quoted "foo" + one = Nodes.build_quoted 1 + zero = Nodes.build_quoted 0 - case1 = Case.new foo - case1.conditions = [When.new(foo, one)] - case1.default = Else.new zero + case1 = Case.new foo + case1.conditions = [When.new(foo, one)] + case1.default = Else.new zero - case2 = Case.new foo - case2.conditions = [When.new(foo, one)] - case2.default = Else.new zero + case2 = Case.new foo + case2.conditions = [When.new(foo, one)] + case2.default = Else.new zero - array = [case1, case2] + array = [case1, case2] - assert_equal 1, array.uniq.size - end + assert_equal 1, array.uniq.size + end - it "is not equal with different ivars" do - foo = Nodes.build_quoted "foo" - bar = Nodes.build_quoted "bar" - one = Nodes.build_quoted 1 - zero = Nodes.build_quoted 0 + it "is not equal with different ivars" do + foo = Nodes.build_quoted "foo" + bar = Nodes.build_quoted "bar" + one = Nodes.build_quoted 1 + zero = Nodes.build_quoted 0 - case1 = Case.new foo - case1.conditions = [When.new(foo, one)] - case1.default = Else.new zero + case1 = Case.new foo + case1.conditions = [When.new(foo, one)] + case1.default = Else.new zero - case2 = Case.new foo - case2.conditions = [When.new(bar, one)] - case2.default = Else.new zero + case2 = Case.new foo + case2.conditions = [When.new(bar, one)] + case2.default = Else.new zero - array = [case1, case2] + array = [case1, case2] - assert_equal 2, array.uniq.size + assert_equal 2, array.uniq.size + end end end end diff --git a/activerecord/test/cases/arel/nodes/descending_test.rb b/activerecord/test/cases/arel/nodes/descending_test.rb index 45e22de17b..5f1747e1da 100644 --- a/activerecord/test/cases/arel/nodes/descending_test.rb +++ b/activerecord/test/cases/arel/nodes/descending_test.rb @@ -24,7 +24,7 @@ module Arel def test_ascending? descending = Descending.new "zomg" - assert !descending.ascending? + assert_not descending.ascending? end def test_descending? diff --git a/activerecord/test/cases/arel/nodes/select_core_test.rb b/activerecord/test/cases/arel/nodes/select_core_test.rb index 1cdc7a2360..0b698205ff 100644 --- a/activerecord/test/cases/arel/nodes/select_core_test.rb +++ b/activerecord/test/cases/arel/nodes/select_core_test.rb @@ -17,9 +17,9 @@ module Arel assert_equal core.projections, dolly.projections assert_equal core.wheres, dolly.wheres - refute_same core.froms, dolly.froms - refute_same core.projections, dolly.projections - refute_same core.wheres, dolly.wheres + assert_not_same core.froms, dolly.froms + assert_not_same core.projections, dolly.projections + assert_not_same core.wheres, dolly.wheres end def test_set_quantifier diff --git a/activerecord/test/cases/arel/select_manager_test.rb b/activerecord/test/cases/arel/select_manager_test.rb index 1bc9f6abf2..f318577b94 100644 --- a/activerecord/test/cases/arel/select_manager_test.rb +++ b/activerecord/test/cases/arel/select_manager_test.rb @@ -1144,7 +1144,7 @@ module Arel assert_match("LIMIT", manager.to_sql) manager.limit = nil - refute_match("LIMIT", manager.to_sql) + assert_no_match("LIMIT", manager.to_sql) end end diff --git a/activerecord/test/cases/associations/belongs_to_associations_test.rb b/activerecord/test/cases/associations/belongs_to_associations_test.rb index a85b56ac4b..5011a9bbde 100644 --- a/activerecord/test/cases/associations/belongs_to_associations_test.rb +++ b/activerecord/test/cases/associations/belongs_to_associations_test.rb @@ -272,6 +272,15 @@ class BelongsToAssociationsTest < ActiveRecord::TestCase assert_equal apple, citibank.firm end + def test_creating_the_belonging_object_from_new_record + citibank = Account.new("credit_limit" => 10) + apple = citibank.create_firm("name" => "Apple") + assert_equal apple, citibank.firm + citibank.save + citibank.reload + assert_equal apple, citibank.firm + end + def test_creating_the_belonging_object_with_primary_key client = Client.create(name: "Primary key client") apple = client.create_firm_with_primary_key("name" => "Apple") diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb index fd008ca8e3..fcfab074a2 100644 --- a/activerecord/test/cases/base_test.rb +++ b/activerecord/test/cases/base_test.rb @@ -982,7 +982,7 @@ class BasicsTest < ActiveRecord::TestCase end end - def test_clear_cash_when_setting_table_name + def test_clear_cache_when_setting_table_name original_table_name = Joke.table_name Joke.table_name = "funny_jokes" diff --git a/activerecord/test/cases/migration/index_test.rb b/activerecord/test/cases/migration/index_test.rb index f9b2dc0c73..f8fecc83cd 100644 --- a/activerecord/test/cases/migration/index_test.rb +++ b/activerecord/test/cases/migration/index_test.rb @@ -135,9 +135,12 @@ module ActiveRecord end def test_remove_named_index - connection.add_index :testings, :foo, name: "custom_index_name" + connection.add_index :testings, :foo, name: "index_testings_on_custom_index_name" assert connection.index_exists?(:testings, :foo) + + assert_raise(ArgumentError) { connection.remove_index(:testings, "custom_index_name") } + connection.remove_index :testings, :foo assert_not connection.index_exists?(:testings, :foo) end diff --git a/activerecord/test/cases/test_case.rb b/activerecord/test/cases/test_case.rb index 024b5bd8a1..409b07e56c 100644 --- a/activerecord/test/cases/test_case.rb +++ b/activerecord/test/cases/test_case.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -require "active_support/test_case" +require "active_support" require "active_support/testing/autorun" require "active_support/testing/method_call_assertions" require "active_support/testing/stream" diff --git a/activerecord/test/cases/transaction_callbacks_test.rb b/activerecord/test/cases/transaction_callbacks_test.rb index e89ac53732..05941c75ac 100644 --- a/activerecord/test/cases/transaction_callbacks_test.rb +++ b/activerecord/test/cases/transaction_callbacks_test.rb @@ -367,6 +367,26 @@ class TransactionCallbacksTest < ActiveRecord::TestCase assert_match(/:on conditions for after_commit and after_rollback callbacks have to be one of \[:create, :destroy, :update\]/, e.message) end + def test_after_commit_chain_not_called_on_errors + record_1 = TopicWithCallbacks.create! + record_2 = TopicWithCallbacks.create! + record_3 = TopicWithCallbacks.create! + callbacks = [] + record_1.after_commit_block { raise } + record_2.after_commit_block { callbacks << record_2.id } + record_3.after_commit_block { callbacks << record_3.id } + begin + TopicWithCallbacks.transaction do + record_1.save! + record_2.save! + record_3.save! + end + rescue + # From record_1.after_commit + end + assert_equal [], callbacks + end + def test_saving_a_record_with_a_belongs_to_that_specifies_touching_the_parent_should_call_callbacks_on_the_parent_object pet = Pet.first owner = pet.owner |