PERF: Cache categories in Site model take 3.

PERF: Cache categories in Site model take 3.

Previous attempt resulted in custom fields going missing in the serialized output.

This reverts commit 83a6ad32ffe75ae222028feddeca169fc5be54ac.

diff --git a/app/models/category.rb b/app/models/category.rb
index 6692d2d..86ba876 100644
--- a/app/models/category.rb
+++ b/app/models/category.rb
@@ -91,6 +91,7 @@ class Category < ActiveRecord::Base
   after_commit :trigger_category_created_event, on: :create
   after_commit :trigger_category_updated_event, on: :update
   after_commit :trigger_category_destroyed_event, on: :destroy
+  after_commit :clear_site_cache
 
   after_save_commit :index_search
 
@@ -957,6 +958,14 @@ class Category < ActiveRecord::Base
 
     result.map { |row| [row.group_id, row.permission_type] }
   end
+
+  def clear_site_cache
+    Site.clear_cache
+  end
+
+  def on_custom_fields_change
+    clear_site_cache
+  end
 end
 
 # == Schema Information
diff --git a/app/models/category_tag.rb b/app/models/category_tag.rb
index 1e21409..e9cba7c 100644
--- a/app/models/category_tag.rb
+++ b/app/models/category_tag.rb
@@ -3,6 +3,10 @@
 class CategoryTag < ActiveRecord::Base
   belongs_to :category
   belongs_to :tag
+
+  after_commit do
+    Site.clear_cache
+  end
 end
 
 # == Schema Information
diff --git a/app/models/category_tag_group.rb b/app/models/category_tag_group.rb
index 06e64ad..ea27bc5 100644
--- a/app/models/category_tag_group.rb
+++ b/app/models/category_tag_group.rb
@@ -3,6 +3,10 @@
 class CategoryTagGroup < ActiveRecord::Base
   belongs_to :category
   belongs_to :tag_group
+
+  after_commit do
+    Site.clear_cache
+  end
 end
 
 # == Schema Information
diff --git a/app/models/concerns/has_custom_fields.rb b/app/models/concerns/has_custom_fields.rb
index 31b096d..925bb8a 100644
--- a/app/models/concerns/has_custom_fields.rb
+++ b/app/models/concerns/has_custom_fields.rb
@@ -142,6 +142,11 @@ module HasCustomFields
     super
   end
 
+  def on_custom_fields_change
+    # Callback when custom fields have changed
+    # Override in model
+  end
+
   def custom_fields_preloaded?
     !!@preloaded_custom_fields
   end
@@ -197,8 +202,11 @@ module HasCustomFields
       if row_count == 0
         _custom_fields.create!(name: k, value: v)
       end
+
       custom_fields[k.to_s] = v # We normalize custom_fields as strings
     end
+
+    on_custom_fields_change
   end
 
   def save_custom_fields(force = false)
@@ -253,6 +261,7 @@ module HasCustomFields
         end
       end
 
+      on_custom_fields_change
       refresh_custom_fields_from_db
     end
   end
diff --git a/app/models/site.rb b/app/models/site.rb
index f8c131e..ced763c 100644
--- a/app/models/site.rb
+++ b/app/models/site.rb
@@ -9,7 +9,6 @@ class Site
 
   def initialize(guardian)
     @guardian = guardian
-    Category.preload_custom_fields(categories, preloaded_category_custom_fields) if preloaded_category_custom_fields.present?
   end
 
   def site_setting
@@ -28,16 +27,49 @@ class Site
     UserField.order(:position).all
   end
 
-  def categories
-    @categories ||= begin
+  CATEGORIES_CACHE_KEY = "site_categories"
+
+  def self.clear_cache
+    Discourse.cache.delete(CATEGORIES_CACHE_KEY)
+  end
+
+  def self.all_categories_cache
+    # Categories do not change often so there is no need for us to run the
+    # same query and spend time creating ActiveRecord objects for every requests.
+    #
+    # Do note that any new association added to the eager loading needs a
+    # corresponding ActiveRecord callback to clear the categories cache.
+    Discourse.cache.fetch(CATEGORIES_CACHE_KEY, expires_in: 30.minutes) do
       categories = Category
-        .includes(:uploaded_logo, :uploaded_background, :tags, :tag_groups)
-        .secured(@guardian)
+        .includes(:uploaded_logo, :uploaded_background, :tags, :tag_groups, :required_tag_group)
         .joins('LEFT JOIN topics t on t.id = categories.topic_id')
         .select('categories.*, t.slug topic_slug')
         .order(:position)
