PERF: Combine avatar_lookup and primary_group_lookup into user_lookup (#10253)

These two classes were running very similar queries, which could be expensive on large topics

app/models/topic_list.rb

@@ -110,8 +110,7 @@ class TopicList
       user_ids << ft.user_id << ft.last_post_user_id << ft.featured_user_ids << ft.allowed_user_ids
     end
 
-    avatar_lookup = AvatarLookup.new(user_ids)
+    user_lookup = UserLookup.new(user_ids)
-    primary_group_lookup = PrimaryGroupLookup.new(user_ids)
 
     @topics.each do |ft|
       ft.user_data = @topic_lookup[ft.id] if @topic_lookup.present?
@@ -122,11 +121,13 @@ class TopicList
       end
 
       ft.posters = ft.posters_summary(
-        avatar_lookup: avatar_lookup,
+        user_lookup: user_lookup
-        primary_group_lookup: primary_group_lookup
       )
 
-      ft.participants = ft.participants_summary(avatar_lookup: avatar_lookup, user: @current_user)
+      ft.participants = ft.participants_summary(
+        user_lookup: user_lookup,
+        user: @current_user
+      )
       ft.topic_list = self
     end
 

app/models/topic_participants_summary.rb

@@ -27,7 +27,7 @@ class TopicParticipantsSummary
   end
 
   def top_participants
-    user_ids.map { |id| avatar_lookup[id] }.compact.uniq.take(PARTICIPANT_COUNT)
+    user_ids.map { |id| user_lookup[id] }.compact.uniq.take(PARTICIPANT_COUNT)
   end
 
   def user_ids
@@ -35,7 +35,7 @@ class TopicParticipantsSummary
     [topic.user_id] + topic.allowed_user_ids - [@user.id]
   end
 
-  def avatar_lookup
+  def user_lookup
-    @avatar_lookup ||= options[:avatar_lookup] || AvatarLookup.new(user_ids)
+    @user_lookup ||= options[:user_lookup] || UserLookup.new(user_ids)
   end
 end

app/models/topic_posters_summary.rb

@@ -31,7 +31,7 @@ class TopicPostersSummary
     topic_poster = TopicPoster.new
     topic_poster.user = user
     topic_poster.description = descriptions_for(user)
-    topic_poster.primary_group = primary_group_lookup[user.id]
+    topic_poster.primary_group = user_lookup.primary_groups[user.id]
     if topic.last_post_user_id == user.id
       topic_poster.extras = +'latest'
       topic_poster.extras << ' single' if user_ids.uniq.size == 1
@@ -70,7 +70,7 @@ class TopicPostersSummary
   def shuffle_last_poster_to_back_in(summary)
     unless last_poster_is_topic_creator?
       summary.reject! { |u| u.id == topic.last_post_user_id }
-      summary << avatar_lookup[topic.last_post_user_id]
+      summary << user_lookup[topic.last_post_user_id]
     end
     summary
   end
@@ -84,18 +84,14 @@ class TopicPostersSummary
   end
 
   def top_posters
-    user_ids.map { |id| avatar_lookup[id] }.compact.uniq.take(5)
+    user_ids.map { |id| user_lookup[id] }.compact.uniq.take(5)
   end
 
   def user_ids
     [ topic.user_id, topic.last_post_user_id, *topic.featured_user_ids ]
   end
 
-  def avatar_lookup
+  def user_lookup
-    @avatar_lookup ||= options[:avatar_lookup] || AvatarLookup.new(user_ids)
+    @user_lookup ||= options[:user_lookup] || UserLookup.new(user_ids)
-  end
-
-  def primary_group_lookup
-    @primary_group_lookup ||= options[:primary_group_lookup] || PrimaryGroupLookup.new(user_ids)
   end
 end

app/models/user_action.rb

@@ -167,7 +167,7 @@ class UserAction < ActiveRecord::Base
       'u.name AS acting_name'
     ]
 
-    AvatarLookup.lookup_columns.each do |c|
+    UserLookup.lookup_columns.each do |c|
       next if c == :id || c['.']
       acting_cols << "u.#{c} AS acting_#{c}"
     end

