Merge branch 'fix-slow-queries-for-branches-index' into 'master'

removes n+1 query from tags and branches indexes

See merge request !9905

10 files changed, 56 insertions, 6 deletions

diff --git a/app/controllers/projects/branches_controller.rb b/app/controllers/projects/branches_controller.rb
index 22714d9c5a4..840405f38cb 100644
--- a/app/controllers/projects/branches_controller.rb
+++ b/app/controllers/projects/branches_controller.rb
@@ -11,16 +11,18 @@ class Projects::BranchesController < Projects::ApplicationController
     @sort = params[:sort].presence || sort_value_name
     @branches = BranchesFinder.new(@repository, params).execute
 
-    @branches = Kaminari.paginate_array(@branches).page(params[:page]) unless params[:show_all].present?
-
     respond_to do |format|
       format.html do
+        paginate_branches
+        @refs_pipelines = @project.pipelines.latest_successful_for_refs(@branches.map(&:name))
+
         @max_commits = @branches.reduce(0) do |memo, branch|
           diverging_commit_counts = repository.diverging_commit_counts(branch)
           [memo, diverging_commit_counts[:behind], diverging_commit_counts[:ahead]].max
         end
       end
       format.json do
+        paginate_branches unless params[:show_all]
         render json: @branches.map(&:name)
       end
     end
@@ -91,6 +93,10 @@ class Projects::BranchesController < Projects::ApplicationController
     end
   end
 
