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

require reflection_extensions when ActiveRecordExtensions is extended #286

Merged

Conversation

iberianpig
Copy link
Contributor

@iberianpig iberianpig commented Aug 2, 2023

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.

@iberianpig iberianpig force-pushed the fix/hook-for-reflection-extensions branch from c574c04 to f80699a Compare August 2, 2023 09:50
`extend ActiveHash::Associations::ActiveRecordExtensions` does not require 'reflection_extensions'
so the following error occurs when `compute_class` is called in Rails 7.

```
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.
```
@iberianpig iberianpig force-pushed the fix/hook-for-reflection-extensions branch from f80699a to afb99de Compare August 2, 2023 09:51
@iberianpig iberianpig marked this pull request as ready for review August 2, 2023 10:30
@flavorjones
Copy link
Collaborator

@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
Copy link
Contributor Author

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

@iberianpig
Copy link
Contributor Author

iberianpig commented Aug 6, 2023

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
Copy link
Collaborator

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.

@flavorjones flavorjones merged commit 133b47d into active-hash:master Aug 11, 2023
10 checks passed
@iberianpig iberianpig deleted the fix/hook-for-reflection-extensions branch August 16, 2023 02:53
@flavorjones flavorjones mentioned this pull request Aug 31, 2023
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.

2 participants