app/models/user_summary.rb

@@ -194,7 +194,7 @@ protected
   def user_counts(user_hash)
     user_ids = user_hash.keys
 
-    lookup = AvatarLookup.new(user_ids)
+    lookup = UserLookup.new(user_ids)
     user_ids.map do |user_id|
       lookup_hash = lookup[user_id]
 

lib/avatar_lookup.rb

@@ -1,32 +0,0 @@
-# frozen_string_literal: true
-
-class AvatarLookup
-
-  def initialize(user_ids = [])
-    @user_ids = user_ids.tap(&:compact!).tap(&:uniq!).tap(&:flatten!)
-  end
-
-  # Lookup a user by id
-  def [](user_id)
-    users[user_id]
-  end
-
-  private
-
-  def self.lookup_columns
-    @lookup_columns ||= %i{id user_emails.email username name uploaded_avatar_id}
-  end
-
-  def users
-    @users ||= user_lookup_hash
-  end
-
-  def user_lookup_hash
-    hash = {}
-    User.joins(:user_emails)
-      .where(id: @user_ids)
-      .select(AvatarLookup.lookup_columns)
-      .each { |user| hash[user.id] = user }
-    hash
-  end
-end

lib/plugin/instance.rb

@@ -192,8 +192,8 @@ class Plugin::Instance
 
   def custom_avatar_column(column)
     reloadable_patch do |plugin|
-      AvatarLookup.lookup_columns << column
+      UserLookup.lookup_columns << column
-      AvatarLookup.lookup_columns.uniq!
+      UserLookup.lookup_columns.uniq!
     end
   end
 

lib/primary_group_lookup.rb

@@ -1,41 +0,0 @@
-# frozen_string_literal: true
-
-class PrimaryGroupLookup
-  def initialize(user_ids = [])
-    @user_ids = user_ids.tap(&:compact!).tap(&:uniq!).tap(&:flatten!)
-  end
-
-  # Lookup primary group for a given user id
-  def [](user_id)
-    users[user_id]
-  end
-
-  private
-
-  def self.lookup_columns
-    @lookup_columns ||= %i{id name flair_icon flair_upload_id flair_bg_color flair_color}
-  end
-
-  def users
-    @users ||= user_lookup_hash
-  end
-
-  def user_lookup_hash
-    users_with_primary_group = User.where(id: @user_ids)
-      .where.not(primary_group_id: nil)
-      .select(:id, :primary_group_id)
-
-    group_lookup = {}
-    group_ids = users_with_primary_group.map(&:primary_group_id)
-    group_ids.uniq!
-
-    Group.includes(:flair_upload).where(id: group_ids).select(self.class.lookup_columns)
-      .each { |g| group_lookup[g.id] = g }
-
-    hash = {}
-    users_with_primary_group.each do |u|
-      hash[u.id] = group_lookup[u.primary_group_id]
-    end
-    hash
-  end
-end

lib/topic_query.rb

@@ -433,16 +433,14 @@ class TopicQuery
         user_ids << ft.user_id << ft.last_post_user_id << ft.featured_user_ids << ft.allowed_user_ids
       end
 
-      avatar_lookup = AvatarLookup.new(user_ids)
+      user_lookup = UserLookup.new(user_ids)
-      primary_group_lookup = PrimaryGroupLookup.new(user_ids)
 
       # memoize for loop so we don't keep looking these up
       translations = TopicPostersSummary.translations
 
       topics.each do |t|
         t.posters = t.posters_summary(
-          avatar_lookup: avatar_lookup,
+          user_lookup: user_lookup,
-          primary_group_lookup: primary_group_lookup,
           translations: translations
         )
       end

lib/user_lookup.rb

@@ -0,0 +1,59 @@
+# frozen_string_literal: true
+
+class UserLookup
+
+  def initialize(user_ids = [])
+    @user_ids = user_ids.tap(&:compact!).tap(&:uniq!).tap(&:flatten!)
+  end
+
+  # Lookup a user by id
+  def [](user_id)
+    users[user_id]
+  end
+
+  def primary_groups
+    @groups ||= group_lookup_hash
+  end
+
+  private
+
+  def self.lookup_columns
+    @user_lookup_columns ||= %i{id username name uploaded_avatar_id primary_group_id}
+  end
+
+  def self.group_lookup_columns
+    @group_lookup_columns ||= %i{id name flair_icon flair_upload_id flair_bg_color flair_color}
+  end
+
+  def users
+    @users ||= user_lookup_hash
+  end
+
+  def user_lookup_hash
+    hash = {}
+    User.where(id: @user_ids)
+      .select(self.class.lookup_columns)
+      .each { |user| hash[user.id] = user }
+    hash
+  end
+
+  def group_lookup_hash
+    users_with_primary_group = users.values.reject { |u| u.primary_group_id.nil? }
+
+    group_lookup = {}
+    group_ids = users_with_primary_group.map { |u| u.primary_group_id }
+    group_ids.uniq!
+
+    Group.includes(:flair_upload)
+      .where(id: group_ids)
+      .select(self.class.group_lookup_columns)
+      .each { |g| group_lookup[g.id] = g }
+
+    hash = {}
+    users_with_primary_group.each do |u|
+      hash[u.id] = group_lookup[u.primary_group_id]
+    end
+    hash
+  end
+
+end

spec/components/avatar_lookup_spec.rb

@@ -1,29 +0,0 @@
-# encoding: utf-8
-# frozen_string_literal: true
-
-require 'rails_helper'
-
-describe AvatarLookup do
-  fab!(:user) { Fabricate(:user, username: "john_doe", name: "John Doe") }
-
-  describe '#[]' do
-    before do
-      @avatar_lookup = AvatarLookup.new([user.id, nil])
-    end
-
-    it 'returns nil if user_id does not exists' do
-      expect(@avatar_lookup[0]).to eq(nil)
-    end
-
-    it 'returns nil if user_id is nil' do
-      expect(@avatar_lookup[nil]).to eq(nil)
-    end
-
-    it 'returns user if user_id exists' do
-      avatar_lookup_user = @avatar_lookup[user.id]
-      expect(avatar_lookup_user).to eq(user)
-      expect(avatar_lookup_user.username).to eq("john_doe")
-      expect(avatar_lookup_user.name).to eq("John Doe")
-    end
-  end
-end

spec/components/user_lookup_spec.rb

@@ -0,0 +1,56 @@
+# encoding: utf-8
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe UserLookup do
+  fab!(:user) { Fabricate(:user, username: "john_doe", name: "John Doe") }
+
+  describe '#[]' do
+    before do
+      @user_lookup = UserLookup.new([user.id, nil])
+    end
+
+    it 'returns nil if user_id does not exists' do
+      expect(@user_lookup[0]).to eq(nil)
+    end
+
+    it 'returns nil if user_id is nil' do
+      expect(@user_lookup[nil]).to eq(nil)
+    end
+
+    it 'returns user if user_id exists' do
+      user_lookup_user = @user_lookup[user.id]
+      expect(user_lookup_user).to eq(user)
+      expect(user_lookup_user.username).to eq("john_doe")
+      expect(user_lookup_user.name).to eq("John Doe")
+    end
+  end
+
+  describe '#primary_groups' do
+    fab!(:group) { Fabricate(:group, name: 'testgroup') }
+    fab!(:user2) { Fabricate(:user, username: "jane_doe", name: "Jane Doe", primary_group: group) }
+
+    before do
+      @user_lookup = UserLookup.new([user.id, user2.id, nil])
+    end
+
+    it 'returns nil if user_id does not exists' do
+      expect(@user_lookup.primary_groups[0]).to eq(nil)
+    end
+
+    it 'returns nil if user_id is nil' do
+      expect(@user_lookup.primary_groups[nil]).to eq(nil)
+    end
+
+    it 'returns nil if user has no primary group' do
+      expect(@user_lookup.primary_groups[user.id]).to eq(nil)
+    end
+
+    it 'returns group if user has primary group' do
+      user_lookup_group = @user_lookup.primary_groups[user2.id]
+      expect(user_lookup_group).to eq(group)
+      expect(user_lookup_group.name).to eq("testgroup")
+    end
+  end
+end