Merge branch...

Merge branch '43096-controller-projects-issuescontroller-referenced_merge_requests-json-executes-more-than-100-sql-queries' into 'master'

Resolve "Controller Projects::IssuesController#referenced_merge_requests.json executes more than 100 SQL queries"

Closes #43096

See merge request gitlab-org/gitlab-ce!21237

app/controllers/projects/issues_controller.rb

@@ -113,7 +113,7 @@ class Projects::IssuesController < Projects::ApplicationController
   end
 
   def referenced_merge_requests
-    @merge_requests, @closed_by_merge_requests = ::Issues::FetchReferencedMergeRequestsService.new(project, current_user).execute(issue)
+    @merge_requests, @closed_by_merge_requests = ::Issues::ReferencedMergeRequestsService.new(project, current_user).execute(issue)
 
     respond_to do |format|
       format.json do

app/models/issue.rb

@@ -170,27 +170,6 @@ class Issue < ActiveRecord::Base
     "#{project.to_reference(from, full: full)}#{reference}"
   end
 
-  def referenced_merge_requests(current_user = nil)
-    ext = all_references(current_user)
-
-    notes_with_associations.each do |object|
-      object.all_references(current_user, extractor: ext)
-    end
-
-    merge_requests = ext.merge_requests.sort_by(&:iid)
-
-    cross_project_filter = -> (merge_requests) do
-      merge_requests.select { |mr| mr.target_project == project }
-    end
-
-    Ability.merge_requests_readable_by_user(
-      merge_requests, current_user,
-      filters: {
-        read_cross_project: cross_project_filter
-      }
-    )
-  end
-
   # All branches containing the current issue's ID, except for
   # those with a merge request open referencing the current issue.
   def related_branches(current_user)
@@ -198,7 +177,11 @@ class Issue < ActiveRecord::Base
       branch =~ /\A#{iid}-(?!\d+-stable)/i
     end
 
-    branches_with_merge_request = self.referenced_merge_requests(current_user).map(&:source_branch)
+    branches_with_merge_request =
+      Issues::ReferencedMergeRequestsService
+        .new(project, current_user)
+        .referenced_merge_requests(self)
+        .map(&:source_branch)
 
     branches_with_iid - branches_with_merge_request
   end
@@ -225,26 +208,6 @@ class Issue < ActiveRecord::Base
     project
   end
 
-  # From all notes on this issue, we'll select the system notes about linked
-  # merge requests. Of those, the MRs closing `self` are returned.
-  def closed_by_merge_requests(current_user = nil)
-    return [] unless open?
-
-    ext = all_references(current_user)
-
-    notes.system.each do |note|
-      note.all_references(current_user, extractor: ext)
-    end
-
-    merge_requests = ext.merge_requests.select(&:open?)
-    if merge_requests.any?
-      ids = MergeRequestsClosingIssues.where(merge_request_id: merge_requests.map(&:id), issue_id: id).pluck(:merge_request_id)
-      merge_requests.select { |mr| mr.id.in?(ids) }
-    else
-      []
-    end
-  end
-
   def moved?
     !moved_to.nil?
   end

app/services/issues/fetch_referenced_merge_requests_service.rb

-# frozen_string_literal: true
-
-module Issues
-  class FetchReferencedMergeRequestsService < Issues::BaseService
-    def execute(issue)
-      referenced_merge_requests = issue.referenced_merge_requests(current_user)
-      referenced_merge_requests = Gitlab::IssuableSorter.sort(project, referenced_merge_requests) { |i| i.iid.to_s }
-      closed_by_merge_requests = issue.closed_by_merge_requests(current_user)
-      closed_by_merge_requests = Gitlab::IssuableSorter.sort(project, closed_by_merge_requests) { |i| i.iid.to_s }
-
-      [referenced_merge_requests, closed_by_merge_requests]
-    end
-  end
-end

app/services/issues/referenced_merge_requests_service.rb

