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

Maintain Rails 4.2 compatibility #863

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

astupka
Copy link

@astupka astupka commented Apr 25, 2024

This PR patches the active record adapter to work with 4.2. The two issues were originally noticed by @clinejj

#857
#858

Getting errors with some tests needing sqllite 1.4.4 though:

Update:
Need to fix tests in rails 5.2, 6, and 6.1. Appears to be a setup issue since Rails::VERSION::MAJOR is definitely still defined in Rails 6.

Loading staging environment (Rails 6.0.0)
irb(main):001:0> Rails::VERSION::MAJOR
=> 6
SQLITE3_VERSION='2.0.1' bundle exec rake

Failures:

  1) Flipper::Model::ActiveRecord flipper_id returns class name and id
     Failure/Error: ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:')

     LoadError:
       Error loading the 'sqlite3' Active Record adapter. Missing a gem it depends on? can't activate sqlite3 (~> 1.4), already activated sqlite3-2.0.1-x86_64-darwin. Make sure all dependencies are added to Gemfile.
     # ./spec/flipper/model/active_record_spec.rb:9:in `block (2 levels) in <top (required)>'
     # ------------------
     # --- Caused by: ---
     # Gem::LoadError:
     #   can't activate sqlite3 (~> 1.4), already activated sqlite3-2.0.1-x86_64-darwin. Make sure all dependencies are added to Gemfile.
     #   ./spec/flipper/model/active_record_spec.rb:9:in `block (2 levels) in <top (required)>'

  2) Flipper::Model::ActiveRecord flipper_id uses base class name
     Failure/Error: ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:')

     LoadError:
       Error loading the 'sqlite3' Active Record adapter. Missing a gem it depends on? can't activate sqlite3 (~> 1.4), already activated sqlite3-2.0.1-x86_64-darwin. Make sure all dependencies are added to Gemfile.
     # ./spec/flipper/model/active_record_spec.rb:9:in `block (2 levels) in <top (required)>'
     # ------------------
     # --- Caused by: ---
     # Gem::LoadError:
     #   can't activate sqlite3 (~> 1.4), already activated sqlite3-2.0.1-x86_64-darwin. Make sure all dependencies are added to Gemfile.
     #   ./spec/flipper/model/active_record_spec.rb:9:in `block (2 levels) in <top (required)>'

  3) Flipper::Model::ActiveRecord flipper_properties includes all attributes
     Failure/Error: ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:')

     LoadError:
       Error loading the 'sqlite3' Active Record adapter. Missing a gem it depends on? can't activate sqlite3 (~> 1.4), already activated sqlite3-2.0.1-x86_64-darwin. Make sure all dependencies are added to Gemfile.
     # ./spec/flipper/model/active_record_spec.rb:9:in `block (2 levels) in <top (required)>'
     # ------------------
     # --- Caused by: ---
     # Gem::LoadError:
     #   can't activate sqlite3 (~> 1.4), already activated sqlite3-2.0.1-x86_64-darwin. Make sure all dependencies are added to Gemfile.
     #   ./spec/flipper/model/active_record_spec.rb:9:in `block (2 levels) in <top (required)>'

Finished in 30.49 seconds (files took 5.89 seconds to load)
2622 examples, 3 failures, 131 pending

Copy link
Collaborator

@bkeepers bkeepers left a comment

Choose a reason for hiding this comment

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

Thanks @astupka. Below are a few suggestions. Have you run the tests against 4.2?

If you get the tests passing I'm happy to merge this since it's minimal changes, but I can't guarantee that 4.2 won't break again soon. We have plans to drop support Rails 5.2 and 6.0 soon too (#776). If your team is interested in an LTS version of Flipper, we would consider offering a paid version. But we can't sustain supporting 10 years of Rails versions for free. Send us an email at [email protected] if you want to talk more about it.

@@ -154,6 +156,10 @@ def get_all
end
end

def rails_version_over_4_2?
Rails::VERSION::MAJOR * 10 + Rails::VERSION::MINOR > 42
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Rails::VERSION::MAJOR * 10 + Rails::VERSION::MINOR > 42
Rails.version.to_f > 4.2

If version detection is the only way, this to_f trick is a nice way to get the major/minor version.

@@ -135,7 +135,9 @@ def get_all
rows_query = features.join(gates, ::Arel::Nodes::OuterJoin)
.on(features[:key].eq(gates[:feature_key]))
.project(features[:key].as('feature_key'), gates[:key], gates[:value])
gates = @feature_class.connection.select_rows(rows_query)
select_method = rails_version_over_4_2? ? :select_rows : :select_all
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
select_method = rails_version_over_4_2? ? :select_rows : :select_all
select_method = respond_to?(:select_rows, true) ? :select_rows : :select_all

Instead of version detection, can this use feature detection here?

@severin
Copy link

severin commented Aug 27, 2024

@astupka are you still working on this? Otherwise I'm happy to give it a shot!

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.

3 participants