thoughtbot / factory_bot

A library for setting up Ruby objects as test data.
https://thoughtbot.com
MIT License
7.91k stars 2.6k forks source link

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

Open composerinteralia opened 6 years ago

composerinteralia commented 6 years ago

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.

shyam-habarakada commented 6 years ago

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

composerinteralia commented 6 years ago

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!.

agirdler-lumoslabs commented 5 years ago

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?

composerinteralia commented 5 years ago

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.

jakswa commented 5 years ago

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:

jsmartt commented 5 years ago

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.

viral810 commented 4 years ago

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.

jvenezia commented 4 years ago

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
fdutey commented 3 years ago

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

composerinteralia commented 3 years ago

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

natematykiewicz commented 2 years ago

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}
stephannv commented 1 year ago

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:

natematykiewicz commented 1 year ago

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.

gap777 commented 4 weeks ago

Is there really no workaround to this?

natematykiewicz commented 4 weeks ago

@gap777 I was able to work around this issue:

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}

By adding this in the factory:

after :build do |product, options|
  product.platform_id = options.platform_id
  product.platform = options.platform
end
gap777 commented 4 weeks ago

Grrrr... trying that.. not working for me... Did you declare transients as well?

natematykiewicz commented 4 weeks ago

No, just regular FactoryBot attributes:

platform { 'stripe' }
platform_id { "price_#{SecureRandom.base58(24)}" }
gap777 commented 4 weeks ago

@composerinteralia's suggestion worked for me (https://github.com/thoughtbot/factory_bot/issues/1142#issuecomment-450763790):

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

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