From fb7cc2d3754d23e4c30fa2794eb7ac84ffdc667e Mon Sep 17 00:00:00 2001 From: Krzysztof Kotlarek Date: Mon, 22 Jul 2024 17:35:49 +1000 Subject: [PATCH] FIX: stop memoize PostActionTypes (#28005) Memoizing all_flags on PostActionType was a mistake. This commit brings back the cache on the serialize level. --- app/models/post_action_type.rb | 18 +++++++++++++++--- app/serializers/post_serializer.rb | 23 +++++++++++++++++++++-- app/serializers/site_serializer.rb | 12 ++++++++---- lib/guardian/post_guardian.rb | 8 ++++++-- lib/topic_view.rb | 16 ++++++++++++++++ spec/models/post_action_type_spec.rb | 18 ++++++++++++++++++ 6 files changed, 84 insertions(+), 11 deletions(-) diff --git a/app/models/post_action_type.rb b/app/models/post_action_type.rb index 8ade7c2977b..6cf93f946af 100644 --- a/app/models/post_action_type.rb +++ b/app/models/post_action_type.rb @@ -1,8 +1,16 @@ # frozen_string_literal: true class PostActionType < ActiveRecord::Base + after_save :expire_cache + after_destroy :expire_cache + include AnonCacheInvalidator + def expire_cache + ApplicationSerializer.expire_cache_fragment!(/\Apost_action_types_/) + ApplicationSerializer.expire_cache_fragment!(/\Apost_action_flag_types_/) + end + class << self attr_reader :flag_settings @@ -13,7 +21,6 @@ class PostActionType < ActiveRecord::Base def replace_flag_settings(settings) Discourse.deprecate("Flags should not be replaced. Insert custom flags as database records.") @flag_settings = settings || FlagSettings.new - @all_flags = nil end def types @@ -23,10 +30,15 @@ class PostActionType < ActiveRecord::Base Enum.new(like: 2).merge(flag_types) end + 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) } + end + def reload_types - @all_flags = nil @flag_settings = FlagSettings.new ReviewableScore.reload_types + PostActionType.new.expire_cache end def overridden_by_plugin_or_skipped_db? @@ -34,7 +46,7 @@ class PostActionType < ActiveRecord::Base end def all_flags - @all_flags ||= Flag.unscoped.order(:position).all + Flag.unscoped.order(:position).all end def auto_action_flag_types diff --git a/app/serializers/post_serializer.rb b/app/serializers/post_serializer.rb index 1c575b1dc60..0929031a920 100644 --- a/app/serializers/post_serializer.rb +++ b/app/serializers/post_serializer.rb @@ -290,7 +290,10 @@ class PostSerializer < BasicPostSerializer result = [] can_see_post = scope.can_see_post?(object) - PostActionType.types.each do |sym, id| + public_flag_types = + (@topic_view.present? ? @topic_view.public_flag_types : PostActionType.public_types) + + (@topic_view.present? ? @topic_view.flag_types : PostActionType.types).each do |sym, id| count_col = "#{sym}_count".to_sym count = object.public_send(count_col) if object.respond_to?(count_col) @@ -301,6 +304,22 @@ class PostSerializer < BasicPostSerializer sym, opts: { taken_actions: actions, + notify_flag_types: + ( + if @topic_view.present? + @topic_view.notify_flag_types + else + PostActionType.notify_flag_types + end + ), + additional_message_types: + ( + if @topic_view.present? + @topic_view.additional_message_types + else + PostActionType.additional_message_types + end + ), }, can_see_post: can_see_post, ) @@ -327,7 +346,7 @@ class PostSerializer < BasicPostSerializer end # only show public data - unless scope.is_staff? || PostActionType.public_types.values.include?(id) + unless scope.is_staff? || public_flag_types.values.include?(id) summary[:count] = summary[:acted] ? 1 : 0 end diff --git a/app/serializers/site_serializer.rb b/app/serializers/site_serializer.rb index 7b741a2b299..c25a51e794d 100644 --- a/app/serializers/site_serializer.rb +++ b/app/serializers/site_serializer.rb @@ -111,13 +111,17 @@ class SiteSerializer < ApplicationSerializer end def post_action_types - types = ordered_flags(PostActionType.types.values) - ActiveModel::ArraySerializer.new(types).as_json + cache_fragment("post_action_types_#{I18n.locale}") do + types = ordered_flags(PostActionType.types.values) + ActiveModel::ArraySerializer.new(types).as_json + end end def topic_flag_types - types = ordered_flags(PostActionType.topic_flag_types.values) - ActiveModel::ArraySerializer.new(types, each_serializer: TopicFlagTypeSerializer).as_json + cache_fragment("post_action_flag_types_#{I18n.locale}") do + types = ordered_flags(PostActionType.topic_flag_types.values) + ActiveModel::ArraySerializer.new(types, each_serializer: TopicFlagTypeSerializer).as_json + end end def default_archetype diff --git a/lib/guardian/post_guardian.rb b/lib/guardian/post_guardian.rb index eec02252639..73365e66302 100644 --- a/lib/guardian/post_guardian.rb +++ b/lib/guardian/post_guardian.rb @@ -38,8 +38,12 @@ module PostGuardian taken = opts[:taken_actions].try(:keys).to_a is_flag = - PostActionType.notify_flag_types[action_key] || - PostActionType.additional_message_types[action_key] + if (opts[:notify_flag_types] && opts[:additional_message_types]) + opts[:notify_flag_types][action_key] || opts[:additional_message_types][action_key] + else + PostActionType.notify_flag_types[action_key] || + PostActionType.additional_message_types[action_key] + end already_taken_this_action = taken.any? && taken.include?(PostActionType.types[action_key]) already_did_flagging = taken.any? && (taken & PostActionType.notify_flag_types.values).any? diff --git a/lib/topic_view.rb b/lib/topic_view.rb index fd82742d1ed..4123bb48dcf 100644 --- a/lib/topic_view.rb +++ b/lib/topic_view.rb @@ -623,6 +623,22 @@ class TopicView @pm_params ||= TopicQuery.new(@user).get_pm_params(topic) end + def flag_types + @flag_types ||= PostActionType.types + end + + def public_flag_types + @public_flag_types ||= PostActionType.public_types + end + + def notify_flag_types + @notify_flag_types ||= PostActionType.notify_flag_types + end + + def additional_message_types + @additional_message_types ||= PostActionType.additional_message_types + end + def suggested_topics if @include_suggested @suggested_topics ||= diff --git a/spec/models/post_action_type_spec.rb b/spec/models/post_action_type_spec.rb index 26ecc2552d5..201a83ff503 100644 --- a/spec/models/post_action_type_spec.rb +++ b/spec/models/post_action_type_spec.rb @@ -1,6 +1,24 @@ # frozen_string_literal: true RSpec.describe PostActionType do + describe "Callbacks" do + describe "#expiry_cache" do + it "should clear the cache on save" do + cache = ApplicationSerializer.fragment_cache + + cache["post_action_types_#{I18n.locale}"] = "test" + cache["post_action_flag_types_#{I18n.locale}"] = "test2" + + PostActionType.new(name_key: "some_key").save! + + expect(cache["post_action_types_#{I18n.locale}"]).to eq(nil) + expect(cache["post_action_flag_types_#{I18n.locale}"]).to eq(nil) + ensure + ApplicationSerializer.fragment_cache.clear + end + end + end + describe "#types" do context "when verifying enum sequence" do before { @types = PostActionType.types }