Open rmehner opened 11 months ago
Hi. I think this validation should skip a case where the primary key isn't auto-generated by default, but what do you think?
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.
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 id
s
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)
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.
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.
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.
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
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.
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.
To keep the conversation going I put together a proof-of-concept in https://github.com/thoughtbot/factory_bot_rails/pull/444.
We are affected by this as well, was fun to learn it exists :)
+1 for a factory/attribute level disabling of the protection.
I think this has been partially addressed (by disabling the feature) via 7be631f90ab26a0fec8ac5462afded2599ce6bd9 in 6.4.3
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:
will lead to tests failing with:
We can disable this with by setting
config.factory_bot.reject_primary_key_attributes
tofalse
, 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:
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!