Fix redefine a has_and_belongs_to_many inside inherited class

After ad7b5efb55, which changed how
has_an_belongs_to_many used to work, we start raising an error when
redefining the same has_an_belongs_to_many association. This commits fix
that regression.

[Fixes #14983]

activerecord/CHANGELOG.md

@@ -1,3 +1,11 @@
+*   Fix redefine a has_and_belongs_to_many inside inherited class
+    Fixing regression case, where redefining the same has_an_belongs_to_many
+    definition into a subclass would raise.
+
+    Fixes #14983.
+
+    *arthurnn*
+
 *   Add a properties API to allow custom types and type casting behavior
     to be specified. Will enable many edge cases to be deprecated, and
     allow for additional interesting features in the future.

activerecord/lib/active_record/autosave_association.rb

@@ -147,6 +147,7 @@ module ActiveRecord
       private
 
         def define_non_cyclic_method(name, &block)
+          return if method_defined?(name)
           define_method(name) do |*args|
             result = true; @_already_called ||= {}
             # Loop prevention for validation of associations
@@ -179,30 +180,28 @@ module ActiveRecord
           validation_method = :"validate_associated_records_for_#{reflection.name}"
           collection = reflection.collection?
 
-          unless method_defined?(save_method)
-            if collection
-              before_save :before_save_collection_association
+          if collection
+            before_save :before_save_collection_association
 
-              define_non_cyclic_method(save_method) { save_collection_association(reflection) }
-              # Doesn't use after_save as that would save associations added in after_create/after_update twice
-              after_create save_method
-              after_update save_method
-            elsif reflection.macro == :has_one
-              define_method(save_method) { save_has_one_association(reflection) }
-              # Configures two callbacks instead of a single after_save so that
-              # the model may rely on their execution order relative to its
-              # own callbacks.
-              #
-              # For example, given that after_creates run before after_saves, if
-              # we configured instead an after_save there would be no way to fire
-              # a custom after_create callback after the child association gets
-              # created.
-              after_create save_method
-              after_update save_method
-            else
-              define_non_cyclic_method(save_method) { save_belongs_to_association(reflection) }
-              before_save save_method
-            end
+            define_non_cyclic_method(save_method) { save_collection_association(reflection) }
+            # Doesn't use after_save as that would save associations added in after_create/after_update twice
+            after_create save_method
+            after_update save_method
+          elsif reflection.macro == :has_one
+            define_method(save_method) { save_has_one_association(reflection) } unless method_defined?(save_method)
+            # Configures two callbacks instead of a single after_save so that
+            # the model may rely on their execution order relative to its
+            # own callbacks.
+            #
+            # For example, given that after_creates run before after_saves, if
+            # we configured instead an after_save there would be no way to fire
+            # a custom after_create callback after the child association gets
+            # created.
+            after_create save_method
+            after_update save_method
+          else
+            define_non_cyclic_method(save_method) { save_belongs_to_association(reflection) }
+            before_save save_method
           end
 
           if reflection.validate? && !method_defined?(validation_method)

activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb

@@ -70,6 +70,14 @@ class DeveloperWithSymbolsForKeys < ActiveRecord::Base
     :foreign_key => "developer_id"
 end
 
+class SubDeveloper < Developer
+  self.table_name = 'developers'
+  has_and_belongs_to_many :special_projects,
+    :join_table => 'developers_projects',
+    :foreign_key => "project_id",
+    :association_foreign_key => "developer_id"
+end
+
 class HasAndBelongsToManyAssociationsTest < ActiveRecord::TestCase
   fixtures :accounts, :companies, :categories, :posts, :categories_posts, :developers, :projects, :developers_projects,
            :parrots, :pirates, :parrots_pirates, :treasures, :price_estimates, :tags, :taggings
@@ -814,7 +822,7 @@ class HasAndBelongsToManyAssociationsTest < ActiveRecord::TestCase
     assert_equal [], Pirate.where(id: redbeard.id)
   end
 
-  test "has and belongs to many associations on new records use null relations" do
+  def test_has_and_belongs_to_many_associations_on_new_records_use_null_relations
     projects = Developer.new.projects
     assert_no_queries do
       assert_equal [], projects
@@ -860,4 +868,10 @@ class HasAndBelongsToManyAssociationsTest < ActiveRecord::TestCase
 
     assert_includes magazine.articles, article
   end
+
+  def test_redefine_habtm
+    child = SubDeveloper.new("name" => "Aredridel")
+    child.special_projects << SpecialProject.new("name" => "Special Project")
+    assert_equal true, child.save
+  end
 end