active-hash / active_hash

A readonly ActiveRecord-esque base class that lets you use a hash, a Yaml file or a custom file as the datasource
MIT License
1.21k stars 179 forks source link

require reflection_extensions when ActiveRecordExtensions is extended #286

Closed iberianpig closed 1 year ago

iberianpig commented 1 year ago

Problem

Previously, unless include ActiveHash::Associations was executed, the override to compute_class when setting belongs_to_active_hash would not be performed. This is an unintended behavior.

When compute_class is called in Rails 7, the following error occurs:

ArgumentError: Rails couldn't find a valid model for Rank association.
Please provide the :class_name option on the association declaration.
If :class_name is already provided, make sure it's an ActiveRecord::Base subclass.

Solution

This pull request proposes a change to require "reflection_extensions" when extend ActiveHash::Associations::ActiveRecordExtensions is called instead of include ActiveHash::Associations

The reflection is defined only when extend ActiveHash::Associations::ActiveRecordExtensions. As a result, the compute_class is properly overridden.

flavorjones commented 1 year ago

@iberianpig Thank you for pointing out this issue and for suggesting a fix!

Can you help me reproduce the issue that you're seeing? I'm not entirely sure I understand what you're seeing.

iberianpig commented 1 year ago

I want to make the situation easier to understand, so I will create a reproduction procedure. Please wait for a while.

iberianpig commented 1 year ago

I've created a reproduction procedure. The following code can be used to reproduce the issue.

begin
  require "bundler/inline"
rescue LoadError => e
  $stderr.puts "Bundler version 1.10 or later is required. Please update your Bundler"
  raise e
end

gemfile = File.read(File.expand_path("Gemfile", __dir__)).strip
sqlite3_version = gemfile.lines.find { |line| line.include?('sqlite3') }
  .then { |sqlite3_line| sqlite3_line.strip.split(',').last.delete('"') }

gemfile(true) do
  source "https://rubygems.org"
  gem "activerecord", "~> 7.0.0"
  gem "sqlite3", sqlite3_version
  gem "debug"
  gem "active_hash" # original
  # gem "active_hash", github: "iberianpig/active_hash", branch: "fix/hook-for-reflection-extensions" # fixed version
end

require "active_record"
require "active_hash"
require "minitest/autorun"
require "logger"

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :People, force: true do |t|
    t.integer :country_id
  end
end

class Country < ActiveHash::Base
  self.data = [
    {:id => 1, :name => "US"},
    {:id => 2, :name => "Canada"}
  ]
end

class Person < ActiveRecord::Base
  extend ActiveHash::Associations::ActiveRecordExtensions
  belongs_to_active_hash :country, :shortcuts => [:name]
end

class BugTest < Minitest::Test
  def test_association_stuff
    john = Person.new
    john.country_name = "US"
    # Is the same as doing `john.country = Country.find_by_name("US")`
    john.country_name
    # Will return "US", and is the same as doing `john.country.try(:name)`
    assert_equal Country.find_by_name("US"), john.country

    # Should compute and return the Country class
    assert_equal Country, Person.reflect_on_all_associations.find { |a| a.name == :country }.klass
    # => eval error: Rails couldn't find a valid model for Country association. Please provide the :class_name option on the association declaration. If :class_name is already provided, make sure it's an ActiveRecord::Base subclass.
    #   /home/iberianpig/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/activerecord-7.0.6/lib/active_record/reflection.rb:436:in `compute_class'
    #   /home/iberianpig/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/activerecord-7.0.6/lib/active_record/reflection.rb:382:in `klass'
  end
end

Please save this file as "issue.rb" and execute it by running ruby issue.rb .

Additionally, by uncommenting the line, you can test my correction.

  # gem "active_hash", github: "iberianpig/active_hash", branch: "fix/hook-for-reflection-extensions" # fixed

The problem arises because the code for Rails 7.0 isn't being applied with extend ActiveHash::Associations::ActiveRecordExtensions.

flavorjones commented 1 year ago

Ah! Right, that totally makes sense, and this isn't being caught by the test suite. This is a bug introduced (by me) in #272 that doesn't show up if any class in the application is decorated with include ActiveHash::Associations.

So: I'm not sure how to test this, but this is the right fix.