Handle ActiveRecord caching properly

app/controllers/advertisement_controller.rb

@@ -35,10 +35,10 @@ def specific_question
 
   def specific_category
     @category = Category.unscoped.find(params[:id])
-    @post = Rails.cache.fetch "ca_random_category_post/#{params[:id]}",
-                              expires_in: 5.minutes do
-      select_random_post(@category, days: params[:days]&.to_i, score: params[:score]&.to_f)
-    end
+    @post = Rails.cache.fetch_collection "ca_random_category_post/#{params[:id]}",
+                                         expires_in: 5.minutes do
+      random_post_collection(@category, days: params[:days]&.to_i, score: params[:score]&.to_f)
+    end.sample
 
     if @post.nil?
       not_found!
@@ -53,9 +53,9 @@ def specific_category
   end
 
   def random_question
-    @post = Rails.cache.fetch 'ca_random_hot_post', expires_in: 5.minutes do
-      select_random_post
-    end
+    @post = Rails.cache.fetch_collection 'ca_random_hot_post', expires_in: 5.minutes do
+      random_post_collection
+    end.sample
     if @post.nil?
       return community
     end
@@ -95,7 +95,7 @@ def promoted_post
   # @param score [Float] the minimum post score to consider
   # @param count [Integer] a maximum number of posts to query for; the final post will be randomly selected from this
   # @return [Post]
-  def select_random_post(category = nil, days: nil, score: nil, count: nil)
+  def random_post_collection(category = nil, days: nil, score: nil, count: nil)
     if category.nil?
       category = Category.where(use_for_advertisement: true)
     end
@@ -106,7 +106,7 @@ def select_random_post(category = nil, days: nil, score: nil, count: nil)
         .where(posts: { last_activity: days.days.ago..DateTime.now })
         .where(posts: { category: category })
         .where('posts.score > ?', score.nil? ? SiteSetting['HotPostsScoreThreshold'] : score)
-        .order('posts.score DESC').limit(count.nil? ? SiteSetting['HotQuestionsCount'] : count).all.sample
+        .order('posts.score DESC').limit(count.nil? ? SiteSetting['HotQuestionsCount'] : count).all
   end
 
   def send_resp(data)

app/controllers/application_controller.rb

@@ -205,9 +205,10 @@ def setup_request_context
     RequestContext.clear!
 
     host_name = request.raw_host_with_port # include port to support multiple localhost instances
-    RequestContext.community = @community = Rails.cache.fetch("#{host_name}/community", expires_in: 1.hour) do
-      Community.unscoped.find_by(host: host_name)
-    end
+    @community = Rails.cache.fetch_collection("#{host_name}/community", expires_in: 1.hour) do
+      Community.unscoped.where(host: host_name)
+    end.first
+    RequestContext.community = @community
 
     Rails.logger.info "  Host #{host_name}, community ##{RequestContext.community_id} " \
                       "(#{RequestContext.community&.name})"
@@ -230,7 +231,7 @@ def setup_user
   end
 
   def pull_pinned_links_and_hot_questions
-    @pinned_links = Rails.cache.fetch('pinned_links', expires_in: 2.hours) do
+    @pinned_links = Rails.cache.fetch_collection('pinned_links', expires_in: 2.hours) do
       Rack::MiniProfiler.step 'pinned_links: cache miss' do
         PinnedLink.where(active: true).where('shown_before IS NULL OR shown_before > NOW()').all
       end
@@ -247,7 +248,7 @@ def pull_pinned_links_and_hot_questions
     # I.e., if pinned_post_ids contains null, the selection will never return records
     pinned_post_ids = @pinned_links.map(&:post_id).compact
 
-    @hot_questions = Rails.cache.fetch('hot_questions', expires_in: 4.hours) do
+    @hot_questions = Rails.cache.fetch_collection('hot_questions', expires_in: 4.hours) do
       Rack::MiniProfiler.step 'hot_questions: cache miss' do
         Post.undeleted.not_locked.where(closed: false)
             .where(last_activity: (Rails.env.development? ? 365 : 7).days.ago..DateTime.now)
@@ -261,7 +262,7 @@ def pull_pinned_links_and_hot_questions
   end
 
   def pull_categories
