has_many/has_one, :dependent => :restrict, deprecation added #4727

Merged
merged 4 commits into from Jan 31, 2012

9 participants

@hindenbug

has_many/has_one, :dependent => :restrict, deprecation added, for ActiveRecord::DeleteRestrictionError.

Tests included.

@jonleighton applied the suggested changes, as per issue #4706, are these good enough? Please advise.

Thanks.

fixes #2502
fixes #3881

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Jan 27, 2012
.../generators/rails/app/templates/config/application.rb
@@ -56,6 +56,12 @@ class Application < Rails::Application
# parameters by using an attr_accessible or attr_protected declaration.
# config.active_record.whitelist_attributes = true
+ # Specifies wether 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.
+ config.active_record.dependent_restrict_raises = false
+
+
@carlosantoniodasilva
Ruby on Rails member

✂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva carlosantoniodasilva and 1 other commented on an outdated diff Jan 27, 2012
...rd/lib/active_record/associations/builder/has_many.rb
@@ -55,7 +57,14 @@ def define_nullify_dependency_method
def define_restrict_dependency_method
name = self.name
mixin.redefine_method(dependency_method_name) do
- raise ActiveRecord::DeleteRestrictionError.new(name) unless send(name).empty?
+ if send(name).exists?
+ if dependent_restrict_raises == true
+ raise ActiveRecord::DeleteRestrictionError.new(name)
+ else
+ self.errors.add(:base, "Cannot delete record because dependent #{name} exist")
@carlosantoniodasilva
Ruby on Rails member

I think these messages should rely on the I18n api somehow, it'd be better for those who not build only apps in English.

@jonleighton
Ruby on Rails member
jonleighton added a line comment Jan 28, 2012

Yes, I agree with that. Can you look into it please? Also, it would be good to remove the duplication between the HasMany and HasOne builders.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva carlosantoniodasilva and 2 others commented on an outdated diff Jan 27, 2012
activerecord/lib/active_record/associations.rb
@@ -1251,8 +1253,10 @@ def has_many(name, options = {}, &extension)
# 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+.
- # Also, association is assigned. If set to <tt>:restrict</tt> this object raises an
- # <tt>ActiveRecord::DeleteRestrictionError</tt> exception and cannot be deleted if it has any associated object.
+ # If set to <tt>:restrict</tt> this object cannot be deleted if it has any associated object and
+ # would raise an <tt>ActiveRecord::DeleteRestrictionError</tt> exception, you can configure the
+ # <tt>ActiveRecord::Base.dependent_restrict_raises = false</tt> which will add an error to the model
+ # instead of raising an excpetion.
@carlosantoniodasilva
Ruby on Rails member

Should the docs explain that raising is deprecated?

@hindenbug
hindenbug added a line comment Jan 27, 2012

@carlosantoniodasilva I think you are right , docs should not be explaining the deprecation :) Thanks for pointing it out

@NoICE
NoICE added a line comment Jan 29, 2012

a side note, line 1259 says "excpetion"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva
Ruby on Rails member

Awesome, this feature is a lot more useful this way, great work 👍

@asanghi

👍 Fantastic work @Manoj! Looking forward to seeing this merged! /cc @jonleighton

@jonleighton jonleighton commented on an outdated diff Jan 28, 2012
.../test/cases/associations/has_one_associations_test.rb
+ assert firm.account.present?
+ end
+
+ def test_dependence_with_restrict_with_dependent_restrict_raises_config_set_to_false
+ # ActiveRecord::Base.dependent_restrict_raises = true, by default
+ # Shows Deprecation Warning
+
+ ActiveRecord::Base.dependent_restrict_raises = false
+ # adds an error on the model instead of raising ActiveRecord::DeleteRestrictionError
+
+ firm = RestrictedFirm.create!(:name => 'restrict')
+ firm.create_account(:credit_limit => 10)
+
+ assert_not_nil firm.account
+
+ ActiveSupport::Deprecation.silence { firm.destroy }
@jonleighton
Ruby on Rails member
jonleighton added a line comment Jan 28, 2012

I'm confused - why does this cause a deprecation warning? The deprecation should happen when the association is defined, not when it's used. (And we will need a test that builds an association and uses assert_deprecated to make sure there is a warning.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jonleighton jonleighton commented on an outdated diff Jan 28, 2012
activerecord/lib/active_record/core.rb
@@ -72,6 +72,16 @@ module Core
# The connection handler
config_attribute :connection_handler
self.connection_handler = ConnectionAdapters::ConnectionHandler.new
+
+ ##
+ # :singleton-method:
+ # Specifies wether or not has_many or has_one association option
+ # :dependent => :restrict raises an exception. If set to true, the
+ # ActiveRecord::DeleteRestrictionError exception would be raised
+ # along with a DEPRECATION WARNING. If set to false, an error would
@jonleighton
Ruby on Rails member
jonleighton added a line comment Jan 28, 2012

"would be" -> "will be"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jonleighton
Ruby on Rails member

Added some more comments, thanks.

@hindenbug

@asanghi @carlosantoniodasilva @NoICE Thank you all for your input. :)

