Make the Relation -> Model delegation stricter

In rails#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 allows 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 global scope setter.

This also has led to bugs in the past, like rails#51776

So I think we should be more strict about it.

We can't deprecate this behavior because gems might depend on it, however we
can ban it from Active Record's own test suite to avoid regressions.

Author: byroot

activerecord/lib/active_record/associations/preloader/association.rb

@@ -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)

activerecord/lib/active_record/encryption/extended_deterministic_queries.rb

@@ -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)

activerecord/lib/active_record/relation.rb

@@ -437,7 +437,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)}"
@@ -456,7 +456,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
@@ -475,7 +475,7 @@ def compute_cache_version(timestamp_column) # :nodoc:
 
         with_connection do |c|
           column = c.visitor.compile(table[timestamp_column])
-          select_values = "COUNT(*) AS #{adapter_class.quote_column_name("size")}, MAX(%s) AS timestamp"
+          select_values = "COUNT(*) AS #{klass.adapter_class.quote_column_name("size")}, MAX(%s) AS timestamp"
 
           if collection.has_limit_or_offset?
             query = collection.select("#{column} AS collection_cache_key_timestamp")
@@ -501,7 +501,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
@@ -1291,7 +1291,7 @@ def has_limit_or_offset? # :nodoc:
     end
 
     def alias_tracker(joins = [], aliases = nil) # :nodoc:
-      ActiveRecord::Associations::AliasTracker.create(connection_pool, table.name, joins, aliases)
+      ActiveRecord::Associations::AliasTracker.create(klass.connection_pool, table.name, joins, aliases)
     end
 
     class StrictLoadingScope # :nodoc:
@@ -1451,7 +1451,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

activerecord/lib/active_record/relation/batches.rb

@@ -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
 

activerecord/lib/active_record/relation/calculations.rb

@@ -522,13 +522,13 @@ def execute_grouped_calculation(operation, column_name, distinct) # :nodoc:
           column = aggregate_column(column_name)
           column_alias = column_alias_tracker.alias_for("#{operation} #{column_name.to_s.downcase}")
           select_value = operation_over_aggregate_column(column, operation, distinct)
-          select_value.as(adapter_class.quote_column_name(column_alias))
+          select_value.as(klass.adapter_class.quote_column_name(column_alias))
 
           select_values = [select_value]
           select_values += self.select_values unless having_clause.empty?
 
           select_values.concat group_columns.map { |aliaz, field|
-            aliaz = adapter_class.quote_column_name(aliaz)
+            aliaz = klass.adapter_class.quote_column_name(aliaz)
             if field.respond_to?(:as)
               field.as(aliaz)
             else
@@ -633,6 +633,7 @@ def select_for_count
         if select_values.present?
           return select_values.first if select_values.one?
 
+          adapter_class = klass.adapter_class
           select_values.map do |field|
             column = arel_column(field.to_s) do |attr_name|
               Arel.sql(attr_name)

activerecord/lib/active_record/relation/delegation.rb

@@ -22,6 +22,9 @@ def uncacheable_methods
     end
 
     module DelegateCache # :nodoc:
+      @delegate_base_methods = true
+      singleton_class.attr_accessor :delegate_base_methods
+
       def relation_delegate_class(klass)
         @relation_delegate_cache[klass]
       end
@@ -100,7 +103,7 @@ 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, :with_connection, :connection, :table_name, :transaction, :sanitize_sql_like, :unscoped, to: :klass
 
     module ClassSpecificRelation # :nodoc:
       extend ActiveSupport::Concern
@@ -114,9 +117,17 @@ def name
       private
         def method_missing(method, ...)
           if @klass.respond_to?(method)
-            unless Delegation.uncacheable_methods.include?(method)
+            if !DelegateCache.delegate_base_methods && Base.respond_to?(method)
+              # A common mistake in Active Record's own code is to call `ActiveRecord::Base`
+              # class methods on Association. It works because it's automatically delegated, but
+              # can introduce subtle bugs because it sets the global scope.
+              # We can't deprecate this behavior because gems might depend on it, however we
+              # can ban it from Active Record's own test suite to avoid regressions.
+              raise NotImplementedError, "Active Record code shouldn't rely on association delegation into ActiveRecord::Base methods"
+            elsif !Delegation.uncacheable_methods.include?(method)
               @klass.generate_relation_method(method)
             end
+
             scoping { @klass.public_send(method, ...) }
           else
             super

activerecord/lib/active_record/relation/finder_methods.rb

@@ -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
 
@@ -376,7 +376,7 @@ def exists?(conditions = :none)
 
       skip_query_cache_if_necessary do
         with_connection do |c|
-          c.select_rows(relation.arel, "#{name} Exists?").size == 1
+          c.select_rows(relation.arel, "#{klass.name} Exists?").size == 1
         end
       end
     end
@@ -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
@@ -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

activerecord/lib/active_record/relation/query_methods.rb

@@ -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
@@ -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:
     #
@@ -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 =
@@ -1914,7 +1919,7 @@ def arel_columns(columns)
           case field
           when Symbol
             arel_column(field.to_s) do |attr_name|
-              adapter_class.quote_table_name(attr_name)
+              klass.adapter_class.quote_table_name(attr_name)
             end
           when String
             arel_column(field, &:itself)
@@ -1946,7 +1951,7 @@ def arel_column(field)
 
       def table_name_matches?(from)
         table_name = Regexp.escape(table.name)
-        quoted_table_name = Regexp.escape(adapter_class.quote_table_name(table.name))
+        quoted_table_name = Regexp.escape(klass.adapter_class.quote_table_name(table.name))
         /(?:\A|(?<!FROM)\s)(?:\b#{table_name}\b|#{quoted_table_name})(?!\.)/i.match?(from.to_s)
       end
 
@@ -2081,7 +2086,7 @@ def order_column(field)
           if attr_name == "count" && !group_values.empty?
             table[attr_name]
           else
-            Arel.sql(adapter_class.quote_table_name(attr_name), retryable: true)
+            Arel.sql(klass.adapter_class.quote_table_name(attr_name), retryable: true)
           end
         end
       end

activerecord/lib/active_record/relation/record_fetch_warning.rb

@@ -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

activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb

@@ -823,21 +823,6 @@ def test_association_proxy_transaction_method_starts_transaction_in_association_
     end
   end
 
-  def test_caching_of_columns
-    david = Developer.find(1)
-    # clear cache possibly created by other tests
-    david.projects.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
-
-    assert_queries_count(include_schema: true) { david.projects.columns }
-    assert_no_queries { david.projects.columns }
-  end
-
   def test_attributes_are_being_set_when_initialized_from_habtm_association_with_where_clause
     new_developer = projects(:action_controller).developers.where(name: "Marcelo").build
     assert_equal "Marcelo", new_developer.name