Merge branch 'projects-controller-show' into 'master'

SQL performance improvements for ProjectsController#show

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

7 files changed, 121 insertions, 16 deletions

diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index 476db384bbd..8d017b9b3b1 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -453,6 +453,10 @@ module Ci
         .fabricate!
     end
 
+    def latest_builds_with_artifacts
+      @latest_builds_with_artifacts ||= builds.latest.with_artifacts
+    end
+
     private
 
     def ci_yaml_from_repo
diff --git a/app/models/project.rb b/app/models/project.rb
index ff5638dd155..add272cc5ce 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1163,6 +1163,23 @@ class Project < ActiveRecord::Base
     pipelines.order(id: :desc).find_by(sha: sha, ref: ref)
   end
 
+  def latest_successful_pipeline_for_default_branch
+    if defined?(@latest_successful_pipeline_for_default_branch)
+      return @latest_successful_pipeline_for_default_branch
+    end
+
+    @latest_successful_pipeline_for_default_branch =
+      pipelines.latest_successful_for(default_branch)
+  end
+
+  def latest_successful_pipeline_for(ref = nil)
+    if ref && ref != default_branch
+      pipelines.latest_successful_for(ref)
+    else
+      latest_successful_pipeline_for_default_branch
+    end
+  end
+
   def enable_ci
     project_feature.update_attribute(:builds_access_level, ProjectFeature::ENABLED)
   end
diff --git a/app/views/projects/buttons/_download.html.haml b/app/views/projects/buttons/_download.html.haml
index 883922dbf04..9d85e027ac9 100644
--- a/app/views/projects/buttons/_download.html.haml
+++ b/app/views/projects/buttons/_download.html.haml
@@ -1,4 +1,4 @@
-- pipeline = local_assigns.fetch(:pipeline) { project.pipelines.latest_successful_for(ref) }
+- pipeline = local_assigns.fetch(:pipeline) { project.latest_successful_pipeline_for(ref) }
 
 - if !project.empty_repo? && can?(current_user, :download_code, project)
   .project-action-button.dropdown.inline>
@@ -26,18 +26,16 @@
           %i.fa.fa-download
           %span= _('Download tar')
 
-      - if pipeline
-        - artifacts = pipeline.builds.latest.with_artifacts
-        - if artifacts.any?
-          %li.dropdown-header Artifacts
-          - unless pipeline.latest?
-            - latest_pipeline = project.pipeline_for(ref)
-            %li
-              .unclickable= ci_status_for_statuseable(latest_pipeline)
-            %li.dropdown-header Previous Artifacts
-          - artifacts.each do |job|
-            %li
-              = link_to latest_succeeded_project_artifacts_path(project, "#{ref}/download", job: job.name), rel: 'nofollow', download: '' do
-                %i.fa.fa-download
-                %span
-                  #{ s_('DownloadArtifacts|Download') } '#{job.name}'
+      - if pipeline && pipeline.latest_builds_with_artifacts.any?
+        %li.dropdown-header Artifacts
+        - unless pipeline.latest?
+          - latest_pipeline = project.pipeline_for(ref)
+          %li
+            .unclickable= ci_status_for_statuseable(latest_pipeline)
+          %li.dropdown-header Previous Artifacts
+        - pipeline.latest_builds_with_artifacts.each do |job|
+          %li
+            = link_to latest_succeeded_project_artifacts_path(project, "#{ref}/download", job: job.name), rel: 'nofollow', download: '' do
+              %i.fa.fa-download
+              %span
+                #{s_('DownloadArtifacts|Download')} '#{job.name}'
diff --git a/changelogs/unreleased/memoize-the-latest-builds-of-a-pipeline.yml b/changelogs/unreleased/memoize-the-latest-builds-of-a-pipeline.yml
new file mode 100644
index 00000000000..5a7cd42b888
--- /dev/null
+++ b/changelogs/unreleased/memoize-the-latest-builds-of-a-pipeline.yml
@@ -0,0 +1,5 @@
+---
+title: "Memoize the latest builds of a pipeline on a project's homepage"
+merge_request:
+author:
+type: other
diff --git a/changelogs/unreleased/projects-controller-show.yml b/changelogs/unreleased/projects-controller-show.yml
new file mode 100644
index 00000000000..25f4a72710b
--- /dev/null
+++ b/changelogs/unreleased/projects-controller-show.yml
@@ -0,0 +1,5 @@
+---
+title: Memoize pipelines for project download buttons
+merge_request:
+author:
+type: other
diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index 95da97b7bc5..77f0be6b120 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -1439,4 +1439,24 @@ describe Ci::Pipeline, :mailer do
       it_behaves_like 'not sending any notification'
     end
   end
+
+  describe '#latest_builds_with_artifacts' do
+    let!(:pipeline) { create(:ci_pipeline, :success) }
+
+    let!(:build) do
+      create(:ci_build, :success, :artifacts, pipeline: pipeline)
+    end
+
+    it 'returns the latest builds' do
+      expect(pipeline.latest_builds_with_artifacts).to eq([build])
+    end
+
+    it 'memoizes the returned relation' do
+      query_count = ActiveRecord::QueryRecorder
+        .new { 2.times { pipeline.latest_builds_with_artifacts.to_a } }
+        .count
+
+      expect(query_count).to eq(1)
+    end
+  end
 end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 48fc77423ff..78226c6c3fa 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -2682,4 +2682,60 @@ describe Project do
       end
     end
   end
+
+  describe '#latest_successful_builds_for' do
+    let(:project) { build(:project) }
+
+    before do
+      allow(project).to receive(:default_branch).and_return('master')
+    end
+
+    context 'without a ref' do
+      it 'returns a pipeline for the default branch' do
+        expect(project)
+          .to receive(:latest_successful_pipeline_for_default_branch)
+
+        project.latest_successful_pipeline_for
+      end
+    end
+
+    context 'with the ref set to the default branch' do
+      it 'returns a pipeline for the default branch' do
+        expect(project)
+          .to receive(:latest_successful_pipeline_for_default_branch)
+
+        project.latest_successful_pipeline_for(project.default_branch)
+      end
+    end
+
+    context 'with a ref that is not the default branch' do
+      it 'returns the latest successful pipeline for the given ref' do
+        expect(project.pipelines).to receive(:latest_successful_for).with('foo')
+
+        project.latest_successful_pipeline_for('foo')
+      end
+    end
+  end
+
+  describe '#latest_successful_pipeline_for_default_branch' do
+    let(:project) { build(:project) }
+
+    before do
+      allow(project).to receive(:default_branch).and_return('master')
+    end
+
+    it 'memoizes and returns the latest successful pipeline for the default branch' do
+      pipeline = double(:pipeline)
+
+      expect(project.pipelines).to receive(:latest_successful_for)
+        .with(project.default_branch)
+        .and_return(pipeline)
+        .once
+
+      2.times do
+        expect(project.latest_successful_pipeline_for_default_branch)
+          .to eq(pipeline)
+      end
+    end
+  end
 end