+# frozen_string_literal: true
+
+module Issues
+  class ReferencedMergeRequestsService < Issues::BaseService
+    def execute(issue)
+      referenced = referenced_merge_requests(issue)
+      closed_by = closed_by_merge_requests(issue)
+      preloader = ActiveRecord::Associations::Preloader.new
+
+      preloader.preload(referenced + closed_by,
+                        head_pipeline: { project: [:route, { namespace: :route }] })
+
+      [sort_by_iid(referenced), sort_by_iid(closed_by)]
+    end
+
+    def referenced_merge_requests(issue)
+      merge_requests = extract_merge_requests(issue)
+
+      cross_project_filter = -> (merge_requests) do
+        merge_requests.select { |mr| mr.target_project == project }
+      end
+
+      Ability.merge_requests_readable_by_user(
+        merge_requests,
+        current_user,
+        filters: {
+          read_cross_project: cross_project_filter
+        }
+      )
+    end
+
+    def closed_by_merge_requests(issue)
+      return [] unless issue.open?
+
+      merge_requests = extract_merge_requests(issue, filter: :system).select(&:open?)
+
+      return [] if merge_requests.empty?
+
+      ids = MergeRequestsClosingIssues.where(merge_request_id: merge_requests.map(&:id), issue_id: issue.id).pluck(:merge_request_id)
+      merge_requests.select { |mr| mr.id.in?(ids) }
+    end
+
+    private
+
+    def extract_merge_requests(issue, filter: nil)
+      ext = issue.all_references(current_user)
+      notes = issue_notes(issue)
+      notes = notes.select(&filter) if filter
+
+      notes.each do |note|
+        note.all_references(current_user, extractor: ext)
+      end
+
+      ext.merge_requests
+    end
+
+    def issue_notes(issue)
+      @issue_notes ||= {}
+      @issue_notes[issue] ||= issue.notes.includes(:author)
+    end
+
+    def sort_by_iid(merge_requests)
+      Gitlab::IssuableSorter.sort(project, merge_requests) { |mr| mr.iid.to_s }
+    end
+  end
+end

changelogs/unreleased/43096-controller-projects-issuescontroller-referenced_merge_requests-json-executes-more-than-100-sql-queries.yml

+---
+title: Improve performance when fetching related merge requests for an issue
+merge_request: 21237
+author:
+type: performance

lib/banzai/reference_parser/merge_request_parser.rb

@@ -14,11 +14,12 @@ module Banzai
               # Eager loading these ensures we don't end up running dozens of
               # queries in this process.
               target_project: [
-                { namespace: :owner },
+                { namespace: [:owner, :route] },
                 { group: [:owners, :group_members] },
                 :invited_groups,
                 :project_members,
-                :project_feature
+                :project_feature,
+                :route
               ]
             }),
           self.class.data_attribute

spec/models/issue_spec.rb

@@ -188,98 +188,6 @@ describe Issue do
     end
   end
 
-  describe '#closed_by_merge_requests' do
-    let(:project) { create(:project, :repository) }
-    let(:issue) { create(:issue, project: project)}
-    let(:closed_issue) { build(:issue, :closed, project: project)}
-
-    let(:mr) do
-      opts = {
-        title: 'Awesome merge_request',
-        description: "Fixes #{issue.to_reference}",
-        source_branch: 'feature',
-        target_branch: 'master'
-      }
-      MergeRequests::CreateService.new(project, project.owner, opts).execute
-    end
-
-    let(:closed_mr) do
-      opts = {
-        title: 'Awesome merge_request 2',
-        description: "Fixes #{issue.to_reference}",
-        source_branch: 'feature',
-        target_branch: 'master',
-        state: 'closed'
-      }
-      MergeRequests::CreateService.new(project, project.owner, opts).execute
-    end
-
-    it 'returns the merge request to close this issue' do
-      expect(issue.closed_by_merge_requests(mr.author)).to eq([mr])
-    end
-
-    it "returns an empty array when the merge request is closed already" do
-      expect(issue.closed_by_merge_requests(closed_mr.author)).to eq([])
-    end
-
-    it "returns an empty array when the current issue is closed already" do
-      expect(closed_issue.closed_by_merge_requests(closed_issue.author)).to eq([])
-    end
-  end
-
-  describe '#referenced_merge_requests' do
-    let(:project) { create(:project, :public) }
-    let(:issue) do
-      create(:issue, description: merge_request.to_reference, project: project)
-    end
-    let!(:merge_request) do
-      create(:merge_request,
-             source_project: project,
-             source_branch:  'master',
-             target_branch:  'feature')
-    end
-
-    it 'returns the referenced merge requests' do
-      mr2 = create(:merge_request,
-                   source_project: project,
-                   source_branch:  'feature',
-                   target_branch:  'master')
-
-      create(:note_on_issue,
-             noteable:   issue,
-             note:       mr2.to_reference,
-             project_id: project.id)
-
-      expect(issue.referenced_merge_requests).to eq([merge_request, mr2])
-    end
-
-    it 'returns cross project referenced merge requests' do
-      other_project = create(:project, :public)
-      cross_project_merge_request = create(:merge_request, source_project: other_project)
-      create(:note_on_issue,
-             noteable:   issue,
-             note:       cross_project_merge_request.to_reference(issue.project),
-             project_id: issue.project.id)
-
-      expect(issue.referenced_merge_requests).to eq([merge_request, cross_project_merge_request])
-    end
-
-    it 'excludes cross project references if the user cannot read cross project' do
-      user = create(:user)
-      allow(Ability).to receive(:allowed?).and_call_original
-      expect(Ability).to receive(:allowed?).with(user, :read_cross_project) { false }
-
-      other_project = create(:project, :public)
-      cross_project_merge_request = create(:merge_request, source_project: other_project)
-      create(:note_on_issue,
-             noteable:   issue,
-             note:       cross_project_merge_request.to_reference(issue.project),
-             project_id: issue.project.id)
-
-      expect(issue.referenced_merge_requests(user)).to eq([merge_request])
-    end
-  end
-
   describe '#can_move?' do
     let(:user) { create(:user) }
     let(:issue) { create(:issue) }
