factory_bot icon indicating copy to clipboard operation
factory_bot copied to clipboard

Confusion around aliases for `{attribute}` and `{attribute}_id`

Open composerinteralia opened this issue 7 years ago • 18 comments

We have had several issues over the years related to FactoryBot.aliases: #522 #734 #851 #989 #1096 #1138, #1417. In most of these issues people do not realize that factory_bot assumes{attribute}_id will be the foreign key of an {attribute} ActiveRecord association.

At the very least I think we need better documentation around this. We could probably also offer better configuration for this, and maybe move anything specific to ActiveRecord into factory_bot_rails.

composerinteralia avatar Jun 12 '18 18:06 composerinteralia

Just wasted a few hours because of this. The workaround I applied (based on other issues linked and other research) was as below.

factory :person
  name
  email
  ...

  transient do
    thing_id { 0 }
    thing { nil } # Thing.to_s or similar
  end

  after :create do |person, options|
    person.thing_id = options.thing_id
    person.thing = options.thing
    person.save!
  end
end

Feedback on this would be great. Thanks

shyam-habarakada avatar Aug 12 '18 01:08 shyam-habarakada

Yup, that seems to match the solution in #1096. I think you could use after(:build) instead of an after(:create), then get rid of the call to save!.

composerinteralia avatar Aug 12 '18 11:08 composerinteralia

Is there a planned "official" solution to this? While hacking transient and after callbacks works in most cases I haven't been able to get it to work properly with a hanami/model backed model (the object is frozen during initialization) and instead resorted to hacking an non_aliasable method on FactoryBot then using that in a monkey patched definition of FactoryBot.aliases_for:

module FactoryBot
  def self.non_aliasable
    %w[processor processor_id]
  end

  def self.aliases_for(attribute)
    aliases.map do |(pattern, replace)|
      if pattern.match(attribute.to_s) && !non_aliasable.include?(attribute.to_s)
        attribute.to_s.sub(pattern, replace).to_sym
      end
    end.compact << attribute
  end
end

This has worked great so far for the project I'm working on, but it doesn't really feel maintainable (as it's a list stored in a module). I'm also not familiar enough with the code to add a method to the DSL to allow defining these on a per model basis but I think that would be the most flexible solution (but may require a rewrite so FactoryBot.aliases_for is builder aware?). Are there any opinions on this approach?

agirdler-lumoslabs avatar Dec 26 '18 23:12 agirdler-lumoslabs

I haven't spent too much time thinking about an official solution for this. I could imagine something like:

factory :person do
  skip_aliases

  # ...
end

to opt-out of aliases for just that one factory, but I haven't fully thought that through.

In the meantime, you could try using initialize_with instead of the after callback:

factory :person do
  name { "Daniel" }

  transient do
    thing_id { 0 }
    thing { "thing" }
  end

  initialize_with do
    new(attributes.merge(thing_id: thing_id, thing: thing))
  end
end

And maybe something like

factory :person do
  skip_aliases :thing, :thing_id

  # ...
end

to just skip specific aliases for a factory.

composerinteralia avatar Jan 01 '19 23:01 composerinteralia

What a nightmare this was to troubleshoot. Count me and a few other coworkers in on wishing factory bot wasn't opinionated on all _id columns. :tada:

jakswa avatar Apr 08 '19 21:04 jakswa

The workarounds posted above help with building/creating, but the attributes are excluded from the attributes_for response. 😞

I'd really like a way to define these as attributes (which they are), and have factory_bot treat them as plain-text column names if they are not associations. The skip_aliases suggestion by @composerinteralia would work, but why does defining an attribute on a factory not automatically accomplish this? There is a separate method for defining associations, so I'd expect that defining attributes to have different behavior.

jsmartt avatar Jun 14 '19 14:06 jsmartt

If you have a column name that ends with _id i.e. thing_id and thing is not model is itself then make sure that model related to that table is explicitly specifying table name in the model file. In my case, I had the following issue.

FactoryBot.define do
  factory :domain_account, class: "Domain::Account" do
    user
    thing_id { SecureRandom.hex(13) }
    name { "John Doe" }
    sequence(:email_address) { |n| "test+#{n}@example.com" }
  end
end

causing me an exception that thing association doesn't exist but by adding self.table_name to the model resolved the issue.

# app/models/domain/account.rb
class Domain::Account < ApplicationRecord
  belongs_to :user

  self.table_name = "domain_accounts"
end

Not too sure if other people are having a problem because of namespaced models. Interested to see if this resolves the problem for anyone else.

viral810 avatar Nov 19 '19 13:11 viral810

I spent way to much time understanding what was happening... I dont have a suggestion but we definitely need an improvement here!

In my case, thing was a boolean and thing_id was a string not related at all to a rails model.

Adding this to the factory was enough for me:

    after :build do |user, options|
      user.thing_id = options.thing_id
      user.thing = options.thing
    end

jvenezia avatar Apr 28 '20 10:04 jvenezia

It was more important to rename from girl to bot to make some snowflakes happy instead of solving real bugs

fdutey avatar Dec 09 '20 08:12 fdutey

We have a Code of conduct that we expect all contributors to follow.

composerinteralia avatar Dec 09 '20 12:12 composerinteralia

Just bumping this to say that I just ran into this as well. I have a model that has a platform column, and a platform_id column. For example:

{
  platform: 'stripe',
  platform_id: 'sub_123456789',
}

{
  platform: 'apple',
  platform_id: 'SOME_APPLE_ITEM_ID',
}

I just spent a good 2 hours trying to figure out why this was happening:

FactoryBot.build(:product, platform: 'apple', platform_id: 'abc123').attributes.slice('platform', 'platform_id')
=> {"platform"=>"apple", "platform_id"=>nil}

natematykiewicz avatar Jul 08 '22 19:07 natematykiewicz

Two years ago I opened this issue https://github.com/thoughtbot/factory_bot/issues/1512, and now I lost some time investigating why my tests aren't passing 😅.

I think a solution could be:

  • remove _id alias from FactoryBot
  • If this feature is still wanted, move it to FactoryBot Rails
    • handle only foreign key attributes, not all attributes with _id suffix
    • this can be made with:
      ActiveRecord::Base.connection.foreign_keys(TABLE_NAME_HERE).any? { |fk| fk.options[:name] == ATTR_HERE }
      

stephannv avatar Jun 28 '23 18:06 stephannv

In talking with Shopify on the Rails Discord about something different, I learned that they don't use actual foreign keys in the database because they do online schema migrations with 0 downtime.

So, it's probably best to simply check the model and see if the field is a foreign key in an association. The whole point of this code is to treat the _id field and association the same, so just checking associations rather than columns makes more sense anyways.

model = Foo
field = 'bar_id'

foreign_key = model.reflect_on_association(field.delete_suffix('_id'))&.foreign_key

needs_alias = foreign_key.is_a?(Array) ? foreign_key.include?(field) : foreign_key == field

If needs_alias is true, do what it does today. If it's false, just treat it as a regular field instead of assuming it's a foreign key to an association.

Note: The array check is for Rails 7.1 composite primary keys.

natematykiewicz avatar Jul 18 '23 14:07 natematykiewicz