@hindenbug

@jonleighton Thanks for the comments, I have addressed the suggested modifications. Please comment. :)

@pacoguzman

I'd simplified how the error message is defined 15154c0

@hindenbug

@pacoguzman thx for the refactoring :)

@jonleighton jonleighton commented on an outdated diff Jan 30, 2012
activerecord/lib/active_record/locale/en.yml
@@ -10,6 +10,7 @@ en:
messages:
taken: "has already been taken"
record_invalid: "Validation failed: %{errors}"
+ restrict_dependent_destroy: "Cannot delete record because dependent %{model} exist"
@jonleighton
Ruby on Rails member
jonleighton added a line comment Jan 30, 2012

"exists", rather than "exist"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jonleighton jonleighton commented on an outdated diff Jan 30, 2012
...test/cases/associations/has_many_associations_test.rb
@@ -1649,4 +1674,22 @@ def test_replace
assert_equal [bulb2], car.bulbs
assert_equal [bulb2], car.reload.bulbs
end
+
+ def test_building_has_many_association_with_restrict_dependency
+ assert_deprecated do
+ class_eval <<-EOF
@jonleighton
Ruby on Rails member
jonleighton added a line comment Jan 30, 2012

this test with change things globally, which is undesirable. please could you refactor along these lines:

klass = Class.new(ActiveRecord::Base)
assert_deprecated { klass.has_many :companies, :dependent => :restrict }
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jonleighton jonleighton commented on an outdated diff Jan 30, 2012
activerecord/lib/active_record/associations.rb
@@ -1097,8 +1097,7 @@ module ClassMethods
# 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> this object raises an <tt>ActiveRecord::DeleteRestrictionError</tt> exception and
- # cannot be deleted if it has any associated objects.
+ # <tt>:restrict</tt> this object cannot be deleted if it has any associated objects.
@jonleighton
Ruby on Rails member
jonleighton added a line comment Jan 30, 2012

please change to "If set to restrict, an error will be added to the object, preventing its deletion, if any associated objects are present."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jonleighton jonleighton and 2 others commented on an outdated diff Jan 30, 2012
...rd/lib/active_record/associations/builder/has_many.rb
@@ -55,7 +56,15 @@ def define_nullify_dependency_method
def define_restrict_dependency_method
name = self.name
mixin.redefine_method(dependency_method_name) do
- raise ActiveRecord::DeleteRestrictionError.new(name) unless send(name).empty?
+ if send(name).exists?
@jonleighton
Ruby on Rails member
jonleighton added a line comment Jan 30, 2012

please could you abstract out this define_restrict_dependency_method as it's repeated code?

@pacoguzman
pacoguzman added a line comment Jan 31, 2012

Maybe this could be the refactor needed 5099a5be26

i'd moved the method name definition to the bottom to both builders for consistency too

@hindenbug
hindenbug added a line comment Jan 31, 2012

@pacoguzman thanks again :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jonleighton
Ruby on Rails member

added some more comments. please also pull in @pacoguzman's refactorings.

@hindenbug

@jonleighton addressed the suggested improvements, also included @pacoguzman's refactorings. :)

@jonleighton jonleighton merged commit 30a2328 into rails:master Jan 31, 2012
@asanghi

@jonleighton Any chance we can get this "dont raise an exception" feature backported to Rails 3.2.x ? Or has that boat has totally sailed?

There won't be any deprecation warning in 3.2 of course but at least we'll be able to use the feature by using the configuration option. What do you think?

@jonleighton
Ruby on Rails member

@asanghi I think the ship has sailed I'm afraid - we can't add more features to 3.2 now it has been released.

@josevalim

This is wrong. It should get the reflection object, the class from the object and then the human name.

@jaredbeck

I'm not sure if this is the right place to ask, but don't forget to document :restrict here: http://guides.rubyonrails.org/association_basics.html

Thanks, looking forward to using this feature.

@asanghi

@jaredbeck done .. committed to docrails rails/docrails@f49ec92
/cc @vijaydev

@dhh
Ruby on Rails member
dhh commented Aug 7, 2012

I really don't like this global setting. Let's push this down to where it's being defined and go explicit. So have dependent: :restrict_with_exception and :restrict_with_validation or something like that.

@jonleighton jonleighton added a commit that referenced this pull request Aug 10, 2012
@jonleighton jonleighton 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.
5ad7998
@ka8725

If I try to delete associated model with nested attributes will it work correctly? I think that parent model won't hold error messages so. I don't see tests for this case, so I see that it won't work as it expected

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment