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 rails 7.2 loading #1534

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

Conversation

jdlubrano
Copy link

Fixes #1518

Problem

In a typical Rails app, the Ransack gem gets loaded before ActiveRecord gets loaded. Rails recommends that gems avoid loading Rails frameworks when they themselves are loaded. Ransack accomplishes that (as far as I can tell); however, Ransack initializes certain parts of its code within blocks like

if defined?(::ActiveRecord)
  ...set up some stuff
end

If Ransack is loaded before ActiveRecord is loaded, those blocks of code are never reached. I think these blocks of code should be wrapped with ActiveSupport.on_load(:active_record) instead of if defined?(::ActiveRecord). The blocks of code should run after ActiveRecord is loaded, which is what on_load(:active_record) is meant to accomplish.

Test coverage

I wasn't quite sure how to cover this in the specs, so I adjusted the load order in spec/spec_helper.rb to require "ransack" first. That did expose the load order issue(s) within the gem, so maybe that is good enough. It might be ideal to set up a test that spins up a Rails app and runs a basic query just to check that the basic functionality of the gem works within Rails 🤷‍♂️ . For local development, I used a single file Rails app like this:

# frozen_string_literal: true

require 'bundler/inline'

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

  gem 'debug'
  gem 'irb'
  gem 'rails', '>= 7.1'
  gem 'sqlite3', '~> 1.4'
  gem 'ransack' # , path: "./ransack"
end

require 'debug'
require 'rails/all'
database = 'development.sqlite3'

ENV['DATABASE_URL'] = "sqlite3:#{database}"

class App < Rails::Application
  config.load_defaults 7.2
  config.eager_load = false
end

ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: database)
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :posts, force: true do |t|
    t.string :title, null: false
  end
end

App.initialize!

class Post < ActiveRecord::Base

  def self.ransackable_associations(_auth_object = nil) = []

  def self.ransackable_attributes(_auth_object = nil)
    %w[title]
  end

end

1.upto(10) { |i| Post.create!(title: "Title #{i}") }

IRB.start

Post.ransack(title_eq: "Title 1").result.to_sql
=> SELECT * FROM posts where title = 'Title 1'

@@ -1,3 +1,4 @@
require 'active_support/dependencies/autoload'
Copy link
Author

Choose a reason for hiding this comment

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

Just requiring this explicitly since on_load does get used further down in this file (and I suppose should be considered in any other files that get required from here).

@@ -6,13 +6,12 @@ module ActionView::Helpers::Tags
# https://github.com/rails/rails/commit/c1a118a
class Base
private
if defined? ::ActiveRecord
Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure that this was ever truly dependent on ActiveRecord. At one point, this was a version check. I agree with the comment that it would be nice to avoid this patch to ActionView, but I also think that the patch is necessary regardless of whether ActiveRecord is defined or not. Moreover, the code within doesn't depend on ActiveRecord as we are just calling send on an object 🤔 .

@jdlubrano jdlubrano marked this pull request as ready for review October 29, 2024 17:14
@jdlubrano
Copy link
Author

jdlubrano commented Oct 29, 2024

cc: @scottbarrow

require 'action_controller'
require 'ransack/helpers'
require 'pry'
require 'simplecov'
require 'byebug'
require 'machinist/active_record'
Copy link
Author

Choose a reason for hiding this comment

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

machinist/active_record requires active_record, which made all of the code wrapped in defined?(::ActiveRecord) execute. Requiring machinist/active_record after required ransack, I think, defines a load order that better imitates a Rails app.

I am not sure whether going down this path is the best approach. I feel
that perhaps ActiveSupport could require everything within
active_support/core_ext to ensure that active_support/core_ext can load
itself. Still, there's no changing old versions of Rails, so I suppose
this solution works? I am also not sure what the Rails team would
recommend when it comes to requiring specific pieces of ActiveSupport
(to leverage things like mattr_accessor).

In any case, I was able to reproduce the CI failures locally and these
changes make everything pass for me locally.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant