Merge branch '325690-avoid-loading-user-objects-when-bulk-updating-todos' into 'master'

Avoid loading user objects when bulk updating todos

See merge request gitlab-org/gitlab!59909

app/models/todo.rb

@@ -149,8 +149,8 @@ class Todo < ApplicationRecord
         .order('todos.created_at')
     end
 
-    def pluck_user_id
-      pluck(:user_id)
+    def distinct_user_ids
+      distinct.pluck(:user_id)
     end
 
     # Count todos grouped by user_id and state, using an UNION query

app/models/user.rb

@@ -33,6 +33,8 @@ class User < ApplicationRecord
 
   BLOCKED_PENDING_APPROVAL_STATE = 'blocked_pending_approval'
 
+  COUNT_CACHE_VALIDITY_PERIOD = 24.hours
+
   add_authentication_token_field :incoming_email_token, token_generator: -> { SecureRandom.hex.to_i(16).to_s(36) }
   add_authentication_token_field :feed_token
   add_authentication_token_field :static_object_token
@@ -1621,36 +1623,32 @@ class User < ApplicationRecord
     @global_notification_setting
   end
 
-  def count_cache_validity_period
-    24.hours
-  end
-
   def assigned_open_merge_requests_count(force: false)
-    Rails.cache.fetch(['users', id, 'assigned_open_merge_requests_count'], force: force, expires_in: count_cache_validity_period) do
+    Rails.cache.fetch(['users', id, 'assigned_open_merge_requests_count'], force: force, expires_in: COUNT_CACHE_VALIDITY_PERIOD) do
       MergeRequestsFinder.new(self, assignee_id: self.id, state: 'opened', non_archived: true).execute.count
     end
   end
 
   def review_requested_open_merge_requests_count(force: false)
-    Rails.cache.fetch(['users', id, 'review_requested_open_merge_requests_count'], force: force, expires_in: count_cache_validity_period) do
+    Rails.cache.fetch(['users', id, 'review_requested_open_merge_requests_count'], force: force, expires_in: COUNT_CACHE_VALIDITY_PERIOD) do
       MergeRequestsFinder.new(self, reviewer_id: id, state: 'opened', non_archived: true).execute.count
     end
   end
 
   def assigned_open_issues_count(force: false)
-    Rails.cache.fetch(['users', id, 'assigned_open_issues_count'], force: force, expires_in: count_cache_validity_period) do
+    Rails.cache.fetch(['users', id, 'assigned_open_issues_count'], force: force, expires_in: COUNT_CACHE_VALIDITY_PERIOD) do
       IssuesFinder.new(self, assignee_id: self.id, state: 'opened', non_archived: true).execute.count
     end
   end
 
   def todos_done_count(force: false)
-    Rails.cache.fetch(['users', id, 'todos_done_count'], force: force, expires_in: count_cache_validity_period) do
+    Rails.cache.fetch(['users', id, 'todos_done_count'], force: force, expires_in: COUNT_CACHE_VALIDITY_PERIOD) do
       TodosFinder.new(self, state: :done).execute.count
     end
   end
 
   def todos_pending_count(force: false)
-    Rails.cache.fetch(['users', id, 'todos_pending_count'], force: force, expires_in: count_cache_validity_period) do
+    Rails.cache.fetch(['users', id, 'todos_pending_count'], force: force, expires_in: COUNT_CACHE_VALIDITY_PERIOD) do
       TodosFinder.new(self, state: :pending).execute.count
     end
   end

app/services/todo_service.rb

@@ -43,11 +43,11 @@ class TodoService
   # updates the todo counts for those users.
   #
   def destroy_target(target)
-    todo_users = User.for_todos(target.todos).to_a
+    todo_user_ids = target.todos.distinct_user_ids
 
     yield target
 
-    Users::UpdateTodoCountCacheService.new(todo_users).execute if todo_users.present?
+    Users::UpdateTodoCountCacheService.new(todo_user_ids).execute if todo_user_ids.present?
   end
 
   # When we reassign an assignable object (issuable, alert) we should:
@@ -224,7 +224,7 @@ class TodoService
 
     return if users.empty?
 
