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

Fix: Respect Rails defaults in an initializer by delaying loading ActiveRecord adapter until framework is loaded #832

Merged

Conversation

pbstriker38
Copy link
Contributor

@pbstriker38 pbstriker38 commented Jan 31, 2024

The Flipper models are loading too soon and causing weirdness with saving models that have a belongs_to. The belongs_to association is being loaded on save even when there have not been any changes to the association.

BEFORE

irb(main):001> token = OrganizationAccessToken.find(1)
...
irb(main):002> token.update(description: 'a token')
DEBUG:   TRANSACTION (0.7ms)  BEGIN
DEBUG:   Organization Load (1.0ms)  SELECT "organizations".* FROM "organizations" WHERE "organizations"."status" != $1 AND "organizations"."id" = $2 LIMIT $3  [["status", "deactivated"], ["id", 3], ["LIMIT", 1]]
DEBUG:   OrganizationAccessToken Update (1.0ms)  UPDATE "organization_access_tokens" SET "description" = $1, "updated_at" = $2 WHERE "organization_access_tokens"."id" = $3  [["description", "a token"], ["updated_at", "2024-01-31 22:19:38.068599"], ["id", 1]]
DEBUG:   TRANSACTION (1.4ms)  COMMIT
=> true

AFTER

irb(main):001> token = OrganizationAccessToken.find(1)
...
irb(main):002> token.update(description: 'a token')
DEBUG:   TRANSACTION (0.7ms)  BEGIN
DEBUG:   OrganizationAccessToken Update (1.8ms)  UPDATE "organization_access_tokens" SET "description" = $1, "updated_at" = $2 WHERE "organization_access_tokens"."id" = $3  [["description", "a token"], ["updated_at", "2024-01-31 22:21:21.908915"], ["id", 1]]
DEBUG:   TRANSACTION (0.9ms)  COMMIT
=> true

@pbstriker38
Copy link
Contributor Author

I moved the 3 commits that fix the tests to this other PR #833

@pbstriker38 pbstriker38 force-pushed the wait_for_active_record_to_be_loaded branch 2 times, most recently from 35c29cc to 9ab2d7e Compare February 7, 2024 17:39
@bkeepers
Copy link
Collaborator

bkeepers commented Mar 15, 2024

@pbstriker38 thanks for the PR. I'd love to figure out a test for this. We used to wait to load the AR adaptar with ActiveSupport.on_load(:active_record), but it caused some other weirdness that I can't remember now. Any ideas for a failing test that demonstrates the issue you're running into?

@pbstriker38
Copy link
Contributor Author

I could see if I can get it to reproduce with one of the Rails executable test case examples

https://edgeguides.rubyonrails.org/contributing_to_ruby_on_rails.html#create-an-executable-test-case

@pbstriker38
Copy link
Contributor Author

We've been running this fork for 2 months now because without it many of our tests that use FactoryBot.build fail since it attempts to load the association from the DB.

@pbstriker38
Copy link
Contributor Author

@bkeepers I found where it was initially added and removed.

#192

#652

My implementation is a bit different though as it does not wrap the entire adapter but only the definition of the models.

@pbstriker38 pbstriker38 force-pushed the wait_for_active_record_to_be_loaded branch from 9ab2d7e to 93be628 Compare March 27, 2024 01:14
@@ -215,18 +215,27 @@
end
end

it "works when table doesn't exist" do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that this test wasn't actually testing the table not existing, but the fact that it's not even connected to the DB. Without the silence, you can see the message:

No connection pool for 'ActiveRecord::Base' found.. You likely need to run `rails g flipper:active_record` and/or `rails db:migrate`.

@pbstriker38
Copy link
Contributor Author

Ok. So I created a new rails app to create a minimal reproduction. I could not get it to reproduce. The one difference from this new app and ours is the configuration defaults. A new app has config.load_defaults 7.1, while our app has config.load_defaults 7.0 because we have not completed enabling all of the configs in new_framework_defaults_7_1.rb.

image

Flipper is causing ActiveRecord to load too soon and it ignores our new_framework_defaults_7_1.rb configuration for belongs_to_required_validates_foreign_key.

Rails.application.config.active_record.belongs_to_required_validates_foreign_key = false

Without my fix

irb(main):001> ActiveRecord.belongs_to_required_validates_foreign_key
=> true

with my fix

irb(main):001> ActiveRecord.belongs_to_required_validates_foreign_key
=> false

rails/rails@e5d1514

@pbstriker38
Copy link
Contributor Author

Using require: false in the Gemfile also appears to fix the issue

gem 'flipper-active_record', require: false

class Model < ::ActiveRecord::Base
self.abstract_class = true
end
ActiveSupport.on_load(:active_record) do
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be simpler to just move all the things that depends on Active Record into another file and then require it when AR is loaded, or something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's already a generator for adding the DB migrations, so another option is to just have the generator add these models to the app/models directory.

@pbstriker38 pbstriker38 force-pushed the wait_for_active_record_to_be_loaded branch from 93be628 to bad0fc1 Compare April 23, 2024 22:52
@pbstriker38 pbstriker38 force-pushed the wait_for_active_record_to_be_loaded branch from bad0fc1 to 9e62a45 Compare April 24, 2024 18:04
@jnunemaker
Copy link
Collaborator

@bkeepers you have more experience with these bits. Any thoughts? Good to merge?

@bkeepers
Copy link
Collaborator

@bkeepers you have more experience with these bits. Any thoughts? Good to merge?

Yes, I'm fine moving forward with this as-is, but do want to highlight a concern:

I'm a little nervious that there could be unintended consequences with this specific solution. For example, what happens if this adapter gets loaded in the app boot process before AR is loaded? Previously, it would work fine. Now, it will raise a NameError with uninitialized constant Flipper::Adapters::ActiveRecord::Model and the app will fail to boot.

It's probably a non-issue since AR gets loaded so early in the app boot process and before any user code runs. I'm just trying to think through scenarios.

If it causes any issues we could explore a couple other options:

  1. Moving the models out to separate files and using autoload to load them when they are referenced, along with explicitly loading them when ActiveRecord is loaded.
  2. A generator as suggested in Fix: Respect Rails defaults in an initializer by delaying loading ActiveRecord adapter until framework is loaded #832 (comment) (although I would prefer this as a last resort)

@bkeepers bkeepers changed the title Wait for active record to be loaded Fix: Respect Rails defaults in an initializer by delaying loading ActiveRecord adapter until framework is loaded Apr 30, 2024
@bkeepers bkeepers merged commit 5aa9cfd into flippercloud:main Apr 30, 2024
36 checks passed
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.

4 participants