Skip to content

Commit

Permalink
Make the Relation -> Model delegation stricter
Browse files Browse the repository at this point in the history
In #50395 I noticed lots of
methods are delegated from `Relation` to the model. The intent of
this code is to allow using use defined class methods like scopes.

But because of this autmated delegation it allowed calling any
`ActiveRecord::Base` class method on a `Relation`, which in itself
may be desireable, however we very wastefully define the delegator
on the first call, and worse we wrap it with a current scope setter.

So I think we should be more strict about it.
  • Loading branch information
byroot committed May 10, 2024
1 parent f8537e9 commit 8127973
Show file tree
Hide file tree
Showing 11 changed files with 65 additions and 39 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,12 @@ def initialize(scope, association_key_name)
def eql?(other)
association_key_name == other.association_key_name &&
scope.table_name == other.scope.table_name &&
scope.connection_specification_name == other.scope.connection_specification_name &&
scope.model.connection_specification_name == other.scope.model.connection_specification_name &&
scope.values_for_queries == other.scope.values_for_queries
end

def hash
[association_key_name, scope.table_name, scope.connection_specification_name, scope.values_for_queries].hash
[association_key_name, scope.model.table_name, scope.model.connection_specification_name, scope.values_for_queries].hash
end

def records_for(loaders)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ def self.install_support
module EncryptedQuery # :nodoc:
class << self
def process_arguments(owner, args, check_for_additional_values)
owner = owner.model if owner.is_a?(Relation)

return args if owner.deterministic_encrypted_attributes&.empty?

if args.is_a?(Array) && (options = args.first).is_a?(Hash)
Expand Down
8 changes: 4 additions & 4 deletions activerecord/lib/active_record/relation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,7 @@ def compute_cache_key(timestamp_column = :updated_at) # :nodoc:
query_signature = ActiveSupport::Digest.hexdigest(to_sql)
key = "#{klass.model_name.cache_key}/query-#{query_signature}"

if collection_cache_versioning
if model.collection_cache_versioning
key
else
"#{key}-#{compute_cache_version(timestamp_column)}"
Expand All @@ -455,7 +455,7 @@ def compute_cache_key(timestamp_column = :updated_at) # :nodoc:
#
# SELECT COUNT(*), MAX("products"."updated_at") FROM "products" WHERE (name like '%Cosmic Encounter%')
def cache_version(timestamp_column = :updated_at)
if collection_cache_versioning
if model.collection_cache_versioning
@cache_versions ||= {}
@cache_versions[timestamp_column] ||= compute_cache_version(timestamp_column)
end
Expand Down Expand Up @@ -500,7 +500,7 @@ def compute_cache_version(timestamp_column) # :nodoc:
end

if timestamp
"#{size}-#{timestamp.utc.to_fs(cache_timestamp_format)}"
"#{size}-#{timestamp.utc.to_fs(model.cache_timestamp_format)}"
else
"#{size}"
end
Expand Down Expand Up @@ -1111,7 +1111,7 @@ def instantiate_records(rows, &block)

def skip_query_cache_if_necessary(&block)
if skip_query_cache_value
uncached(&block)
model.uncached(&block)
else
yield
end
Expand Down
4 changes: 2 additions & 2 deletions activerecord/lib/active_record/relation/batches.rb
Original file line number Diff line number Diff line change
Expand Up @@ -327,8 +327,8 @@ def act_on_ignored_order(error_on_ignore)

if raise_error
raise ArgumentError.new(ORDER_IGNORE_MESSAGE)
elsif logger
logger.warn(ORDER_IGNORE_MESSAGE)
elsif model.logger
model.logger.warn(ORDER_IGNORE_MESSAGE)
end
end

Expand Down
13 changes: 11 additions & 2 deletions activerecord/lib/active_record/relation/delegation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,16 @@ def #{method}(...)
:to_sentence, :to_fs, :to_formatted_s, :as_json,
:shuffle, :split, :slice, :index, :rindex, to: :records

delegate :primary_key, :lease_connection, :connection, :with_connection, :transaction, to: :klass
delegate :primary_key, :lease_connection, :with_connection, :connection, :table_name, :transaction, :sanitize_sql_like, :unscoped, to: :klass

# TODO: scoped delegate
[:find_signed, :find_signed!, :delete, :find_by_token_for, :find_by_token_for!, :upsert_all, :insert_all, :insert_all!].each do |method|
module_eval <<-RUBY, __FILE__, __LINE__ + 1
def #{method}(...)
scoping { klass.#{method}(...) }
end
RUBY
end

module ClassSpecificRelation # :nodoc:
extend ActiveSupport::Concern
Expand All @@ -113,7 +122,7 @@ def name

private
def method_missing(method, ...)
if @klass.respond_to?(method)
if @klass.respond_to?(method) && !Base.respond_to?(method)
unless Delegation.uncacheable_methods.include?(method)
@klass.generate_relation_method(method)
end
Expand Down
16 changes: 8 additions & 8 deletions activerecord/lib/active_record/relation/finder_methods.rb
Original file line number Diff line number Diff line change
Expand Up @@ -145,10 +145,10 @@ def sole

if found.nil?
raise_record_not_found_exception!
elsif undesired.present?
raise ActiveRecord::SoleRecordExceeded.new(self)
else
elsif undesired.nil?
found
else
raise ActiveRecord::SoleRecordExceeded.new(model)
end
end

Expand Down Expand Up @@ -638,7 +638,7 @@ def find_last(limit)
end

def ordered_relation
if order_values.empty? && (implicit_order_column || !query_constraints_list.nil? || primary_key)
if order_values.empty? && (model.implicit_order_column || !model.query_constraints_list.nil? || primary_key)
order(_order_columns.map { |column| table[column].asc })
else
self
Expand All @@ -648,11 +648,11 @@ def ordered_relation
def _order_columns
oc = []

oc << implicit_order_column if implicit_order_column
oc << query_constraints_list if query_constraints_list
oc << model.implicit_order_column if model.implicit_order_column
oc << model.query_constraints_list if model.query_constraints_list

if primary_key && query_constraints_list.nil?
oc << primary_key
if model.primary_key && model.query_constraints_list.nil?
oc << model.primary_key
end

oc.flatten.uniq.compact
Expand Down
11 changes: 8 additions & 3 deletions activerecord/lib/active_record/relation/query_methods.rb
Original file line number Diff line number Diff line change
Expand Up @@ -136,9 +136,10 @@ def missing(*associations)

private
def scope_association_reflection(association)
reflection = @scope.klass._reflect_on_association(association)
model = @scope.model
reflection = model._reflect_on_association(association)
unless reflection
raise ArgumentError.new("An association named `:#{association}` does not exist on the model `#{@scope.name}`.")
raise ArgumentError.new("An association named `:#{association}` does not exist on the model `#{model.name}`.")
end
reflection
end
Expand Down Expand Up @@ -254,6 +255,10 @@ def includes!(*args) # :nodoc:
self
end

def all # :nodoc:
spawn
end

# Specify associations +args+ to be eager loaded using a <tt>LEFT OUTER JOIN</tt>.
# Performs a single query joining all specified associations. For example:
#
Expand Down Expand Up @@ -703,7 +708,7 @@ def in_order_of(column, values)
references = column_references([column])
self.references_values |= references unless references.empty?

values = values.map { |value| type_caster.type_cast_for_database(column, value) }
values = values.map { |value| model.type_caster.type_cast_for_database(column, value) }
arel_column = column.is_a?(Arel::Nodes::SqlLiteral) ? column : order_column(column.to_s)

where_clause =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@ def exec_queries
QueryRegistry.reset

super.tap do |records|
if logger && ActiveRecord.warn_on_records_fetched_greater_than
if model.logger && ActiveRecord.warn_on_records_fetched_greater_than
if records.length > ActiveRecord.warn_on_records_fetched_greater_than
logger.warn "Query fetched #{records.size} #{@klass} records: #{QueryRegistry.queries.join(";")}"
model.logger.warn "Query fetched #{records.size} #{@klass} records: #{QueryRegistry.queries.join(";")}"
end
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -824,18 +824,18 @@ def test_association_proxy_transaction_method_starts_transaction_in_association_
end

def test_caching_of_columns
david = Developer.find(1)
Developer.find(1)
# clear cache possibly created by other tests
david.projects.reset_column_information
Project.reset_column_information

assert_queries_count(include_schema: true) { david.projects.columns }
assert_no_queries { david.projects.columns }
assert_queries_count(include_schema: true) { Project.columns }
assert_no_queries { Project.columns }

## and again to verify that reset_column_information clears the cache correctly
david.projects.reset_column_information
Project.reset_column_information

assert_queries_count(include_schema: true) { david.projects.columns }
assert_no_queries { david.projects.columns }
assert_queries_count(include_schema: true) { Project.columns }
assert_no_queries { Project.columns }
end

def test_attributes_are_being_set_when_initialized_from_habtm_association_with_where_clause
Expand Down
22 changes: 16 additions & 6 deletions activerecord/test/cases/relation/delegation_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ class QueryingMethodsDelegationTest < ActiveRecord::TestCase
ActiveRecord::SpawnMethods.public_instance_methods(false) - [:spawn, :merge!] +
ActiveRecord::QueryMethods.public_instance_methods(false).reject { |method|
method.end_with?("=", "!", "?", "value", "values", "clause")
} - [:reverse_order, :arel, :extensions, :construct_join_dependency] + [
} - [:all, :reverse_order, :arel, :extensions, :construct_join_dependency] + [
:any?, :many?, :none?, :one?,
:first_or_create, :first_or_create!, :first_or_initialize,
:find_or_create_by, :find_or_create_by!, :find_or_initialize_by,
Expand All @@ -89,14 +89,24 @@ class DelegationCachingTest < ActiveRecord::TestCase

test "delegation doesn't override methods defined in other relation subclasses" do
# precondition, some methods are available on ActiveRecord::Relation subclasses
# but not ActiveRecord::Relation itself. Here `delete` is just an example.
assert_equal false, ActiveRecord::Relation.method_defined?(:delete)
assert_equal true, ActiveRecord::Associations::CollectionProxy.method_defined?(:delete)
# but not ActiveRecord::Relation itself. Here `clear` is just an example.
assert_equal false, ActiveRecord::Relation.method_defined?(:clear)
assert_equal true, ActiveRecord::Associations::CollectionProxy.method_defined?(:clear)

project = projects(:active_record)
original_owner = project.developers_with_callbacks.method(:delete).owner
original_owner = project.developers_with_callbacks.method(:clear).owner
Developer.all.delete(12345)
assert_equal original_owner, project.developers_with_callbacks.method(:delete).owner
assert_equal original_owner, project.developers_with_callbacks.method(:clear).owner
end

test "delegation automatically delegate ActiveRecord::Base methods" do
assert_equal false, ActiveRecord::Relation.method_defined?(:superclass)
assert_equal true, ActiveRecord::Base.respond_to?(:superclass)

error = assert_raises NoMethodError do
Developer.all.superclass
end
assert_equal :superclass, error.name
end
end
end
6 changes: 3 additions & 3 deletions activerecord/test/models/author.rb
Original file line number Diff line number Diff line change
Expand Up @@ -209,10 +209,10 @@ def ratings
has_many :posts_with_default_include, class_name: "PostWithDefaultInclude"
has_many :comments_on_posts_with_default_include, through: :posts_with_default_include, source: :comments

has_many :posts_with_signature, ->(record) { where(arel_table[:title].matches("%by #{record.name.downcase}%")) }, class_name: "Post"
has_many :posts_mentioning_author, ->(record = nil) { where(arel_table[:body].matches("%#{record&.name&.downcase}%")) }, class_name: "Post"
has_many :posts_with_signature, ->(record) { where(model.arel_table[:title].matches("%by #{record.name.downcase}%")) }, class_name: "Post"
has_many :posts_mentioning_author, ->(record = nil) { where(model.arel_table[:body].matches("%#{record&.name&.downcase}%")) }, class_name: "Post"
has_many :comments_on_posts_mentioning_author, through: :posts_mentioning_author, source: :comments
has_many :comments_mentioning_author, ->(record) { where(arel_table[:body].matches("%#{record.name.downcase}%")) }, through: :posts, source: :comments
has_many :comments_mentioning_author, ->(record) { where(model.arel_table[:body].matches("%#{record.name.downcase}%")) }, through: :posts, source: :comments

has_one :recent_post, -> { order(id: :desc) }, class_name: "Post"
has_one :recent_response, through: :recent_post, source: :comments
Expand Down

0 comments on commit 8127973

Please sign in to comment.