Fix association override precedence over trait foreign keys

lib/factory_bot/attribute_assigner.rb

@@ -9,6 +9,7 @@ def initialize(evaluator, build_class, &instance_builder)
       @attribute_names_assigned = []
     end
 
+    # constructs an object-based factory product
     def object
       @evaluator.instance = build_class_instance
       build_class_instance.tap do |instance|
@@ -19,6 +20,7 @@ def object
       end
     end
 
+    # constructs a Hash-based factory product
     def hash
       @evaluator.instance = build_hash
 
@@ -29,6 +31,8 @@ def hash
 
     private
 
+    # Track evaluation of methods on the evaluator to prevent the duplicate
+    # assignment of attributes accessed and via `initialize_with` syntax
     def method_tracking_evaluator
       @method_tracking_evaluator ||= Decorator::AttributeHash.new(
         decorated_evaluator,
@@ -67,12 +71,15 @@ def attributes_to_set_on_hash
       attribute_names_to_assign - association_names
     end
 
+    # Builds a list of attributes names that should be assigned to the factory product
     def attribute_names_to_assign
-      @attribute_names_to_assign ||=
-        non_ignored_attribute_names +
-        override_names -
-        ignored_attribute_names -
-        aliased_attribute_names_to_ignore
+      @attribute_names_to_assign ||= begin
+        # start a list of candidates containing non-transient attributes and overrides
+        assignment_candidates = non_ignored_attribute_names + override_names
+        # then remove any transient attributes (potentially reintroduced by the overrides),
+        # and remove ignorable aliased attributes from the candidate list
+        assignment_candidates - ignored_attribute_names - attribute_names_overriden_by_alias
+      end
     end
 
     def non_ignored_attribute_names
@@ -99,31 +106,63 @@ def hash_instance_methods_to_respond_to
       attribute_names + override_names + @build_class.instance_methods
     end
 
-    ##
-    # Creat a list of attribute names that will be
-    # overridden by an alias, so any defaults can
-    # ignored.
+    # Builds a list of attribute names which are slated to be interrupted by an override.
+    def attribute_names_overriden_by_alias
+      @attribute_list
+        .non_ignored
+        .flat_map { |attribute|
+          override_names.map do |override|
+            attribute.name if ignorable_alias?(attribute, override)
+          end
+        }
+        .compact
+    end
+
+    # Is the attribute an ignorable alias of the override?
+    # An attribute is ignorable when it is an alias of the override AND it is
+    # either interrupting an assocciation OR is not the name of another attribute
     #
-    def aliased_attribute_names_to_ignore
-      @attribute_list.non_ignored.flat_map { |attribute|
-        override_names.map do |override|
-          attribute.name if aliased_attribute?(attribute, override)
-        end
-      }.compact
+    # @note An "alias" is currently an overloaded term for two distinct cases:
+    #   (1) attributes which are aliases and reference the same value
+    #   (2) a logical grouping of a foreign key and an associated object
+    def ignorable_alias?(attribute, override)
+      return false unless attribute.alias_for?(override)
+
+      # The attribute alias should be ignored when the override interrupts an association
+      return true if override_interrupts_association?(attribute, override)
+
+      # Remaining aliases should be ignored when the override does not match a declared attribute.
+      # An override which is an alias to a declared attribute should not interrupt the aliased
+      # attribute and interrupt only the attribute with a matching name. This workaround allows a
+      # factory to declare both <attribute> and <attribute>_id as separate and distinct attributes.
+      !override_matches_declared_attribute?(override)
     end
 
-    ##
-    # Is the override an alias for the attribute and not the
-    # actual name of another attribute?
+    # Does this override interrupt an association?
+    # When true, this indicates the aliased attribute is related to a declared association and the
+    # override does not match the attribute name.
+    #
+    # @note Association overrides should take precedence over a declared foreign key attribute.
     #
-    # Note: Checking against the names of all attributes, resolves any
-    #       issues with having both <attribute> and <attribute>_id
-    #       in the same factory.
+    # @note An override may interrupt an association by providing the associated object or
+    #   by providing the foreign key.
     #
-    def aliased_attribute?(attribute, override)
-      return false if attribute_names.include?(override)
+    # @param [FactoryBot::Attribute] aliased_attribute
+    # @param [Symbol] override name of an override which is an alias to the attribute name
+    def override_interrupts_association?(aliased_attribute, override)
+      (aliased_attribute.association? || association_names.include?(override)) &&
+        aliased_attribute.name != override
+    end
 
-      attribute.alias_for?(override)
+    # Does this override match the name of any declared attribute?
+    #
+    # @note Checking against the names of all attributes, resolves any issues with having both
+    #   <attribute> and <attribute>_id in the same factory. This also takes into account ignored
+    #   attributes that should not be assigned (aka transient attributes)
+    #
+    # @param [Symbol] override the name of an override
+    def override_matches_declared_attribute?(override)
+      attribute_names.include?(override)
     end
   end
 end

spec/acceptance/attribute_aliases_spec.rb

@@ -272,5 +272,188 @@
       expect(user.response).to eq "new response"
       expect(user.response_id).to eq 13.75
     end
+
+    context "when association overrides trait foreign key" do
+      before do
+        define_model("User", name: :string)
+        define_model("Post", user_id: :integer, title: :string) do
+          belongs_to :user, optional: true
+        end
+
+        FactoryBot.define do
+          factory :user do
+            name { "Test User" }
+          end
+          factory :post do
+            association :user
+            title { "Test Post" }
+
+            trait :with_system_user_id do
+              user_id { 999 }
+            end
+
+            trait :with_user_id_100 do
+              user_id { 100 }
+            end
+
+            trait :with_user_id_200 do
+              user_id { 200 }
+            end
+          end
+        end
+      end
+
+      it "prefers association override over trait foreign key" do
+        user = FactoryBot.create(:user)
+        post = FactoryBot.build(:post, :with_system_user_id, user: user)
+
+        expect(post.user).to be user
+        expect(post.user_id).to eq user.id
+      end
+
+      it "uses trait foreign key when no association override is provided" do
+        post = FactoryBot.build(:post, :with_system_user_id)
+
+        expect(post.user).to be nil
+        expect(post.user_id).to eq 999
+      end
+
+      it "handles multiple traits with foreign keys correctly" do
+        user = FactoryBot.create(:user)
+        post = FactoryBot.build(:post, :with_user_id_100, :with_user_id_200, user: user)
+
+        expect(post.user).to be user
+        expect(post.user_id).to eq user.id
+      end
+    end
+  end
+
+  context "when a factory defines attributes for both sides of an association" do
+    before do
+      define_model("User", name: :string, age: :integer)
+      define_model("Post", user_id: :integer, title: :string) do
+        belongs_to :user
+      end
+    end
+
+    context "when using the build strategy" do
+      it "prefers the :user association when defined after the :user_id attribute" do
+        FactoryBot.define do
+          factory :user
+          factory :post do
+            user_id { 999 }
+            user
+          end
+        end
+
+        user = FactoryBot.build_stubbed(:user)
+        post = FactoryBot.build(:post, user_id: user.id)
+
+        # A regression from v6.5.5 resulted in an extraneous user instance being
+        # built and assigned to post.user; it also failed to use the user_id override
+        expect(post.user).to be nil
+        expect(post.user_id).to eq user.id
+      end
+
+      it "prefers the :user_id attribute when defined after the :user attribute" do
+        FactoryBot.define do
+          factory :user
+          factory :post do
+            user
+            user_id { 999 }
+          end
+        end
+
+        user = FactoryBot.build_stubbed(:user)
+        post = FactoryBot.build(:post, user: user)
+
+        # A regression from v6.5.5 erroneously assigns the value of 999 to post.user_id
+        # and fails to assign the user override
+        expect(post.user).to be user
+        expect(post.user_id).to be user.id
+      end
+    end
+
+    context "when using the create strategy" do
+      it "handles an override of the foreign key when the :user association is declared last" do
+        FactoryBot.define do
+          factory :user
+          factory :post do
+            user_id { 999 }
+            user
+          end
+        end
+
+        user = FactoryBot.create(:user)
+        post = FactoryBot.create(:post, user_id: user.id)
+
+        # A regression in v6.5.5 created an erroneous second user and assigned
+        # that to post.user and post.user_id.
+        expect(post.user.id).to be user.id
+        expect(post.user_id).to eq user.id
+        expect(User.count).to eq 1
+      end
+
+      it "handles an override of the associated object when the :user association is declared last" do
+        FactoryBot.define do
+          factory :user
+          factory :post do
+            user_id { 999 }
+            user
+          end
+        end
+
+        user = FactoryBot.create(:user)
+        post = FactoryBot.create(:post, user: user)
+
+        # This worked fine in v6.5.5, no regression behavior exhibited
+        expect(post.user).to eq user
+        expect(post.user_id).to eq user.id
+        expect(User.count).to eq 1
+      end
+
+      it "handles an override of the associated object when :user_id is declared last" do
+        FactoryBot.define do
+          factory :user
+          factory :post do
+            user
+            # this :user_id attribute is purposely declared after :user
+            user_id { 999 }
+          end
+        end
+
+        user = FactoryBot.create(:user)
+        post = FactoryBot.create(:post, user: user)
+
+        # A regression from v6.5.5 erroneously asignes 999 to post.user_id
+        # and leaves post.user assigned to nil
+        expect(post.user_id).to eq user.id
+        expect(post.user).to eq user
+        expect(User.count).to eq 1
+      end
+
+      it "handles an override of the foreign key when :user_id is declared last" do
+        FactoryBot.define do
+          factory :user do
+            name { "tester" }
+            age { 99 }
+          end
+          factory :post do
+            user
+            # this :user_id attribute is purposely declared after :user
+            user_id { 999 }
+          end
+        end
+
+        user = FactoryBot.create(:user)
+        post = FactoryBot.create(:post, user_id: user.id)
+
+        # A regression from v6.5.5 assigns the expected values to post.user and post.user_id
+        # An erroneous second user instance, however, is created in the background
+        expect(post.user_id).to eq user.id
+        expect(post.user.id).to be user.id
+        expect(User.count).to eq 1
+      end
+    end
   end
 end