Merge pull request rails#50396 from Shopify/stricter-relation-delegation

byroot · web-flow · commit 407031f8b86c · 2024-05-28T08:47:41.000+02:00
Make the Relation -&gt; Model delegation stricter
diff --git a/activerecord/lib/active_record/associations/preloader/association.rb b/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)
diff --git a/activerecord/lib/active_record/encryption/extended_deterministic_queries.rb b/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)
diff --git a/activerecord/lib/active_record/relation.rb b/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
diff --git a/activerecord/lib/active_record/relation/batches.rb b/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
 
diff --git a/activerecord/lib/active_record/relation/calculations.rb b/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)
diff --git a/activerecord/lib/active_record/relation/delegation.rb b/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
diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/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
diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/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
diff --git a/activerecord/lib/active_record/relation/record_fetch_warning.rb b/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
diff --git a/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb b/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
diff --git a/activerecord/test/cases/helper.rb b/activerecord/test/cases/helper.rb
@@ -26,6 +26,8 @@
 # to ensure it's not used internally.
 ActiveRecord.permanent_connection_checkout = :disallowed
 
+ActiveRecord::Delegation::DelegateCache.delegate_base_methods = false
+
 # Disable available locale checks to avoid warnings running the test suite.
 I18n.enforce_available_locales = false
 
diff --git a/activerecord/test/cases/relation/delegation_test.rb b/activerecord/test/cases/relation/delegation_test.rb
@@ -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,
diff --git a/activerecord/test/models/author.rb b/activerecord/test/models/author.rb
@@ -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
