Commit fade1a4c authored by Alejandro Rodríguez's avatar Alejandro Rodríguez

Fix pseudo n+1 queries with Note and Note Authors in issuables APIs

This was not a clear cut n+1 query, given that if you're directly subscribed to all issues
that the API is returning you never really need to check for the notes. However, if you're
subscribed to _all_ of them, then for each issuable you need to go once to `notes`, and
once to `users` (for the authors). By preemtively loading notes and authors, at worst you have
1 extra query, and at best you saved 2n extra queries. We also took advantage of this preloading
of notes when counting user notes.
parent ad83c308
...@@ -17,7 +17,12 @@ module Issuable ...@@ -17,7 +17,12 @@ module Issuable
belongs_to :assignee, class_name: "User" belongs_to :assignee, class_name: "User"
belongs_to :updated_by, class_name: "User" belongs_to :updated_by, class_name: "User"
belongs_to :milestone belongs_to :milestone
has_many :notes, as: :noteable, dependent: :destroy has_many :notes, as: :noteable, dependent: :destroy do
def authors_loaded?
# We check first if we're loaded to not load unnecesarily.
loaded? && to_a.all? { |note| note.association(:author).loaded? }
end
end
has_many :label_links, as: :target, dependent: :destroy has_many :label_links, as: :target, dependent: :destroy
has_many :labels, through: :label_links has_many :labels, through: :label_links
has_many :todos, as: :target, dependent: :destroy has_many :todos, as: :target, dependent: :destroy
...@@ -44,6 +49,7 @@ module Issuable ...@@ -44,6 +49,7 @@ module Issuable
scope :without_label, -> { joins("LEFT OUTER JOIN label_links ON label_links.target_type = '#{name}' AND label_links.target_id = #{table_name}.id").where(label_links: { id: nil }) } scope :without_label, -> { joins("LEFT OUTER JOIN label_links ON label_links.target_type = '#{name}' AND label_links.target_id = #{table_name}.id").where(label_links: { id: nil }) }
scope :join_project, -> { joins(:project) } scope :join_project, -> { joins(:project) }
scope :inc_notes_with_associations, -> { includes(notes: :author) }
scope :references_project, -> { references(:project) } scope :references_project, -> { references(:project) }
scope :non_archived, -> { join_project.where(projects: { archived: false }) } scope :non_archived, -> { join_project.where(projects: { archived: false }) }
...@@ -179,8 +185,14 @@ module Issuable ...@@ -179,8 +185,14 @@ module Issuable
end end
def user_notes_count def user_notes_count
if notes.loaded?
# Use the in-memory association to select and count to avoid hitting the db
notes.to_a.count { |note| !note.system? }
else
# do the count query
notes.user.count notes.user.count
end end
end
def subscribed_without_subscriptions?(user) def subscribed_without_subscriptions?(user)
participants(user).include?(user) participants(user).include?(user)
...@@ -239,7 +251,13 @@ module Issuable ...@@ -239,7 +251,13 @@ module Issuable
end end
def notes_with_associations def notes_with_associations
notes.includes(:author) # If A has_many Bs, and B has_many Cs, and you do
# `A.includes(b: :c).each { |a| a.b.includes(:c) }`, sadly ActiveRecord
# will do the inclusion again. So, we check if all notes in the relation
# already have their authors loaded (possibly because the scope
# `inc_notes_with_associations` was used) and skip the inclusion if that's
# the case.
notes.authors_loaded? ? notes : notes.includes(:author)
end end
def updated_tasks def updated_tasks
......
...@@ -51,7 +51,7 @@ module API ...@@ -51,7 +51,7 @@ module API
# GET /issues?labels=foo,bar # GET /issues?labels=foo,bar
# GET /issues?labels=foo,bar&state=opened # GET /issues?labels=foo,bar&state=opened
get do get do
issues = current_user.issues issues = current_user.issues.inc_notes_with_associations
issues = filter_issues_state(issues, params[:state]) unless params[:state].nil? issues = filter_issues_state(issues, params[:state]) unless params[:state].nil?
issues = filter_issues_labels(issues, params[:labels]) unless params[:labels].nil? issues = filter_issues_labels(issues, params[:labels]) unless params[:labels].nil?
issues.reorder(issuable_order_by => issuable_sort) issues.reorder(issuable_order_by => issuable_sort)
...@@ -82,7 +82,7 @@ module API ...@@ -82,7 +82,7 @@ module API
# GET /projects/:id/issues?milestone=1.0.0&state=closed # GET /projects/:id/issues?milestone=1.0.0&state=closed
# GET /issues?iid=42 # GET /issues?iid=42
get ":id/issues" do get ":id/issues" do
issues = user_project.issues.visible_to_user(current_user) issues = user_project.issues.inc_notes_with_associations.visible_to_user(current_user)
issues = filter_issues_state(issues, params[:state]) unless params[:state].nil? issues = filter_issues_state(issues, params[:state]) unless params[:state].nil?
issues = filter_issues_labels(issues, params[:labels]) unless params[:labels].nil? issues = filter_issues_labels(issues, params[:labels]) unless params[:labels].nil?
issues = filter_by_iid(issues, params[:iid]) unless params[:iid].nil? issues = filter_by_iid(issues, params[:iid]) unless params[:iid].nil?
......
...@@ -41,7 +41,7 @@ module API ...@@ -41,7 +41,7 @@ module API
# #
get ":id/merge_requests" do get ":id/merge_requests" do
authorize! :read_merge_request, user_project authorize! :read_merge_request, user_project
merge_requests = user_project.merge_requests merge_requests = user_project.merge_requests.inc_notes_with_associations
unless params[:iid].nil? unless params[:iid].nil?
merge_requests = filter_by_iid(merge_requests, params[:iid]) merge_requests = filter_by_iid(merge_requests, params[:iid])
......
...@@ -10,6 +10,16 @@ describe Issue, "Issuable" do ...@@ -10,6 +10,16 @@ describe Issue, "Issuable" do
it { is_expected.to belong_to(:assignee) } it { is_expected.to belong_to(:assignee) }
it { is_expected.to have_many(:notes).dependent(:destroy) } it { is_expected.to have_many(:notes).dependent(:destroy) }
it { is_expected.to have_many(:todos).dependent(:destroy) } it { is_expected.to have_many(:todos).dependent(:destroy) }
context 'Notes' do
let!(:note) { create(:note, noteable: issue, project: issue.project) }
let(:scoped_issue) { Issue.includes(notes: :author).find(issue.id) }
it 'indicates if the notes have their authors loaded' do
expect(issue.notes).not_to be_authors_loaded
expect(scoped_issue.notes).to be_authors_loaded
end
end
end end
describe 'Included modules' do describe 'Included modules' do
...@@ -245,6 +255,22 @@ describe Issue, "Issuable" do ...@@ -245,6 +255,22 @@ describe Issue, "Issuable" do
end end
end end
describe '#user_notes_count' do
let(:project) { create(:project) }
let(:issue1) { create(:issue, project: project) }
let(:issue2) { create(:issue, project: project) }
before do
create_list(:note, 3, noteable: issue1, project: project)
create_list(:note, 6, noteable: issue2, project: project)
end
it 'counts the user notes' do
expect(issue1.user_notes_count).to be(3)
expect(issue2.user_notes_count).to be(6)
end
end
describe "votes" do describe "votes" do
let(:project) { issue.project } let(:project) { issue.project }
......
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