factory_bot
factory_bot copied to clipboard
Traits possible race condition when creating dependent associations
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 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
- Create a main model with a conditional presence validation
class User
enum role: %I[user admin]
validates_presence_of :some_value, if: :admin?
end
- Create a second model linked to the First model
class User
# ...
has_one :address
# ...
end
class Address
belongs_to :user
end
- 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
- Create a simple factory for the second model
FactoryBot.define do
factory :address do
user { association(:user, address: instance) }
end
end
- 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
- 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
Hi, just wondering if anyone had a chance to take a look at this issue ?
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.
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) }
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.
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 !
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).
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.
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.