Merge branch '329751_fix_n+1_problem' into 'master'

Fix N+1 problem for ForksController#new

See merge request gitlab-org/gitlab!62323

app/controllers/projects/forks_controller.rb

@@ -56,7 +56,13 @@ class Projects::ForksController < Projects::ApplicationController
           can_fork_to?(current_user.namespace)
 
         render json: {
-          namespaces: ForkNamespaceSerializer.new.represent(namespaces, project: project, current_user: current_user, memberships: memberships_hash)
+          namespaces: ForkNamespaceSerializer.new.represent(
+            namespaces,
+            project: project,
+            current_user: current_user,
+            memberships: memberships_hash,
+            forked_projects: forked_projects_by_namespace(namespaces)
+          )
         }
       end
     end
@@ -129,6 +135,10 @@ class Projects::ForksController < Projects::ApplicationController
   def memberships_hash
     current_user.members.where(source: load_namespaces_with_associations).index_by(&:source_id)
   end
+
+  def forked_projects_by_namespace(namespaces)
+    project.forks.where(namespace: namespaces).includes(:namespace).index_by(&:namespace_id)
+  end
 end
 
 Projects::ForksController.prepend_mod_with('Projects::ForksController')

app/serializers/fork_namespace_entity.rb

@@ -13,7 +13,7 @@ class ForkNamespaceEntity < Grape::Entity
   end
 
   expose :forked_project_path do |namespace, options|
-    if forked_project = namespace.find_fork_of(options[:project])
+    if forked_project = options.dig(:forked_projects, namespace.id)
       project_path(forked_project)
     end
   end

ee/app/controllers/ee/projects/forks_controller.rb

@@ -14,7 +14,7 @@ module EE
 
       override :load_namespaces_with_associations
       def load_namespaces_with_associations
-        super.with_deletion_schedule_only
+        super.with_deletion_schedule_only.with_saml_provider
       end
     end
   end

ee/app/models/ee/group.rb

@@ -78,6 +78,8 @@ module EE
       scope :with_deletion_schedule, -> { preload(deletion_schedule: :deleting_user) }
       scope :with_deletion_schedule_only, -> { preload(:deletion_schedule) }
 
+      scope :with_saml_provider, -> { preload(:saml_provider) }
+
       scope :where_group_links_with_provider, ->(provider) do
         joins(:ldap_group_links).where(ldap_group_links: { provider: provider })
       end

ee/spec/models/ee/group_spec.rb

@@ -63,6 +63,16 @@ RSpec.describe Group do
       end
     end
 
+    describe '.with_saml_provider' do
+      subject(:relation) { described_class.with_saml_provider }
+
+      it 'preloads saml_providers' do
+        create(:saml_provider, group: group)
+
+        expect(relation.first.association(:saml_provider)).to be_loaded
+      end
+    end
+
     describe '.aimed_for_deletion' do
       let!(:date) { 10.days.ago }
 

spec/controllers/projects/forks_controller_spec.rb

@@ -195,6 +195,29 @@ RSpec.describe Projects::ForksController do
           expect(json_response['namespaces'].length).to eq(1)
           expect(json_response['namespaces'][0]['id']).to eq(group.id)
         end
+
+        context 'N+1 queries' do
+          before do
+            create(:fork_network, root_project: project)
+          end
+
+          it 'avoids N+1 queries' do
+            do_request = -> { get :new, format: format, params: { namespace_id: project.namespace, project_id: project } }
+
+            # warm up
+            do_request.call
+
+            control = ActiveRecord::QueryRecorder.new { do_request.call }
+
+            create(:group, :public).add_owner(user)
+
+            # TODO: There is another N+1 caused by user.can?(:create_projects, namespace)
+            # Defined in ForkNamespaceEntity
+            extra_count = 1
+
+            expect { do_request.call }.not_to exceed_query_limit(control.count + extra_count)
+          end
+        end
       end
     end
 

spec/serializers/fork_namespace_entity_spec.rb

@@ -9,12 +9,15 @@ RSpec.describe ForkNamespaceEntity do
   let_it_be(:user) { create(:user) }
   let_it_be(:project) { create(:project) }
   let_it_be(:namespace) { create(:group, :with_avatar, description: 'test') }
+  let_it_be(:forked_project) { build(:project) }
 
   let(:memberships) do
     user.members.index_by(&:source_id)
   end
 
-  let(:entity) { described_class.new(namespace, current_user: user, project: project, memberships: memberships) }
+  let(:forked_projects) { { namespace.id => forked_project } }
+
+  let(:entity) { described_class.new(namespace, current_user: user, project: project, memberships: memberships, forked_projects: forked_projects) }
 
   subject(:json) { entity.as_json }
 
@@ -46,10 +49,7 @@ RSpec.describe ForkNamespaceEntity do
   end
 
   it 'exposes forked_project_path when fork exists in namespace' do
-    namespace.add_maintainer(user)
-    fork_in_namespace = fork_project(project, user, namespace: namespace)
-
-    expect(json[:forked_project_path]).to eql project_path(fork_in_namespace)
+    expect(json[:forked_project_path]).to eql project_path(forked_project)
   end
 
   it 'exposes relative path to the namespace' do