Merge pull request rails#49056 from joshuay03/raise-on-duplicate-accepts-nested-attributes-for

[Fix rails#49055] Raise an `ArgumentError` when `#accepts_nested_attributes_for` is redeclared for an association

Author: rafaelfranca

activerecord/CHANGELOG.md

@@ -1,3 +1,9 @@
+*   Raise an `ArgumentError` when `#accepts_nested_attributes_for` is declared more than once for an association in
+    the same class. Previously, the last declaration would silently override the previous one. Overriding in a subclass
+    is still allowed.
+
+    *Joshua Young*
+
 *   Deprecate `rewhere` argument on `#merge`.
 
     The `rewhere` argument on `#merge`is deprecated without replacement and

activerecord/lib/active_record/nested_attributes.rb

@@ -355,12 +355,17 @@ def accepts_nested_attributes_for(*attr_names)
         options.update(attr_names.extract_options!)
         options.assert_valid_keys(:allow_destroy, :reject_if, :limit, :update_only)
         options[:reject_if] = REJECT_ALL_BLANK_PROC if options[:reject_if] == :all_blank
+        options[:class] = self
 
         attr_names.each do |association_name|
           if reflection = _reflect_on_association(association_name)
             reflection.autosave = true
             define_autosave_validation_callbacks(reflection)
 
+            if nested_attributes_options.dig(association_name.to_sym, :class) == self
+              raise ArgumentError, "Already declared #{association_name} as an accepts_nested_attributes association for this class."
+            end
+
             nested_attributes_options = self.nested_attributes_options.dup
             nested_attributes_options[association_name.to_sym] = options
             self.nested_attributes_options = nested_attributes_options

activerecord/test/cases/nested_attributes_test.rb

@@ -18,6 +18,7 @@
 
 class TestNestedAttributesInGeneral < ActiveRecord::TestCase
   teardown do
+    Pirate.nested_attributes_options.delete :ship
     Pirate.accepts_nested_attributes_for :ship, allow_destroy: true, reject_if: proc(&:empty?)
   end
 
@@ -74,6 +75,7 @@ def test_should_raise_an_UnknownAttributeError_for_non_existing_nested_attribute
   end
 
   def test_should_disable_allow_destroy_by_default
+    Pirate.nested_attributes_options.delete :ship
     Pirate.accepts_nested_attributes_for :ship
 
     pirate = Pirate.create!(catchphrase: "Don' botharrr talkin' like one, savvy?")
@@ -92,6 +94,7 @@ def test_a_model_should_respond_to_underscore_destroy_and_return_if_it_is_marked
   end
 
   def test_reject_if_method_without_arguments
+    Pirate.nested_attributes_options.delete :ship
     Pirate.accepts_nested_attributes_for :ship, reject_if: :new_record?
 
     pirate = Pirate.new(catchphrase: "Stop wastin' me time")
@@ -100,6 +103,7 @@ def test_reject_if_method_without_arguments
   end
 
   def test_reject_if_method_with_arguments
+    Pirate.nested_attributes_options.delete :ship
     Pirate.accepts_nested_attributes_for :ship, reject_if: :reject_empty_ships_on_create
 
     pirate = Pirate.new(catchphrase: "Stop wastin' me time")
@@ -113,6 +117,7 @@ def test_reject_if_method_with_arguments
   end
 
   def test_reject_if_with_indifferent_keys
+    Pirate.nested_attributes_options.delete :ship
     Pirate.accepts_nested_attributes_for :ship, reject_if: proc { |attributes| attributes[:name].blank? }
 
     pirate = Pirate.new(catchphrase: "Stop wastin' me time")
@@ -121,6 +126,7 @@ def test_reject_if_with_indifferent_keys
   end
 
   def test_reject_if_with_a_proc_which_returns_true_always_for_has_one
+    Pirate.nested_attributes_options.delete :ship
     Pirate.accepts_nested_attributes_for :ship, reject_if: proc { |attributes| true }
     pirate = Pirate.create(catchphrase: "Stop wastin' me time")
     ship = pirate.create_ship(name: "s1")
@@ -143,6 +149,7 @@ def test_do_not_allow_assigning_foreign_key_when_reusing_existing_new_record
   end
 
   def test_reject_if_with_a_proc_which_returns_true_always_for_has_many
+    Human.nested_attributes_options.delete :interests
     Human.accepts_nested_attributes_for :interests, reject_if: proc { |attributes| true }
     human = Human.create(name: "John")
     interest = human.interests.create(topic: "photography")
@@ -151,6 +158,7 @@ def test_reject_if_with_a_proc_which_returns_true_always_for_has_many
   end
 
   def test_destroy_works_independent_of_reject_if
+    Human.nested_attributes_options.delete :interests
     Human.accepts_nested_attributes_for :interests, reject_if: proc { |attributes| true }, allow_destroy: true
     human = Human.create(name: "Jon")
     interest = human.interests.create(topic: "the ladies")
@@ -159,6 +167,7 @@ def test_destroy_works_independent_of_reject_if
   end
 
   def test_reject_if_is_not_short_circuited_if_allow_destroy_is_false
+    Pirate.nested_attributes_options.delete :ship
     Pirate.accepts_nested_attributes_for :ship, reject_if: ->(a) { a[:name] == "The Golden Hind" }, allow_destroy: false
 
     pirate = Pirate.create!(catchphrase: "Stop wastin' me time", ship_attributes: { name: "White Pearl", _destroy: "1" })
@@ -172,6 +181,7 @@ def test_reject_if_is_not_short_circuited_if_allow_destroy_is_false
   end
 
   def test_has_many_association_updating_a_single_record
+    Human.nested_attributes_options.delete(:interests)
     Human.accepts_nested_attributes_for(:interests)
     human = Human.create(name: "John")
     interest = human.interests.create(topic: "photography")
@@ -180,6 +190,7 @@ def test_has_many_association_updating_a_single_record
   end
 
   def test_reject_if_with_blank_nested_attributes_id
+    Pirate.nested_attributes_options.delete :ship
     # When using a select list to choose an existing 'ship' id, with include_blank: true
     Pirate.accepts_nested_attributes_for :ship, reject_if: proc { |attributes| attributes[:id].blank? }
 
@@ -189,6 +200,7 @@ def test_reject_if_with_blank_nested_attributes_id
   end
 
   def test_first_and_array_index_zero_methods_return_the_same_value_when_nested_attributes_are_set_to_update_existing_record
+    Human.nested_attributes_options.delete(:interests)
     Human.accepts_nested_attributes_for(:interests)
     human = Human.create(name: "John")
     interest = human.interests.create topic: "gardening"
@@ -198,6 +210,7 @@ def test_first_and_array_index_zero_methods_return_the_same_value_when_nested_at
   end
 
   def test_allows_class_to_override_setter_and_call_super
+    Pirate.nested_attributes_options.delete :parrot
     mean_pirate_class = Class.new(Pirate) do
       accepts_nested_attributes_for :parrot
       def parrot_attributes=(attrs)
@@ -222,6 +235,7 @@ def test_accepts_nested_attributes_for_can_be_overridden_in_subclasses
   end
 
   def test_should_not_create_duplicates_with_create_with
+    Human.nested_attributes_options.delete(:interests)
     Human.accepts_nested_attributes_for(:interests)
 
     assert_difference("Interest.count", 1) do
@@ -338,12 +352,14 @@ def test_should_not_destroy_an_existing_record_if_destroy_is_not_truthy
   end
 
   def test_should_not_destroy_an_existing_record_if_allow_destroy_is_false
+    Pirate.nested_attributes_options.delete :ship
     Pirate.accepts_nested_attributes_for :ship, allow_destroy: false, reject_if: proc(&:empty?)
 
     @pirate.update(ship_attributes: { id: @pirate.ship.id, _destroy: "1" })
 
     assert_equal @ship, @pirate.reload.ship
 
+    Pirate.nested_attributes_options.delete :ship
     Pirate.accepts_nested_attributes_for :ship, allow_destroy: true, reject_if: proc(&:empty?)
   end
 
@@ -411,6 +427,7 @@ def test_should_update_existing_when_update_only_is_true_and_id_is_given
   end
 
   def test_should_destroy_existing_when_update_only_is_true_and_id_is_given_and_is_marked_for_destruction
+    Pirate.nested_attributes_options.delete :update_only_ship
     Pirate.accepts_nested_attributes_for :update_only_ship, update_only: true, allow_destroy: true
     @ship.delete
     @ship = @pirate.create_update_only_ship(name: "Nights Dirty Lightning")
@@ -420,6 +437,7 @@ def test_should_destroy_existing_when_update_only_is_true_and_id_is_given_and_is
     assert_nil @pirate.reload.ship
     assert_raise(ActiveRecord::RecordNotFound) { Ship.find(@ship.id) }
 
+    Pirate.nested_attributes_options.delete :update_only_ship
     Pirate.accepts_nested_attributes_for :update_only_ship, update_only: true, allow_destroy: false
   end
 end
@@ -532,11 +550,13 @@ def test_should_not_destroy_an_existing_record_if_destroy_is_not_truthy
   end
 
   def test_should_not_destroy_an_existing_record_if_allow_destroy_is_false
+    Ship.nested_attributes_options.delete :pirate
     Ship.accepts_nested_attributes_for :pirate, allow_destroy: false, reject_if: proc(&:empty?)
 
     @ship.update(pirate_attributes: { id: @ship.pirate.id, _destroy: "1" })
     assert_nothing_raised { @ship.pirate.reload }
   ensure
+    Ship.nested_attributes_options.delete :pirate
     Ship.accepts_nested_attributes_for :pirate, allow_destroy: true, reject_if: proc(&:empty?)
   end
 
@@ -588,6 +608,7 @@ def test_should_update_existing_when_update_only_is_true_and_id_is_given
   end
 
   def test_should_destroy_existing_when_update_only_is_true_and_id_is_given_and_is_marked_for_destruction
+    Ship.nested_attributes_options.delete :update_only_pirate
     Ship.accepts_nested_attributes_for :update_only_pirate, update_only: true, allow_destroy: true
     @pirate.delete
     @pirate = @ship.create_update_only_pirate(catchphrase: "Aye")
@@ -596,6 +617,7 @@ def test_should_destroy_existing_when_update_only_is_true_and_id_is_given_and_is
 
     assert_raise(ActiveRecord::RecordNotFound) { @pirate.reload }
 
+    Ship.nested_attributes_options.delete :update_only_pirate
     Ship.accepts_nested_attributes_for :update_only_pirate, update_only: true, allow_destroy: false
   end
 end
@@ -820,6 +842,7 @@ def test_should_automatically_enable_autosave_on_the_association
   end
 
   def test_validate_presence_of_parent_works_with_inverse_of
+    Human.nested_attributes_options.delete(:interests)
     Human.accepts_nested_attributes_for(:interests)
     assert_equal :human, Human.reflect_on_association(:interests).options[:inverse_of]
     assert_equal :interests, Interest.reflect_on_association(:human).options[:inverse_of]
@@ -842,6 +865,7 @@ def test_can_use_symbols_as_object_identifier
   end
 
   def test_numeric_column_changes_from_zero_to_no_empty_string
+    Human.nested_attributes_options.delete(:interests)
     Human.accepts_nested_attributes_for(:interests)
 
     repair_validations(Interest) do
@@ -909,6 +933,7 @@ def setup
 
 module NestedAttributesLimitTests
   def teardown
+    Pirate.nested_attributes_options.delete :parrots
     Pirate.accepts_nested_attributes_for :parrots, allow_destroy: true, reject_if: proc(&:empty?)
   end
 
@@ -933,6 +958,7 @@ def test_limit_with_exceeding_records
 
 class TestNestedAttributesLimitNumeric < ActiveRecord::TestCase
   def setup
+    Pirate.nested_attributes_options.delete :parrots
     Pirate.accepts_nested_attributes_for :parrots, limit: 2
 
     @pirate = Pirate.create!(catchphrase: "Don' botharrr talkin' like one, savvy?")
@@ -943,6 +969,7 @@ def setup
 
 class TestNestedAttributesLimitSymbol < ActiveRecord::TestCase
   def setup
+    Pirate.nested_attributes_options.delete :parrots
     Pirate.accepts_nested_attributes_for :parrots, limit: :parrots_limit
 
     @pirate = Pirate.create!(catchphrase: "Don' botharrr talkin' like one, savvy?", parrots_limit: 2)
@@ -953,6 +980,7 @@ def setup
 
 class TestNestedAttributesLimitProc < ActiveRecord::TestCase
   def setup
+    Pirate.nested_attributes_options.delete :parrots
     Pirate.accepts_nested_attributes_for :parrots, limit: proc { 2 }
 
     @pirate = Pirate.create!(catchphrase: "Don' botharrr talkin' like one, savvy?")
@@ -965,6 +993,7 @@ class TestNestedAttributesWithNonStandardPrimaryKeys < ActiveRecord::TestCase
   fixtures :owners, :pets
 
   def setup
+    Owner.nested_attributes_options.delete :pets
     Owner.accepts_nested_attributes_for :pets, allow_destroy: true
 
     @owner = owners(:ashley)
@@ -1132,3 +1161,22 @@ def test_should_build_a_new_record_based_on_the_delegated_type
     assert_equal "Hello world!", @entry.entryable.subject
   end
 end
+
+class TestPreDeclaredNestedAttributesAssociation < ActiveRecord::TestCase
+  setup do
+    assert @current_options = Developer.nested_attributes_options[:projects]
+  end
+
+  def test_should_raise_an_argument_error_with_similar_options
+    assert_raises ArgumentError do
+      Developer.accepts_nested_attributes_for :projects, **@current_options
+    end
+  end
+
+  def test_should_raise_an_argument_error_with_varying_options
+    assert_equal false, @current_options[:update_only]
+    assert_raises ArgumentError do
+      Developer.accepts_nested_attributes_for :projects, **@current_options.merge(update_only: true)
+    end
+  end
+end