diff options
| -rw-r--r-- | CHANGELOG | 1 | ||||
| -rw-r--r-- | app/models/commit.rb | 2 | ||||
| -rw-r--r-- | app/models/concerns/issuable.rb | 24 | ||||
| -rw-r--r-- | app/models/snippet.rb | 2 | ||||
| -rw-r--r-- | lib/api/issues.rb | 4 | ||||
| -rw-r--r-- | lib/api/merge_requests.rb | 2 | ||||
| -rw-r--r-- | spec/models/concerns/issuable_spec.rb | 26 |
7 files changed, 53 insertions, 8 deletions
diff --git a/CHANGELOG b/CHANGELOG index 0506854599f..657c6f48334 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -49,6 +49,7 @@ v 8.9.0 (unreleased) - 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. - RepositoryCheck::SingleRepositoryWorker public and private methods are now instrumented + - Improve issuables APIs performance when accessing notes !4471 v 8.8.4 (unreleased) - Ensure branch cleanup regardless of whether the GitHub import process succeeds diff --git a/app/models/commit.rb b/app/models/commit.rb index b5637bc4fbc..d69d518fadd 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -198,7 +198,7 @@ class Commit end def notes_with_associations - notes.includes(:author, :project) + notes.includes(:author) end def method_missing(m, *args, &block) diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 92526a99147..0ccd3474b81 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -17,7 +17,12 @@ module Issuable belongs_to :assignee, class_name: "User" belongs_to :updated_by, class_name: "User" 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 :labels, through: :label_links has_many :todos, as: :target, dependent: :destroy @@ -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 :join_project, -> { joins(:project) } + scope :inc_notes_with_associations, -> { includes(notes: :author) } scope :references_project, -> { references(:project) } scope :non_archived, -> { join_project.where(projects: { archived: false }) } @@ -179,7 +185,13 @@ module Issuable end 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 def subscribed_without_subscriptions?(user) @@ -239,7 +251,13 @@ module Issuable end 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 def updated_tasks diff --git a/app/models/snippet.rb b/app/models/snippet.rb index 407697b745c..f8034cb5e6b 100644 --- a/app/models/snippet.rb +++ b/app/models/snippet.rb @@ -102,7 +102,7 @@ class Snippet < ActiveRecord::Base end def notes_with_associations - notes.includes(:author, :project) + notes.includes(:author) end class << self diff --git a/lib/api/issues.rb b/lib/api/issues.rb index f59a4d6c012..4c43257c48a 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -51,7 +51,7 @@ module API # GET /issues?labels=foo,bar # GET /issues?labels=foo,bar&state=opened 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_labels(issues, params[:labels]) unless params[:labels].nil? issues.reorder(issuable_order_by => issuable_sort) @@ -82,7 +82,7 @@ module API # GET /projects/:id/issues?milestone=1.0.0&state=closed # GET /issues?iid=42 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_labels(issues, params[:labels]) unless params[:labels].nil? issues = filter_by_iid(issues, params[:iid]) unless params[:iid].nil? diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index 2e7836dc8fb..43221d5622a 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -41,7 +41,7 @@ module API # get ":id/merge_requests" do 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? merge_requests = filter_by_iid(merge_requests, params[:iid]) diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb index dd03d64f750..efbcbf72f76 100644 --- a/spec/models/concerns/issuable_spec.rb +++ b/spec/models/concerns/issuable_spec.rb @@ -10,6 +10,16 @@ describe Issue, "Issuable" do it { is_expected.to belong_to(:assignee) } it { is_expected.to have_many(:notes).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 describe 'Included modules' do @@ -245,6 +255,22 @@ describe Issue, "Issuable" do 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 let(:project) { issue.project } |