Load commit in batches for pipelines#index

Uses `list_commits_by_oid` on the CommitService, to request the needed
commits for pipelines. These commits are needed to display the user that
created the commit and the commit title.

This includes fixes for tests failing that depended on the commit
being `nil`. However, now these are batch loaded, this doesn't happen
anymore and the commits are an instance of BatchLoader.

14 files changed, 184 insertions, 49 deletions

diff --git a/Gemfile b/Gemfile
index 6b1c6e16851..b6ffaf80f24 100644
--- a/Gemfile
+++ b/Gemfile
@@ -263,7 +263,7 @@ gem 'gettext_i18n_rails', '~> 1.8.0'
 gem 'gettext_i18n_rails_js', '~> 1.2.0'
 gem 'gettext', '~> 3.2.2', require: false, group: :development
 
-gem 'batch-loader'
+gem 'batch-loader', '~> 1.2.1'
 
 # Perf bar
 gem 'peek', '~> 1.0.1'
diff --git a/Gemfile.lock b/Gemfile.lock
index 11040fab805..a6e3c9e27cc 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -78,7 +78,7 @@ GEM
       thread_safe (~> 0.3, >= 0.3.1)
     babosa (1.0.2)
     base32 (0.3.2)
-    batch-loader (1.1.1)
+    batch-loader (1.2.1)
     bcrypt (3.1.11)
     bcrypt_pbkdf (1.0.0)
     benchmark-ips (2.3.0)
@@ -988,7 +988,7 @@ DEPENDENCIES
   awesome_print (~> 1.2.0)
   babosa (~> 1.0.2)
   base32 (~> 0.3.0)
-  batch-loader
+  batch-loader (~> 1.2.1)
   bcrypt_pbkdf (~> 1.0)
   benchmark-ips (~> 2.3.0)
   better_errors (~> 2.1.0)
diff --git a/app/controllers/projects/pipelines_controller.rb b/app/controllers/projects/pipelines_controller.rb
index 7ad7b3003af..e146d0d3cd5 100644
--- a/app/controllers/projects/pipelines_controller.rb
+++ b/app/controllers/projects/pipelines_controller.rb
@@ -29,6 +29,8 @@ class Projects::PipelinesController < Projects::ApplicationController
     @pipelines_count = PipelinesFinder
       .new(project).execute.count
 
+    @pipelines.map(&:commit) # List commits for batch loading
+
     respond_to do |format|
       format.html
       format.json do
diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index 28f154581a9..d4690da3be6 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -287,8 +287,12 @@ module Ci
       Ci::Pipeline.truncate_sha(sha)
     end
 
+    # NOTE: This is loaded lazily and will never be nil, even if the commit
+    # cannot be found.
+    #
+    # Use constructs like: `pipeline.commit.present?`
     def commit
-      @commit ||= project.commit_by(oid: sha)
+      @commit ||= Commit.lazy(project, sha)
     end
 
     def branch?
@@ -338,12 +342,9 @@ module Ci
     end
 
     def latest?
-      return false unless ref
-
-      commit = project.commit(ref)
-      return false unless commit
+      return false unless ref && commit.present?
 
-      commit.sha == sha
+      project.commit(ref) == commit
     end
 
     def retried
