Skip to content

Commit

Permalink
Define missing attribute methods from method_missing
Browse files Browse the repository at this point in the history
Ref: mastodon/mastodon#27622

If applications don't eager load and use the old Rails 6.1 Marshal
format, they can load Active Record instances from caches without
calling `init_internals` hence attribute methods and aliases are
nnot defined yet, leading to `NoMethodError`

Initially I was hopping to fully get rid of the `define_attribute_methods`
call in `init_internals` in favor of this new method missing, as it would
remove work from the happy path, unfortunately that isn't possible
because if a generated method overrides a default method inherited from
object, `method_missing` won't be called. e.g. `Kernel#format`

We may want to get rid of this extra code once we remove support
for the 6.1 marshal format.
  • Loading branch information
byroot committed Jan 5, 2024
1 parent a2ed343 commit aa44aab
Show file tree
Hide file tree
Showing 7 changed files with 86 additions and 31 deletions.
76 changes: 55 additions & 21 deletions activerecord/lib/active_record/attribute_methods.rb
Original file line number Diff line number Diff line change
Expand Up @@ -63,24 +63,6 @@ def eagerly_generate_alias_attribute_methods(_new_name, _old_name) # :nodoc:
# alias attributes in Active Record are lazily generated
end

def generate_alias_attributes # :nodoc:
superclass.generate_alias_attributes unless superclass == Base
return if @alias_attributes_mass_generated

GeneratedAttributeMethods::LOCK.synchronize do
return if @alias_attributes_mass_generated
ActiveSupport::CodeGenerator.batch(generated_attribute_methods, __FILE__, __LINE__) do |code_generator|
aliases_by_attribute_name.each do |old_name, new_names|
new_names.each do |new_name|
generate_alias_attribute_methods(code_generator, new_name, old_name)
end
end
end

@alias_attributes_mass_generated = true
end
end

