Traits possible race condition when creating dependent associations

[jwoodrow]

Description

When using FactoryBot.create to create a model with a dependent association validations that depend on traits will fail because they are applied too late

This can be doubly confirmed if using a custom validation method and debugging within the method to check the value of the record

Another side effect is that validations will get called twice, once when the dependent association is created and once when the main model is created. This isn't the main issue however since having 2 validations that pass is better than having one that fails.

EDIT 1: It might also be some sort of race condition since the validation should not be required anyway if the trait wasn't at least partially applied, which makes this all the more confusing to me. EDIT 2: Added a reproduction script

Reproduction Steps

Reproduction repo

Reproduction script

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"
  git_source(:github) { |repo| "https://github.com/#{repo}.git" }
  gem "factory_bot", "~> 6.0"
  gem "activerecord"
  gem "sqlite3"
end

require "active_record"
require "factory_bot"
require "minitest/autorun"
require "logger"

ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :users, force: true do |t|
    t.integer :role, default: 0
    t.string :some_value
  end

  create_table :addresses, force: true do |t|
    t.references :user, null: false, foreign_key: true
  end
end

class User < ActiveRecord::Base
  enum role: %i[user admin]

  has_one :address

  validates_presence_of :some_value, if: :admin?
end

class Address < ActiveRecord::Base
  belongs_to :user
end

FactoryBot.define do
  factory :user do
    role { :user }

    address { association(:address, user: instance) }

    trait :admin do
      role { :admin }
      some_value { 'present' }
    end
  end

  factory :address do
    user
  end
end

class FactoryBotTest < Minitest::Test
  def test_factory_bot_stuff
    user = FactoryBot.create(:user, :admin)

    assert user.persisted?
  end
end

# Run the tests with `ruby <filename>`

Reproduction detailed steps

1. Create a main model with a conditional presence validation

class User
  enum role: %I[user admin]
  
  validates_presence_of :some_value, if: :admin?
end

2. Create a second model linked to the First model

class User
# ...
  has_one :address
# ...
end

class Address
  belongs_to :user
end

3. Create a factory with a trait to validate the given condition

FactoryBot.define do
  factory :user do
    role { :user }

    address { association(:address, user: instance) }

    trait :admin do
      role { :admin }
      some_value { 'present' }
    end
  end
end

4. Create a simple factory for the second model

FactoryBot.define do
  factory :address do
    user { association(:user, address: instance) }
  end
end

5. Add some simple specs

require 'rails_helper'

RSpec.describe User, type: :model do
  context 'without trait' do
    it 'builds' do
      expect(build(:user)).to be_valid
    end

    it 'creates' do
      expect(create(:user)).to be_persisted
    end
  end

  context 'with admin trait' do
    it 'builds' do
      expect(build(:user, :admin)).to be_valid
    end

    # this will fail
    it 'creates' do
      expect(create(:user, :admin)).to be_persisted
    end
  end
end

6. Run specs

rails db:migrate
bundle exec rspec

Expected behavior

bundle exec rspec
#=> ....

Actual behavior

bundle exec rspec

#=> ...F
#=> Failures:

#=>  1) User with admin trait creates
#=>     Failure/Error: address { association(:address, user: instance) }

#=>     ActiveRecord::NotNullViolation:
#=>       PG::NotNullViolation: ERROR:  null value in column "user_id" violates not-null constraint

System configuration

factory_bot version: 6.2.0 rails version: 6.1.3.2 ruby version: 2.7.3

[jwoodrow]

Hi, just wondering if anyone had a chance to take a look at this issue ?

[composerinteralia]

The way factory_bot's create strategy works is that it first builds the object, then assigns the attributes one by one (with traits getting assigned last, if I recall correctly), then calls save!. So in your example that would look like something like this:

user = User.new
user.address = FactoryBot.create(:address, user: user)
user.role = :admin
user.some_value = 'present'
user.save!

FactoryBot will create the address association with the same strategy, so expanded out that would look like:

user = User.new
address = Address.new
address.user = user
address.save!
user.address = address
user.role = :admin
user.some_value = 'present'
user.save!

Because you've got validations on both sides of the user/address association, you need to assign everything before saving anything, but that's not what happens here. The address.save! in the middle there causes everything to save before the user is valid.

I think you can fix this by changing the strategy of the address association to :build rather than :create, which essentially removes the address.save! in the middle:

- address { association(:address, user: instance) } 
+ address { association(:address, strategy: :build, user: instance) } 

Then the user and address will get saved together at the end.

[jwoodrow]

Thanks @composerinteralia for taking the time to reply.

I should have specified that I knew about the workaround (since I'm using it in my projects to make the specs work).

I do not think this is what's happening here though because in my example because if I refer to your explanation then this test should fail too but this isn't the case

FactoryBot.create(:user)

The failure really only happens when I apply the trait because for some reason on the model Address the user has the role admin but some_value is blank this then leads the user to be ineligible for saving which further leads to the PG::NotNullViolation.

Anyway, I still believe no save should be done before a full application of all traits on all nested factories since this means for a hypothetical 5 layer deep nest using traits each test doing a create will lead to 10 writes to the database in place of 5.

Shouldn't saving last be the normal/standard behaviour ?

I don't get the last part though, association is meant to be the smart way of handling factories. rather than creating all the time (even when building) or building all the time (even when creating) you let factorybot use the original builder method. Especially because this allows the build/create hooks to actually be used as expected.

If that was/is the only solution, I'd also be clearer and more readable if I did this rather than adding the strategy: :build

address { build(:address, instance) }

[composerinteralia]

I do not think this is what's happening here though because in my example because if I refer to your explanation then this test should fail too but this isn't the case

Ah, I think I see what you mean. It looks like the attributes are getting assigned slightly differently than I thought: role, then address, then some_value. So the user happens to be invalid when the address is created because the role is set to :admin but some_value isn't set yet. Since the user is invalid and can't be saved, the address will fail to save as well.

Interestingly I can get your test passing by moving the address declaration to the top of the factory, above the role declaration. But that's probably not ideal still because we save the user, save the address, but then have to update the user again with the role and some_value.

Shouldn't saving last be the normal/standard behaviour ?

I can see why you want that here. But that's not the way the create strategy works, and changing that behavior would break a lot of applications (most notably any applications that have validations on a foreign key rather than the association). To avoid saving each association as you go, you'd need to switch to the build strategy (in which case using the build method instead of strategy: :build makes sense to me).

Especially because this allows the build/create hooks to actually be used as expected.

That's an interesting point. I'm not sure I see a way around that at the moment.

---

FWIW, one approach I sometimes use when I feel I'm fighting with factory_bot is to write test helper methods to tie everything together:

def create_user_with_address(address_attributes: {}, user_attributes: {})
  address = FactoryBot.build(:address, **address_attributes)
  FactoryBot.create(:user, address: address, **user_attributes) do |user|
    # stuff that needs to happen after everything is saved
  end
end

That way I can keep the factories minimal, with only the attributes needed to create a valid object. In your example, I might have left out the address association altogether in the user factory, since it's not required to build a valid user. In cases where I want the address, I'd use the helper method.

[jwoodrow]

Interestingly I can get your test passing by moving the address declaration to the top of the factory, above the role declaration. But that's probably not ideal still because we save the user, save the address, but then have to update the user again with the role and some_value.

Now that is interesting. I had thought there might be something along those lines but never thought to try and change the declaration order.

From your reply I would guess if I changed the factory declaration to something like this things would probably work (as strange as it seems)

role { :user }
some_value { nil }

address { association(:address, user: instance) }

trait :user do
  role { :admin }
  some_value { 'present' }
end

Probably because since FB has an original value explicitly set for the attribute it will copy over any overridden value from the trait before passing the instance on to the associations. I'd have to try it out once I get home but this makes things even more confusing because it would seem order and explicit definition of attributes is a requirement to make things work as expected !

[composerinteralia]

I'd guess you are correct that adding a some_value declaration above the association would work. I'd also agree that it would be a confusing and perhaps brittle solution (I could imagine somebody coming along and trying to put things in alphabetical order and breaking the factory).

[jwoodrow]

I could imagine somebody coming along and trying to put things in alphabetical order and breaking the factory

It all depends on the dev I suppose. I like to declare in the following order attributes->associations->traits and in each block I order alphabetically so it wouldn't be an issue. But I do concur that declaration order should not come into account when creating with traits 🤔.

I haven't dug too much into the code but I'd guess changing the order a bit when creating so it would do this:

model = Model.new
model.assign_attributes(**traitless_attributes)
traits.each do |trait|
  user.assign_attributes(**trait.attributes)
end
# logic for build/create associations using the last override here
model.save!

would probably not create any breaking changes since the end result would be the same 🤔

I find the codebase for FactoryBot pretty hard to navigate to figure out where this whole logic takes place to better understand the problem here si the block of code is the best proposal I can come up with right now.

[flatrocks]

Saving on create before applying the provided attributes is remarkably counterintuitive. I too have wasted a lot of time trying to create factory objects with depended associations.

It's mind-boggling that this fails because of missing my_required_attribute.

# Fails on validation because my_required_attribute is missing ?!?!
model1 = create :my_model, my_required_association: other_thing
let(:model1) { create :my_model, my_required_association: other_thing }

However, understanding how create works leads to an (anti?) pattern like this:

# Works!
model1 = build(:my_model, my_required_association: other_thing).tap { |m| m.save }
let(:model1) { build(:my_model, my_required_association: other_thing).tap { |m| m.save } }
# or...
let(:model1) { build(:my_model, my_required_association: other_thing).tap(&:save) }

It's ugly but functional, and it's compact enough for a one-line let() statement.