Merge branch 'zj-gitaly-pipelines-n-1' into 'master'

Improve performance of Pipelines#index.json

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

4 files changed, 95 insertions, 11 deletions

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 bdc430c9095..1d7069feebd 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