Skip to content

Commit

Permalink
Merge pull request #50594 from Shopify/ar-marshal-define-attribute-me…
Browse files Browse the repository at this point in the history
…thods

Define missing attribute methods from `method_missing`
  • Loading branch information
byroot committed Jan 5, 2024
2 parents 21090a1 + a0993f8 commit d429bfb
Show file tree
Hide file tree
Showing 6 changed files with 74 additions and 30 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

This comment has been minimized.

Copy link
@ghiculescu

ghiculescu Jan 10, 2024

Member

This is causing problems in a multi-db application. The specific example we have is using delayed job connected to a different DB to the primary DB.

Here's a simplified setup: https://gist.github.com/ghiculescu/4beb81cae0f99660fda992811a3aebf6

To break it:

  • config.eager_load = true and config.cache_classes = true in environments/test.rb (to match a CI config)
  • In a test, do anything that interacts with delayed job

The issue is the recursive calls to define_attribute_methods. When running in CI, we end up trying to call load_schema for the Delayed::Backend::ActiveRecord::Job model (since we go DelayedJobImplementation -> DelayedJobAbstractParentClass -> Delayed::Backend::ActiveRecord::Job).

Since we have not set connects_to on that model (we can't, because it's not abstract), it tries to connect to the primary database, but the delayed_jobs table does not exist in the primary database, so it crashes.

Note that if I make this change, then things work as expected:

         # superclass.define_attribute_methods unless superclass == Base
         superclass.define_attribute_methods unless base_class?

I can work on a more comprehensive replication app but just wanted to dump what I had so far in case the issue is obvious.

This comment has been minimized.

Copy link
@ghiculescu

ghiculescu Jan 10, 2024

Member

ps. I agree it would be better if delayed_job had a cleaner way to connect to a different database, so if this behaviour is unsupported I can accept that.

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Jan 10, 2024

Member

Can you open an issue please? We don't check commit messages often and we can't track them.


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 @@ -457,6 +461,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 explicitly 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)
@attributes&.key?(attr_name)
end
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
6 changes: 3 additions & 3 deletions activerecord/lib/active_record/model_schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -421,12 +421,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 @@ -525,7 +525,7 @@ def reset_column_information
def load_schema # :nodoc:
return if schema_loaded?
@load_schema_monitor.synchronize do
return if @columns_hash
return 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
Binary file not shown.

0 comments on commit d429bfb

Please sign in to comment.