From 1e8a1a0d24b3407f26302337de4386d044ad8282 Mon Sep 17 00:00:00 2001
From: Alan Guo Xiang Tan <gxtan1990@gmail.com>
Date: Tue, 17 Jan 2023 15:50:21 +0800
Subject: [PATCH] PERF: N+1 queries when viewing tags (#19891)

When the `tags_listed_by_group` site setting is enabled, we were seeing
the N+1 queries problem when multiple `TagGroup` records are listed.
This commit fixes that by ensuring that we are not filtering through the
`tags` association after the association has been eager loaded.
---
 app/controllers/tags_controller.rb    |  4 +--
 app/models/tag_group.rb               |  4 +++
 spec/requests/tags_controller_spec.rb | 46 +++++++++++++++++++++++++--
 3 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/app/controllers/tags_controller.rb b/app/controllers/tags_controller.rb
index 9b27b69e6a4..5a8a8137b78 100644
--- a/app/controllers/tags_controller.rb
+++ b/app/controllers/tags_controller.rb
@@ -46,14 +46,14 @@ class TagsController < ::ApplicationController
         TagGroup
           .visible(guardian)
           .order("name ASC")
-          .includes(:tags)
+          .includes(:none_synonym_tags)
           .map do |tag_group|
             {
               id: tag_group.id,
               name: tag_group.name,
               tags:
                 self.class.tag_counts_json(
-                  tag_group.tags.where(target_tag_id: nil),
+                  tag_group.none_synonym_tags,
                   show_pm_tags: guardian.can_tag_pms?,
                 ),
             }
diff --git a/app/models/tag_group.rb b/app/models/tag_group.rb
index 54ee33bc1cc..802ab651ac2 100644
--- a/app/models/tag_group.rb
+++ b/app/models/tag_group.rb
@@ -5,6 +5,10 @@ class TagGroup < ActiveRecord::Base
 
   has_many :tag_group_memberships, dependent: :destroy
   has_many :tags, through: :tag_group_memberships
+  has_many :none_synonym_tags,
+           -> { where(target_tag_id: nil) },
+           through: :tag_group_memberships,
+           source: "tag"
   has_many :category_tag_groups, dependent: :destroy
   has_many :category_required_tag_groups, dependent: :destroy
   has_many :categories, through: :category_tag_groups
diff --git a/spec/requests/tags_controller_spec.rb b/spec/requests/tags_controller_spec.rb
index ba044221206..bd4e0908f5b 100644
--- a/spec/requests/tags_controller_spec.rb
+++ b/spec/requests/tags_controller_spec.rb
@@ -80,9 +80,10 @@ RSpec.describe TagsController do
 
       it "works for tags in groups" do
         tag_group = Fabricate(:tag_group, tags: [test_tag, topic_tag, synonym])
-        get "/tags.json"
-        expect(response.status).to eq(200)
 
+        get "/tags.json"
+
+        expect(response.status).to eq(200)
         tags = response.parsed_body["tags"]
         expect(tags.length).to eq(0)
         group = response.parsed_body.dig("extras", "tag_groups")&.first
@@ -90,6 +91,47 @@ RSpec.describe TagsController do
         expect(group["tags"].length).to eq(2)
         expect(group["tags"].map { |t| t["id"] }).to contain_exactly(test_tag.name, topic_tag.name)
       end
+
+      it "does not result in N+1 queries with multiple tag_groups" do
+        tag_group = Fabricate(:tag_group, tags: [test_tag, topic_tag, synonym])
+
+        # warm up
+        get "/tags.json"
+        expect(response.status).to eq(200)
+
+        initial_sql_queries_count =
+          track_sql_queries do
+            get "/tags.json"
+
+            expect(response.status).to eq(200)
+
+            tag_groups = response.parsed_body.dig("extras", "tag_groups")
+
+            expect(tag_groups.length).to eq(1)
+            expect(tag_groups.map { |tag_group| tag_group["name"] }).to contain_exactly(
+              tag_group.name,
+            )
+          end.length
+
+        tag_group2 = Fabricate(:tag_group, tags: [topic_tag])
+
+        new_sql_queries_count =
+          track_sql_queries do
+            get "/tags.json"
+
+            expect(response.status).to eq(200)
+
+            tag_groups = response.parsed_body.dig("extras", "tag_groups")
+
+            expect(tag_groups.length).to eq(2)
+            expect(tag_groups.map { |tag_group| tag_group["name"] }).to contain_exactly(
+              tag_group.name,
+              tag_group2.name,
+            )
+          end.length
+
+        expect(new_sql_queries_count).to be <= initial_sql_queries_count
+      end
     end
 
     context "with tags_listed_by_group disabled" do