From b50fab2d72c14ead753b9adaa149a6372f5d14dd Mon Sep 17 00:00:00 2001
From: Guo Xiang Tan <tgx_world@hotmail.com>
Date: Mon, 19 Nov 2018 12:53:33 +0800
Subject: [PATCH] PERF: Fix N+1 for non-staff users when tagging is enabled.

---
 app/serializers/concerns/topic_tags_mixin.rb  | 14 ++++++++++-
 app/serializers/topic_list_serializer.rb      | 19 +++++++++++++--
 .../topic_list_item_serializer_spec.rb        | 24 +++++++++++++++++--
 3 files changed, 52 insertions(+), 5 deletions(-)

diff --git a/app/serializers/concerns/topic_tags_mixin.rb b/app/serializers/concerns/topic_tags_mixin.rb
index c2dc1254d22..56cdcd79745 100644
--- a/app/serializers/concerns/topic_tags_mixin.rb
+++ b/app/serializers/concerns/topic_tags_mixin.rb
@@ -9,10 +9,22 @@ module TopicTagsMixin
 
   def tags
     # Calling method `pluck` along with `includes` causing N+1 queries
-    DiscourseTagging.filter_visible(topic.tags, scope).map(&:name)
+    tags = topic.tags.map(&:name)
+
+    if scope.is_staff?
+      tags
+    else
+      tags - (@options[:hidden_tag_names] || hidden_tag_names)
+    end
   end
 
   def topic
     object.is_a?(Topic) ? object : object.topic
   end
+
+  private
+
+  def hidden_tag_names
+    @hidden_tag_names ||= DiscourseTagging.hidden_tag_names(scope)
+  end
 end
diff --git a/app/serializers/topic_list_serializer.rb b/app/serializers/topic_list_serializer.rb
index 6ee27854ca8..8b55e042afd 100644
--- a/app/serializers/topic_list_serializer.rb
+++ b/app/serializers/topic_list_serializer.rb
@@ -9,12 +9,27 @@ class TopicListSerializer < ApplicationSerializer
              :per_page,
              :top_tags,
              :tags,
-             :shared_drafts
+             :shared_drafts,
+             :topics
 
-  has_many :topics, serializer: TopicListItemSerializer, embed: :objects
   has_many :shared_drafts, serializer: TopicListItemSerializer, embed: :objects
   has_many :tags, serializer: TagSerializer, embed: :objects
 
+  def topics
+    opts = {
+      scope: scope,
+      root: false
+    }
+
+    if SiteSetting.tagging_enabled && !scope.is_staff?
+      opts.merge!(hidden_tag_names: DiscourseTagging.hidden_tag_names(scope))
+    end
+
+    object.topics.map do |topic|
+      TopicListItemSerializer.new(topic, opts)
+    end
+  end
+
   def can_create_topic
     scope.can_create?(Topic)
   end
diff --git a/spec/serializers/topic_list_item_serializer_spec.rb b/spec/serializers/topic_list_item_serializer_spec.rb
index 319d8f02c6e..9afb792bc3f 100644
--- a/spec/serializers/topic_list_item_serializer_spec.rb
+++ b/spec/serializers/topic_list_item_serializer_spec.rb
@@ -45,6 +45,8 @@ describe TopicListItemSerializer do
   end
 
   describe 'hidden tags' do
+    let(:admin) { Fabricate(:admin) }
+    let(:user) { Fabricate(:user) }
     let(:hidden_tag) { Fabricate(:tag, name: 'hidden') }
     let(:staff_tag_group) { Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: [hidden_tag.name]) }
 
@@ -55,12 +57,30 @@ describe TopicListItemSerializer do
     end
 
     it 'returns hidden tag to staff' do
-      json = TopicListItemSerializer.new(topic, scope: Guardian.new(Fabricate(:admin)), root: false).as_json
+      json = TopicListItemSerializer.new(topic,
+        scope: Guardian.new(admin),
+        root: false
+      ).as_json
+
       expect(json[:tags]).to eq([hidden_tag.name])
     end
 
     it 'does not return hidden tag to non-staff' do
-      json = TopicListItemSerializer.new(topic, scope: Guardian.new(Fabricate(:user)), root: false).as_json
+      json = TopicListItemSerializer.new(topic,
+        scope: Guardian.new(user),
+        root: false
+      ).as_json
+
+      expect(json[:tags]).to eq([])
+    end
+
+    it 'accepts an option to remove hidden tags' do
+      json = TopicListItemSerializer.new(topic,
+        scope: Guardian.new(user),
+        hidden_tag_names: [hidden_tag.name],
+        root: false
+      ).as_json
+
       expect(json[:tags]).to eq([])
     end
   end