Skip to content
Browse files

Remove the dependent_restrict_raises option.

It's not really a good idea to have this as a global config option. We
should allow people to specify the behaviour per association.

There will now be two new values:

* :dependent => :restrict_with_exception implements the current
  behaviour of :restrict. :restrict itself is deprecated in favour of
  :restrict_with_exception.
* :dependent => :restrict_with_error implements the new behaviour - it
  adds an error to the owner if there are dependent records present

See #4727 for the original discussion of this.
  • Loading branch information...
1 parent d1835db commit 5ad79989ef0a015fd22cfed90b2e8a56881e6c36 @jonleighton jonleighton committed Aug 10, 2012
View
21 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
View
27 activerecord/lib/active_record/associations.rb
@@ -1094,12 +1094,14 @@ module ClassMethods
# [: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 @@ def has_many(name, scope = nil, options = {}, &extension)
# 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
View
38 activerecord/lib/active_record/associations/builder/association.rb
@@ -85,35 +85,35 @@ def check_valid_dependent!(dependent, valid_options)
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
View
4 activerecord/lib/active_record/associations/builder/has_many.rb
@@ -19,8 +19,8 @@ def build
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
View
3 activerecord/lib/active_record/associations/builder/has_one.rb
@@ -25,8 +25,7 @@ def build
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
View
9 activerecord/lib/active_record/core.rb
@@ -82,15 +82,6 @@ module Core
# 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
View
41 activerecord/test/cases/associations/has_many_associations_test.rb
@@ -1091,25 +1091,32 @@ def test_depends_and_nullify
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')
assert !firm.companies.empty?
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 @@ def test_restrict_when_dependent_restrict_raises_config_set_to_false
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 @@ def test_replace_returns_target
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
View
56 activerecord/test/cases/associations/has_one_associations_test.rb
@@ -156,10 +156,7 @@ def test_dependence_with_nil_associate
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,51 +165,36 @@ def test_dependence_with_restrict
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
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 @@ def test_association_attributes_are_available_to_after_initialize
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)
View
3 activerecord/test/cases/helper.rb
@@ -19,9 +19,6 @@
# 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
View
16 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
View
5 railties/lib/rails/generators/rails/app/templates/config/application.rb
@@ -56,11 +56,6 @@ class Application < Rails::Application
# in your app. As such, your models will need to explicitly whitelist or blacklist accessible
# parameters by using an attr_accessible or attr_protected declaration.
<%= comment_if :skip_active_record %>config.active_record.whitelist_attributes = true
-
- # Specifies whether or not has_many or has_one association option :dependent => :restrict raises
- # an exception. If set to true, then an ActiveRecord::DeleteRestrictionError exception would be
- # raised. If set to false, then an error will be added on the model instead.
- <%= comment_if :skip_active_record %>config.active_record.dependent_restrict_raises = false
<% unless options.skip_sprockets? -%>
# Enable the asset pipeline.
View
5 railties/test/generators/app_generator_test.rb
@@ -367,11 +367,6 @@ def test_active_record_whitelist_attributes_is_present_application_config
assert_file "config/application.rb", /config\.active_record\.whitelist_attributes = true/
end
- def test_active_record_dependent_restrict_raises_is_present_application_config
- run_generator
- assert_file "config/application.rb", /config\.active_record\.dependent_restrict_raises = false/
- end
-
def test_pretend_option
output = run_generator [File.join(destination_root, "myapp"), "--pretend"]
assert_no_match(/run bundle install/, output)

0 comments on commit 5ad7998

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