Open dev-Gois opened 6 months ago
We experience the same. After updating Rails from 7.0 to 7.1 it seems that uniqueness validations of models instantiated with build_stubbed
are passing even when they should not.
Consider the following spec, asserting that users with non-unique usernames are invalid:
# Model
class User < ApplicationRecord
# To reproduce the issue, the model table must also have a unique index on username (see later why)
validates :username, uniqueness: true
end
# Spec
it 'prevents the creation of multiple users with the same username' do
# First create a user with a given username
create(:user, username: 'someone')
# Then, assert that another user with the same username would be invalid
user = build_stubbed(:user, username: 'someone')
expect(user).not_to be_valid
end
The spec used to pass in Rails 7.0, but is breaking with Rails 7.1, because user.valid?
mistakenly returns true
even if it should return false
due to non-unique username. This only happens when using build_stubbed
, and instead works correctly when using build
.
I think that this is caused by Rails 7.1 treating persisted records differently in uniqueness validations: in case the record is persisted (and build_stubbed
defines persisted?
as returning always true
), and the validated attribute didn't change, Rails 7.1 will skip validations if no attribute changed, and the uniqueness constraint is covered by a database index.
This is an optimization to avoid hitting the database when unnecessary (introduced in Rails 7.1 by this pull request), but it has the side effect of skipping the uniqueness validations for models created by build_stubbed
having a unique index on the column being validated (stubbed models "pretend" to be persisted, but never actually hit the database, and therefore the Rails 7.1 assumption that the index must have already ensured uniqueness is false).
If that's correct, this is not really a bug with FactoryBot
, although this Rails optimization does result in failing tests once one upgrades Rails. It might help documenting this behavior though, as it is very surprising, and can usually be fixed by using build
instead of build_stubbed
(unfortunately at a performance cost), or by changing the validated attribute after creation (which is surprising and apparently useless if one is not aware of this issue).
Regarding options to "fix" this on FactoryBot
side, it's tricky. It would probably involve patching private methods on the UniquenessValidator
instances bound to a stubbed model, for example to make validation_needed?
return always true
, skipping the optimization. That's a bit hacky, but would solve this issue.
Thx for the reply @lucaong, we ended up just replacing it with the build
Reasoning on what FactoryBot
could possibly do to make this issue better, i think there is a need for an additional strategy on top of build
and build_stubbed
.
My (erroneous) mental model of build_stubbed
was that it worked the same as build
(instantiating a new model without persisting it), just stubbing all associations to avoid triggering a lot of database inserts. That's useful for many use cases where persisting the model or creating real associations is not needed. Instead, build_stubbed
is much more similar to create
: it does not actually save the object to the database, but it does redefine methods like persisted?
and new_record?
to make it look like the model was saved in the database. This behavior is well documented, but I feel that a more appropriate name for it would have been create_stubbed
, as the resulting object looks like a persisted one.
There is the need for a currently missing method that builds a new model stubbing all the associations, but without persisting not pretending to persist. Basically, a method doing the same as build
, but stubbing the associations to make the test faster (which is what I initially thought build_stubbed
was doing). The tests for uniqueness validation mentioned in this thread are one example where such method would be useful.
In my ideal scenario, FactoryBot
would provide these methods:
build
- instantiates a model using a factory, without persisting itbuild_stubbed
- instantiates a model using a factory, and stubbing associations for performance, without persisting it and without faking persistence like the current implementation doescreate
- creates and persists a model using a factorycreate_stubbed
- instantiates a model using a factory, stubbing associations, and stubbing persisted?
and other similar methods to make it look like the model was persisted (what build_stubbed
does currently)This would involve changing the current documented behavior of build_stubbed
, which introduces a breaking change. The alternative would be to avoid a breaking change, keep the current default behavior of build_stubbed
(accepting it's slightly misleading name), and add a new option to skip stubbing persisted?
, new_record?
, and other persistence methods. Something like build_stubbed(:user, stub_persistence: false)
.
What do the maintainers think about this? If you agree this would be a good feature, and a general direction is agreed (breaking change with cleaner naming vs. non-breaking change with slightly misleading naming), I could hopefully devote some time to send a pull request.
Description
When i try to upgrade to Rails 7.1 from Rails 6.0, i get some errors in my specs, i found for some reason when i use build_stubbed in validations, it don't work properly.
Reproduction Steps
I just runned my specs and for some reason this broke:
idk, for some reason this dont work, is expected to the invalid_seasonality_item to be invalid, but, its valid in this tests, so, i just changed the
build_stubbed
forbuild
and thats worksExpected behavior
I want some response about what happened with build_stubbed and a solution for that
Actual behavior
At the moment, my specs with build_stubbed are broke.
System configuration
factory_bot version:
"factory_bot_rails", "~> 6.4.0"
rails version:"rails", "~> 7.1.3"
ruby version:ruby 3.3.0