From aa4f0aee67d6f9802856ab4abb5a7560359854b6 Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Thu, 17 Jun 2021 15:17:49 +0800 Subject: [PATCH] Revert "PERF: Cache categories in Site model." This reverts commit 7dc0f88acd7cef0f4cc5944ef634f8ecc4886a27. --- app/models/category.rb | 5 ---- app/models/category_tag.rb | 4 --- app/models/category_tag_group.rb | 4 --- app/models/site.rb | 36 ++++-------------------- app/serializers/site_serializer.rb | 8 ++---- lib/guardian/category_guardian.rb | 5 ---- spec/serializers/site_serializer_spec.rb | 29 +++---------------- 7 files changed, 11 insertions(+), 80 deletions(-) diff --git a/app/models/category.rb b/app/models/category.rb index 93ee75536e2..6692d2d1fdd 100644 --- a/app/models/category.rb +++ b/app/models/category.rb @@ -91,7 +91,6 @@ 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 @@ -958,10 +957,6 @@ class Category < ActiveRecord::Base result.map { |row| [row.group_id, row.permission_type] } end - - def clear_site_cache - Site.clear_cache - end end # == Schema Information diff --git a/app/models/category_tag.rb b/app/models/category_tag.rb index e9cba7c189e..1e21409c31d 100644 --- a/app/models/category_tag.rb +++ b/app/models/category_tag.rb @@ -3,10 +3,6 @@ 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 ea27bc50c15..06e64ad65ff 100644 --- a/app/models/category_tag_group.rb +++ b/app/models/category_tag_group.rb @@ -3,10 +3,6 @@ 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/site.rb b/app/models/site.rb index 12b520445a4..f8c131ef1d0 100644 --- a/app/models/site.rb +++ b/app/models/site.rb @@ -28,42 +28,16 @@ class Site UserField.order(:position).all end - 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 + def categories + @categories ||= begin categories = Category - .includes(:uploaded_logo, :uploaded_background, :tags, :tag_groups, :required_tag_group) + .includes(:uploaded_logo, :uploaded_background, :tags, :tag_groups) + .secured(@guardian) .joins('LEFT JOIN topics t on t.id = categories.topic_id') .select('categories.*, t.slug topic_slug') .order(:position) - .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 + categories = categories.to_a with_children = Set.new categories.each do |c| diff --git a/app/serializers/site_serializer.rb b/app/serializers/site_serializer.rb index c7d8d5d66c2..40da58e355e 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, - :categories + :watched_words_link ) + 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,10 +190,6 @@ 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 8fc6fe28b59..4e9050f29a3 100644 --- a/lib/guardian/category_guardian.rb +++ b/lib/guardian/category_guardian.rb @@ -46,11 +46,6 @@ module CategoryGuardian nil end - def can_see_serialized_category?(category_id:, 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/serializers/site_serializer_spec.rb b/spec/serializers/site_serializer_spec.rb index 5bf58564c0c..1a53cfd11ff 100644 --- a/spec/serializers/site_serializer_spec.rb +++ b/spec/serializers/site_serializer_spec.rb @@ -10,34 +10,13 @@ describe SiteSerializer do category.custom_fields["enable_marketplace"] = true category.save_custom_fields - serialized = described_class.new(Site.new(guardian), scope: guardian, root: false).as_json - c1 = serialized[:categories].find { |c| c[:id] == category.id } - - expect(c1[:preloaded_custom_fields]).to eq(nil) + data = MultiJson.dump(described_class.new(Site.new(guardian), scope: guardian, root: false)) + expect(data).not_to include("enable_marketplace") Site.preloaded_category_custom_fields << "enable_marketplace" - serialized = described_class.new(Site.new(guardian), scope: guardian, root: false).as_json - c1 = serialized[:categories].find { |c| c[:id] == category.id } - - expect(c1[:preloaded_custom_fields]["enable_marketplace"]).to eq("t") - end - - it "includes category tags" do - tag = Fabricate(:tag) - tag_group = Fabricate(:tag_group) - tag_group_2 = Fabricate(:tag_group) - - category.tags << tag - category.tag_groups << tag_group - category.update!(required_tag_group: tag_group_2) - - serialized = described_class.new(Site.new(guardian), scope: guardian, root: false).as_json - c1 = serialized[:categories].find { |c| c[:id] == category.id } - - expect(c1[:allowed_tags]).to contain_exactly(tag.name) - expect(c1[:allowed_tag_groups]).to contain_exactly(tag_group.name) - expect(c1[:required_tag_group_name]).to eq(tag_group_2.name) + data = MultiJson.dump(described_class.new(Site.new(guardian), scope: guardian, root: false)) + expect(data).to include("enable_marketplace") end it "returns correct notification level for categories" do