Merge pull request rails#44616 from ghiculescu/preload-all-if-proc

Always preload if using proc with multifetch cache

Author: byroot

actionview/lib/action_view/renderer/collection_renderer.rb

@@ -51,6 +51,10 @@ def size
       def length
         @collection.respond_to?(:length) ? @collection.length : size
       end
+
+      def preload!
+        # no-op
+      end
     end
 
     class SameCollectionIterator < CollectionIterator # :nodoc:
@@ -84,9 +88,13 @@ def from_collection(collection)
 
       def each_with_info
         return super unless block_given?
-        @relation.preload_associations(@collection)
+        preload!
         super
       end
+
+      def preload!
+        @relation.preload_associations(@collection)
+      end
     end
 
     class MixedCollectionIterator < CollectionIterator # :nodoc:

actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb

@@ -59,6 +59,7 @@ def collection_by_cache_keys(view, template, collection)
         seed = callable_cache_key? ? @options[:cached] : ->(i) { i }
 
         digest_path = view.digest_path_from_template(template)
+        collection.preload! if callable_cache_key?
 
         collection.each_with_object([{}, []]) do |item, (hash, ordered_keys)|
           key = expanded_cache_key(seed.call(item), view, template, digest_path)

actionview/test/active_record_unit.rb

@@ -105,6 +105,32 @@ def self.fixtures(*args)
   def run(*args)
     super if ActiveRecordTestConnector.connected
   end
+
+  def capture_sql
+    ActiveRecord::Base.connection.materialize_transactions
+    SQLCounter.clear_log
+    yield
+    SQLCounter.log.dup
+  end
+
+  class SQLCounter
+    class << self
+      attr_accessor :log, :log_all
+      def clear_log; self.log = []; self.log_all = []; end
+    end
+
+    clear_log
+
+    def call(name, start, finish, message_id, values)
+      return if values[:cached]
+
+      sql = values[:sql]
+      self.class.log_all << sql
+      self.class.log << sql unless ["SCHEMA", "TRANSACTION"].include? values[:name]
+    end
+  end
+
+  ActiveSupport::Notifications.subscribe("sql.active_record", SQLCounter.new)
 end
 
 ActiveRecordTestConnector.setup

actionview/test/activerecord/multifetch_cache_test.rb

@@ -2,36 +2,79 @@
 
 require "active_record_unit"
 
+class MultifetchController < ActionController::Base
+  ROUTES = test_routes do
+    get :cached_true, to: "multifetch#cached_true"
+    get :cached_proc, to: "multifetch#cached_proc"
+  end
+
+  def cached_true
+    load_all_topics
+    render partial: "test/multifetch", collection: @topics, as: :topic, cached: true
+  end
+
+  def cached_proc
+    load_all_topics
+    render partial: "test/multifetch", collection: @topics, as: :topic, cached: proc { |topic| [topic] }
+  end
+
+  private
+    def load_all_topics
+      @topics = Topic.preload(:replies)
+    end
+end
+
 class MultifetchCacheTest < ActiveRecordTestCase
+  tests MultifetchController
   fixtures :topics, :replies
 
-  def setup
-    view_paths = ActionController::Base.view_paths
-    view_paths.each(&:clear_cache)
+  setup do
+    Topic.update_all(updated_at: Time.now)
+  end
+
+  def test_only_preloading_for_records_that_miss_the_cache
+    Topic.update_all(title: "title")
 
-    @view = Class.new(ActionView::Base.with_empty_template_cache) do
-      def view_cache_dependencies
-        []
-      end
+    # The first request will not have anything cached, so we need to render all the records
+    first_req = capture_sql do
+      get :cached_true
+    end
+    assert_equal 3, @response.body.scan(/title/).length
+    assert_equal 2, first_req.length
+    assert_includes first_req.last, %(WHERE "replies"."topic_id" IN (?, ?, ?))
 
-      def combined_fragment_cache_key(key)
-        [ :views, key ]
-      end
-    end.with_view_paths(view_paths, {})
+    Topic.first.update(updated_at: 1.hour.ago) # reset the cache key for this object
 
-    controller = ActionController::Base.new
-    controller.perform_caching = true
-    @view.controller = controller
+    # Now we only load the one record that we don't have a cached view for.
+    second_req = capture_sql do
+      get :cached_true
+    end
+    assert_equal 3, @response.body.scan(/title/).length
+    assert_equal 2, second_req.length
+    assert_equal first_req.first, second_req.first
+    assert_includes second_req.last, %(WHERE "replies"."topic_id" = ?)
   end
 
-  def test_only_preloading_for_records_that_miss_the_cache
-    @view.render partial: "test/partial", collection: [topics(:rails)], cached: true
+  def test_preloads_all_records_if_using_cached_proc
+    Topic.update_all(title: "title")
 
-    @topics = Topic.preload(:replies)
+    # The first request will not have anything cached, so we need to render all the records
+    first_req = capture_sql do
+      get :cached_proc
+    end
+    assert_equal 3, @response.body.scan(/title/).length
+    assert_equal 2, first_req.length
+    assert_includes first_req.last, %(WHERE "replies"."topic_id" IN (?, ?, ?))
 
-    @view.render partial: "test/partial", collection: @topics, cached: true
+    Topic.first.update(updated_at: 1.hour.ago) # reset the cache key for this object
 
-    assert_not @topics.detect { |topic| topic.id == topics(:rails).id }.replies.loaded?
-    assert     @topics.detect { |topic| topic.id != topics(:rails).id }.replies.loaded?
+    # Since we are using a proc, we will preload the entire association.
+    second_req = capture_sql do
+      get :cached_proc
+    end
+    assert_equal 3, @response.body.scan(/title/).length
+    assert_equal 2, second_req.length
+    assert_equal first_req.first, second_req.first
+    assert_includes second_req.last, %(WHERE "replies"."topic_id" IN (?, ?, ?))
   end
 end

actionview/test/fixtures/test/_multifetch.html.erb

@@ -0,0 +1 @@
+<%= topic.title %>, rendered: <%= topic.updated_at %>