Ability to disable reject_primary_key_attributes per factory

[rmehner]

Newer versions of factory_bot/factory_bot_rails have this neat footgun preventing feature of rejecting sequences for primary key columns.

However, we have one use case where the primary key column is an externally provided string that is not an UUID, but just a string. With the newest update, our factories which look like this:

FactoryBot.define do
  factory :cms_item do
    name { "CMS item" }
    sequence(:id) { |n| n.to_s }
  end
end

will lead to tests failing with:

Attribute generates "id" primary key for CmsItem"
     
Do not define "id". Instead, rely on the database to generate it.

We can disable this with by setting config.factory_bot.reject_primary_key_attributes to false, however this will disable it for all factories. Since I'm not a fan of footguns, I'd love to have more fine grained control over this.

Desired solution

I'd love to disable this per factory, kinda like this:

FactoryBot.define do
  factory :cms_item do
    name { "CMS item" }
	# this is an external id and we hope they know what they're doing
    do_not_reject_primary_key_attributes!
    sequence(:id) { |n| n.to_s }
  end
end

Naming obviously happy to be changed.

Alternatives considered

I'm not hard stuck on the exact syntax being used, anything that allows me to have more fine grained control over this very nice feature is welcome. I don't want to get rid of it, since I have seen this gun hurting foots before 🚑

Additional context

Thank you for your work on this!

[ybiquitous]

Hi. I think this validation should skip a case where the primary key isn't auto-generated by default, but what do you think?

factory_bot_rails/lib/factory_bot_rails/factory_validator/active_record_validator.rb

Line 7 in 54c7254

if for_class < ActiveRecord::Base && for_class.primary_key == attribute.name.to_s

For example, ActiveRecord::Base.sequence_name may be available for achieving it:

-if for_class < ActiveRecord::Base && for_class.primary_key == attribute.name.to_s
+if for_class < ActiveRecord::Base && for_class.primary_key == attribute.name.to_s && for_class.sequence_name

Although I'm unsure about an auto-increment primary key like MySQL.

[tagliala]

Hello,

I'm here for the same issue. Assuming that all IDs are auto-generated is a strong assumption.

Even if having an auto-generated ID is always the best practice, this may not be the case when the system has a country table with ISO3 codes as ids

Maybe checking whether if Model.columns_hash[Model.primary_key].default_function is present would help (don't know if this works on all AR adapters)

[mikdiet]

This issue is also update blocker for us.

In some (tiny) subset of our tests we need to set explicit ID for models, because they are in sync with 3rd party services.

[ybiquitous]

FYI. If using Rails 7.1.0 or later, we may be able to use ActiveRecord::ConnectionAdapters::Column#auto_incremented_by_db?, although it's a private API.
https://github.com/rails/rails/blob/v7.1.0/activerecord/lib/active_record/connection_adapters/column.rb#L67

Considering such implementation difficulties, I agree with reject_primary_key_attributes at a factory level (instead of a global level), as the second option.

[bkDJ]

Also an update blocker for me, as I have a factory that generates IDs to resemble those from an external vendor. While safety-first is appreciated, I wonder if this kind of protection wouldn't have been better suited for a rubocop-rspec rule instead?

+1 for a factory-level disabling of the protection.

[tagliala]

I wonder if this kind of protection wouldn't have been better suited for a rubocop-rspec rule instead?

The cop is already here in the rubocop-factory_bot family:

https://docs.rubocop.org/rubocop-factory_bot/cops_factorybot.html#factorybotidsequence

but of course it will only check for ID, while this validation looks for the primary key

[modosc]

we have another usage case that's breaking with the upgrade. we have some pre-seeded data that we'd like specific factories to return rather than recreate, eg:

FactoryBot.define do
  factory :my_model_name do
    to_create do |instance|
      if instance.id.present?
        # only do this if there's an id present (because that's a seeded value)
        instance.attributes = MyModelName.find(instance.id).attributes
        instance.instance_variable_set :@new_record, false
      else
        # otherwise it's a random factory with fake data and we just save it
        instance.save!
      end
    end

    factory "my_model_name_foo" do 
      id { MyModelName::FOO_ID }
    end
  end
end

ideally we could disable reject_primary_key_attributes only for these factories but keep it enabled for others.

[olliebennett]

Our use case is that we're storing data on specific postcode/zipcode - and joining properties to that table via their postcode/zipcode value directly (instead of bothering with an auto-incrementing ID and a superfluous "postcode_data_id" column alongside (and in sync with) the "postcode" column on every property. We just have the "postcode data" table's primary key as "postcode" 👌

I'm also reluctant to disable the check entirely (with config.factory_bot.reject_primary_key_attributes) since it feels like a useful check in most cases.

[zorab47]

To keep the conversation going I put together a proof-of-concept in #444.

[aliaksandrb]

We are affected by this as well, was fun to learn it exists :)

+1 for a factory/attribute level disabling of the protection.

[tagliala]

I think this has been partially addressed (by disabling the feature) via 7be631f in 6.4.3