PERF: N+1 queries on `/tags` with multiple categories tags (#19906)

When the `tags_listed_by_group` site setting is disable, we were seeing
the N+1 queries problem when multiple `CategoryTag` records are listed.
This commit fixes that by ensuring that we are not filtering through the
category `tags` association after the association has been eager loaded.

app/controllers/tags_controller.rb

@@ -63,20 +63,22 @@ class TagsController < ::ApplicationController
       unrestricted_tags = DiscourseTagging.filter_visible(tags.where(target_tag_id: nil), guardian)
 
       categories =
-        Category
+        Category.where(
-          .where("id IN (SELECT category_id FROM category_tags)")
+          "id IN (SELECT category_id FROM category_tags WHERE category_id IN (?))",
-          .where("id IN (?)", guardian.allowed_category_ids)
+          guardian.allowed_category_ids,
-          .includes(:tags)
+        ).includes(:none_synonym_tags)
 
       category_tag_counts =
         categories
           .map do |c|
             category_tags =
               self.class.tag_counts_json(
-                DiscourseTagging.filter_visible(c.tags.where(target_tag_id: nil), guardian),
+                DiscourseTagging.filter_visible(c.none_synonym_tags, guardian),
                 guardian,
               )
+
             next if category_tags.empty?
+
             { id: c.id, tags: category_tags }
           end
           .compact

app/models/category.rb

@@ -125,6 +125,10 @@ class Category < ActiveRecord::Base
 
   has_many :category_tags, dependent: :destroy
   has_many :tags, through: :category_tags
+  has_many :none_synonym_tags,
+           -> { where(target_tag_id: nil) },
+           through: :category_tags,
+           source: :tag
   has_many :category_tag_groups, dependent: :destroy
   has_many :tag_groups, through: :category_tag_groups
 

spec/requests/tags_controller_spec.rb

@@ -11,7 +11,7 @@ RSpec.describe TagsController do
   before { SiteSetting.tagging_enabled = true }
 
   describe "#index" do
-    fab!(:test_tag) { Fabricate(:tag, name: "test") }
+    fab!(:test_tag) { Fabricate(:tag, name: "test", description: "some description") }
     fab!(:topic_tag) do
       Fabricate(:tag, name: "topic-test", public_topic_count: 1, staff_topic_count: 1)
     end
@@ -162,6 +162,97 @@ RSpec.describe TagsController do
     context "with tags_listed_by_group disabled" do
       before { SiteSetting.tags_listed_by_group = false }
       include_examples "retrieves the right tags"
+
+      it "returns the right tags and categories tags for admin user" do
+        category.update!(tags: [test_tag])
+
+        sign_in(admin)
+
+        get "/tags.json"
+
+        expect(response.status).to eq(200)
+
+        tags = response.parsed_body["tags"]
+
+        expect(tags.length).to eq(2)
+
+        expect(tags[0]["name"]).to eq(test_tag.name)
+        expect(tags[0]["text"]).to eq(test_tag.name)
+        expect(tags[0]["description"]).to eq(test_tag.description)
+        expect(tags[0]["count"]).to eq(0)
+        expect(tags[0]["pm_count"]).to eq(0)
+        expect(tags[0]["target_tag"]).to eq(nil)
+
+        expect(tags[1]["name"]).to eq(topic_tag.name)
+
+        categories = response.parsed_body["extras"]["categories"]
+
+        expect(categories[0]["id"]).to eq(category.id)
+        expect(categories[0]["tags"].length).to eq(1)
+        expect(categories[0]["tags"][0]["name"]).to eq(test_tag.name)
+        expect(categories[0]["tags"][0]["text"]).to eq(test_tag.name)
+        expect(categories[0]["tags"][0]["description"]).to eq(test_tag.description)
+        expect(categories[0]["tags"][0]["count"]).to eq(0)
+        expect(categories[0]["tags"][0]["pm_count"]).to eq(0)
+        expect(categories[0]["tags"][0]["target_tag"]).to eq(nil)
+      end
+
+      it "does not result in N+1 queries when there are multiple categories configured with tags for an admin user" do
+        category.update!(tags: [test_tag])
+
+        sign_in(admin)
+
+        # warm up
+        get "/tags.json"
+        expect(response.status).to eq(200)
+
+        tags = response.parsed_body["tags"]
+
+        expect(tags.length).to eq(2)
+        expect(tags.map { |tag| tag["name"] }).to eq([test_tag.name, topic_tag.name])
+
+        initial_sql_queries_count =
+          track_sql_queries do
+            get "/tags.json"
+
+            expect(response.status).to eq(200)
+
+            tags = response.parsed_body["tags"]
+
+            expect(tags.length).to eq(2)
+            expect(tags.map { |tag| tag["name"] }).to eq([test_tag.name, topic_tag.name])
+
+            categories = response.parsed_body["extras"]["categories"]
+
+            expect(categories.length).to eq(1)
+            expect(categories[0]["id"]).to eq(category.id)
+            expect(categories[0]["tags"].map { |tag| tag["name"] }).to eq([test_tag.name])
+          end.length
+
+        category2 = Fabricate(:category, tags: [topic_tag])
+
+        new_sql_queries_count =
+          track_sql_queries do
+            get "/tags.json"
+
+            expect(response.status).to eq(200)
+
+            tags = response.parsed_body["tags"]
+
+            expect(tags.length).to eq(2)
+            expect(tags.map { |tag| tag["name"] }).to eq([test_tag.name, topic_tag.name])
+
+            categories = response.parsed_body["extras"]["categories"]
+
+            expect(categories.length).to eq(2)
+            expect(categories[0]["id"]).to eq(category.id)
+            expect(categories[0]["tags"].map { |tag| tag["name"] }).to eq([test_tag.name])
+            expect(categories[1]["id"]).to eq(category2.id)
+            expect(categories[1]["tags"].map { |tag| tag["name"] }).to eq([topic_tag.name])
+          end.length
+
+        expect(new_sql_queries_count).to eq(initial_sql_queries_count)
+      end
     end
 
     context "with hidden tags" do