diff options
11 files changed, 178 insertions, 21 deletions
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index b45aba6bb1..e9e97f5d62 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,5 +1,24 @@ ## Rails 4.0.0 (unreleased) ## +* Added deprecation for the `:dependent => :restrict` association option. + + 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. + + *Manoj Kumar* + * Added `create_join_table` migration helper to create HABTM join tables create_join_table :products, :categories diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 58725246c8..b44ab26a73 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -1097,8 +1097,7 @@ module ActiveRecord # 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. # # 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 @@ -1251,8 +1250,7 @@ module ActiveRecord # 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. # [: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 6e2e5f9de0..54e51b7f44 100644 --- a/activerecord/lib/active_record/associations/builder/association.rb +++ b/activerecord/lib/active_record/associations/builder/association.rb @@ -51,5 +51,19 @@ module ActiveRecord::Associations::Builder association(name).writer(value) end end - end + + def dependent_restrict_raises? + ActiveRecord::Base.dependent_restrict_raises == true + 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 + end + 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 fc6799fb15..0fe32b5f16 100644 --- a/activerecord/lib/active_record/associations/builder/has_many.rb +++ b/activerecord/lib/active_record/associations/builder/has_many.rb @@ -21,6 +21,7 @@ module ActiveRecord::Associations::Builder ":nullify or :restrict (#{options[:dependent].inspect})" end + dependent_restrict_deprecation_warning if options[:dependent] == :restrict send("define_#{options[:dependent]}_dependency_method") model.before_destroy dependency_method_name end @@ -55,7 +56,15 @@ module ActiveRecord::Associations::Builder 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? + raise ActiveRecord::DeleteRestrictionError.new(name) + else + errors.add(:base, I18n.t("activerecord.errors.messages.restrict_dependent_destroy", + :model => name)) + return false + end + end end end diff --git a/activerecord/lib/active_record/associations/builder/has_one.rb b/activerecord/lib/active_record/associations/builder/has_one.rb index 7a6cd3890f..d9fed4b7f2 100644 --- a/activerecord/lib/active_record/associations/builder/has_one.rb +++ b/activerecord/lib/active_record/associations/builder/has_one.rb @@ -34,6 +34,7 @@ module ActiveRecord::Associations::Builder ":nullify or :restrict (#{options[:dependent].inspect})" end + dependent_restrict_deprecation_warning if options[:dependent] == :restrict send("define_#{options[:dependent]}_dependency_method") model.before_destroy dependency_method_name end @@ -55,7 +56,15 @@ module ActiveRecord::Associations::Builder def define_restrict_dependency_method name = self.name mixin.redefine_method(dependency_method_name) do - raise ActiveRecord::DeleteRestrictionError.new(name) unless send(name).nil? + unless send(name).nil? + if dependent_restrict_raises? + raise ActiveRecord::DeleteRestrictionError.new(name) + else + errors.add(:base, I18n.t("activerecord.errors.messages.restrict_dependent_destroy", + :model => name)) + return false + end + end end end end diff --git a/activerecord/lib/active_record/core.rb b/activerecord/lib/active_record/core.rb index a774af6024..a2ce620354 100644 --- a/activerecord/lib/active_record/core.rb +++ b/activerecord/lib/active_record/core.rb @@ -72,6 +72,16 @@ module ActiveRecord # 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 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, :global => true + self.dependent_restrict_raises = true end module ClassMethods diff --git a/activerecord/lib/active_record/locale/en.yml b/activerecord/lib/active_record/locale/en.yml index 44328f63b6..88edabfd41 100644 --- a/activerecord/lib/active_record/locale/en.yml +++ b/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" # Append your own errors here or at the model/attributes scope. # You can define own errors for models or model attributes. diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb index f1a341437f..a710900054 100644 --- a/activerecord/test/cases/associations/has_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_associations_test.rb @@ -1140,16 +1140,41 @@ class HasManyAssociationsTest < ActiveRecord::TestCase assert_nil companies(:leetsoft).reload.client_of assert_nil companies(:jadedpixel).reload.client_of - assert_equal num_accounts, Account.count end def test_restrict - firm = RestrictedFirm.new(:name => 'restrict') - firm.save! + # ActiveRecord::Base.dependent_restrict_raises = true, by default + + 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') + end + + def test_restrict_when_dependent_restrict_raises_config_set_to_false + # ActiveRecord::Base.dependent_restrict_raises = true, by default + + ActiveRecord::Base.dependent_restrict_raises = false + # add an error on the model instead of raising ActiveRecord::DeleteRestrictionError + + firm = RestrictedFirm.create!(:name => 'restrict') + firm.companies.create(:name => 'child') + + assert !firm.companies.empty? + + firm.destroy + + assert !firm.errors.empty? + + assert_equal "Cannot delete record because dependent companies exist", firm.errors[:base].first + assert RestrictedFirm.exists?(:name => 'restrict') + assert firm.companies.exists?(:name => 'child') + ensure + ActiveRecord::Base.dependent_restrict_raises = true end def test_included_in_collection @@ -1401,29 +1426,29 @@ class HasManyAssociationsTest < ActiveRecord::TestCase firm.clients.last end end - + def test_custom_primary_key_on_new_record_should_fetch_with_query author = Author.new(:name => "David") assert !author.essays.loaded? - - assert_queries 1 do + + assert_queries 1 do assert_equal 1, author.essays.size end - + assert_equal author.essays, Essay.find_all_by_writer_id("David") - + end - + def test_has_many_custom_primary_key david = authors(:david) assert_equal david.essays, Essay.find_all_by_writer_id("David") end - + def test_blank_custom_primary_key_on_new_record_should_not_run_queries author = Author.new assert !author.essays.loaded? - - assert_queries 0 do + + assert_queries 0 do assert_equal 0, author.essays.size end end @@ -1649,4 +1674,22 @@ class HasManyAssociationsTest < ActiveRecord::TestCase 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 + class RestrictedFirm < ActiveRecord::Base + has_many :companies, :dependent => :restrict + end + EOF + end + + assert_not_deprecated do + class_eval <<-EOF + class Firm < ActiveRecord::Base + has_many :companies + end + EOF + end + end end diff --git a/activerecord/test/cases/associations/has_one_associations_test.rb b/activerecord/test/cases/associations/has_one_associations_test.rb index 26931e3e85..a17f33c8df 100644 --- a/activerecord/test/cases/associations/has_one_associations_test.rb +++ b/activerecord/test/cases/associations/has_one_associations_test.rb @@ -157,11 +157,37 @@ class HasOneAssociationsTest < ActiveRecord::TestCase end def test_dependence_with_restrict - firm = RestrictedFirm.new(:name => 'restrict') - firm.save! + # ActiveRecord::Base.dependent_restrict_raises = true, by default + + firm = RestrictedFirm.create!(:name => 'restrict') firm.create_account(:credit_limit => 10) + assert_not_nil firm.account + assert_raise(ActiveRecord::DeleteRestrictionError) { firm.destroy } + assert RestrictedFirm.exists?(:name => 'restrict') + 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 + + 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 + + firm.destroy + + assert !firm.errors.empty? + assert_equal "Cannot delete record because dependent account exist", firm.errors[:base].first + assert RestrictedFirm.exists?(:name => 'restrict') + assert firm.account.present? + ensure + ActiveRecord::Base.dependent_restrict_raises = true end def test_successful_build_association @@ -456,4 +482,22 @@ class HasOneAssociationsTest < ActiveRecord::TestCase assert_equal car.id, bulb.attributes_after_initialize['car_id'] end + + def test_building_has_one_association_with_dependent_restrict + assert_deprecated do + class_eval <<-EOF + class RestrictedFirm < ActiveRecord::Base + has_one :account, :dependent => :restrict + end + EOF + end + + assert_not_deprecated do + class_eval <<-EOF + class Firm < ActiveRecord::Base + has_one :account + end + EOF + end + end end diff --git a/railties/lib/rails/generators/rails/app/templates/config/application.rb b/railties/lib/rails/generators/rails/app/templates/config/application.rb index 3517956e4a..c3a87d30b4 100644 --- a/railties/lib/rails/generators/rails/app/templates/config/application.rb +++ b/railties/lib/rails/generators/rails/app/templates/config/application.rb @@ -56,6 +56,11 @@ module <%= app_const_base %> # 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 + <% unless options.skip_sprockets? -%> # Enable the asset pipeline. config.assets.enabled = true diff --git a/railties/test/generators/app_generator_test.rb b/railties/test/generators/app_generator_test.rb index f3071843e2..33a51fe782 100644 --- a/railties/test/generators/app_generator_test.rb +++ b/railties/test/generators/app_generator_test.rb @@ -349,6 +349,11 @@ class AppGeneratorTest < Rails::Generators::TestCase end 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 + protected def action(*args, &block) |