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 Dec 22, 2023
1 parent b1d57fb commit c511237
Show file tree
Hide file tree
Showing 8 changed files with 26 additions and 19 deletions.
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
6 changes: 3 additions & 3 deletions activerecord/lib/active_record/relation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -365,7 +365,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 @@ -384,7 +384,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 @@ -427,7 +427,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
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
4 changes: 2 additions & 2 deletions activerecord/lib/active_record/relation/delegation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ def #{method}(...)
:to_sentence, :to_fs, :to_formatted_s, :as_json,
:shuffle, :split, :slice, :index, :rindex, to: :records

delegate :primary_key, :connection, :transaction, to: :klass
delegate :primary_key, :connection, :table_name, :transaction, :arel_table, :uncached, :sanitize_sql_like, to: :klass

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

private
def method_missing(method, *args, &block)
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
12 changes: 6 additions & 6 deletions activerecord/lib/active_record/relation/finder_methods.rb
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,7 @@ def exists?(conditions = :none)
relation = construct_relation_for_exists(conditions)
return false if relation.where_clause.contradiction?

skip_query_cache_if_necessary { connection.select_rows(relation.arel, "#{name} Exists?").size == 1 }
skip_query_cache_if_necessary { connection.select_rows(relation.arel, "#{model.name} Exists?").size == 1 }
end

# Returns true if the relation contains the given record or false otherwise.
Expand Down Expand Up @@ -624,7 +624,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 @@ -634,11 +634,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
9 changes: 7 additions & 2 deletions activerecord/lib/active_record/relation/query_methods.rb
Original file line number Diff line number Diff line change
Expand Up @@ -120,9 +120,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 @@ -238,6 +239,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
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,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 @@ -826,13 +826,13 @@ def test_association_proxy_transaction_method_starts_transaction_in_association_
def test_caching_of_columns
david = 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 }

## 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 }
Expand Down

0 comments on commit c511237

Please sign in to comment.