-    users_with_pending_todos = pending_todos(users, attributes).pluck_user_id
+    users_with_pending_todos = pending_todos(users, attributes).distinct_user_ids
     users.reject! { |user| users_with_pending_todos.include?(user.id) && Feature.disabled?(:multiple_todos, user) }
 
     todos = users.map do |user|
@@ -234,7 +234,7 @@ class TodoService
       Todo.create(attributes.merge(user_id: user.id))
     end
 
-    Users::UpdateTodoCountCacheService.new(users).execute
+    Users::UpdateTodoCountCacheService.new(users.map(&:id)).execute
 
     todos
   end

app/services/users/update_todo_count_cache_service.rb

@@ -4,31 +4,34 @@ module Users
   class UpdateTodoCountCacheService < BaseService
     QUERY_BATCH_SIZE = 10
 
-    attr_reader :users
+    attr_reader :user_ids
 
-    # users - An array of User objects
-    def initialize(users)
-      @users = users
+    # user_ids - An array of User IDs
+    def initialize(user_ids)
+      @user_ids = user_ids
     end
 
     def execute
-      users.each_slice(QUERY_BATCH_SIZE) do |users_batch|
-        todo_counts = Todo.for_user(users_batch).count_grouped_by_user_id_and_state
+      user_ids.each_slice(QUERY_BATCH_SIZE) do |user_ids_batch|
+        todo_counts = Todo.for_user(user_ids_batch).count_grouped_by_user_id_and_state
 
-        users_batch.each do |user|
-          update_count_cache(user, todo_counts, :done)
-          update_count_cache(user, todo_counts, :pending)
+        user_ids_batch.each do |user_id|
+          update_count_cache(user_id, todo_counts, :done)
+          update_count_cache(user_id, todo_counts, :pending)
         end
       end
     end
 
     private
 
-    def update_count_cache(user, todo_counts, state)
-      count = todo_counts.fetch([user.id, state.to_s], 0)
-      expiration_time = user.count_cache_validity_period
+    def update_count_cache(user_id, todo_counts, state)
+      count = todo_counts.fetch([user_id, state.to_s], 0)
 
-      Rails.cache.write(['users', user.id, "todos_#{state}_count"], count, expires_in: expiration_time)
+      Rails.cache.write(
+        ['users', user_id, "todos_#{state}_count"],
+        count,
+        expires_in: User::COUNT_CACHE_VALIDITY_PERIOD
+      )
     end
   end
 end

changelogs/unreleased/325690-avoid-loading-user-objects-when-bulk-updating-todos.yml

+---
+title: Avoid loading user objects when bulk updating todos
+merge_request: 59909
+author:
+type: performance

spec/models/todo_spec.rb

@@ -452,11 +452,15 @@ RSpec.describe Todo do
     end
   end
 
-  describe '.pluck_user_id' do
-    subject { described_class.pluck_user_id }
+  describe '.distinct_user_ids' do
+    subject { described_class.distinct_user_ids }
 
-    let_it_be(:todo) { create(:todo) }
+    let_it_be(:user1) { create(:user) }
+    let_it_be(:user2) { create(:user) }
+    let_it_be(:todo) { create(:todo, user: user1) }
+    let_it_be(:todo) { create(:todo, user: user1) }
+    let_it_be(:todo) { create(:todo, user: user2) }
 
-    it { is_expected.to eq([todo.user_id]) }
+    it { is_expected.to contain_exactly(user1.id, user2.id) }
   end
 end

spec/models/user_spec.rb

@@ -20,6 +20,10 @@ RSpec.describe User do
     it { is_expected.to include_module(AsyncDeviseEmail) }
   end
 
+  describe 'constants' do
+    it { expect(described_class::COUNT_CACHE_VALIDITY_PERIOD).to be_a(Integer) }
+  end
+
   describe 'delegations' do
     it { is_expected.to delegate_method(:path).to(:namespace).with_prefix }
 

spec/services/todo_service_spec.rb

@@ -348,7 +348,7 @@ RSpec.describe TodoService do
         create(:todo, state: :pending, target: issue, user: author, author: author, project: issue.project)
         create(:todo, state: :done, target: issue, user: assignee, author: assignee, project: issue.project)
 
