diff options
Diffstat (limited to 'activerecord')
| -rw-r--r-- | activerecord/CHANGELOG.md | 21 | ||||
| -rw-r--r-- | activerecord/lib/active_record/associations.rb | 27 | ||||
| -rw-r--r-- | activerecord/lib/active_record/associations/builder/association.rb | 38 | ||||
| -rw-r--r-- | activerecord/lib/active_record/associations/builder/has_many.rb | 4 | ||||
| -rw-r--r-- | activerecord/lib/active_record/associations/builder/has_one.rb | 3 | ||||
| -rw-r--r-- | activerecord/lib/active_record/core.rb | 9 | ||||
| -rw-r--r-- | activerecord/test/cases/associations/has_many_associations_test.rb | 41 | ||||
| -rw-r--r-- | activerecord/test/cases/associations/has_one_associations_test.rb | 56 | ||||
| -rw-r--r-- | activerecord/test/cases/helper.rb | 3 | ||||
| -rw-r--r-- | activerecord/test/models/company.rb | 16 |
10 files changed, 87 insertions, 131 deletions
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index a99d7fdde8..e6237ef437 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -486,24 +486,13 @@ * Added the `ActiveRecord::NullRelation` class implementing the null object pattern for the Relation class. *Juanjo Bazán* -* Added deprecation for the `:dependent => :restrict` association option. +* Added new `:dependent => :restrict_with_error` option. This will add + an error to the model, rather than raising an exception. - Please note: - - * Up until now `has_many` and `has_one`, `:dependent => :restrict` - option raised a `DeleteRestrictionError` at the time of destroying - the object. Instead, it will add an error on the model. - - * To fix this warning, make sure your code isn't relying on a - `DeleteRestrictionError` and then add - `config.active_record.dependent_restrict_raises = false` to your - application config. - - * New rails application would be generated with the - `config.active_record.dependent_restrict_raises = false` in the - application config. + The `:restrict` option is renamed to `:restrict_with_exception` to + make this distinction explicit. - *Manoj Kumar* + *Manoj Kumar & Jon Leighton* * Added `create_join_table` migration helper to create HABTM join tables diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index f5ee4f3ebe..d9857b8fbd 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -1094,12 +1094,14 @@ module ActiveRecord # [:primary_key] # Specify the method that returns the primary key used for the association. By default this is +id+. # [:dependent] - # If set to <tt>:destroy</tt> all the associated objects are destroyed - # alongside this object by calling their +destroy+ method. If set to <tt>:delete_all</tt> all associated - # objects are deleted *without* calling their +destroy+ method. If set to <tt>:nullify</tt> all associated - # objects' foreign keys are set to +NULL+ *without* calling their +save+ callbacks. If set to - # <tt>:restrict</tt> an error will be added to the object, preventing its deletion, if any associated - # objects are present. + # Controls what happens to the associated objects when + # their owner is destroyed: + # + # * <tt>:destroy</tt> causes all the associated objects to also be destroyed + # * <tt>:delete_all</tt> causes all the asssociated objects to be deleted directly from the database (so callbacks will not execute) + # * <tt>:nullify</tt> causes the foreign keys to be set to +NULL+. Callbacks are not executed. + # * <tt>:restrict_with_exception</tt> causes an exception to be raised if there are any associated records + # * <tt>:restrict_with_error</tt> causes an error to be added to the owner if there are any associated objects # # If using with the <tt>:through</tt> option, the association on the join model must be # a +belongs_to+, and the records which get deleted are the join records, rather than @@ -1203,11 +1205,14 @@ module ActiveRecord # from the association name. So <tt>has_one :manager</tt> will by default be linked to the Manager class, but # if the real class name is Person, you'll have to specify it with this option. # [:dependent] - # If set to <tt>:destroy</tt>, the associated object is destroyed when this object is. If set to - # <tt>:delete</tt>, the associated object is deleted *without* calling its destroy method. - # If set to <tt>:nullify</tt>, the associated object's foreign key is set to +NULL+. - # If set to <tt>:restrict</tt>, an error will be added to the object, preventing its deletion, if an - # associated object is present. + # Controls what happens to the associated objects when + # their owner is destroyed: + # + # * <tt>:destroy</tt> causes all the associated objects to also be destroyed + # * <tt>:delete</tt> causes all the asssociated objects to be deleted directly from the database (so callbacks will not execute) + # * <tt>:nullify</tt> causes the foreign keys to be set to +NULL+. Callbacks are not executed. + # * <tt>:restrict_with_exception</tt> causes an exception to be raised if there are any associated records + # * <tt>:restrict_with_error</tt> causes an error to be added to the owner if there are any associated objects # [:foreign_key] # Specify the foreign key used for the association. By default this is guessed to be the name # of this class in lower-case and "_id" suffixed. So a Person class that makes a +has_one+ association diff --git a/activerecord/lib/active_record/associations/builder/association.rb b/activerecord/lib/active_record/associations/builder/association.rb index c3f32b5ed9..47f06421ee 100644 --- a/activerecord/lib/active_record/associations/builder/association.rb +++ b/activerecord/lib/active_record/associations/builder/association.rb @@ -85,35 +85,35 @@ module ActiveRecord::Associations::Builder raise ArgumentError, "The :dependent option expects either " \ "#{valid_options_message} (#{dependent.inspect})" end - end - def dependent_restrict_raises? - ActiveRecord::Base.dependent_restrict_raises == true + if dependent == :restrict + ActiveSupport::Deprecation.warn( + "The :restrict option is deprecated. Please use :restrict_with_exception instead, which " \ + "provides the same functionality." + ) + end end - def dependent_restrict_deprecation_warning - if dependent_restrict_raises? - msg = "In the next release, `:dependent => :restrict` will not raise a `DeleteRestrictionError`. "\ - "Instead, it will add an error on the model. To fix this warning, make sure your code " \ - "isn't relying on a `DeleteRestrictionError` and then add " \ - "`config.active_record.dependent_restrict_raises = false` to your application config." - ActiveSupport::Deprecation.warn msg + def define_restrict_with_exception_dependency_method + name = self.name + mixin.redefine_method(dependency_method_name) do + has_one_macro = association(name).reflection.macro == :has_one + if has_one_macro ? !send(name).nil? : send(name).exists? + raise ActiveRecord::DeleteRestrictionError.new(name) + end end end + alias define_restrict_dependency_method define_restrict_with_exception_dependency_method - def define_restrict_dependency_method + def define_restrict_with_error_dependency_method name = self.name mixin.redefine_method(dependency_method_name) do has_one_macro = association(name).reflection.macro == :has_one if has_one_macro ? !send(name).nil? : send(name).exists? - if dependent_restrict_raises? - raise ActiveRecord::DeleteRestrictionError.new(name) - else - key = has_one_macro ? "one" : "many" - errors.add(:base, :"restrict_dependent_destroy.#{key}", - :record => self.class.human_attribute_name(name).downcase) - return false - end + key = has_one_macro ? "one" : "many" + errors.add(:base, :"restrict_dependent_destroy.#{key}", + :record => self.class.human_attribute_name(name).downcase) + false end end end diff --git a/activerecord/lib/active_record/associations/builder/has_many.rb b/activerecord/lib/active_record/associations/builder/has_many.rb index 9e60dbc30b..63278c8cfd 100644 --- a/activerecord/lib/active_record/associations/builder/has_many.rb +++ b/activerecord/lib/active_record/associations/builder/has_many.rb @@ -19,8 +19,8 @@ module ActiveRecord::Associations::Builder def configure_dependency if dependent = options[:dependent] - check_valid_dependent! dependent, [:destroy, :delete_all, :nullify, :restrict] - dependent_restrict_deprecation_warning if dependent == :restrict + check_valid_dependent! dependent, [:destroy, :delete_all, :nullify, :restrict, + :restrict_with_error, :restrict_with_exception] send("define_#{dependent}_dependency_method") model.before_destroy dependency_method_name diff --git a/activerecord/lib/active_record/associations/builder/has_one.rb b/activerecord/lib/active_record/associations/builder/has_one.rb index 9c84f1913a..82c5905caf 100644 --- a/activerecord/lib/active_record/associations/builder/has_one.rb +++ b/activerecord/lib/active_record/associations/builder/has_one.rb @@ -25,8 +25,7 @@ module ActiveRecord::Associations::Builder def configure_dependency if dependent = options[:dependent] - check_valid_dependent! dependent, [:destroy, :delete, :nullify, :restrict] - dependent_restrict_deprecation_warning if dependent == :restrict + check_valid_dependent! dependent, [:destroy, :delete, :nullify, :restrict, :restrict_with_error, :restrict_with_exception] send("define_#{dependent}_dependency_method") model.before_destroy dependency_method_name diff --git a/activerecord/lib/active_record/core.rb b/activerecord/lib/active_record/core.rb index 1145d2138c..0fddfdf0cb 100644 --- a/activerecord/lib/active_record/core.rb +++ b/activerecord/lib/active_record/core.rb @@ -82,15 +82,6 @@ module ActiveRecord # The connection handler config_attribute :connection_handler - ## - # :singleton-method: - # Specifies whether or not has_many or has_one association option - # :dependent => :restrict raises an exception. If set to true, the - # ActiveRecord::DeleteRestrictionError exception will be raised - # along with a DEPRECATION WARNING. If set to false, an error would - # be added to the model instead. - config_attribute :dependent_restrict_raises - %w(logger configurations default_timezone schema_format timestamped_migrations).each do |name| config_attribute name, global: true end diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb index 43440c1146..04714f42e9 100644 --- a/activerecord/test/cases/associations/has_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_associations_test.rb @@ -1091,9 +1091,6 @@ class HasManyAssociationsTest < ActiveRecord::TestCase end def test_restrict - option_before = ActiveRecord::Base.dependent_restrict_raises - ActiveRecord::Base.dependent_restrict_raises = true - firm = RestrictedFirm.create!(:name => 'restrict') firm.companies.create(:name => 'child') @@ -1101,15 +1098,25 @@ class HasManyAssociationsTest < ActiveRecord::TestCase assert_raise(ActiveRecord::DeleteRestrictionError) { firm.destroy } assert RestrictedFirm.exists?(:name => 'restrict') assert firm.companies.exists?(:name => 'child') - ensure - ActiveRecord::Base.dependent_restrict_raises = option_before end - def test_restrict_when_dependent_restrict_raises_config_set_to_false - option_before = ActiveRecord::Base.dependent_restrict_raises - ActiveRecord::Base.dependent_restrict_raises = false + def test_restrict_is_deprecated + klass = Class.new(ActiveRecord::Base) + assert_deprecated { klass.has_many :posts, dependent: :restrict } + end - firm = RestrictedFirm.create!(:name => 'restrict') + def test_restrict_with_exception + firm = RestrictedWithExceptionFirm.create!(:name => 'restrict') + firm.companies.create(:name => 'child') + + assert !firm.companies.empty? + assert_raise(ActiveRecord::DeleteRestrictionError) { firm.destroy } + assert RestrictedWithExceptionFirm.exists?(:name => 'restrict') + assert firm.companies.exists?(:name => 'child') + end + + def test_restrict_with_error + firm = RestrictedWithErrorFirm.create!(:name => 'restrict') firm.companies.create(:name => 'child') assert !firm.companies.empty? @@ -1119,10 +1126,8 @@ class HasManyAssociationsTest < ActiveRecord::TestCase assert !firm.errors.empty? assert_equal "Cannot delete record because dependent companies exist", firm.errors[:base].first - assert RestrictedFirm.exists?(:name => 'restrict') + assert RestrictedWithErrorFirm.exists?(:name => 'restrict') assert firm.companies.exists?(:name => 'child') - ensure - ActiveRecord::Base.dependent_restrict_raises = option_before end def test_included_in_collection @@ -1602,18 +1607,6 @@ class HasManyAssociationsTest < ActiveRecord::TestCase assert_equal [bulb1, bulb3], result end - def test_building_has_many_association_with_restrict_dependency - option_before = ActiveRecord::Base.dependent_restrict_raises - ActiveRecord::Base.dependent_restrict_raises = true - - klass = Class.new(ActiveRecord::Base) - - assert_deprecated { klass.has_many :companies, :dependent => :restrict } - assert_not_deprecated { klass.has_many :companies } - ensure - ActiveRecord::Base.dependent_restrict_raises = option_before - end - def test_collection_association_with_private_kernel_method firm = companies(:first_firm) assert_equal [accounts(:signals37)], firm.accounts.open diff --git a/activerecord/test/cases/associations/has_one_associations_test.rb b/activerecord/test/cases/associations/has_one_associations_test.rb index 752c33b1db..8bc633f2b5 100644 --- a/activerecord/test/cases/associations/has_one_associations_test.rb +++ b/activerecord/test/cases/associations/has_one_associations_test.rb @@ -156,10 +156,7 @@ class HasOneAssociationsTest < ActiveRecord::TestCase assert_nothing_raised { firm.destroy } end - def test_dependence_with_restrict - option_before = ActiveRecord::Base.dependent_restrict_raises - ActiveRecord::Base.dependent_restrict_raises = true - + def test_restrict firm = RestrictedFirm.create!(:name => 'restrict') firm.create_account(:credit_limit => 10) @@ -168,38 +165,26 @@ class HasOneAssociationsTest < ActiveRecord::TestCase assert_raise(ActiveRecord::DeleteRestrictionError) { firm.destroy } assert RestrictedFirm.exists?(:name => 'restrict') assert firm.account.present? - ensure - ActiveRecord::Base.dependent_restrict_raises = option_before end - def test_dependence_with_restrict_with_dependent_restrict_raises_config_set_to_false - option_before = ActiveRecord::Base.dependent_restrict_raises - ActiveRecord::Base.dependent_restrict_raises = false + def test_restrict_is_deprecated + klass = Class.new(ActiveRecord::Base) + assert_deprecated { klass.has_one :post, dependent: :restrict } + end - firm = RestrictedFirm.create!(:name => 'restrict') + def test_restrict_with_exception + firm = RestrictedWithExceptionFirm.create!(:name => 'restrict') firm.create_account(:credit_limit => 10) assert_not_nil firm.account - firm.destroy - - assert !firm.errors.empty? - assert_equal "Cannot delete record because a dependent account exists", firm.errors[:base].first - assert RestrictedFirm.exists?(:name => 'restrict') + assert_raise(ActiveRecord::DeleteRestrictionError) { firm.destroy } + assert RestrictedWithExceptionFirm.exists?(:name => 'restrict') assert firm.account.present? - ensure - ActiveRecord::Base.dependent_restrict_raises = option_before end - def test_dependence_with_restrict_with_dependent_restrict_raises_config_set_to_false_and_attribute_name - old_backend = I18n.backend - I18n.backend = I18n::Backend::Simple.new - I18n.backend.store_translations 'en', :activerecord => {:attributes => {:restricted_firm => {:account => "account model"}}} - - option_before = ActiveRecord::Base.dependent_restrict_raises - ActiveRecord::Base.dependent_restrict_raises = false - - firm = RestrictedFirm.create!(:name => 'restrict') + def test_restrict_with_error + firm = RestrictedWithErrorFirm.create!(:name => 'restrict') firm.create_account(:credit_limit => 10) assert_not_nil firm.account @@ -207,12 +192,9 @@ class HasOneAssociationsTest < ActiveRecord::TestCase firm.destroy assert !firm.errors.empty? - assert_equal "Cannot delete record because a dependent account model exists", firm.errors[:base].first - assert RestrictedFirm.exists?(:name => 'restrict') + assert_equal "Cannot delete record because a dependent account exists", firm.errors[:base].first + assert RestrictedWithErrorFirm.exists?(:name => 'restrict') assert firm.account.present? - ensure - ActiveRecord::Base.dependent_restrict_raises = option_before - I18n.backend = old_backend end def test_successful_build_association @@ -524,18 +506,6 @@ class HasOneAssociationsTest < ActiveRecord::TestCase assert_equal car.id, bulb.attributes_after_initialize['car_id'] end - def test_building_has_one_association_with_dependent_restrict - option_before = ActiveRecord::Base.dependent_restrict_raises - ActiveRecord::Base.dependent_restrict_raises = true - - klass = Class.new(ActiveRecord::Base) - - assert_deprecated { klass.has_one :account, :dependent => :restrict } - assert_not_deprecated { klass.has_one :account } - ensure - ActiveRecord::Base.dependent_restrict_raises = option_before - end - def test_has_one_transaction company = companies(:first_firm) account = Account.find(1) diff --git a/activerecord/test/cases/helper.rb b/activerecord/test/cases/helper.rb index 018064233a..4c6d4666ed 100644 --- a/activerecord/test/cases/helper.rb +++ b/activerecord/test/cases/helper.rb @@ -19,9 +19,6 @@ require 'support/connection' # Show backtraces for deprecated behavior for quicker cleanup. ActiveSupport::Deprecation.debug = true -# Avoid deprecation warning setting dependent_restrict_raises to false. The default is true -ActiveRecord::Base.dependent_restrict_raises = false - # Connect to the database ARTest.connect diff --git a/activerecord/test/models/company.rb b/activerecord/test/models/company.rb index 5bfbb5e855..75f38d275c 100644 --- a/activerecord/test/models/company.rb +++ b/activerecord/test/models/company.rb @@ -115,8 +115,20 @@ class DependentFirm < Company end class RestrictedFirm < Company - has_one :account, -> { order("id") }, :foreign_key => "firm_id", :dependent => :restrict - has_many :companies, -> { order("id") }, :foreign_key => 'client_of', :dependent => :restrict + ActiveSupport::Deprecation.silence do + has_one :account, -> { order("id") }, :foreign_key => "firm_id", :dependent => :restrict + has_many :companies, -> { order("id") }, :foreign_key => 'client_of', :dependent => :restrict + end +end + +class RestrictedWithExceptionFirm < Company + has_one :account, -> { order("id") }, :foreign_key => "firm_id", :dependent => :restrict_with_exception + has_many :companies, -> { order("id") }, :foreign_key => 'client_of', :dependent => :restrict_with_exception +end + +class RestrictedWithErrorFirm < Company + has_one :account, -> { order("id") }, :foreign_key => "firm_id", :dependent => :restrict_with_error + has_many :companies, -> { order("id") }, :foreign_key => 'client_of', :dependent => :restrict_with_error end class Client < Company |