@@ -365,7 +273,12 @@ describe Issue do
                                                source_project: subject.project,
                                                source_branch: "#{subject.iid}-branch" })
       merge_request.create_cross_references!(user)
-      expect(subject.referenced_merge_requests(user)).not_to be_empty
+
+      referenced_merge_requests = Issues::ReferencedMergeRequestsService
+                                    .new(subject.project, user)
+                                    .referenced_merge_requests(subject)
+
+      expect(referenced_merge_requests).not_to be_empty
       expect(subject.related_branches(user)).to eq([subject.to_branch_name])
     end
 

spec/services/issues/fetch_referenced_merge_requests_service_spec.rb

-require 'spec_helper.rb'
-
-describe Issues::FetchReferencedMergeRequestsService do
-  let(:project) { create(:project) }
-  let(:issue) { create(:issue, project: project) }
-  let(:other_project) { create(:project) }
-
-  let(:mr) { create(:merge_request, source_project: project, target_project: project, id: 2)}
-  let(:other_mr) { create(:merge_request, source_project: other_project, target_project: other_project, id: 1)}
-
-  let(:user) { create(:user) }
-  let(:service) { described_class.new(project, user) }
-
-  context 'with mentioned merge requests' do
-    it 'returns a list of sorted merge requests' do
-      allow(issue).to receive(:referenced_merge_requests).with(user).and_return([other_mr, mr])
-
-      mrs, closed_by_mrs = service.execute(issue)
-
-      expect(mrs).to match_array([mr, other_mr])
-      expect(closed_by_mrs).to match_array([])
-    end
-  end
-
-  context 'with closed-by merge requests' do
-    it 'returns a list of sorted merge requests' do
-      allow(issue).to receive(:closed_by_merge_requests).with(user).and_return([other_mr, mr])
-
-      mrs, closed_by_mrs = service.execute(issue)
-
-      expect(mrs).to match_array([])
-      expect(closed_by_mrs).to match_array([mr, other_mr])
-    end
-  end
-end

spec/services/issues/referenced_merge_requests_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper.rb'
+
+describe Issues::ReferencedMergeRequestsService do
+  def create_referencing_mr(attributes = {})
+    create(:merge_request, attributes).tap do |merge_request|
+      create(:note, :system, project: project, noteable: issue, author: user, note: merge_request.to_reference(full: true))
+    end
+  end
+
+  def create_closing_mr(attributes = {})
+    create_referencing_mr(attributes).tap do |merge_request|
+      create(:merge_requests_closing_issues, issue: issue, merge_request: merge_request)
+    end
+  end
+
+  set(:user) { create(:user) }
+  set(:project) { create(:project, :public, :repository) }
+  set(:other_project) { create(:project, :public, :repository) }
+  set(:issue) { create(:issue, author: user, project: project) }
+
+  set(:closing_mr) { create_closing_mr(source_project: project) }
+  set(:closing_mr_other_project) { create_closing_mr(source_project: other_project) }
+
+  set(:referencing_mr) { create_referencing_mr(source_project: project, source_branch: 'csv') }
+  set(:referencing_mr_other_project) { create_referencing_mr(source_project: other_project, source_branch: 'csv') }
+
+  let(:service) { described_class.new(project, user) }
+
+  describe '#execute' do
+    it 'returns a list of sorted merge requests' do
+      mrs, closed_by_mrs = service.execute(issue)
+
+      expect(mrs).to eq([closing_mr, referencing_mr, closing_mr_other_project, referencing_mr_other_project])
+      expect(closed_by_mrs).to eq([closing_mr, closing_mr_other_project])
+    end
+
+    context 'performance' do
+      it 'does not run extra queries when extra namespaces are included', :use_clean_rails_memory_store_caching do
+        service.execute(issue) # warm cache
+        control_count = ActiveRecord::QueryRecorder.new { service.execute(issue) }.count
+
+        third_project = create(:project, :public)
+        create_closing_mr(source_project: third_project)
+        service.execute(issue) # warm cache
+
+        expect { service.execute(issue) }.not_to exceed_query_limit(control_count)
+      end
+
+      it 'preloads the head pipeline for each merge request, and its routes' do
+        # Hack to ensure no data is preserved on issue before starting the spec,
+        # to avoid false negatives
+        reloaded_issue = Issue.find(issue.id)
+
+        pipeline_routes = lambda do |merge_requests|
+          merge_requests.map { |mr| mr.head_pipeline&.project&.full_path }
+        end
+
+        closing_mr_other_project.update!(head_pipeline: create(:ci_pipeline))
+        control_count = ActiveRecord::QueryRecorder.new { service.execute(reloaded_issue).each(&pipeline_routes) }
+
+        closing_mr.update!(head_pipeline: create(:ci_pipeline))
+
+        expect { service.execute(issue).each(&pipeline_routes) }
+          .not_to exceed_query_limit(control_count)
+      end
+
+      it 'only loads issue notes once' do
+        expect(issue).to receive(:notes).once.and_call_original
+
+        service.execute(issue)
+      end
+    end
+  end
+
+  describe '#referenced_merge_requests' do
+    it 'returns the referenced merge requests' do
+      expect(service.referenced_merge_requests(issue)).to match_array([
+                                                                        closing_mr,
+                                                                        closing_mr_other_project,
+                                                                        referencing_mr,
+                                                                        referencing_mr_other_project
+                                                                      ])
+    end
+
+    it 'excludes cross project references if the user cannot read cross project' do
+      allow(Ability).to receive(:allowed?).and_call_original
+      expect(Ability).to receive(:allowed?).with(user, :read_cross_project).at_least(:once).and_return(false)
+
+      expect(service.referenced_merge_requests(issue)).not_to include(closing_mr_other_project)
+      expect(service.referenced_merge_requests(issue)).not_to include(referencing_mr_other_project)
+    end
+
+    context 'performance' do
+      it 'does not run a query for each note author', :use_clean_rails_memory_store_caching do
+        service.referenced_merge_requests(issue) # warm cache
+        control_count = ActiveRecord::QueryRecorder.new { service.referenced_merge_requests(issue) }.count
+
+        create(:note, project: project, noteable: issue, author: create(:user))
+        service.referenced_merge_requests(issue) # warm cache
+
+        expect { service.referenced_merge_requests(issue) }.not_to exceed_query_limit(control_count)
+      end
+    end
+  end
+
+  describe '#closed_by_merge_requests' do
+    let(:closed_issue) { build(:issue, :closed, project: project)}
+
+    it 'returns the open merge requests that close this issue' do
+      create_closing_mr(source_project: project, state: 'closed')
+
+      expect(service.closed_by_merge_requests(issue)).to match_array([closing_mr, closing_mr_other_project])
+    end
+
+    it 'returns an empty array when the current issue is closed already' do
+      expect(service.closed_by_merge_requests(closed_issue)).to eq([])
+    end
+
+    context 'performance' do
+      it 'does not run a query for each note author', :use_clean_rails_memory_store_caching do
+        service.closed_by_merge_requests(issue) # warm cache
+        control_count = ActiveRecord::QueryRecorder.new { service.closed_by_merge_requests(issue) }.count
+
+        create(:note, :system, project: project, noteable: issue, author: create(:user))
+        service.closed_by_merge_requests(issue) # warm cache
+
+        expect { service.closed_by_merge_requests(issue) }.not_to exceed_query_limit(control_count)
+      end
+    end
+  end
+end

spec/support/helpers/cycle_analytics_helpers.rb

@@ -65,7 +65,9 @@ module CycleAnalyticsHelpers
   end
 
   def merge_merge_requests_closing_issue(user, project, issue)
-    merge_requests = issue.closed_by_merge_requests(user)
+    merge_requests = Issues::ReferencedMergeRequestsService
+                       .new(project, user)
+                       .closed_by_merge_requests(issue)
 
     merge_requests.each { |merge_request| MergeRequests::MergeService.new(project, user).execute(merge_request) }
   end