-    @header_categories = Rails.cache.fetch('header_categories') do
+    @header_categories = Rails.cache.fetch_collection('header_categories') do
       Category.all.order(sequence: :asc, id: :asc)
     end
   end

app/helpers/advertisements/article_helper.rb

@@ -6,6 +6,7 @@ module Advertisements::ArticleHelper
   # rubocop:disable Metrics/MethodLength
   # rubocop:disable Metrics/BlockLength
   def article_ad(article)
+    # TODO: trying to cache like this is probably a terrible idea - review options
     Rails.cache.fetch "posts/#{article.id}/ad", expires_in: 60.minutes do
       ad = Image.new(600, 500)
       ad.background_color = 'white'

app/helpers/advertisements/codidact_helper.rb

@@ -5,6 +5,7 @@ module Advertisements::CodidactHelper
 
   # rubocop:disable Metrics/BlockLength
   def codidact_ad
+    # TODO: trying to cache like this is probably a terrible idea - review options
     Rails.cache.fetch 'network/codidact_ad', expires_in: 60.minutes, include_community: false do
       ad = Image.new(600, 500)
       ad.background_color = 'white'

app/helpers/advertisements/community_helper.rb

@@ -5,6 +5,7 @@ module Advertisements::CommunityHelper
 
   # rubocop:disable Metrics/BlockLength
   def community_ad
+    # TODO: trying to cache like this is probably a terrible idea - review options
     Rails.cache.fetch 'community_ad', expires_in: 60.minutes do
       ad = Image.new(600, 500)
       ad.background_color = 'white'

app/helpers/advertisements/question_helper.rb

@@ -6,6 +6,7 @@ module Advertisements::QuestionHelper
   # rubocop:disable Metrics/MethodLength
   # rubocop:disable Metrics/BlockLength
   def question_ad(question)
+    # TODO: trying to cache like this is probably a terrible idea - review options
     Rails.cache.fetch "posts/#{question.id}/ad", expires_in: 60.minutes do
       ad = Image.new(600, 500)
       ad.background_color = 'white'

app/helpers/users/avatar_helper.rb

@@ -37,6 +37,7 @@ def user_auto_avatar(size, user: nil, letter: nil, color: nil)
                   "network/avatars/#{letter}+#{color}/#{size}px"
                 end
 
+    # TODO: trying to cache like this is probably a terrible idea - review options
     Rails.cache.fetch cache_key, include_community: false, expires_in: 24.hours do
       ava = Image.new(size, size)
       text_color = yiq_contrast(color, 'black', 'white')

app/models/category.rb

@@ -56,11 +56,12 @@ def self.accessible_to(user)
 
   def self.by_lowercase_name(name)
     categories = Rails.cache.fetch 'categories/by_lowercase_name' do
-      Category.all.to_h { |c| [c.name.downcase, c] }
+      Category.all.to_h { |c| [c.name.downcase, c.id] }
     end
-    categories[name]
+    Category.find_by(id: categories[name])
   end
 
+  # @todo: Do we need this method?
   def self.by_id(id)
     categories = Rails.cache.fetch 'categories/by_id' do
       Category.all.to_h { |c| [c.id, c] }

app/models/post_type.rb

@@ -8,7 +8,7 @@ class PostType < ApplicationRecord
   validates :answer_type_id, presence: true, if: :has_answers?
 
   def reactions
-    Rails.cache.fetch "post_type/#{name}/reactions" do
+    Rails.cache.fetch_collection "post_type/#{name}/reactions" do
       return [] unless has_reactions
 
       if has_only_specific_reactions
@@ -39,13 +39,13 @@ def self.[](key)
   end
 
   def self.top_level
-    Rails.cache.fetch 'network/post_types/top_level', include_community: false do
+    Rails.cache.fetch_collection 'network/post_types/top_level', include_community: false do
       where(is_top_level: true)
     end
   end
 
   def self.second_level
-    Rails.cache.fetch 'network/post_types/second_level', include_community: false do
+    Rails.cache.fetch_collection 'network/post_types/second_level', include_community: false do
       where(is_top_level: false)
     end
   end

lib/namespaced_env_cache.rb

@@ -72,6 +72,65 @@ def persistent(name, **opts, &block)
       end
     end
 
+    ##
+    # Cache an ActiveRecord collection. Supports only a basic collection of one type of object. Column selections or
+    # joins etc. will NOT be respected when the collection is read back out.
+    # @param name [String] cache key name
+    # @param value [ActiveRecord::Relation] collection to cache
+    # @param opts [Hash] options hash - any unlisted options will be passed to the underlying cache
+    # @option opts [Boolean] :include_community whether to include the community ID in the cache key
+    def write_collection(name, value, **opts)
+      types = value.map(&:class).uniq
+      if types.size > 1
+        raise TypeError, "Can't cache more than one type of object via write_collection"
+      end
+
+      data = [types[0].to_s, *value.map(&:id)]
+      namespaced = construct_ns_key(name, include_community: include_community(opts))
+      @underlying.write(namespaced, data, **opts)
+    end
+
+    ##
+    # Read an ActiveRecord collection from cache. Returns a basic collection of the records that were cached, with
+    # no selects or joins applied.
+    # @param name [String] cache key name
+    # @param opts [Hash] options hash - any unlisted options will be passed to the underlying cache
+    # @options opts [Boolean] :include_community whether to include the community ID in the cache key
+    def read_collection(name, **opts)
+      namespaced = construct_ns_key(name, include_community: include_community(opts))
+      data = @underlying.read(namespaced, **opts)
+      return nil if data.nil?
+      type = data.slice!(0)
+      begin
+        type.constantize.where(id: data)
+      rescue NameError
+        delete(name)
+        nil
+      end
+    end
+
+    ##
+    # Fetch an ActiveRecord collection from cache if it is present, otherwise cache the value returned by +block+.
+    # @param name [String] cache key name
+    # @param opts [Hash] options hash - any unlisted options will be passed to the underlying cache
+    # @option opts [Boolean] :include_community whether to include the community ID in the cache key
+    # @yieldreturn [ActiveRecord::Relation]
+    def fetch_collection(name, **opts, &block)
+      existing = if exist?(name, include_community: include_community(opts))
+                   read_collection(name, **opts)
+                 end
+      if existing.nil?
+        unless block_given?
+          raise ArgumentError, "Can't fetch collection without a block given"
+        end
+        data = block.call
+        write_collection(name, data, **opts)
+        data
+      else
+        existing
+      end
+    end
+
     # We have to statically report that we support cache versioning even though this depends on the underlying class.
     # However, this is not really a problem since all cache stores provided by activesupport support the feature and
     # we only use the redis cache (by activesupport) for QPixel.

test/lib/namespaced_env_cache_test.rb

@@ -0,0 +1,56 @@
+require 'test_helper'
+
+class NamespacedEnvCacheTest < ActiveSupport::TestCase
+  setup :new_cache_store
+
+  test 'write_collection/read_collection' do
+    collection = Post.where(user: users(:standard_user))
+    assert_nothing_raised do
+      @cache.write_collection('test_posts_cache', collection)
+    end
+    data = @cache.read_collection('test_posts_cache')
+    assert_not_nil data
+    original_ids = collection.map(&:id)
+    cached_ids = data.map(&:id)
+    cached_ids.each do |cached|
+      assert_includes original_ids, cached
+    end
+  end
+
+  test 'fetch_collection' do
+    data = assert_nothing_raised do
+      @cache.fetch_collection('test_posts_cache') do
+        Post.where(user: users(:standard_user))
+      end
+    end
+    assert_not_nil data
+  end
+
+  test 'fetch_collection on subsequent fetches' do
+    @cache.fetch_collection('test_posts_cache') do
+      Post.where(user: users(:standard_user))
+    end
+    data = assert_nothing_raised do
+      @cache.fetch_collection('test_posts_cache')
+    end
+    assert_not_nil data
+  end
+
+  test 'fetch_collection raises without block on first call' do
+    assert_raises ArgumentError do
+      @cache.fetch_collection('test_posts_cache')
+    end
+  end
+
+  test 'write_collection raises on multiple types' do
+    assert_raises TypeError do
+      @cache.write_collection('test_posts_cache', [Post.last, User.last])
+    end
+  end
+
+  private
+
+  def new_cache_store
+    @cache = QPixel::NamespacedEnvCache.new(ActiveSupport::Cache::MemoryStore.new)
+  end
+end