build_stubbed broke when i upgrade to Rails 7.1.3

[dev-Gois]

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:

  context "validations" do
    it { is_expected.to(validate_presence_of(:product_id)) }

    it "validate uniqueness of product_id" do
      valid_seasonality_item = create(:seasonality_item)
      invalid_seasonality_item = build_stubbed(:seasonality_item,
        product: valid_seasonality_item.product,
        seasonality: valid_seasonality_item.seasonality,
      )
      invalid_seasonality_item.valid?
      expect(invalid_seasonality_item.errors&.first&.attribute).to(eq(:product_id))
    end
  end

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 for build and thats works

Expected 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

[lucaong]

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.

Example:

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.

[dev-Gois]

Thx for the reply @lucaong, we ended up just replacing it with the build

[lucaong]

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 it
• build_stubbed - instantiates a model using a factory, and stubbing associations for performance, without persisting it and without faking persistence like the current implementation does
• create - creates and persists a model using a factory
• create_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.