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

Attribute aliases don't resolve dynamically like normal attribute methods #51717

Open
ajvondrak opened this issue May 2, 2024 · 1 comment
Open

Comments

@ajvondrak
Copy link

With #48533 circa Rails 7.1, attribute methods and aliases are defined "lazily" on AR subclasses, meaning they get defined on the first call to ActiveRecord::Base#initialize via #init_internals:

klass.define_attribute_methods
klass.generate_alias_attributes

Since defining attribute methods (and thus aliases atop of them) may involve querying the database, it's preferable to evaluate these lazily, rather than (say) tying them to class definition time as the Rails app starts up.

However, when loading a model from a cache in a process that has yet to connect to the database, we can manage to create a new instance of an ActiveRecord::Base subclass without calling the initializer, so the attribute methods & aliases never get defined by #init_internals. (Yes, we probably don't want to be storing models in the cache due to other issues like #49826. But the lazy evaluation does still cause a regression in behavior versus Rails 7.0, so I thought it was worth bringing up.)

This works out fine for regular attribute methods, since the #attribute_missing hook will dynamically look up values in the @attributes at run time:

def method_missing(method, *args, &block)
if respond_to_without_attributes?(method, true)
super
else
match = matched_attribute_method(method.to_s)
match ? attribute_missing(match, *args, &block) : super
end
end
def attribute_missing(match, *args, &block)
__send__(match.proxy_target, match.attr_name, *args, &block)
end

However, attribute aliases perform no such method_missing magic. This can lead us to fail with a NoMethodError—or perhaps more insidiously, silently delegate to a supermethod that would have been clobbered by the alias.

Steps to reproduce

# frozen_string_literal: true

require "bundler/inline"

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

  gem "rails", "~> 7.1.0"
  gem "sqlite3", "~> 1.4"
end

require "active_record"
require "active_support/cache"
require "minitest/autorun"
require "logger"
require "tempfile"

database = Tempfile.create # shared database file so separate processes can reference it
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: database.path)
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :models, force: true do |t|
    t.string :old_name
    t.string :value_from_attribute
  end
end

class Model < ActiveRecord::Base
  alias_attribute :new_name, :old_name

  def value_from_attribute_or_supermethod
    "value from supermethod"
  end
end

class ModelWithSupermethod < Model
  alias_attribute :value_from_attribute_or_supermethod, :value_from_attribute
end

$cache = ActiveSupport::Cache::FileStore.new(Dir.mktmpdir("file-store-")) # file store so separate processes can reference it

class AliasedAttributesTest < Minitest::Test
  def setup
    $cache.clear
  end

  def test_read_model_from_cache_without_connecting_to_database
    pid = fork do
      refute Model.instance_methods.include?(:old_name), "expected model not to define attribute methods until initialization"
      refute Model.instance_methods.include?(:new_name), "expected model not to define attribute aliases until initialization"
      model = Model.create!(old_name: "test")
      assert Model.instance_methods.include?(:old_name), "expected model to have lazily defined attribute methods on initialization"
      assert Model.instance_methods.include?(:new_name), "expected model to have lazily defined attribute aliases on initialization"
      assert_equal "test", model.old_name
      assert_equal "test", model.new_name
      $cache.write('model', model)
    end
    Process.wait(pid)
    assert $cache.exist?('model'), "expected cache to be populated by a separate process"
    refute Model.instance_methods.include?(:old_name), "expected separate process not to affect method definitions in this process"
    refute Model.instance_methods.include?(:new_name), "expected separate process not to affect method definitions in this process"
    model = $cache.read('model')
    refute Model.instance_methods.include?(:old_name), "expected model not to have defined attribute methods yet, since we haven't initialized"
    refute Model.instance_methods.include?(:new_name), "expected model not to have defined attribute aliases yet, since we haven't initialized"
    assert_instance_of Model, model
    assert_equal "test", model.old_name, "expected attribute method to still work dynamically due to attribute_missing hook"
    begin
      assert_equal "test", model.new_name
    rescue NoMethodError => e
      flunk "attribute aliases do not perform any method_missing magic\n#{e.class}: #{e.message}"
    end
  end

  def test_alias_that_should_have_clobbered_supermethod
    pid = fork do
      model = ModelWithSupermethod.new(value_from_attribute: "value from attribute")
      assert_equal "value from attribute", model.value_from_attribute_or_supermethod
      $cache.write('model', model)
    end
    Process.wait(pid)
    assert $cache.exist?('model'), "expected cache to be populated by a separate process"
    model = $cache.read('model')
    assert_instance_of ModelWithSupermethod, model
    assert_equal "value from attribute", model.value_from_attribute_or_supermethod, "supermethod did not get shadowed by attribute alias"
  end
end

Expected behavior

Test cases above should pass.

Actual behavior

F

Failure:
AliasedAttributesTest#test_read_model_from_cache_without_connecting_to_database [repro.rb:71]:
attribute aliases do not perform any method_missing magic
NoMethodError: undefined method `new_name' for #<Model id: 1, old_name: "test", value_from_attribute: nil>


bin/rails test repro.rb:48

F

Failure:
AliasedAttributesTest#test_alias_that_should_have_clobbered_supermethod [repro.rb:85]:
supermethod did not get shadowed by attribute alias.
Expected: "value from attribute"
  Actual: "value from supermethod"


bin/rails test repro.rb:75

System configuration

Rails version: 7.1

Ruby version: 3.2.2

justinko added a commit to justinko/rails that referenced this issue May 4, 2024
justinko added a commit to justinko/rails that referenced this issue May 4, 2024
justinko added a commit to justinko/rails that referenced this issue May 4, 2024
@justinko
Copy link
Contributor

justinko commented May 6, 2024

If attribute methods are lazily loaded, and aliases are tied to attributes (and should only be tied to attributes), then how would it be possible to make aliases lazy?

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

No branches or pull requests

2 participants