Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Creating associations initialized via 'build' no longer works in FactoryBot 5.0 #1255

Closed
k-rudy opened this issue Feb 1, 2019 · 14 comments
Closed

Comments

@k-rudy
Copy link

k-rudy commented Feb 1, 2019

He guys,

After upgrading to v5.0 of FactoryBot today we started having the following issue.
Our factory is defined like this:

FactoryBot.define do
  factory :task_profile do
    profile
    task { build(:task, account: profile.account) }
  end
end

Where TaskProfile is a model, that belongs_to :profile and belongs_to :task.
This allows us to make sure both profile and task belong to the same account.

So now after we do create(:task_profile), it creates the entry with task_id: nil. Previously it was correctly filled in with id of associated task.

Could you please suggest a fix for the problem? Thank you.

@composerinteralia
Copy link
Collaborator

I wasn't able to reproduce this. Can you update this script to produce the error you described?

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails", "5.2.2"
  gem "sqlite3"
  gem "factory_bot", "5.0.0"
end

require "active_record"
require "minitest/autorun"
require "logger"
require "factory_bot"

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :task_profiles, force: true do |t|
    t.integer :profile_id
    t.integer :task_id
  end

  create_table :tasks, force: true do |t|
    t.integer :account_id
  end

  create_table :profiles, force: true do |t|
    t.integer :account_id
  end

  create_table :accounts, force: true do |t|
  end
end

class Account < ActiveRecord::Base
end

class Profile < ActiveRecord::Base
  belongs_to :account
end

class Task < ActiveRecord::Base
  belongs_to :account
end

class TaskProfile < ActiveRecord::Base
  belongs_to :profile
  belongs_to :task
end

FactoryBot.define do
  factory :account do
  end

  factory :profile do
    account
  end

  factory :task do
    account
  end

  factory :task_profile do
    profile
    task { build(:task, account: profile.account) }
  end
end

class BugTest < Minitest::Test
  def test_factory_bot_stuff
    task_profile = FactoryBot.create(:task_profile)

    assert task_profile.task_id.present?
  end
end

@k-rudy
Copy link
Author

k-rudy commented Feb 1, 2019

@composerinteralia I have entered my versions to your script and couldn't reproduce as well. We're using pg v. 0.21.0 as a database adapter and Postgres 9.6 and also Rspec v.3.8.2. Unfortunately pg setup in this script seems a bit more complicated and didn't work for me straight away. I'll try to play with it over the weekend and make it reproducing the issue. Thank you.

@pboling
Copy link
Contributor

pboling commented Feb 1, 2019

Upgrading to factory_bot_rails v5 broke some factories for me too, for what appears to be the same reason.

@composerinteralia
Copy link
Collaborator

This may also be related to thoughtbot/factory_bot_rails#314

damianlegawiec added a commit to spree/spree that referenced this issue Feb 3, 2019
5.0 includes breaking changes that older versions of Spree won't be able to fix easily.

thoughtbot/factory_bot#1255
damianlegawiec added a commit to spark-solutions/spree that referenced this issue Feb 4, 2019
5.0 includes breaking changes that older versions of Spree won't be able to fix easily.

thoughtbot/factory_bot#1255
@k-rudy
Copy link
Author

k-rudy commented Feb 5, 2019

@composerinteralia, here is the test for the failing scenario. Feel free to uncomment the earlier version of FactoryBot gem to make it passing. Hope it helps

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails", "5.0.7.1"
  gem "sqlite3", '1.3.13'
  gem "factory_bot", "5.0.0"
  # Uncomment to make the test pass
  # gem "factory_bot", "< 5"
end

require "active_record"
require "minitest/autorun"
require "logger"
require "factory_bot"

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :task_profiles, force: true do |t|
    t.integer :profile_id
    t.integer :task_id
  end

  create_table :tasks, force: true do |t|
    t.integer :account_id
    t.integer :profile_id
  end

  create_table :profiles, force: true do |t|
    t.integer :account_id
  end

  create_table :accounts, force: true do |t|
  end
end

class Account < ActiveRecord::Base
end

class Profile < ActiveRecord::Base
  belongs_to :account
end

class Task < ActiveRecord::Base
  belongs_to :account
  belongs_to :profile

  validates :account_id, presence: true
end

class TaskProfile < ActiveRecord::Base
  belongs_to :profile
  belongs_to :task
end

FactoryBot.define do
  factory :account do
  end

  factory :profile do
    account
  end

  factory :task do
    profile
    account { profile.account }
  end

  factory :task_profile do
    profile
    task { build(:task) }
  end
