Reject sequence definitions for Active Record primary keys

[seanpdoyle]

Alternative to thoughtbot/factory_bot#1586 Depends on thoughtbot/factory_bot#1587

First, introduce the FactoryBotRails::FactoryValidator class to serve as a generic factory_bot.compile_factory event observer.

Throughout the lifecycle of the FactoryBotRails::Railtie, afford various initializers with opportunities to add purpose-built validators to the instance's internal set.

When factory_bot.compile_factory events are published, iterate through the list of validators and forward along the value of the event.payload to the validator's #validate! method.

Next, introduce the Active Record-specific FactoryBotRails::FactoryValidator::ActiveRecordValidator class. Only require the module whenever Active Record's engine is loaded.

The ActiveRecordValidator#validate! method rejects attributes that define primary key generation logic for ActiveRecord::Base descendants.

In order to test this behavior, add a development dependency on sqlite3 and activerecord, along with some model and database table generating helper methods.

[mike-burns]

The next steps, as I understand it:

1. merge the factory_bot PR
2. release factory_bot
3. remove the Gemfile modification here
4. merge this
5. release factory_bot_rails
6. announce it on our blog & fediverse

[seanpdoyle]

The test suite passes locally, but fails in CI. I'm investigating.

[mike-burns]

CI

I assume this is because factory_bot isn't released yet.

[mjobin-mdsol]

is this fixing issues with upcoming Rails 7.1 ?

[mike-burns]

is this fixing issues with upcoming Rails 7.1 ?

More details in https://github.com/thoughtbot/factory_bot/pull/1586#issuecomment-1673732719. The short version is: no, it's not fixing anything. It is instead making sure people don't use sequences for something that is generated by the DB, because that will break tests, and because we could not think of a reason to do so.

Nothing is merged yet, so if you can think of a usecase then we'd like to know soon.

[mike-burns]

factory_bot 6.3.0 is out with support for the compile_factory notification.

[seanpdoyle]

@mike-burns thanks for pushing that through! CI seems to be content with newer versions of Ruby, but failing on 2.7 and below. I'm not sure why.

[franzliedke]

@seanpdoyle This causes problems for us, as we have a table with a primary key that is not generated by the database (those two are not 100% the same thing).

Would it be possible to check the schema instead of the primary_key config? Something like this:

ModelKlass.column_for_attribute(primary_key_name).serial?

This seems to work for me - not sure if this is Postgres-specific.

[mike-burns]

Ah I see, we should check for SERIAL, not PRIMARY KEY. I think that makes sense -- do you agree, @seanpdoyle ?

[seanpdoyle]

I agree, this conditional should account for whether or not the database is generating the value itself.

Unfortunately, I can't find any documentation for #serial?. There is a Postgres-specific implementation that's available in activerecord>=5.2, but it isn't part of the Column object's public interface.

Similarly, there is a Column#auto_populated? predicate defined as public interface that Postgres implements in terms of #serial?, but that's only available from activerecord>=7.1.

Is there another way to determine whether or not a column is managed by the database that works with older versions?

[aprescott]

I ran into the same issue, with a model that has a primary key not generated by the database. auto_populated? (or an equivalent) seems like it would be preferable over checking serial?. An app I work on uses id: :uuid, default: -> { "gen_random_uuid()" } for almost all of its tables, resulting in serial? being falsey (nil in fact), whereas default_function is present so auto_populated? would be truthy.

[jagthedrummer]

if you can think of a usecase then we'd like to know soon.

@mike-burns, I'm way late to the party, but I just wanted to chime in.

We have a usecase where we use FB to generate example JSON payloads (using build instead of create) and we want the example to contain IDs (and timestamps and other values that would be present in a "complete record").

For example, we have a factory defined like this:

FactoryBot.define do
  factory :team do
    sequence(:name) { |n| "Generic Team #{n}" }
    sequence(:slug) { |n| "team_#{n}" }
    time_zone { nil }
    locale { nil }

    factory :team_example do
      id { 42 }
      sequence(:name) { |n| "EXAMPLE Generic Team #{n}" }
      sequence(:slug) { |n| "EXAMPLE team_#{n}" }
      time_zone { ActiveSupport::TimeZone.all.first.name }
      locale { "en" }
      created_at { DateTime.new(2023, 1, 1) }
      updated_at { DateTime.new(2023, 1, 2) }
    end
  end
end

And then to demonstrate what a Team JSON payload looks like we do something like this:

team = FactoryBot.build(:team_example)
payload = team.as_json

Ideally we'd like for the FactoryBot::AttributeDefinitionError to be raised for the :team factory, but not for the :team_example factory. So it would be nice to have a factory-by-factory way to preserve the previous behavior.

(BTW, I'm aware that this is a weird way to use FactoryBot that it's at best an unintended use, and may be drifting into "flagrant misuse" territory.)

[arcreative]

This one bit me too... just trying to figure out why this was released in a minor version when it's a breaking change for people who use sequences that also happen to be primary keys.

In my particular use case, we use varchar primary keys that are imported from another system, but when running specs, we just coerce a serial (integer) to a string with an arbitrary prefix. For now, I've just used factory_bot.reject_primary_key_attributes = false, but would rather not, as we have almost a dozen projects where this would need to be implemented.

[mike-burns]

I haven't forgotten about this. My plan is to revert the hard rejection and add it as a configurable lint. I'd also like it to only happen for serial/autoincrementing columns.

[arcreative]

@mike-burns not sure how others feel about this, but I’m okay with the rejection as long as it doesn’t happen for primary keys that aren’t actually serial. As long as that’s reliable, I think the error is sensible enough.

[aprescott]

I'd also like it to only happen for serial/autoincrementing columns

Just to mention again: some attributes are database-generated but aren't serial/autoincrementing (but rather go through a default function). It seems like it would be great to keep things consistent using something like auto_populated? so the broader class of problem is caught.

(A per-factory opt-out mechanism could also be good!)

[mike-burns]

We should switch to using ActiveRecord::ConnectionAdapters::Column#auto_populated?. This method was added to Rails on June 1.

[1] pry(main)> User.column_for_attribute("email").auto_populated?
=> nil
[2] pry(main)> User.column_for_attribute("id").auto_populated?
=> "gen_random_uuid()"

As a second step, sequence should be able to opt in to overriding these auto-populated columns. Something like,

sequence(:id, intentionally_override_auto_populated_database_column: true) { |n| "#{n}#{n}" }

[mike-burns]

For now I've removed it: https://github.com/thoughtbot/factory_bot_rails/pull/451.