+        .to_a
+
+      if preloaded_category_custom_fields.present?
+        Category.preload_custom_fields(
+          categories,
+          preloaded_category_custom_fields
+        )
+      end
 
-      categories = categories.to_a
+      ActiveModel::ArraySerializer.new(
+        categories,
+        each_serializer: SiteCategorySerializer
+      ).as_json
+    end
+  end
+
+  def categories
+    @categories ||= begin
+      categories = []
+
+      self.class.all_categories_cache.each do |category|
+        if @guardian.can_see_serialized_category?(category_id: category[:id], read_restricted: category[:read_restricted])
+          categories << OpenStruct.new(category)
+        end
+      end
 
       with_children = Set.new
       categories.each do |c|
diff --git a/app/serializers/site_serializer.rb b/app/serializers/site_serializer.rb
index 40da58e..c7d8d5d 100644
--- a/app/serializers/site_serializer.rb
+++ b/app/serializers/site_serializer.rb
@@ -30,10 +30,10 @@ class SiteSerializer < ApplicationSerializer
     :shared_drafts_category_id,
     :custom_emoji_translation,
     :watched_words_replace,
-    :watched_words_link
+    :watched_words_link,
+    :categories
   )
 
-  has_many :categories, serializer: SiteCategorySerializer, embed: :objects
   has_many :archetypes, embed: :objects, serializer: ArchetypeSerializer
   has_many :user_fields, embed: :objects, serializer: UserFieldSerializer
   has_many :auth_providers, embed: :objects, serializer: AuthProviderSerializer
@@ -190,6 +190,10 @@ class SiteSerializer < ApplicationSerializer
     WordWatcher.word_matcher_regexps(:link)
   end
 
+  def categories
+    object.categories.map { |c| c.to_h }
+  end
+
   private
 
   def ordered_flags(flags)
diff --git a/lib/guardian/category_guardian.rb b/lib/guardian/category_guardian.rb
index 4e9050f..ef84410 100644
--- a/lib/guardian/category_guardian.rb
+++ b/lib/guardian/category_guardian.rb
@@ -46,6 +46,14 @@ module CategoryGuardian
     nil
   end
 
+  def can_see_serialized_category?(category_id:, read_restricted: true)
+    # Guard to ensure only a boolean is passed in
+    read_restricted = true unless !!read_restricted == read_restricted
+
+    return true if !read_restricted
+    secure_category_ids.include?(category_id)
+  end
+
   def can_see_category?(category)
     return false unless category
     return true if is_admin?
diff --git a/spec/models/site_spec.rb b/spec/models/site_spec.rb
index 78b3fae..8ea6e9b 100644
--- a/spec/models/site_spec.rb
+++ b/spec/models/site_spec.rb
@@ -58,30 +58,85 @@ describe Site do
     expect(Site.new(guardian).categories.last.notification_level).to eq(1)
   end
 
-  it "omits categories users can not write to from the category list" do
-    category = Fabricate(:category)
-    user = Fabricate(:user)
+  describe '#categories' do
+    fab!(:category) { Fabricate(:category) }
+    fab!(:user) { Fabricate(:user) }
+    fab!(:guardian) { Guardian.new(user) }
+
+    after do
+      Site.clear_cache
+    end
+
+    it "omits read restricted categories" do
+      expect(Site.new(guardian).categories.map(&:id)).to contain_exactly(
+        SiteSetting.uncategorized_category_id, category.id
+      )
+
+      category.update!(read_restricted: true)
+
+      expect(Site.new(guardian).categories.map(&:id)).to contain_exactly(
+        SiteSetting.uncategorized_category_id
+      )
+    end
+
+    it "includes categories that a user's group can see" do
+      group = Fabricate(:group)
+      category.update!(read_restricted: true)
+      category.groups << group
+
+      expect(Site.new(guardian).categories.map(&:id)).to contain_exactly(
+        SiteSetting.uncategorized_category_id
+      )
+
+      group.add(user)
+
+      expect(Site.new(Guardian.new(user)).categories.map(&:id)).to contain_exactly(
+        SiteSetting.uncategorized_category_id, category.id
+      )
+    end
+
+    it "omits categories users can not write to from the category list" do
+      expect(Site.new(guardian).categories.count).to eq(2)
+
+      category.set_permissions(everyone: :create_post)
+      category.save!
+
+      guardian = Guardian.new(user)
+
+      expect(Site.new(guardian)

[... diff too long, it was truncated ...]

GitHub sha: 0e4b8c5318569ef7e7a111563709699e3b9ce219

This commit appears in #13491 which was approved by SamSaffron. It was merged by tgxworld.