end

class BugTest < Minitest::Test
  def test_factory_bot_stuff
    task_profile = FactoryBot.create(:task_profile)
    assert task_profile.task_id.present?
  end
end

@composerinteralia
Copy link
Collaborator

Thanks so much! I'll try to take a proper look at this before the end of the week, but based on a quick glance seeing validates :account_id, presence: true, I am thinking this is similar to thoughtbot/factory_bot_rails#314.

A temporary workaround would be to set FactoryBot.use_parent_strategy = false (we changed the default value for that configuration option). But I will see if I can come up with another solution that works with the new default for use_parent_strategy.

@pboling
Copy link
Contributor

pboling commented Feb 7, 2019

Just for additional context on this issue: The upgrade is breaking a quarter of spec suites according to meta analysis by depend-a-bot
https://dependabot.com/compatibility-score/?dependency-name=factory_bot_rails&package-manager=bundler&previous-version=4.11.1&new-version=5.0.0

@jrgns
Copy link

jrgns commented Feb 8, 2019

I'm also seeing something that I think is related to this.

Instead of doing the following:

factory :service
  product # Service has a many_to_one relationship with Product
end

factory :product
  name { 'Product' }
end

I have to do

factory :service
  product { create(:product) } # Service has a many_to_one relationship with Product
end

factory :product
  name { 'Product' }
end

Edit: Setting FactoryBot.use_parent_strategy = false resolves it, though.

@composerinteralia
Copy link
Collaborator

@pboling thank you for that information. That makes sense, since changing the default value for FactoryBot.use_parent_strategy was an intentional breaking change.

@jrgns I'm glad you were able to set FactoryBot.use_parent_strategy = false to go back to the old behavior and get your test suite passing again. If at some point you want to try out the new behavior, instead of updating the product association in the service factory, it might be worth looking at the specific tests that are failing (if there aren't too many of them). In places that you are using FactoryBot.build(:service) you may need to switch to FactoryBot.create(:service) instead.

@k-rudy your issue might be a little bit different. I still haven't had a chance to look into it too closely, but I will try to take a look soon!

@composerinteralia
Copy link
Collaborator

composerinteralia commented Feb 10, 2019

@k-rudy I was able to get your test passing again by changing the task_profile factory from:

  factory :task_profile do
    profile
    task { build(:task) }
  end

to:

  factory :task_profile do
    profile
    task
  end

By letting task be an association instead of a regular attribute, factory_bot will know to create the associated task when you call FactoryBot.create(:task_profile).

What you have before used to work because although you were using the build strategy for the task, its associated profile and account were still constructed using the create strategy. factory_bot 5 changed the default for use_parent_strategy to true, thus causing the build strategy to build associations, rather than create them. building the associates doesn't quite work here, since you are validating the presence of an id.


Another option is to change back to the old behavior for now by setting FactoryBot.use_parent_strategy = false.

@k-rudy
Copy link
Author

k-rudy commented Feb 11, 2019

@composerinteralia thanks for the info, in the test I slightly simplified the factory, as it originally was:

factory :task_profile do
  profile
  task { build(:task, account: profile.account) }
end

to make sure when we don't pass the task explicitly, it is created for the same account as profile. From what you have said - seems like it will no longer be possible in 5.0, unless we change the use_parent_strategy option?

@composerinteralia
Copy link
Collaborator

@k-rudy got it. So there is something you can do, but it is not currently documented anywhere.

task { association(:task, account: profile.account) } 

I have been thinking about documenting this way of creating associations inside the block, and this is a good case for it. It came up in #1063 as well.

Using association will build the association when you are using FactoryBot.build and create the association when you are using FactoryBot.create. If you want to see the details, the method lives on the FactoryBot::Evaluator class.

@k-rudy
Copy link
Author

k-rudy commented Feb 19, 2019

@composerinteralia the proposed solution to replace build with association works like a charm for us. This is how it looks at the end:

FactoryBot.define do
  factory :task_profile do
    profile
    task { association(:task, account: profile.account) }
  end
end

@k-rudy k-rudy closed this as completed Feb 19, 2019
amatriain added a commit to amatriain/feedbunch that referenced this issue Mar 14, 2019
In FactoryBot 5 there's been a change in how associations are built (new
behavior is to use the parent build strategy, old behavior... not sure
what is the strategy then).

This new behavior broke some tests. For now old behavior is restored.

The new behavior is very briefly explained in:

    https://github.com/thoughtbot/factory_bot/blob/master/GETTING_STARTED.md#associations

