From 2225c03455a00b3e87a0682936432e37f1561a44 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Tue, 6 Aug 2024 09:59:49 +1000 Subject: [PATCH] FIX: Cache all flags multisite-safe (#28204) This fixes an N1 in topics when loading all flags and also makes the cache multisite-safe, followup to fb7cc2d3754d23e4c30fa2794eb7ac84ffdc667e --- app/models/flag.rb | 3 +- app/models/post_action_type.rb | 75 ++++++++++++++++++++++++++-------- spec/models/flag_spec.rb | 2 + spec/multisite/flags_spec.rb | 24 +++++++++++ 4 files changed, 87 insertions(+), 17 deletions(-) create mode 100644 spec/multisite/flags_spec.rb diff --git a/app/models/flag.rb b/app/models/flag.rb index 91538ea79cf..de09d0043b1 100644 --- a/app/models/flag.rb +++ b/app/models/flag.rb @@ -27,7 +27,8 @@ class Flag < ActiveRecord::Base end def self.reset_flag_settings! - # Flags are memoized for better performance. After the update, we need to reload them in all processes. + # Flags are cached in Redis for better performance. After the update, + # we need to reload them in all processes. PostActionType.reload_types MessageBus.publish("/reload_post_action_types", {}) end diff --git a/app/models/post_action_type.rb b/app/models/post_action_type.rb index 6cf93f946af..672ce2776dc 100644 --- a/app/models/post_action_type.rb +++ b/app/models/post_action_type.rb @@ -1,6 +1,22 @@ # frozen_string_literal: true class PostActionType < ActiveRecord::Base + POST_ACTION_TYPE_ALL_FLAGS_KEY = "post_action_type_all_flags" + POST_ACTION_TYPE_PUBLIC_TYPE_IDS_KEY = "post_action_public_type_ids" + ATTRIBUTE_NAMES = %i[ + id + name + name_key + description + notify_type + auto_action_type + require_message + applies_to + position + enabled + score_type + ] + after_save :expire_cache after_destroy :expire_cache @@ -33,12 +49,15 @@ class PostActionType < ActiveRecord::Base def expire_cache Discourse.redis.keys("post_action_types_*").each { |key| Discourse.redis.del(key) } Discourse.redis.keys("post_action_flag_types_*").each { |key| Discourse.redis.del(key) } + Discourse.cache.delete(POST_ACTION_TYPE_ALL_FLAGS_KEY) + Discourse.cache.delete(POST_ACTION_TYPE_PUBLIC_TYPE_IDS_KEY) end def reload_types @flag_settings = FlagSettings.new - ReviewableScore.reload_types PostActionType.new.expire_cache + PostActionType.expire_cache + ReviewableScore.reload_types end def overridden_by_plugin_or_skipped_db? @@ -46,12 +65,20 @@ class PostActionType < ActiveRecord::Base end def all_flags - Flag.unscoped.order(:position).all + Discourse + .cache + .fetch(PostActionType::POST_ACTION_TYPE_ALL_FLAGS_KEY) do + Flag + .unscoped + .order(:position) + .pluck(ATTRIBUTE_NAMES) + .map { |attributes| ATTRIBUTE_NAMES.zip(attributes).to_h } + end end def auto_action_flag_types return flag_settings.auto_action_types if overridden_by_plugin_or_skipped_db? - flag_enum(all_flags.select(&:auto_action_type)) + flag_enum(all_flags.select { |flag| flag[:auto_action_type] }) end def public_types @@ -59,12 +86,14 @@ class PostActionType < ActiveRecord::Base end def public_type_ids - @public_type_ids ||= public_types.values + Discourse + .cache + .fetch(PostActionType::POST_ACTION_TYPE_PUBLIC_TYPE_IDS_KEY) { public_types.values } end def flag_types_without_additional_message return flag_settings.without_additional_message_types if overridden_by_plugin_or_skipped_db? - flag_enum(all_flags.reject(&:require_message)) + flag_enum(all_flags.reject { |flag| flag[:require_message] }) end def flag_types @@ -72,7 +101,7 @@ class PostActionType < ActiveRecord::Base # Once replace_flag API is fully deprecated, then we can drop respond_to. It is needed right now for migration to be evaluated. # TODO (krisk) - flag_enum(all_flags.reject { |flag| flag.respond_to?(:score_type) && flag.score_type }) + flag_enum(all_flags.reject { |flag| flag[:score_type] }) end def score_types @@ -80,7 +109,7 @@ class PostActionType < ActiveRecord::Base # Once replace_flag API is fully deprecated, then we can drop respond_to. It is needed right now for migration to be evaluated. # TODO (krisk) - flag_enum(all_flags.filter { |flag| flag.respond_to?(:score_type) && flag.score_type }) + flag_enum(all_flags.filter { |flag| flag[:score_type] }) end # flags resulting in mod notifications @@ -90,40 +119,49 @@ class PostActionType < ActiveRecord::Base def notify_flag_types return flag_settings.notify_types if overridden_by_plugin_or_skipped_db? - flag_enum(all_flags.select(&:notify_type)) + flag_enum(all_flags.select { |flag| flag[:notify_type] }) end def topic_flag_types if overridden_by_plugin_or_skipped_db? flag_settings.topic_flag_types else - flag_enum(all_flags.select { |flag| flag.applies_to?("Topic") }) + flag_enum(all_flags.select { |flag| flag[:applies_to].include?("Topic") }) end end def disabled_flag_types - flag_enum(all_flags.reject(&:enabled)) + flag_enum(all_flags.reject { |flag| flag[:enabled] }) end def enabled_flag_types - flag_enum(all_flags.filter(&:enabled)) + flag_enum(all_flags.filter { |flag| flag[:enabled] }) end def additional_message_types return flag_settings.with_additional_message if overridden_by_plugin_or_skipped_db? - flag_enum(all_flags.select(&:require_message)) + flag_enum(all_flags.select { |flag| flag[:require_message] }) end def names - all_flags.pluck(:id, :name).to_h + all_flags.reduce({}) do |acc, f| + acc[f[:id]] = f[:name] + acc + end end def descriptions - all_flags.pluck(:id, :description).to_h + all_flags.reduce({}) do |acc, f| + acc[f[:id]] = f[:description] + acc + end end def applies_to - all_flags.pluck(:id, :applies_to).to_h + all_flags.reduce({}) do |acc, f| + acc[f[:id]] = f[:applies_to] + acc + end end def is_flag?(sym) @@ -133,7 +171,12 @@ class PostActionType < ActiveRecord::Base private def flag_enum(scope) - Enum.new(scope.map { |flag| [flag.name_key.to_sym, flag.id] }.to_h) + Enum.new( + scope.reduce({}) do |acc, f| + acc[f[:name_key].to_sym] = f[:id] + acc + end, + ) end end diff --git a/spec/models/flag_spec.rb b/spec/models/flag_spec.rb index 884a3b13688..da86820e1e5 100644 --- a/spec/models/flag_spec.rb +++ b/spec/models/flag_spec.rb @@ -3,6 +3,8 @@ RSpec.describe Flag, type: :model do after(:each) { Flag.reset_flag_settings! } + use_redis_snapshotting + it "has id lower than 1000 for system flags" do flag = Fabricate(:flag, id: 1) expect(flag.system?).to be true diff --git a/spec/multisite/flags_spec.rb b/spec/multisite/flags_spec.rb new file mode 100644 index 00000000000..7d36760dd71 --- /dev/null +++ b/spec/multisite/flags_spec.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +RSpec.describe "Custom flags in multisite", type: :multisite do + describe "PostACtionType#all_flags" do + use_redis_snapshotting + + it "does not share flag definitions between sites" do + flag_1 = Flag.create!(name: "test flag 1", position: 99, applies_to: ["Post"]) + + test_multisite_connection("second") do + flag_2 = Flag.create!(name: "test flag 2", position: 99, applies_to: ["Post"]) + PostActionType.expire_cache + expect(PostActionType.all_flags.last).to eq( + flag_2.attributes.except("created_at", "updated_at").transform_keys(&:to_sym), + ) + end + + PostActionType.expire_cache + expect(PostActionType.all_flags.last).to eq( + flag_1.attributes.except("created_at", "updated_at").transform_keys(&:to_sym), + ) + end + end +end