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

@@ -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

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
 

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

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