From 4eee1320b0d25cb6e99a60c494f0a56b090196e7 Mon Sep 17 00:00:00 2001
From: Alan Guo Xiang Tan <gxtan1990@gmail.com>
Date: Thu, 22 Dec 2022 05:30:29 +0800
Subject: [PATCH] PERF: Fix N+1 queries when serializing topic posters (#19545)

At the time of writing, this is how the `TopicPosterSerializer` looks
like:

```
class TopicPosterSerializer < ApplicationSerializer
  attributes :extras, :description

  has_one :user, serializer: PosterSerializer
  has_one :primary_group, serializer: PrimaryGroupSerializer
  has_one :flair_group, serializer: FlairGroupSerializer
end
```

Within `PosterSerializer`, the `primary_group` and `flair_group`
association is requested on the `user` object. However, the associations
have not been loaded on the `user` object at this point leading to the
N+1 queries problem. One may wonder
why the associations have not been loaded when the `TopicPosterSerializer`
has `has_one :primary_group` and `has_one :flair_group`. It turns out that `TopicPoster`
is just a struct containing the  `user`, `primary_group` and
`flair_group` objects. The `primary_group` and `flair_group`
ActiveRecord objects are loaded seperately in `UserLookup` and not preloaded when querying for
the users. This is done for performance reason so that we are able to
load the `primary_group` and `flair_group` records in a single query
without duplication.
---
 lib/user_lookup.rb                    | 13 +++++++--
 spec/requests/list_controller_spec.rb | 40 +++++++++++++++++++++++++++
 2 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/lib/user_lookup.rb b/lib/user_lookup.rb
index 590c6e27545..92ca3806bd2 100644
--- a/lib/user_lookup.rb
+++ b/lib/user_lookup.rb
@@ -21,7 +21,9 @@ class UserLookup
   def primary_groups
     @primary_groups ||= users.values.each_with_object({}) do |user, hash|
       if user.primary_group_id
-        hash[user.id] = groups[user.primary_group_id]
+        group = groups[user.primary_group_id]
+        set_user_group_preload(user, group, :primary_group)
+        hash[user.id] = group
       end
     end
   end
@@ -29,13 +31,20 @@ class UserLookup
   def flair_groups
     @flair_groups ||= users.values.each_with_object({}) do |user, hash|
       if user.flair_group_id
-        hash[user.id] = groups[user.flair_group_id]
+        group = groups[user.flair_group_id]
+        set_user_group_preload(user, group, :flair_group)
+        hash[user.id] = group
       end
     end
   end
 
   private
 
+  def set_user_group_preload(user, group, group_association_name)
+    association = user.association(group_association_name)
+    association.target = group
+  end
+
   def users
     @users ||= User
       .where(id: @user_ids)
diff --git a/spec/requests/list_controller_spec.rb b/spec/requests/list_controller_spec.rb
index a23955f681a..ee7ddbdfd24 100644
--- a/spec/requests/list_controller_spec.rb
+++ b/spec/requests/list_controller_spec.rb
@@ -99,6 +99,46 @@ RSpec.describe ListController do
       expect(first_item.css('[itemprop="position"]')[0]['content']).to eq('1')
       expect(first_item.css('[itemprop="url"]')[0]['href']).to eq(topic.url)
     end
+
+    it 'does not N+1 queries when topic featured users have different primary groups' do
+      user.update!(primary_group: group)
+
+      # warm up
+      get "/latest.json"
+      expect(response.status).to eq(200)
+
+      initial_sql_queries_count = track_sql_queries do
+        get "/latest.json"
+
+        expect(response.status).to eq(200)
+
+        body = response.parsed_body
+
+        expect(body["topic_list"]["topics"].map { |t| t["id"] }).to contain_exactly(topic.id)
+        expect(body["topic_list"]["topics"][0]["posters"].map { |p| p["user_id"] }).to contain_exactly(user.id)
+      end.count
+
+      group2 = Fabricate(:group)
+      user2 = Fabricate(:user, primary_group: group2)
+      topic.update!(last_post_user_id: user2.id)
+
+      group3 = Fabricate(:group)
+      user3 = Fabricate(:user, flair_group: group3)
+      topic.update!(featured_user3_id: user3.id)
+
+      new_sql_queries_count = track_sql_queries do
+        get "/latest.json"
+
+        expect(response.status).to eq(200)
+
+        body = response.parsed_body
+
+        expect(body["topic_list"]["topics"].map { |t| t["id"] }).to contain_exactly(topic.id)
+        expect(body["topic_list"]["topics"][0]["posters"].map { |p| p["user_id"] }).to contain_exactly(user.id, user2.id, user3.id)
+      end.count
+
+      expect(new_sql_queries_count).to be <= initial_sql_queries_count
+    end
   end
 
   describe "categories and X" do