diff --git a/app/models/commit.rb b/app/models/commit.rb
index 13c31111134..2be07ca7d3c 100644
--- a/app/models/commit.rb
+++ b/app/models/commit.rb
@@ -86,6 +86,20 @@ class Commit
     def valid_hash?(key)
       !!(/\A#{COMMIT_SHA_PATTERN}\z/ =~ key)
     end
+
+    def lazy(project, oid)
+      BatchLoader.for({ project: project, oid: oid }).batch do |items, loader|
+        items_by_project = items.group_by { |i| i[:project] }
+
+        items_by_project.each do |project, commit_ids|
+          oids = commit_ids.map { |i| i[:oid] }
+
+          project.repository.commits_by(oids: oids).each do |commit|
+            loader.call({ project: commit.project, oid: commit.id }, commit) if commit
+          end
+        end
+      end
+    end
   end
 
   attr_accessor :raw
@@ -103,7 +117,7 @@ class Commit
   end
 
   def ==(other)
-    (self.class === other) && (raw == other.raw)
+    other.is_a?(self.class) && raw == other.raw
   end
 
   def self.reference_prefix
@@ -224,8 +238,8 @@ class Commit
     notes.includes(:author)
   end
 
-  def method_missing(m, *args, &block)
-    @raw.__send__(m, *args, &block) # rubocop:disable GitlabSecurity/PublicSend
+  def method_missing(method, *args, &block)
+    @raw.__send__(method, *args, &block) # rubocop:disable GitlabSecurity/PublicSend
   end
 
   def respond_to_missing?(method, include_private = false)
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 552a354d1ce..fc7081a9f34 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -118,6 +118,18 @@ class Repository
     @commit_cache[oid] = find_commit(oid)
   end
 
+  def commits_by(oids:)
+    return [] unless oids.present?
+
+    commits = Gitlab::Git::Commit.batch_by_oid(raw_repository, oids)
+
+    if commits.present?
+      Commit.decorate(commits, @project)
+    else
+      []
+    end
+  end
+
   def commits(ref, path: nil, limit: nil, offset: nil, skip_merges: false, after: nil, before: nil)
     options = {
       repo: raw_repository,
diff --git a/app/views/projects/pipelines/_info.html.haml b/app/views/projects/pipelines/_info.html.haml
index 01ea9356af5..85946aec1f2 100644
--- a/app/views/projects/pipelines/_info.html.haml
+++ b/app/views/projects/pipelines/_info.html.haml
@@ -1,6 +1,6 @@
 #js-pipeline-header-vue.pipeline-header-container
 
-- if @commit
+- if @commit.present?
   .commit-box
     %h3.commit-title
       = markdown(@commit.title, pipeline: :single_line)
@@ -8,28 +8,28 @@
       %pre.commit-description
         = preserve(markdown(@commit.description, pipeline: :single_line))
 
-.info-well
-  - if @commit.status
-    .well-segment.pipeline-info
-      .icon-container
-        = icon('clock-o')
-      = pluralize @pipeline.total_size, "job"
-      - if @pipeline.ref
-        from
-        = link_to @pipeline.ref, project_ref_path(@project, @pipeline.ref), class: "ref-name"
-      - if @pipeline.duration
-        in
-        = time_interval_in_words(@pipeline.duration)
-      - if @pipeline.queued_duration
-        = "(queued for #{time_interval_in_words(@pipeline.queued_duration)})"
+  .info-well
+    - if @commit.status
+      .well-segment.pipeline-info
+        .icon-container
+          = icon('clock-o')
+        = pluralize @pipeline.total_size, "job"
+        - if @pipeline.ref
+          from
+          = link_to @pipeline.ref, project_ref_path(@project, @pipeline.ref), class: "ref-name"
+        - if @pipeline.duration
+          in
+          = time_interval_in_words(@pipeline.duration)
+        - if @pipeline.queued_duration
+          = "(queued for #{time_interval_in_words(@pipeline.queued_duration)})"
 
-  .well-segment.branch-info
-    .icon-container.commit-icon
-      = custom_icon("icon_commit")
-    = link_to @commit.short_id, project_commit_path(@project, @pipeline.sha), class: "commit-sha js-details-short"
-    = link_to("#", class: "js-details-expand hidden-xs hidden-sm") do
-      %span.text-expander
-        \...
-    %span.js-details-content.hide
-      = link_to @pipeline.sha, project_commit_path(@project, @pipeline.sha), class: "commit-sha commit-hash-full"
-    = clipboard_button(text: @pipeline.sha, title: "Copy commit SHA to clipboard")
+    .well-segment.branch-info
+      .icon-container.commit-icon
+        = custom_icon("icon_commit")
+      = link_to @commit.short_id, project_commit_path(@project, @pipeline.sha), class: "commit-sha js-details-short"
+      = link_to("#", class: "js-details-expand hidden-xs hidden-sm") do
+        %span.text-expander
+          \...
+      %span.js-details-content.hide
+        = link_to @pipeline.sha, project_commit_path(@project, @pipeline.sha), class: "commit-sha commit-hash-full"
+      = clipboard_button(text: @pipeline.sha, title: "Copy commit SHA to clipboard")
diff --git a/app/workers/expire_pipeline_cache_worker.rb b/app/workers/expire_pipeline_cache_worker.rb
index 3e34de22c19..db73d37868a 100644
--- a/app/workers/expire_pipeline_cache_worker.rb
+++ b/app/workers/expire_pipeline_cache_worker.rb
@@ -13,7 +13,7 @@ class ExpirePipelineCacheWorker
 
     store.touch(project_pipelines_path(project))
     store.touch(project_pipeline_path(project, pipeline))
-    store.touch(commit_pipelines_path(project, pipeline.commit)) if pipeline.commit
+    store.touch(commit_pipelines_path(project, pipeline.commit)) unless pipeline.commit.nil?
     store.touch(new_merge_request_pipelines_path(project))
     each_pipelines_merge_request_path(project, pipeline) do |path|
       store.touch(path)
diff --git a/lib/gitlab/git/commit.rb b/lib/gitlab/git/commit.rb
index e90b158fb34..145721dea76 100644
--- a/lib/gitlab/git/commit.rb
+++ b/lib/gitlab/git/commit.rb
@@ -228,6 +228,19 @@ module Gitlab
             end
           end
         end
+
+        # Only to be used when the object ids will not necessarily have a
+        # relation to each other. The last 10 commits for a branch for example,
+        # should go through .where
+        def batch_by_oid(repo, oids)
+          repo.gitaly_migrate(:list_commits_by_oid) do |is_enabled|
+            if is_enabled
+              repo.gitaly_commit_client.list_commits_by_oid(oids)
+            else
+              oids.map { |oid| find(repo, oid) }.compact
+            end
+          end
+        end
       end
 
       def initialize(repository, raw_commit, head = nil)
diff --git a/lib/gitlab/gitaly_client/commit_service.rb b/lib/gitlab/gitaly_client/commit_service.rb
index 7985f5b5457..fb3e27770b4 100644
--- a/lib/gitlab/gitaly_client/commit_service.rb
+++ b/lib/gitlab/gitaly_client/commit_service.rb
@@ -169,6 +169,15 @@ module Gitlab
         consume_commits_response(response)
       end
 
+      def list_commits_by_oid(oids)
+        request = Gitaly::ListCommitsByOidRequest.new(repository: @gitaly_repo, oid: oids)
+
+        response = GitalyClient.call(@repository.storage, :commit_service, :list_commits_by_oid, request, timeout: GitalyClient.medium_timeout)
+        consume_commits_response(response)
+      rescue GRPC::Unknown # If no repository is found, happens mainly during testing
+        []
+      end
+
       def commits_by_message(query, revision: '', path: '', limit: 1000, offset: 0)
         request = Gitaly::CommitsByMessageRequest.new(
           repository: @gitaly_repo,
diff --git a/spec/controllers/projects/pipelines_controller_spec.rb b/spec/controllers/projects/pipelines_controller_spec.rb
index 1604a2da485..35ac999cc65 100644
--- a/spec/controllers/projects/pipelines_controller_spec.rb
+++ b/spec/controllers/projects/pipelines_controller_spec.rb
@@ -17,13 +17,10 @@ describe Projects::PipelinesController do
 
   describe 'GET index.json' do
     before do
-      branch_head = project.commit
-      parent = branch_head.parent
-
-      create(:ci_empty_pipeline, status: 'pending', project: project, sha: branch_head.id)
-      create(:ci_empty_pipeline, status: 'running', project: project, sha: branch_head.id)
-      create(:ci_empty_pipeline, status: 'created', project: project, sha: parent.id)
-      create(:ci_empty_pipeline, status: 'success', project: project, sha: parent.id)
+      %w(pending running created success).each_with_index do |status, index|
+        sha = project.commit("HEAD~#{index}")
+        create(:ci_empty_pipeline, status: status, project: project, sha: sha)
+      end
     end
 
     subject do
@@ -46,7 +43,7 @@ describe Projects::PipelinesController do
 
     context 'when performing gitaly calls', :request_store do
       it 'limits the Gitaly requests' do
-        expect { subject }.to change { Gitlab::GitalyClient.get_request_count }.by(8)
+        expect { subject }.to change { Gitlab::GitalyClient.get_request_count }.by(3)
       end
     end
   end
diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb
index d18a5c9dfa6..cd955a5eb69 100644
--- a/spec/models/commit_spec.rb
+++ b/spec/models/commit_spec.rb
@@ -13,6 +13,45 @@ describe Commit do
     it { is_expected.to include_module(StaticModel) }
   end
 
+  describe '.lazy' do
+    set(:project) { create(:project, :repository) }
+
+    context 'when the commits are found' do
+      let(:oids) do
+        %w(
+          498214de67004b1da3d820901307bed2a68a8ef6
+          c642fe9b8b9f28f9225d7ea953fe14e74748d53b
+          6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9
+          048721d90c449b244b7b4c53a9186b04330174ec
+          281d3a76f31c812dbf48abce82ccf6860adedd81
+        )
+      end
+
+      subject { oids.map { |oid| described_class.lazy(project, oid) } }
+
+      it 'batches requests for commits' do
+        expect(project.repository).to receive(:commits_by).once.and_call_original
+
+        subject.first.title
+        subject.last.title
+      end
+
+      it 'maintains ordering' do
+        subject.each_with_index do |commit, i|
+          expect(commit.id).to eq(oids[i])
+        end
+      end
+    end
+
+    context 'when not found' do
+      it 'returns nil as commit' do
+        commit = described_class.lazy(project, 'deadbeef').__sync
+
+        expect(commit).to be_nil
+      end
+    end
+  end
+
   describe '#author' do
     it 'looks up the author in a case-insensitive way' do
       user = create(:user, email: commit.author_email.upcase)
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index 799d99c0369..8a3531c1898 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -239,6 +239,54 @@ describe Repository do
     end
   end
 
+  describe '#commits_by' do
+    set(:project) { create(:project, :repository) }
+
+    shared_examples 'batch commits fetching' do
+      let(:oids) { TestEnv::BRANCH_SHA.values }
+
+      subject { project.repository.commits_by(oids: oids) }
+
+      it 'finds each commit' do
+        expect(subject).not_to include(nil)
+        expect(subject.size).to eq(oids.size)
+      end
+
+      it 'returns only Commit instances' do
+        expect(subject).to all( be_a(Commit) )
+      end
+
+      context 'when some commits are not found ' do
+        let(:oids) do
+          ['deadbeef'] + TestEnv::BRANCH_SHA.values.first(10)
+        end
+
+        it 'returns only found commits' do
+          expect(subject).not_to include(nil)
+          expect(subject.size).to eq(10)
+        end
+      end
+
+      context 'when no oids are passed' do
+        let(:oids) { [] }
+
+        it 'does not call #batch_by_oid' do
+          expect(Gitlab::Git::Commit).not_to receive(:batch_by_oid)
+
+          subject
+        end
+      end
+    end
+
+    context 'when Gitaly list_commits_by_oid is enabled' do
+      it_behaves_like 'batch commits fetching'
+    end
+
+    context 'when Gitaly list_commits_by_oid is enabled', :disable_gitaly do
+      it_behaves_like 'batch commits fetching'
+    end
+  end
+
   describe '#find_commits_by_message' do
     shared_examples 'finding commits by message' do
       it 'returns commits with messages containing a given string' do
diff --git a/spec/serializers/pipeline_serializer_spec.rb b/spec/serializers/pipeline_serializer_spec.rb
index 88d347322a6..c38795ad1a1 100644
--- a/spec/serializers/pipeline_serializer_spec.rb
+++ b/spec/serializers/pipeline_serializer_spec.rb
@@ -1,6 +1,7 @@
 require 'spec_helper'
 
 describe PipelineSerializer do
+  set(:project) { create(:project, :repository) }
   set(:user) { create(:user) }
 
   let(:serializer) do
@@ -16,7 +17,7 @@ describe PipelineSerializer do
       end
 
       context 'when a single object is being serialized' do
-        let(:resource) { create(:ci_empty_pipeline) }
+        let(:resource) { create(:ci_empty_pipeline, project: project) }
 
         it 'serializers the pipeline object' do
           expect(subject[:id]).to eq resource.id
@@ -24,7 +25,7 @@ describe PipelineSerializer do
       end
 
       context 'when multiple objects are being serialized' do
-        let(:resource) { create_list(:ci_pipeline, 2) }
+        let(:resource) { create_list(:ci_pipeline, 2, project: project) }
 
         it 'serializers the array of pipelines' do
           expect(subject).not_to be_empty
@@ -100,7 +101,6 @@ describe PipelineSerializer do
 
     context 'number of queries' do
       let(:resource) { Ci::Pipeline.all }
-      let(:project) { create(:project) }
 
       before do
         # Since RequestStore.active? is true we have to allow the