-        expect_next(Users::UpdateTodoCountCacheService, [author, assignee]).to receive(:execute)
+        expect_next(Users::UpdateTodoCountCacheService, [author.id, assignee.id]).to receive(:execute)
 
         service.destroy_target(issue) { issue.destroy! }
       end
@@ -1094,7 +1094,7 @@ RSpec.describe TodoService do
   it 'updates cached counts when a todo is created' do
     issue = create(:issue, project: project, assignees: [john_doe], author: author)
 
-    expect_next(Users::UpdateTodoCountCacheService, [john_doe]).to receive(:execute)
+    expect_next(Users::UpdateTodoCountCacheService, [john_doe.id]).to receive(:execute)
 
     service.new_issue(issue, author)
   end

spec/services/users/update_todo_count_cache_service_spec.rb

@@ -14,13 +14,21 @@ RSpec.describe Users::UpdateTodoCountCacheService do
     let_it_be(:todo5) { create(:todo, user: user2, state: :pending) }
     let_it_be(:todo6) { create(:todo, user: user2, state: :pending) }
 
+    def execute_all
+      described_class.new([user1.id, user2.id]).execute
+    end
+
+    def execute_single
+      described_class.new([user1.id]).execute
+    end
+
     it 'updates the todos_counts for users', :use_clean_rails_memory_store_caching do
       Rails.cache.write(['users', user1.id, 'todos_done_count'], 0)
       Rails.cache.write(['users', user1.id, 'todos_pending_count'], 0)
       Rails.cache.write(['users', user2.id, 'todos_done_count'], 0)
       Rails.cache.write(['users', user2.id, 'todos_pending_count'], 0)
 
-      expect { described_class.new([user1, user2]).execute }
+      expect { execute_all }
         .to change(user1, :todos_done_count).from(0).to(2)
         .and change(user1, :todos_pending_count).from(0).to(1)
         .and change(user2, :todos_done_count).from(0).to(1)
@@ -28,7 +36,7 @@ RSpec.describe Users::UpdateTodoCountCacheService do
 
       Todo.delete_all
 
-      expect { described_class.new([user1, user2]).execute }
+      expect { execute_all }
         .to change(user1, :todos_done_count).from(2).to(0)
         .and change(user1, :todos_pending_count).from(1).to(0)
         .and change(user2, :todos_done_count).from(1).to(0)
@@ -36,26 +44,24 @@ RSpec.describe Users::UpdateTodoCountCacheService do
     end
 
     it 'avoids N+1 queries' do
-      control_count = ActiveRecord::QueryRecorder.new { described_class.new([user1]).execute }.count
+      control_count = ActiveRecord::QueryRecorder.new { execute_single }.count
 
-      expect { described_class.new([user1, user2]).execute }.not_to exceed_query_limit(control_count)
+      expect { execute_all }.not_to exceed_query_limit(control_count)
     end
 
     it 'executes one query per batch of users' do
       stub_const("#{described_class}::QUERY_BATCH_SIZE", 1)
 
-      expect(ActiveRecord::QueryRecorder.new { described_class.new([user1]).execute }.count).to eq(1)
-      expect(ActiveRecord::QueryRecorder.new { described_class.new([user1, user2]).execute }.count).to eq(2)
+      expect(ActiveRecord::QueryRecorder.new { execute_single }.count).to eq(1)
+      expect(ActiveRecord::QueryRecorder.new { execute_all }.count).to eq(2)
     end
 
-    it 'sets the cache expire time to the users count_cache_validity_period' do
-      allow(user1).to receive(:count_cache_validity_period).and_return(1.minute)
-      allow(user2).to receive(:count_cache_validity_period).and_return(1.hour)
-
-      expect(Rails.cache).to receive(:write).with(['users', user1.id, anything], anything, expires_in: 1.minute).twice
-      expect(Rails.cache).to receive(:write).with(['users', user2.id, anything], anything, expires_in: 1.hour).twice
+    it 'sets the correct cache expire time' do
+      expect(Rails.cache).to receive(:write)
+        .with(['users', user1.id, anything], anything, expires_in: User::COUNT_CACHE_VALIDITY_PERIOD)
+        .twice
 
-      described_class.new([user1, user2]).execute
+      execute_single
     end
   end
 end