Merge pull request #15358 from arthurnn/redefine_habtm_fix

Fix redefine a has_and_belongs_to_many inside inherited class

3 files changed, 46 insertions, 25 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index d8157d02ab..736745c3cd 100644
--- a/activerecord/CHANGELOG.md
+++ b/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.
diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb
index 2a7acf6787..74e2a8e6b9 100644
--- a/activerecord/lib/active_record/autosave_association.rb
+++ b/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
-
-              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
+          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) } 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)
diff --git a/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb b/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb
index 878f1877db..8d8201ddae 100644
--- a/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb
+++ b/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