From 83a6ad32ffe75ae222028feddeca169fc5be54ac Mon Sep 17 00:00:00 2001 From: Kane York Date: Mon, 21 Jun 2021 12:06:08 -0700 Subject: [PATCH] Revert "PERF: Cache categories in Site model take 2." This reverts commit 06fa1efd3d8a935a5ae3e7876dbbaa3193316d7c. Breakage in solved plugin --- 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 | 8 --- spec/models/site_spec.rb | 71 ++++++------------------ spec/serializers/site_serializer_spec.rb | 29 ++-------- 8 files changed, 29 insertions(+), 136 deletions(-) diff --git a/app/models/category.rb b/app/models/category.rb index 93ee75536e..6692d2d1fd 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 e9cba7c189..1e21409c31 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 ea27bc50c1..06e64ad65f 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 cf59a1f156..f8c131ef1d 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 c7d8d5d66c..40da58e355 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 ef8441071b..4e9050f29a 100644 --- a/lib/guardian/category_guardian.rb +++ b/lib/guardian/category_guardian.rb @@ -46,14 +46,6 @@ 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 637c812de8..78b3faec63 100644 --- a/spec/models/site_spec.rb +++ b/spec/models/site_spec.rb @@ -58,65 +58,30 @@ describe Site do expect(Site.new(guardian).categories.last.notification_level).to eq(1) end - describe '#categories' do - fab!(:category) { Fabricate(:category) } - fab!(:user) { Fabricate(:user) } - fab!(:guardian) { Guardian.new(user) } + it "omits categories users can not write to from the category list" do + category = Fabricate(:category) + user = Fabricate(:user) - after do - Site.clear_cache - end + expect(Site.new(Guardian.new(user)).categories.count).to eq(2) - it "omits read restricted categories" do - expect(Site.new(guardian).categories.map(&:id)).to contain_exactly( - SiteSetting.uncategorized_category_id, category.id - ) + category.set_permissions(everyone: :create_post) + category.save - category.update!(read_restricted: true) + guardian = Guardian.new(user) - expect(Site.new(guardian).categories.map(&:id)).to contain_exactly( - SiteSetting.uncategorized_category_id - ) - end + expect(Site.new(guardian) + .categories + .keep_if { |c| c.name == category.name } + .first + .permission) + .not_to eq(CategoryGroup.permission_types[:full]) - it "includes categories that a user's group can see" do - group = Fabricate(:group) - category.update!(read_restricted: true) - category.groups << group + # If a parent category is not visible, the child categories should not be returned + category.set_permissions(staff: :full) + category.save - 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) - .categories - .keep_if { |c| c.name == category.name } - .first - .permission) - .not_to eq(CategoryGroup.permission_types[:full]) - - # If a parent category is not visible, the child categories should not be returned - category.set_permissions(staff: :full) - category.save! - - sub_category = Fabricate(:category, parent_category_id: category.id) - expect(Site.new(guardian).categories).not_to include(sub_category) - end + sub_category = Fabricate(:category, parent_category_id: category.id) + expect(Site.new(guardian).categories).not_to include(sub_category) end it "omits groups user can not see" do diff --git a/spec/serializers/site_serializer_spec.rb b/spec/serializers/site_serializer_spec.rb index 5bf58564c0..1a53cfd11f 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