Merge branch...

Merge branch '21087-controller-projects-mergerequests-creationscontroller-new-executes-more-than-100-sql-queries' into 'master'

Improve page load performance of new merge request page by eliminating N+1 queries

See merge request gitlab-org/gitlab!21157

app/finders/merge_request_target_project_finder.rb

@@ -11,15 +11,23 @@ class MergeRequestTargetProjectFinder
   end
 
   # rubocop: disable CodeReuse/ActiveRecord
-  def execute
-    if @source_project.fork_network
-      @source_project.fork_network.projects
-        .public_or_visible_to_user(current_user)
-        .non_archived
-        .with_feature_available_for_user(:merge_requests, current_user)
+  def execute(include_routes: false)
+    if source_project.fork_network
+      include_routes ? projects.inc_routes : projects
     else
       Project.where(id: source_project)
     end
   end
   # rubocop: enable CodeReuse/ActiveRecord
+
+  private
+
+  def projects
+    source_project
+      .fork_network
+      .projects
+      .public_or_visible_to_user(current_user)
+      .non_archived
+      .with_feature_available_for_user(:merge_requests, current_user)
+  end
 end

app/helpers/merge_requests_helper.rb

@@ -88,7 +88,7 @@ module MergeRequestsHelper
 
   def target_projects(project)
     MergeRequestTargetProjectFinder.new(current_user: current_user, source_project: project)
-      .execute
+      .execute(include_routes: true)
   end
 
   def merge_request_button_visibility(merge_request, closed)

app/models/project.rb

@@ -408,6 +408,7 @@ class Project < ApplicationRecord
   scope :for_milestones, ->(ids) { joins(:milestones).where('milestones.id' => ids).distinct }
   scope :with_push, -> { joins(:events).where('events.action = ?', Event::PUSHED) }
   scope :with_project_feature, -> { joins('LEFT JOIN project_features ON projects.id = project_features.project_id') }
+  scope :inc_routes, -> { includes(:route, namespace: :route) }
   scope :with_statistics, -> { includes(:statistics) }
   scope :with_service, ->(service) { joins(service).eager_load(service) }
   scope :with_shared_runners, -> { where(shared_runners_enabled: true) }

spec/finders/merge_request_target_project_finder_spec.rb

@@ -27,6 +27,22 @@ describe MergeRequestTargetProjectFinder do
 
       expect(finder.execute).to contain_exactly(other_fork, forked_project)
     end
+
+    it 'does not include routes by default' do
+      row = finder.execute.first
+
+      expect(row.association(:route).loaded?).to be_falsey
+      expect(row.association(:namespace).loaded?).to be_falsey
+      expect(row.namespace.association(:route).loaded?).to be_falsey
+    end
+
+    it 'includes routes when requested' do
+      row = finder.execute(include_routes: true).first
+
+      expect(row.association(:route).loaded?).to be_truthy
+      expect(row.association(:namespace).loaded?).to be_truthy
+      expect(row.namespace.association(:route).loaded?).to be_truthy
+    end
   end
 
   context 'public projects' do

spec/models/project_spec.rb

@@ -1786,11 +1786,11 @@ describe Project do
     end
   end
 
-  describe '.including_namespace_and_owner' do
+  describe '.eager_load_namespace_and_owner' do
     it 'eager loads the namespace and namespace owner' do
       create(:project)
 
-      row = described_class.eager_load_namespace_and_owner.to_a.first
+      row = described_class.eager_load_namespace_and_owner.first
       recorder = ActiveRecord::QueryRecorder.new { row.namespace.owner }
 
       expect(recorder.count).to be_zero

spec/requests/projects/merge_requests/creations_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'merge requests creations' do
+  describe 'GET /:namespace/:project/merge_requests/new' do
+    include ProjectForksHelper
+
+    let(:project) { create(:project, :repository) }
+    let(:user) { project.owner }
+
+    before do
+      login_as(user)
+    end
+
+    def get_new
+      get namespace_project_new_merge_request_path(namespace_id: project.namespace, project_id: project)
+    end
+
+    it 'avoids N+1 DB queries even with forked projects' do
+      control = ActiveRecord::QueryRecorder.new(skip_cached: false) { get_new }
+
+      5.times { fork_project(project, user) }
+
+      expect { get_new }.not_to exceed_query_limit(control)
+    end
+  end
+end