From 7ec6e6b3d08de5aa41f5ca28a4621851fb8e831f Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Mon, 30 Jan 2023 08:53:17 +0800 Subject: [PATCH] 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 | 12 ++-- app/models/category.rb | 4 ++ spec/requests/tags_controller_spec.rb | 93 ++++++++++++++++++++++++++- 3 files changed, 103 insertions(+), 6 deletions(-) diff --git a/app/controllers/tags_controller.rb b/app/controllers/tags_controller.rb index aa5037e2ea1..2e939db5afd 100644 --- a/app/controllers/tags_controller.rb +++ b/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 - .where("id IN (SELECT category_id FROM category_tags)") - .where("id IN (?)", guardian.allowed_category_ids) - .includes(:tags) + Category.where( + "id IN (SELECT category_id FROM category_tags WHERE category_id IN (?))", + guardian.allowed_category_ids, + ).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 diff --git a/app/models/category.rb b/app/models/category.rb index a049414df0e..c5fd3d7abbd 100644 --- a/app/models/category.rb +++ b/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 diff --git a/spec/requests/tags_controller_spec.rb b/spec/requests/tags_controller_spec.rb index ac85694fc7c..e3622bdaeae 100644 --- a/spec/requests/tags_controller_spec.rb +++ b/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