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.

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'

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)

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

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
+      return false unless ref && commit.present?
 
-      commit = project.commit(ref)
+      project.commit(ref) == commit
-      return false unless commit
-
-      commit.sha == sha
     end
 
     def retried

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)
+  def method_missing(method, *args, &block)
-    @raw.__send__(m, *args, &block) # rubocop:disable GitlabSecurity/PublicSend
+    @raw.__send__(method, *args, &block) # rubocop:disable GitlabSecurity/PublicSend
   end
 
   def respond_to_missing?(method, include_private = false)

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,

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
+  .info-well
-  - if @commit.status
+    - if @commit.status
-    .well-segment.pipeline-info
+      .well-segment.pipeline-info
-      .icon-container
+        .icon-container
-        = icon('clock-o')
+          = icon('clock-o')
-      = pluralize @pipeline.total_size, "job"
+        = pluralize @pipeline.total_size, "job"
-      - if @pipeline.ref
+        - if @pipeline.ref
-        from
+          from
-        = link_to @pipeline.ref, project_ref_path(@project, @pipeline.ref), class: "ref-name"
+          = link_to @pipeline.ref, project_ref_path(@project, @pipeline.ref), class: "ref-name"
-      - if @pipeline.duration
+        - if @pipeline.duration
-        in
+          in
-        = time_interval_in_words(@pipeline.duration)
+          = time_interval_in_words(@pipeline.duration)
-      - if @pipeline.queued_duration
+        - if @pipeline.queued_duration
-        = "(queued for #{time_interval_in_words(@pipeline.queued_duration)})"
+          = "(queued for #{time_interval_in_words(@pipeline.queued_duration)})"
 
-  .well-segment.branch-info
+    .well-segment.branch-info
-    .icon-container.commit-icon
+      .icon-container.commit-icon
-      = custom_icon("icon_commit")
+        = custom_icon("icon_commit")
-    = link_to @commit.short_id, project_commit_path(@project, @pipeline.sha), class: "commit-sha js-details-short"
+      = 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
+      = link_to("#", class: "js-details-expand hidden-xs hidden-sm") do
-      %span.text-expander
+        %span.text-expander
-        \...
+          \...
-    %span.js-details-content.hide
+      %span.js-details-content.hide
-      = link_to @pipeline.sha, project_commit_path(@project, @pipeline.sha), class: "commit-sha commit-hash-full"
+        = 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")
+      = clipboard_button(text: @pipeline.sha, title: "Copy commit SHA to clipboard")

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)

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)

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,

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
+      %w(pending running created success).each_with_index do |status, index|
-      parent = branch_head.parent
+        sha = project.commit("HEAD~#{index}")
-
+        create(:ci_empty_pipeline, status: status, project: project, sha: sha)
-      create(:ci_empty_pipeline, status: 'pending', project: project, sha: branch_head.id)
+      end
-      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)
     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

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)

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

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