The new (backwards-incompatible) behavior is also discussed in these
issues:

    thoughtbot/factory_bot_rails#314
    thoughtbot/factory_bot#1255

FactoryBot dev has mentioned maybe writing a blog post explaining the
rationale behind the change and how to adapt tests. In the future the
old behavior will likely get deprecated. Whenever I decide to use the
new behavior in FeedBunch I'll have to check if there's a post in the
FactoryBot blog about this.
edwardkerry added a commit to alphagov/whitehall that referenced this issue Sep 30, 2019
`FactoryBot.use_parent_stategy` now defaults to `true`, rather than false.

thoughtbot/factory_bot_rails#314
thoughtbot/factory_bot#1255

These  tests were failing as the new configuration prevented the test objects from being persisted for validation checks.
edwardkerry added a commit to alphagov/whitehall that referenced this issue Sep 30, 2019
`FactoryBot.use_parent_stategy` now defaults to `true`, rather than false.

thoughtbot/factory_bot_rails#314
thoughtbot/factory_bot#1255

These  tests were failing as the new configuration prevented the test objects from being persisted for validation checks.
edwardkerry added a commit to alphagov/whitehall that referenced this issue Sep 30, 2019
`FactoryBot.use_parent_stategy` now defaults to `true`, rather than false.

thoughtbot/factory_bot_rails#314
thoughtbot/factory_bot#1255

These  tests were failing as the new configuration prevented the test objects from being persisted for validation checks.
edwardkerry added a commit to alphagov/whitehall that referenced this issue Sep 30, 2019
`FactoryBot.use_parent_stategy` now defaults to `true`, rather than false.

thoughtbot/factory_bot_rails#314
thoughtbot/factory_bot#1255

These  tests were failing as the new configuration prevented the test objects from being persisted for validation checks.
edwardkerry added a commit to alphagov/whitehall that referenced this issue Sep 30, 2019
`FactoryBot.use_parent_stategy` now defaults to `true`, rather than false.

thoughtbot/factory_bot_rails#314
thoughtbot/factory_bot#1255

These  tests were failing as the new configuration prevented the test objects from being persisted for validation checks.
edwardkerry added a commit to alphagov/whitehall that referenced this issue Sep 30, 2019
`FactoryBot.use_parent_stategy` now defaults to `true`, rather than false.

thoughtbot/factory_bot_rails#314
thoughtbot/factory_bot#1255

These  tests were failing as the new configuration prevented the test objects from being persisted for validation checks.
composerinteralia added a commit that referenced this issue Jul 9, 2020
This commit makes `Evaluator#association` and `Evaluator#instance`
officially part of the public API of factory\_bot. Folks were already
using these methods, and all of the examples here were inspired by
examples in the various issues mentioned below.

This commit also adds and updates some tests to match the examples in
the documentation, to ensure that these examples will continue to work
as expected.

Closes #1268 by adding documentation for both `Evaluator#association`
and `Evaluator#instance`. The documentation on interconnected
associations is also relevant to #1063, #1255, and #1309.

Closes #1304 by providing multiple alternative approaches to creating
collection associations, and adding a note about reloading the record in
the callback example. This had also come up back in #549.

Closes #458 by offering `Evaluator#association` as a more flexible way
to build collection associations. This has come up many times over the
years in many different forms, including #426, #487, #640, #1022, #1150,
and #1360.
composerinteralia added a commit that referenced this issue Jul 9, 2020
* Improve documentation for collection associations

This commit makes `Evaluator#association` and `Evaluator#instance`
officially part of the public API of factory\_bot. Folks were already
using these methods, and all of the examples here were inspired by
examples in the various issues mentioned below.

This commit also adds and updates some tests to match the examples in
the documentation, to ensure that these examples will continue to work
as expected.

Closes #1268 by adding documentation for both `Evaluator#association`
and `Evaluator#instance`. The documentation on interconnected
associations is also relevant to #1063, #1255, and #1309.

Closes #1304 by providing multiple alternative approaches to creating
collection associations, and adding a note about reloading the record in
the callback example. This had also come up back in #549.

Closes #458 by offering `Evaluator#association` as a more flexible way
to build collection associations. This has come up many times over the
years in many different forms, including #426, #487, #640, #1022, #1150,
and #1360.
@composerinteralia
Copy link
Collaborator

Evaluator#association is now officially documented: #1268 (comment)

genya0407 added a commit to genya0407/sharecar-rails that referenced this issue Oct 17, 2020
genya0407 added a commit to genya0407/sharecar-rails that referenced this issue Mar 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants