Commit 8fe02ecd authored by Mark Chao's avatar Mark Chao

Merge branch 'improve-todo-service-database-performance' into 'master'

Avoid N+1 queries when checking for pending todos

See merge request gitlab-org/gitlab!57525
parents 17c4304e 1477434a
...@@ -11,17 +11,18 @@ ...@@ -11,17 +11,18 @@
# change the various `by_*` methods in this finder, without having to touch # change the various `by_*` methods in this finder, without having to touch
# everything that uses it. # everything that uses it.
class PendingTodosFinder class PendingTodosFinder
attr_reader :current_user, :params attr_reader :users, :params
# current_user - The user to retrieve the todos for. # users - The list of users to retrieve the todos for.
# params - A Hash containing columns and values to use for filtering todos. # params - A Hash containing columns and values to use for filtering todos.
def initialize(current_user, params = {}) def initialize(users, params = {})
@current_user = current_user @users = users
@params = params @params = params
end end
def execute def execute
todos = current_user.todos.pending todos = Todo.for_user(users)
todos = todos.pending
todos = by_project(todos) todos = by_project(todos)
todos = by_target_id(todos) todos = by_target_id(todos)
todos = by_target_type(todos) todos = by_target_type(todos)
......
...@@ -148,6 +148,10 @@ class Todo < ApplicationRecord ...@@ -148,6 +148,10 @@ class Todo < ApplicationRecord
.order(Gitlab::Database.nulls_last_order('highest_priority', 'ASC')) .order(Gitlab::Database.nulls_last_order('highest_priority', 'ASC'))
.order('todos.created_at') .order('todos.created_at')
end end
def pluck_user_id
pluck(:user_id)
end
end end
def resource_parent def resource_parent
......
...@@ -177,7 +177,7 @@ class TodoService ...@@ -177,7 +177,7 @@ class TodoService
def resolve_todos_for_target(target, current_user) def resolve_todos_for_target(target, current_user)
attributes = attributes_for_target(target) attributes = attributes_for_target(target)
resolve_todos(pending_todos(current_user, attributes), current_user) resolve_todos(pending_todos([current_user], attributes), current_user)
end end
def resolve_todos(todos, current_user, resolution: :done, resolved_by_action: :system_done) def resolve_todos(todos, current_user, resolution: :done, resolved_by_action: :system_done)
...@@ -220,9 +220,14 @@ class TodoService ...@@ -220,9 +220,14 @@ class TodoService
private private
def create_todos(users, attributes) def create_todos(users, attributes)
Array(users).map do |user| users = Array(users)
next if pending_todos(user, attributes).exists? && Feature.disabled?(:multiple_todos, user)
return if users.empty?
users_with_pending_todos = pending_todos(users, attributes).pluck_user_id
users.reject! { |user| users_with_pending_todos.include?(user.id) && Feature.disabled?(:multiple_todos, user) }
users.map do |user|
issue_type = attributes.delete(:issue_type) issue_type = attributes.delete(:issue_type)
track_todo_creation(user, issue_type) track_todo_creation(user, issue_type)
...@@ -353,8 +358,8 @@ class TodoService ...@@ -353,8 +358,8 @@ class TodoService
end end
end end
def pending_todos(user, criteria = {}) def pending_todos(users, criteria = {})
PendingTodosFinder.new(user, criteria).execute PendingTodosFinder.new(users, criteria).execute
end end
def track_todo_creation(user, issue_type) def track_todo_creation(user, issue_type)
......
---
title: Reduce N+1 queries in creating todos after user mentions in a note
merge_request: 57525
author:
type: performance
...@@ -4,13 +4,15 @@ require 'spec_helper' ...@@ -4,13 +4,15 @@ require 'spec_helper'
RSpec.describe PendingTodosFinder do RSpec.describe PendingTodosFinder do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:user2) { create(:user) }
let(:users) { [user, user2] }
describe '#execute' do describe '#execute' do
it 'returns only pending todos' do it 'returns only pending todos' do
create(:todo, :done, user: user) create(:todo, :done, user: user)
todo = create(:todo, :pending, user: user) todo = create(:todo, :pending, user: user)
todos = described_class.new(user).execute todos = described_class.new(users).execute
expect(todos).to eq([todo]) expect(todos).to eq([todo])
end end
...@@ -22,7 +24,7 @@ RSpec.describe PendingTodosFinder do ...@@ -22,7 +24,7 @@ RSpec.describe PendingTodosFinder do
create(:todo, :pending, user: user, project: project2) create(:todo, :pending, user: user, project: project2)
todo = create(:todo, :pending, user: user, project: project1) todo = create(:todo, :pending, user: user, project: project1)
todos = described_class.new(user, project_id: project1.id).execute todos = described_class.new(users, project_id: project1.id).execute
expect(todos).to eq([todo]) expect(todos).to eq([todo])
end end
...@@ -34,7 +36,7 @@ RSpec.describe PendingTodosFinder do ...@@ -34,7 +36,7 @@ RSpec.describe PendingTodosFinder do
create(:todo, :pending, user: user, target: note) create(:todo, :pending, user: user, target: note)
todos = described_class.new(user, target_id: issue.id).execute todos = described_class.new(users, target_id: issue.id).execute
expect(todos).to eq([todo]) expect(todos).to eq([todo])
end end
...@@ -46,7 +48,7 @@ RSpec.describe PendingTodosFinder do ...@@ -46,7 +48,7 @@ RSpec.describe PendingTodosFinder do
create(:todo, :pending, user: user, target: note) create(:todo, :pending, user: user, target: note)
todos = described_class.new(user, target_type: issue.class.name).execute todos = described_class.new(users, target_type: issue.class.name).execute
expect(todos).to eq([todo]) expect(todos).to eq([todo])
end end
...@@ -55,7 +57,7 @@ RSpec.describe PendingTodosFinder do ...@@ -55,7 +57,7 @@ RSpec.describe PendingTodosFinder do
create(:todo, :pending, user: user, commit_id: '456') create(:todo, :pending, user: user, commit_id: '456')
todo = create(:todo, :pending, user: user, commit_id: '123') todo = create(:todo, :pending, user: user, commit_id: '123')
todos = described_class.new(user, commit_id: '123').execute todos = described_class.new(users, commit_id: '123').execute
expect(todos).to eq([todo]) expect(todos).to eq([todo])
end end
......
...@@ -435,4 +435,12 @@ RSpec.describe Todo do ...@@ -435,4 +435,12 @@ RSpec.describe Todo do
end end
end end
end end
describe '.pluck_user_id' do
subject { described_class.pluck_user_id }
let_it_be(:todo) { create(:todo) }
it { is_expected.to eq([todo.user_id]) }
end
end end
...@@ -1007,7 +1007,8 @@ RSpec.describe TodoService do ...@@ -1007,7 +1007,8 @@ RSpec.describe TodoService do
end end
describe '#update_note' do describe '#update_note' do
let(:noteable) { create(:issue, project: project) } let_it_be(:noteable) { create(:issue, project: project) }
let(:note) { create(:note, project: project, note: mentions, noteable: noteable) } let(:note) { create(:note, project: project, note: mentions, noteable: noteable) }
let(:addressed_note) { create(:note, project: project, note: "#{directly_addressed}", noteable: noteable) } let(:addressed_note) { create(:note, project: project, note: "#{directly_addressed}", noteable: noteable) }
...@@ -1044,12 +1045,34 @@ RSpec.describe TodoService do ...@@ -1044,12 +1045,34 @@ RSpec.describe TodoService do
should_not_create_todo(user: skipped, target: noteable, action: Todo::DIRECTLY_ADDRESSED) should_not_create_todo(user: skipped, target: noteable, action: Todo::DIRECTLY_ADDRESSED)
end end
it 'does not create a todo if user was already mentioned and todo is pending' do context 'users already have pending todos and the multiple_todos feature is off' do
before do
stub_feature_flags(multiple_todos: false) stub_feature_flags(multiple_todos: false)
end
create(:todo, :mentioned, user: member, project: project, target: noteable, author: author) let_it_be(:pending_todo_for_member) { create(:todo, :mentioned, user: member, project: project, target: noteable) }
let_it_be(:pending_todo_for_guest) { create(:todo, :mentioned, user: guest, project: project, target: noteable) }
let_it_be(:pending_todo_for_admin) { create(:todo, :mentioned, user: admin, project: project, target: noteable) }
let_it_be(:note_mentioning_1_user) do
create(:note, project: project, note: "FYI #{member.to_reference}", noteable: noteable)
end
let_it_be(:note_mentioning_3_users) do
create(:note, project: project, note: 'FYI: ' + [member, guest, admin].map(&:to_reference).join(' '), noteable: noteable)
end
expect { service.update_note(note, author, skip_users) }.not_to change(member.todos, :count) it 'does not create a todo if user was already mentioned and todo is pending' do
expect { service.update_note(note_mentioning_1_user, author, skip_users) }.not_to change(member.todos, :count)
end
it 'does not create N+1 queries for pending todos' do
# Excluding queries for user permissions because those do execute N+1 queries
allow_any_instance_of(User).to receive(:can?).and_return(true)
control_count = ActiveRecord::QueryRecorder.new { service.update_note(note_mentioning_1_user, author, skip_users) }.count
expect { service.update_note(note_mentioning_3_users, author, skip_users) }.not_to exceed_query_limit(control_count)
end
end end
it 'does not create a todo if user was already mentioned and todo is done' do it 'does not create a todo if user was already mentioned and todo is done' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment