Commit abaaad67 authored by Yorick Peterse's avatar Yorick Peterse

Merge branch '17960-issues-api-endpoint-performs-poorly' into 'master'

Optimize issuable APIs interaction with notes

See merge request !4471
parents ae7fc2e1 83e12741
...@@ -49,6 +49,7 @@ v 8.9.0 (unreleased) ...@@ -49,6 +49,7 @@ v 8.9.0 (unreleased)
- Replace Colorize with Rainbow for coloring console output in Rake tasks. - Replace Colorize with Rainbow for coloring console output in Rake tasks.
- An indicator is now displayed at the top of the comment field for confidential issues. - An indicator is now displayed at the top of the comment field for confidential issues.
- RepositoryCheck::SingleRepositoryWorker public and private methods are now instrumented - RepositoryCheck::SingleRepositoryWorker public and private methods are now instrumented
- Improve issuables APIs performance when accessing notes !4471
v 8.8.4 (unreleased) v 8.8.4 (unreleased)
- Ensure branch cleanup regardless of whether the GitHub import process succeeds - Ensure branch cleanup regardless of whether the GitHub import process succeeds
......
...@@ -198,7 +198,7 @@ class Commit ...@@ -198,7 +198,7 @@ class Commit
end end
def notes_with_associations def notes_with_associations
notes.includes(:author, :project) notes.includes(:author)
end end
def method_missing(m, *args, &block) def method_missing(m, *args, &block)
......
...@@ -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,7 +185,13 @@ module Issuable ...@@ -179,7 +185,13 @@ module Issuable
end end
def user_notes_count def user_notes_count
notes.user.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
end
end end
def subscribed_without_subscriptions?(user) def subscribed_without_subscriptions?(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, :project) # 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
......
...@@ -102,7 +102,7 @@ class Snippet < ActiveRecord::Base ...@@ -102,7 +102,7 @@ class Snippet < ActiveRecord::Base
end end
def notes_with_associations def notes_with_associations
notes.includes(:author, :project) notes.includes(:author)
end end
class << self class << self
......
...@@ -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