Merge branch 'osw-remove-n-plus-ones-from-branches-api-call' into 'master'

Remove N+1 DB calls from branches API

See merge request gitlab-org/gitlab!19661

app/models/concerns/protected_ref.rb

@@ -39,8 +39,8 @@ module ProtectedRef
       end
     end
 
-    def developers_can?(action, ref)
-      access_levels_for_ref(ref, action: action).any? do |access_level|
+    def developers_can?(action, ref, protected_refs: nil)
+      access_levels_for_ref(ref, action: action, protected_refs: protected_refs).any? do |access_level|
         access_level.access_level == Gitlab::Access::DEVELOPER
       end
     end

app/models/project.rb

@@ -663,6 +663,11 @@ class Project < ApplicationRecord
     end
   end
 
+  def preload_protected_branches
+    preloader = ActiveRecord::Associations::Preloader.new
+    preloader.preload(self, protected_branches: [:push_access_levels, :merge_access_levels])
+  end
+
   # returns all ancestor-groups upto but excluding the given namespace
   # when no namespace is given, all ancestors upto the top are returned
   def ancestors_upto(top = nil, hierarchy_order: nil)

app/models/protected_branch.rb

@@ -38,7 +38,7 @@ class ProtectedBranch < ApplicationRecord
   end
 
   def self.protected_refs(project)
-    project.protected_branches.select(:name)
+    project.protected_branches
   end
 
   def self.branch_requires_code_owner_approval?(project, branch_name)

changelogs/unreleased/osw-remove-n-plus-ones-from-branches-api-call.yml

+---
+title: Remove N+1 DB calls from branches API
+merge_request: 19661
+author:
+type: performance

lib/api/branches.rb

@@ -32,7 +32,7 @@ module API
         use :filter_params
       end
       get ':id/repository/branches' do
-        Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/42329')
+        user_project.preload_protected_branches
 
         repository = user_project.repository
 

lib/api/entities.rb

@@ -489,11 +489,11 @@ module API
       end
 
       expose :developers_can_push do |repo_branch, options|
-        options[:project].protected_branches.developers_can?(:push, repo_branch.name)
+        ::ProtectedBranch.developers_can?(:push, repo_branch.name, protected_refs: options[:project].protected_branches)
       end
 
       expose :developers_can_merge do |repo_branch, options|
-        options[:project].protected_branches.developers_can?(:merge, repo_branch.name)
+        ::ProtectedBranch.developers_can?(:merge, repo_branch.name, protected_refs: options[:project].protected_branches)
       end
 
       expose :can_push do |repo_branch, options|

spec/requests/api/branches_spec.rb

@@ -119,6 +119,25 @@ describe API::Branches do
 
         it_behaves_like 'repository branches'
       end
+
+      it 'does not submit N+1 DB queries', :request_store do
+        create(:protected_branch, name: 'master', project: project)
+
+        # Make sure no setup step query is recorded.
+        get api(route, current_user), params: { per_page: 100 }
+
+        control = ActiveRecord::QueryRecorder.new do
+          get api(route, current_user), params: { per_page: 100 }
+        end
+
+        new_branch_name = 'protected-branch'
+        CreateBranchService.new(project, current_user).execute(new_branch_name, 'master')
+        create(:protected_branch, name: new_branch_name, project: project)
+
+        expect do
+          get api(route, current_user), params: { per_page: 100 }
+        end.not_to exceed_query_limit(control)
+      end
     end
 
     context 'when authenticated', 'as a guest' do

spec/serializers/pipeline_serializer_spec.rb

@@ -180,7 +180,7 @@ describe PipelineSerializer do
           # pipeline. With the same ref this check is cached but if refs are
           # different then there is an extra query per ref
           # https://gitlab.com/gitlab-org/gitlab-foss/issues/46368
-          expected_queries = Gitlab.ee? ? 44 : 41
+          expected_queries = Gitlab.ee? ? 41 : 38
 
           expect(recorded.count).to be_within(2).of(expected_queries)
           expect(recorded.cached_count).to eq(0)