Wait for active record to be loaded

[pbstriker38]

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]

I moved the 3 commits that fix the tests to this other PR https://github.com/flippercloud/flipper/pull/833

[bkeepers]

@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]

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]

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]

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

https://github.com/flippercloud/flipper/pull/192

https://github.com/flippercloud/flipper/pull/652

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

[pbstriker38]

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.

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

https://github.com/rails/rails/commit/e5d15140d2dfd25f33e306ab8522f7accd410dc3

[pbstriker38]

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

gem 'flipper-active_record', require: false

[jnunemaker]

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

[bkeepers]

@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 https://github.com/flippercloud/flipper/pull/832#discussion_r1542407000 (although I would prefer this as a last resort)