+  def paginate_branches
+    @branches = Kaminari.paginate_array(@branches).page(params[:page])
+  end
+
   def url_to_autodeploy_setup(project, branch_name)
     namespace_project_new_blob_path(
       project.namespace,
diff --git a/app/controllers/projects/tags_controller.rb b/app/controllers/projects/tags_controller.rb
index ea7e4d9f663..e13f0bde315 100644
--- a/app/controllers/projects/tags_controller.rb
+++ b/app/controllers/projects/tags_controller.rb
@@ -14,7 +14,9 @@ class Projects::TagsController < Projects::ApplicationController
     @tags = TagsFinder.new(@repository, params).execute
     @tags = Kaminari.paginate_array(@tags).page(params[:page])
 
-    @releases = project.releases.where(tag: @tags.map(&:name))
+    tag_names = @tags.map(&:name)
+    @tags_pipelines = @project.pipelines.latest_successful_for_refs(tag_names)
+    @releases = project.releases.where(tag: tag_names)
   end
 
   def show
diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index 65d08a22b4c..d1009f88549 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -115,6 +115,12 @@ module Ci
       success.latest(ref).order(id: :desc).first
     end
 
+    def self.latest_successful_for_refs(refs)
+      success.latest(refs).order(id: :desc).each_with_object({}) do |pipeline, hash|
+        hash[pipeline.ref] ||= pipeline
+      end
+    end
+
     def self.truncate_sha(sha)
       sha[0...8]
     end
diff --git a/app/views/projects/branches/_branch.html.haml b/app/views/projects/branches/_branch.html.haml
index ae63f8184df..9eb610ba9c0 100644
--- a/app/views/projects/branches/_branch.html.haml
+++ b/app/views/projects/branches/_branch.html.haml
@@ -27,7 +27,7 @@
         = link_to namespace_project_compare_index_path(@project.namespace, @project, from: @repository.root_ref, to: branch.name), class: "btn btn-default #{'prepend-left-10' unless merge_project}", method: :post, title: "Compare" do
           Compare
 
-      = render 'projects/buttons/download', project: @project, ref: branch.name
+      = render 'projects/buttons/download', project: @project, ref: branch.name, pipeline: @refs_pipelines[branch.name]
 
       - if can?(current_user, :push_code, @project)
         = link_to namespace_project_branch_path(@project.namespace, @project, branch.name),
diff --git a/app/views/projects/buttons/_download.html.haml b/app/views/projects/buttons/_download.html.haml
index b560ed21f1d..d90d4a27cd6 100644
--- a/app/views/projects/buttons/_download.html.haml
+++ b/app/views/projects/buttons/_download.html.haml
@@ -1,3 +1,5 @@
+- pipeline = local_assigns.fetch(:pipeline) { project.pipelines.latest_successful_for(ref) }
+
 - if !project.empty_repo? && can?(current_user, :download_code, project)
   .project-action-button.dropdown.inline>
     %button.btn{ 'data-toggle' => 'dropdown' }
@@ -24,7 +26,6 @@
           %i.fa.fa-download
           %span Download tar
 
-      - pipeline = project.pipelines.latest_successful_for(ref)
       - if pipeline
         - artifacts = pipeline.builds.latest.with_artifacts
         - if artifacts.any?
diff --git a/app/views/projects/tags/_tag.html.haml b/app/views/projects/tags/_tag.html.haml
index 8ef069b9e05..dffe908e85a 100644
--- a/app/views/projects/tags/_tag.html.haml
+++ b/app/views/projects/tags/_tag.html.haml
@@ -23,7 +23,7 @@
             = markdown_field(release, :description)
 
   .row-fixed-content.controls
-    = render 'projects/buttons/download', project: @project, ref: tag.name
+    = render 'projects/buttons/download', project: @project, ref: tag.name, pipeline: @tags_pipelines[tag.name]
 
     - if can?(current_user, :push_code, @project)
       = link_to edit_namespace_project_tag_release_path(@project.namespace, @project, tag.name), class: 'btn has-tooltip', title: "Edit release notes", data: { container: "body" } do
diff --git a/changelogs/unreleased/fix-slow-queries-for-branches-index.yml b/changelogs/unreleased/fix-slow-queries-for-branches-index.yml
new file mode 100644
index 00000000000..f5bd7003615
--- /dev/null
+++ b/changelogs/unreleased/fix-slow-queries-for-branches-index.yml
@@ -0,0 +1,4 @@
+---
+title: Fixes n+1 query for tags and branches index page
+merge_request: 9905
+author:
diff --git a/spec/features/projects/branches_spec.rb b/spec/features/projects/branches_spec.rb
index d26a0caf036..8e0306ce83b 100644
--- a/spec/features/projects/branches_spec.rb
+++ b/spec/features/projects/branches_spec.rb
@@ -17,6 +17,14 @@ describe 'Branches', feature: true do
         repository.branches { |branch| expect(page).to have_content("#{branch.name}") }
         expect(page).to have_content("Protected branches can be managed in project settings")
       end
+
+      it 'avoids a N+1 query in branches index' do
+        control_count = ActiveRecord::QueryRecorder.new { visit namespace_project_branches_path(project.namespace, project) }.count
+
+        %w(one two three four five).each { |ref| repository.add_branch(@user, ref, 'master') }
+
+        expect { visit namespace_project_branches_path(project.namespace, project) }.not_to exceed_query_limit(control_count)
+      end
     end
 
     describe 'Find branches' do
diff --git a/spec/features/tags/master_views_tags_spec.rb b/spec/features/tags/master_views_tags_spec.rb
index 29d2c244720..555f84c4772 100644
--- a/spec/features/tags/master_views_tags_spec.rb
+++ b/spec/features/tags/master_views_tags_spec.rb
@@ -27,10 +27,20 @@ feature 'Master views tags', feature: true do
 
   context 'when project has tags' do
     let(:project) { create(:project, namespace: user.namespace) }
+    let(:repository) { project.repository }
+
     before do
       visit namespace_project_tags_path(project.namespace, project)
     end
 
+    scenario 'avoids a N+1 query in branches index' do
+      control_count = ActiveRecord::QueryRecorder.new { visit namespace_project_tags_path(project.namespace, project) }.count
+
+      %w(one two three four five).each { |tag| repository.add_tag(user, tag, 'master', 'foo') }
+
+      expect { visit namespace_project_tags_path(project.namespace, project) }.not_to exceed_query_limit(control_count)
+    end
+
     scenario 'views the tags list page' do
       expect(page).to have_content 'v1.0.0'
     end
diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index 4a664e4fae2..53282b999dc 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -532,6 +532,19 @@ describe Ci::Pipeline, models: true do
     end
   end
 
+  describe '.latest_successful_for_refs' do
+    include_context 'with some outdated pipelines'
+
+    let!(:latest_successful_pipeline1) { create_pipeline(:success, 'ref1', 'D') }
+    let!(:latest_successful_pipeline2) { create_pipeline(:success, 'ref2', 'D') }
+
+    it 'returns the latest successful pipeline for both refs' do
+      refs = %w(ref1 ref2 ref3)
+
+      expect(described_class.latest_successful_for_refs(refs)).to eq({ 'ref1' => latest_successful_pipeline1, 'ref2' => latest_successful_pipeline2 })
+    end
+  end
+
   describe '#status' do
     let(:build) do
       create(:ci_build, :created, pipeline: pipeline, name: 'test')