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

Wrap ActiveRecord configuration into initializer #432

Closed

Conversation

leoarnold
Copy link
Contributor

Rails.application is not available when the railties are collected, so we move the ActiveRecord configuration into the initializer phase.

NoMethodError: undefined method `config' for nil:NilClass (NoMethodError)
      application.config
                 ^^^^^^^
/project/app/vendor/bundle/ruby/3.2.0/gems/railties-7.0.8/lib/rails.rb:47:in `configuration'
/project/app/vendor/bundle/ruby/3.2.0/gems/factory_bot_rails-6.4.0/lib/factory_bot_rails/railtie.rb:25:in `block in <class:Railtie>'

`Rails.application` is not available when the railties are collected,
so we move the ActiveRecord configuration into the initializer phase.

```
NoMethodError: undefined method `config' for nil:NilClass (NoMethodError)
      application.config
                 ^^^^^^^
/project/app/vendor/bundle/ruby/3.2.0/gems/railties-7.0.8/lib/rails.rb:47:in `configuration'
/project/app/vendor/bundle/ruby/3.2.0/gems/factory_bot_rails-6.4.0/lib/factory_bot_rails/railtie.rb:25:in `block in <class:Railtie>'
```
@f-asa
Copy link

f-asa commented Nov 17, 2023

APPROUVED

@tagliala
Copy link

This should fix #426

Arie added a commit to Arie/serveme that referenced this pull request Nov 18, 2023
Arie added a commit to Arie/serveme that referenced this pull request Nov 18, 2023
Lock factory_bot_rails to 6.2.x until thoughtbot/factory_bot_rails#432 gets merged and released as a gem
Copy link

@williamromero williamromero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This solution using lazy load hook "initializer" is solving the issue.

lib/factory_bot_rails/railtie.rb Show resolved Hide resolved
Copy link

@simi simi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm this fixes #426 issue.

@torrocus
Copy link

torrocus commented Nov 20, 2023

I can also confirm that this fixes #426 & #433 issues.

@TylerRick
Copy link

I just ran into this as well after a bundle update, and ended up pushing a similar PR, #435, before I saw this one.

This looks good! Merge please?

@LeFnord
Copy link

LeFnord commented Nov 21, 2023

thanks, works perfectly well … any plans to merge and release it?

@jrochkind
Copy link

This bug is making CI builds across the planet fail, a quick release would be great.

I'm curious how the original bug passed factory_bot_rails's own CI build, and wonder if there's a test that could have caught the bug, that should be added.

@JDutil
Copy link
Contributor

JDutil commented Nov 21, 2023

This resolves the issue for me as well.

jagthedrummer added a commit to bullet-train-co/bullet_train that referenced this pull request Nov 21, 2023
There's a bug in the latest version of `factory_bot_rails`:
thoughtbot/factory_bot_rails#433

There's also a fix that hasn't been merged or released yet:
thoughtbot/factory_bot_rails#432

I'm setting the `Gemfile` to specifically avoid those buggy versions so
that we can get `depfu` un-blocked with keeping gems up to date.
jagthedrummer added a commit to bullet-train-co/bullet_train that referenced this pull request Nov 21, 2023
* Avoid buggy versions of factory_bot_rails

There's a bug in the latest version of `factory_bot_rails`:
thoughtbot/factory_bot_rails#433

There's also a fix that hasn't been merged or released yet:
thoughtbot/factory_bot_rails#432

I'm setting the `Gemfile` to specifically avoid those buggy versions so
that we can get `depfu` un-blocked with keeping gems up to date.

* dang linter
Copy link

@joseluistorres joseluistorres left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@mobilutz
Copy link

I would like to see this shipped.

And as @jrochkind said

I'm curious how the original bug passed factory_bot_rails's own CI build, and wonder if there's a test that could have caught the bug, that should be added.

Thanks

@mike-burns
Copy link
Contributor

Hi, thanks for tracking this down. I cannot work on this bug this week, but I do I have time to merge PRs and cut releases.

If someone could add a test to this, that'd be the last step we need. Thank you!

benoittgt added a commit to benoittgt/database_cleaner-active_record that referenced this pull request Nov 23, 2023
This is well known issue. We should prefer to code that reference active
record only when it's already lazy loaded.
It can break other gems initializing process

Related:
- rails/rails#46567
- paper-trail-gem/paper_trail@fc6c5f6
(inspiration)
- thoughtbot/factory_bot_rails#432

Fix: DatabaseCleaner#89
benoittgt added a commit to benoittgt/database_cleaner-active_record that referenced this pull request Nov 23, 2023
This is well known issue. We should prefer to code that reference active
record only when it's already lazy loaded.
It can break other gems initializing process

Related:
- rails/rails#46567
- paper-trail-gem/paper_trail@fc6c5f6
(inspiration)
- thoughtbot/factory_bot_rails#432

Fix: DatabaseCleaner#89
@benoittgt
Copy link
Contributor

I cherry picked this commit and added a non-regression test. I also make sure we are good on Rails 7.1 too.

#440

Copy link

@makicamel makicamel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@leoarnold
Thank you for your PR!
This diff (test codes) might help to merge this PR (ref #432 (comment)):

--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -2,6 +2,10 @@

 ENV["RAILS_ENV"] = "test"

+require "active_record"
+
+class ActiveRecord::Base; end
+
 require "factory_bot_rails"
 require "fake_app"

There are gem monkey-patched to ActiveRecord. With these, ActiveRecord might be loaded before Rails.application is initialized. This is the cause of #426 .
This code snippet reproduces ActiveRecord being loaded before Rails.application is initialized.

lib/factory_bot_rails/railtie.rb Show resolved Hide resolved
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

Successfully merging this pull request may close these issues.