Fix association override precedence over trait foreign keys

[JinOketani]

Hello, this is my first contribution to factory_bot. While using it, I came across what seems to be an unexpected behavior, so I’d like to propose a fix. This PR fixes #1767 — since v6.5.5, associations cannot override trait-defined ID attributes.

Background / Problem

In 6.5.4, passing an association explicitly would override any *_id values provided by traits or factory defaults. After 6.5.5 (related to PR #1709), the *_id value can take precedence, causing the association and foreign key to diverge.

FactoryBot.define do
  factory :user do
    name { "Test User" }
  end

  factory :post do
    association :user
    title { "Test Post" }

    trait :with_user_id_999 do
      user_id { 999 }
    end
  end
end

user = FactoryBot.create(:user)
post = FactoryBot.build(:post, :with_user_id_999, user: user)

# 6.5.5 (broken):
post.user_id # => 999 (trait wins; inconsistent with post.user)
# 6.5.4 (expected) and after this fix:
post.user_id # => user.id (association wins; consistent)

Root Cause

The issue stems from changes made in PR #1709 to resolve attribute/attribute_id conflicts. The modified AttributeAssigner#aliased_attribute? method now returns false when the override is an actual attribute name, but this inadvertently affects association overrides.

Solution / Approach

Refine alias/override precedence so that:

1. Explicit association overrides win over trait/factory defaults for the corresponding *_id.
2. The attribute vs. attribute_id conflict fix from PR BugFix: '<attribute>' and '<attribute>_id' conflict. #1709 remains intact.
3. Behavior is unchanged when no association override is provided (i.e., trait-defined *_id still applies).

This change enhances the alias check in AttributeAssigner to ignore the *_id value when the corresponding association is explicitly overridden. While this serves as a temporary fix for the bug, a more fundamental solution may ultimately be needed.

Changes

• Modify lib/factory_bot/attribute_assigner.rb to prioritize association overrides while preserving PR BugFix: '<attribute>' and '<attribute>_id' conflict. #1709’s fix.
• Add spec/acceptance/association_override_regression_spec.rb covering:
• Association override > trait-defined foreign key
• Trait-defined foreign key when no association override is present
• Multiple traits defining foreign keys

Tests

• New regression specs added (see above).
• Existing test suite passes locally and on CI.
• Alias behavior introduced by PR BugFix: '<attribute>' and '<attribute>_id' conflict. #1709 remains verified.

Backward Compatibility / Risk

• Restores the intuitive, pre-6.5.5 precedence without altering behavior where no override is supplied.
• Touches alias resolution only; no performance impact expected.
• No changes to the public API.

Before / After

# Before (6.5.5 - broken)
user = FactoryBot.create(:user)
post = FactoryBot.build(:post, :with_user_id_999, user: user)
post.user_id # => 999 (WRONG)
post.user    # => user (inconsistent)

# After (this PR - correct)
user = FactoryBot.create(:user)
post = FactoryBot.build(:post, :with_user_id_999, user: user)
post.user_id # => user.id (CORRECT)
post.user    # => user (consistent)

[neilvcarvalho]

Thanks, @JinOketani! I'm playing a bit with the code and trying to find ways to break it. I'm calling it a day today, but I'll be back to continue reviewing this PR next week.

[JinOketani]

Thanks @neilvcarvalho! Hope this helps. Here are the reproduction steps.

Environment

• Ruby: 3.3.6
• Rails: 8.0.2
• ActiveRecord: 8.0.2 (SQLite3)
• FactoryBot: 6.5.5
• FactoryBot Rails: 6.5.1
• RSpec Rails: 8.0.2

Expected Behavior (v6.5.4 and earlier)

When passing both a trait with a foreign key and an explicit association, the explicit association should take precedence and the foreign key should be consistent.

book = create(:book)
book_author = build(:book_author, :with_book_id_999, book: book)

# Expected:
book_author.book_id   # => book.id (1)
book_author.book      # => book (consistent)

Actual Behavior (v6.5.5)

The trait-defined foreign key overrides the explicit association, creating inconsistent state.

book = create(:book)
book_author = build(:book_author, :with_book_id_999, book: book)

# Actual:
book_author.book_id   # => 999 (from trait)
book_author.book      # => nil (association ignored)

Reproduction Steps

1. Set up models and factories

Models:

class Author < ApplicationRecord
  has_many :book_authors, dependent: :destroy
  has_many :books, through: :book_authors

  validates :name, presence: true
  validates :email, presence: true, format: { with: URI::MailTo::EMAIL_REGEXP }
end

class Book < ApplicationRecord
  has_many :book_authors, dependent: :destroy
  has_many :authors, through: :book_authors

  validates :title, presence: true
  validates :isbn, presence: true, uniqueness: true
  validates :published_at, presence: true
end

class BookAuthor < ApplicationRecord
  belongs_to :book
  belongs_to :author

  validates :book, presence: true
  validates :author, presence: true
  validates :role, inclusion: { in: ['author', 'co-author', 'editor', 'translator'] }
end

Factories:

FactoryBot.define do
  factory :book_author do
    role { 'author' }
    contribution_percentage { 100 }
    notes { 'Primary author of this work' }
    active { true }

    association :book
    association :author

    # This trait reproduces the PR #1768 regression
    trait :with_book_id_999 do
      book_id { 999 }
    end
  end

  factory :author do
    sequence(:name) { |n| "Author #{n}" }
    sequence(:email) { |n| "author#{n}@example.com" }
    bio { "A talented writer" }
    active { true }
  end

  factory :book do
    sequence(:title) { |n| "Book Title #{n}" }
    sequence(:isbn) { |n| "978-#{sprintf('%010d', n)}" }
    published_at { 1.year.ago }
    genre { "Fiction" }
    active { true }
  end
end

2. Create test case that demonstrates the issue

RSpec.describe 'FactoryBot: Trait Foreign Keys Override Explicit Associations' do
  it 'demonstrates trait foreign key overriding explicit association' do
    # Create a book we want to associate
    book = create(:book, title: 'Expected Book')

    # This should use the explicit book association, ignoring trait's book_id
    book_author = build(:book_author, :with_book_id_999, book: book)

    puts "Expected book_id: #{book.id}"
    puts "Actual book_id: #{book_author.book_id}"
    puts "Association: #{book_author.book.inspect}"

    # This fails - trait foreign key wins over explicit association
    expect(book_author.book_id).to eq(book.id), 
      "Expected explicit association to override trait foreign key"
    expect(book_author.book).to eq(book), 
      "Expected association to be set correctly"
  end
end

3. Run the test to see the failure

bundle exec rspec spec/models/book_author_factory_regression_spec.rb

Expected Output:

Expected book_id: 1
Actual book_id: 999
Association: nil

Failures:
1) Expected explicit association to override trait foreign key

[oehlschl]

FWIW, this worked for me and didn't introduce any other regressions (that I noticed, at least). Thanks @JinOketani.

@CodeMeister any concerns with this, based on the intentions / implementation of your previous changes?

[JinOketani]

It’s been a while since this was opened, but I wanted to follow up on it. I agree that this is a relatively minor change, but since it addresses a potential underlying issue, I’d prefer not to leave the bug unpatched. It would be great if we could merge this to keep the library stable.

@neilvcarvalho How does that sound to you?

[CodeMeister]

@oehlschl looks good to me, nicely done 🚀

[oehlschl]

since it addresses a potential underlying issue, I’d prefer not to leave the bug unpatched

To this point, we're currently pinned to v6.5.4 because of this issue and will remove that constraint once this is released.

[vburzynski]

Thank you for submitting this and I apologize that it's taken us a while to review it.

I've been digging through the code and comparing v6.5.4, v6.5.5, and the fix you've submitted here. The code in this PR does seem to fix the regression you found. It also preserves the behavior from the #1709 fix. However, I found another regression that exists in both the released v6.5.5 and the code in this PR. This regression also relates to overrides.

When I first started digging, I wanted to confirm the behavior of declaring both user and user_id attributes in a factory (without the use of trait). I ended up writing the following specs, which pass in all three versions:

describe "attribute aliases" 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 "defers to the last declared attribute amongst alias matches" do
    it "defers to the :user_id attribute when declared after the :user attribute" do
      FactoryBot.define do
        factory :user
        factory :post do
          user
          user_id { 99 }
        end
      end

      post = FactoryBot.build(:post)
      expect(post.user_id).to eq 99
      expect(post.user).to eq nil
    end

    it "defers to the :user attribute when declared after the :user_id attribute" do
      FactoryBot.define do
        factory :user
        factory :post do
          user_id { 99 }
          user
        end
      end

      post = FactoryBot.build(:post)
      expect(post.user_id).to eq nil
      expect(post.user).to be_an_instance_of(User)
    end
  end
end

After additional digging, I stumbled upon a scenario where the code from v6.5.4 passes, but the code contained in v6.5.5 and this PR fail:

describe "attribute aliases" 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 overrides include a :user_id foreign key" do
    it "handles setting the user association" do
      FactoryBot.define do
        factory :user
        factory :post do
          user
          user_id { 999 }
        end
      end

      user = FactoryBot.create(:user)
      post = FactoryBot.create(:post, user_id: user.id)

      expect(post.user).to eq user
      expect(post.user_id).to eq user.id
      expect(User.count).to be 1
    end
  end
end

Failures:

  1) attribute aliases when overrides include a :user_id foreign key handles setting the user association
     Failure/Error: expect(User.count).to be 1
     
       expected #<Integer:3> => 1
            got #<Integer:5> => 2
     
       Compared using equal?, which compares object identity,
       but expected and actual are not the same object. Use
       `expect(actual).to eq(expected)` if you don't care about
       object identity in this example.

Some aspect of the original logic found in FactoryBot::AttributeAssigner#ignorable_alias? (renamed to FactoryBot::AttributeAssigner#aliased_attribute?) prevented an additional and erroneous User instance from being created when employing the create strategy.

[vburzynski]

Oops, my expect(User.count).to be 1 should actually be expect(User.count).to eq 1 and the error:

  1) attribute aliases when overrides include a :user_id foreign key handles setting the user association
     Failure/Error: expect(User.count).to eq 1
     
       expected: 1
            got: 2
     
       (compared using ==)

[vburzynski]

🤔 🧠 Sharing a brain dump as I think through this (in case it spurs ideas)

• the original patch from BugFix: '<attribute>' and '<attribute>_id' conflict. #1709 is intended to allow <attribute> and <attribute>_id when neither are FactoryBot::Attribute::Association.

• This fix addresses a regression where <attribute> is declared as an association and an override from a trait instead provides the <attribute>_id foreign key.

• The regression I discovered can result in cases where both attributes may be applied, which may result in an additional and erroneous construction of a second associated object.

• FactoryBot's enforcement of aliases, is agnostic to what types of attributes are in play.

  • FactoryBot doesn't care if they are both dynamic attributes
  • similarly it doesn't care if one is an association while the other is dynamic
  • you could have a dynamic attribute and a sequence attribute as well.

• Alias patterns are enforced globally. If regex patterns are configured to designate :name and :alias_name as aliases. This alias pattern is enforced across all factories. There doesn't seem to be a way to apply aliases to single factories, or to a subset of factories.

• The concept of an "alias" in FactoryBot seems overloaded and covers two similar yet distinct cases

  • A) a "true" alias — two attributes on the receiver (or object produced by factory) reference the same value. The methods on the receiver likely access the same instance variable.
  • B) facets of an association — one attribute is an identifier (foreign key), while the other relates to the associated object. The attributes — as a group — are a construct representing the association. There's some business logic mapping and governing how the foreign key maps to to the associated object. Likely all handled by ActiveRecord when in a Rails app.

• Ideally if you have an attribute :name, that is a dynamic or sequence attribute type (anything that is not an association attribute), then FactoryBot should have no qualms with the factory also declaring an attribute named :name_id. That is unless :name and :name_id are truly aliases of one another. In which case FactoryBot should assign only one of them to the object being constructed.

  • Admittedly :name_id is a bit contrived here as it breaks with naming conventions and it is is more likely that on a User model, you'd have something like :name and :nickname be aliases.

• If, on the other hand, :name is an association...

  • then FactoryBot shouldn't allow you to also declare an unrelated attribute with the name :name_id.
  • perhaps FactoryBot should generate a warning or error if :name is declared as a association and :name_id is also declared within the same factory.
  • though it should still be permissible for traits, child-factories, and syntax method invocations to override :name_id

• Oddly the alias patterns allow for situations like this:

  association = FactoryBot::Attribute::Association.new(:attribute_id, nil, [])
  association.alias_for?(:attribute)
  # => true
  association.alias_for?(:attribute_id_id)
  # => true

  That is FactoryBot detects attribute_id and attribute as aliases; but also detects attribute_id and attribute_id_id as aliases.

The more I dig in, the more it seems that aliases could use some rework. Though that seems like something that would require either a major or minor version release (and careful design).

I'd like to patch/fix the regression before that though 🤔 I too don't like leaving v6.5.5 unpatched

[vburzynski]

@JinOketani and @neilvcarvalho I've expanded upon JinOkeani's original code to additionally address the regression I found. I'd love to hear what others think 🙂

[vburzynski]

Also sorry about the few messy commits, I think I've been at the computer too long and I'm going to step away now 🤣