def alias_attribute_method_definition(code_generator, pattern, new_name, old_name)
method_name = pattern.method_name(new_name).to_s
target_name = pattern.method_name(old_name).to_s
Expand Down Expand Up @@ -120,6 +102,10 @@ def alias_attribute_method_definition(code_generator, pattern, new_name, old_nam
end
end

def attribute_methods_generated? # :nodoc:
@attribute_methods_generated
end

# Generates all the attribute related methods for columns in the database
# accessors, mutators and query methods.
def define_attribute_methods # :nodoc:
Expand All @@ -128,11 +114,29 @@ def define_attribute_methods # :nodoc:
# attribute methods.
GeneratedAttributeMethods::LOCK.synchronize do
return false if @attribute_methods_generated
superclass.define_attribute_methods unless base_class?
super(attribute_names)
alias_attribute(:id_value, :id) if attribute_names.include?("id")

superclass.define_attribute_methods unless superclass == Base

unless abstract_class?
load_schema
super(attribute_names)
if _has_attribute?("id")
alias_attribute(:id_value, :id)
end
end

ActiveSupport::CodeGenerator.batch(generated_attribute_methods, __FILE__, __LINE__) do |code_generator|
aliases_by_attribute_name.each do |old_name, new_names|
new_names.each do |new_name|
generate_alias_attribute_methods(code_generator, new_name, old_name)
end
end
end

@attribute_methods_generated = true
@alias_attributes_mass_generated = true
end
true
end

def undefine_attribute_methods # :nodoc:
Expand Down Expand Up @@ -459,6 +463,36 @@ def accessed_fields
end

private
def respond_to_missing?(name, include_private = false)
if self.class.define_attribute_methods
# Some methods weren't defined yet.
return true if self.class.method_defined?(name)
return true if include_private && self.class.private_method_defined?(name)
end

super
end

def method_missing(name, ...)
unless self.class.attribute_methods_generated?
if self.class.method_defined?(name)
# The method is explictly defined in the model, but calls a generated
# method with super. So we must resume the call chain at the right setp.
last_method = method(name)
last_method = last_method.super_method while last_method.super_method
self.class.define_attribute_methods
if last_method.super_method
return last_method.super_method.call(...)
end
elsif self.class.define_attribute_methods
# Some attribute methods weren't generated yet, we retry the call
return public_send(name, ...)
end
end

super
end

def attribute_method?(attr_name)
# We check defined? because Syck calls respond_to? before actually calling initialize.
defined?(@attributes) && @attributes.key?(attr_name)
Expand Down
1 change: 0 additions & 1 deletion activerecord/lib/active_record/core.rb
Original file line number Diff line number Diff line change
Expand Up @@ -769,7 +769,6 @@ def init_internals
@strict_loading_mode = :all

klass.define_attribute_methods
klass.generate_alias_attributes
end

def initialize_internals_callback
Expand Down
8 changes: 4 additions & 4 deletions activerecord/lib/active_record/model_schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -422,12 +422,12 @@ def attributes_builder # :nodoc:
end

def columns_hash # :nodoc:
load_schema
load_schema unless @columns_hash
@columns_hash
end

def columns
load_schema
load_schema unless @columns
@columns ||= columns_hash.values.freeze
end

Expand Down Expand Up @@ -524,9 +524,9 @@ def reset_column_information
end

def load_schema # :nodoc:
return if schema_loaded?
return false if schema_loaded?
@load_schema_monitor.synchronize do
return if @columns_hash
return false if schema_loaded?

load_schema!

Expand Down
6 changes: 6 additions & 0 deletions activerecord/test/cases/attribute_methods/read_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,19 @@ def setup
@klass = Class.new(Class.new { def self.initialize_generated_modules; end }) do
def self.superclass; Base; end
def self.base_class?; true; end
def self.abstract_class?; false; end
def self.load_schema; end

include ActiveRecord::AttributeMethods

def self.attribute_names
%w{ one two three }
end

def self.attribute_types
{}
end

def self.primary_key
end

Expand Down
15 changes: 10 additions & 5 deletions activerecord/test/cases/attribute_methods_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1034,11 +1034,16 @@ def name

topic = topic_class.new(title: "New topic")
assert_equal("New topic", topic.subject_to_be_undefined)
assert_equal true, topic_class.method_defined?(:subject_to_be_undefined)
topic_class.undefine_attribute_methods
assert_equal false, topic_class.method_defined?(:subject_to_be_undefined)

assert_raises(NoMethodError, match: /undefined method `subject_to_be_undefined'/) do
topic.subject_to_be_undefined
end
topic.subject_to_be_undefined
assert_equal true, topic_class.method_defined?(:subject_to_be_undefined)

topic_class.undefine_attribute_methods
assert_equal true, topic.respond_to?(:subject_to_be_undefined)
assert_equal true, topic_class.method_defined?(:subject_to_be_undefined)
end

test "#define_attribute_methods brings back undefined aliases" do
Expand All @@ -1052,11 +1057,11 @@ def name
assert_equal("New topic", topic.title_alias_to_be_undefined)
topic_class.undefine_attribute_methods

assert_not_respond_to topic, :title_alias_to_be_undefined
assert_equal false, topic_class.method_defined?(:title_alias_to_be_undefined)

topic_class.define_attribute_methods

assert_respond_to topic, :title_alias_to_be_undefined
assert_equal true, topic_class.method_defined?(:title_alias_to_be_undefined)
assert_equal "New topic", topic.title_alias_to_be_undefined
end

Expand Down
11 changes: 11 additions & 0 deletions activerecord/test/cases/marshal_serialization_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,17 @@ def test_deserializing_rails_7_1_marshal_basic
assert_equal "Have a nice day", topic.content
end

class AliasedTopic < ActiveRecord::Base
self.table_name = "topics"

alias_attribute :heading, :title
end

def test_deserializing_record_define_attribute_methods
topic = Marshal.load(marshal_fixture("rails_6_1_aliased_topic"))
assert_equal "The First Topic", topic.heading
end

def test_deserializing_rails_7_1_marshal_with_loaded_association_cache
topic = Marshal.load(marshal_fixture("rails_7_1_topic_associations"))

Expand Down
Binary file not shown.

0 comments on commit aa44aab

Please sign in to comment.