summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorKamil TrzciƄski <ayufan@ayufan.eu>2017-11-17 12:56:12 +0000
committerWinnie Hellmann <winnie@gitlab.com>2017-11-17 14:25:00 +0000
commit40413868083a88f9b5386165184be6758e20bc33 (patch)
tree638526ba7034c99b665ddcae8538f37e8edccfbe
parent4dd9988470fdf2d7e122579602491327697c0f93 (diff)
downloadgitlab-ce-40413868083a88f9b5386165184be6758e20bc33.tar.gz
Merge branch 'ci-pipeline-status-query' into 'master'
Optimise getting the pipeline status of commits See merge request gitlab-org/gitlab-ce!15332 (cherry picked from commit 6d36c3b5de7772415b1176b8247c34053b826a74) ab16a6fb Optimise getting the pipeline status of commits
-rw-r--r--app/controllers/projects/commits_controller.rb1
-rw-r--r--app/controllers/projects/merge_requests_controller.rb3
-rw-r--r--app/models/ci/pipeline.rb66
-rw-r--r--app/models/commit.rb9
-rw-r--r--app/models/commit_collection.rb44
-rw-r--r--app/models/merge_request_diff.rb4
-rw-r--r--app/models/repository.rb16
-rw-r--r--changelogs/unreleased/ci-pipeline-status-query.yml5
-rw-r--r--spec/models/ci/pipeline_spec.rb122
-rw-r--r--spec/models/commit_collection_spec.rb59
-rw-r--r--spec/models/commit_spec.rb11
11 files changed, 274 insertions, 66 deletions
diff --git a/app/controllers/projects/commits_controller.rb b/app/controllers/projects/commits_controller.rb
index 28920877635..5f4afd2cdee 100644
--- a/app/controllers/projects/commits_controller.rb
+++ b/app/controllers/projects/commits_controller.rb
@@ -57,6 +57,7 @@ class Projects::CommitsController < Projects::ApplicationController
@repository.commits(@ref, path: @path, limit: @limit, offset: @offset)
end
+ @commits = @commits.with_pipeline_status
@commits = prepare_commits_for_rendering(@commits)
end
end
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 22de6680511..abe4e5245b1 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -80,7 +80,8 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
def commits
# Get commits from repository
# or from cache if already merged
- @commits = prepare_commits_for_rendering(@merge_request.commits)
+ @commits =
+ prepare_commits_for_rendering(@merge_request.commits.with_pipeline_status)
render json: { html: view_to_html_string('projects/merge_requests/_commits') }
end
diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index fcbe3d2b67b..797a12d4e63 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -149,34 +149,70 @@ module Ci
end
end
- # ref can't be HEAD or SHA, can only be branch/tag name
- scope :latest, ->(ref = nil) do
- max_id = unscope(:select)
- .select("max(#{quoted_table_name}.id)")
- .group(:ref, :sha)
-
- if ref
- where(ref: ref, id: max_id.where(ref: ref))
- else
- where(id: max_id)
- end
- end
scope :internal, -> { where(source: internal_sources) }
+ # Returns the pipelines in descending order (= newest first), optionally
+ # limited to a number of references.
+ #
+ # ref - The name (or names) of the branch(es)/tag(s) to limit the list of
+ # pipelines to.
+ def self.newest_first(ref = nil)
+ relation = order(id: :desc)
+
+ ref ? relation.where(ref: ref) : relation
+ end
+
def self.latest_status(ref = nil)
- latest(ref).status
+ newest_first(ref).pluck(:status).first
end
def self.latest_successful_for(ref)
- success.latest(ref).order(id: :desc).first
+ newest_first(ref).success.take
end
def self.latest_successful_for_refs(refs)
- success.latest(refs).order(id: :desc).each_with_object({}) do |pipeline, hash|
+ relation = newest_first(refs).success
+
+ relation.each_with_object({}) do |pipeline, hash|
hash[pipeline.ref] ||= pipeline
end
end
+ # Returns a Hash containing the latest pipeline status for every given
+ # commit.
+ #
+ # The keys of this Hash are the commit SHAs, the values the statuses.
+ #
+ # commits - The list of commit SHAs to get the status for.
+ # ref - The ref to scope the data to (e.g. "master"). If the ref is not
+ # given we simply get the latest status for the commits, regardless
+ # of what refs their pipelines belong to.
+ def self.latest_status_per_commit(commits, ref = nil)
+ p1 = arel_table
+ p2 = arel_table.alias
+
+ # This LEFT JOIN will filter out all but the newest row for every
+ # combination of (project_id, sha) or (project_id, sha, ref) if a ref is
+ # given.
+ cond = p1[:sha].eq(p2[:sha])
+ .and(p1[:project_id].eq(p2[:project_id]))
+ .and(p1[:id].lt(p2[:id]))
+
+ cond = cond.and(p1[:ref].eq(p2[:ref])) if ref
+ join = p1.join(p2, Arel::Nodes::OuterJoin).on(cond)
+
+ relation = select(:sha, :status)
+ .where(sha: commits)
+ .where(p2[:id].eq(nil))
+ .joins(join.join_sources)
+
+ relation = relation.where(ref: ref) if ref
+
+ relation.each_with_object({}) do |row, hash|
+ hash[row[:sha]] = row[:status]
+ end
+ end
+
def self.truncate_sha(sha)
sha[0...8]
end
diff --git a/app/models/commit.rb b/app/models/commit.rb
index 6dba154a6ea..a31ebe9cc87 100644
--- a/app/models/commit.rb
+++ b/app/models/commit.rb
@@ -80,6 +80,7 @@ class Commit
@raw = raw_commit
@project = project
+ @statuses = {}
end
def id
@@ -236,11 +237,13 @@ class Commit
end
def status(ref = nil)
- @statuses ||= {}
-
return @statuses[ref] if @statuses.key?(ref)
- @statuses[ref] = pipelines.latest_status(ref)
+ @statuses[ref] = project.pipelines.latest_status_per_commit(id, ref)[id]
+ end
+
+ def set_status_for_ref(ref, status)
+ @statuses[ref] = status
end
def signature
diff --git a/app/models/commit_collection.rb b/app/models/commit_collection.rb
new file mode 100644
index 00000000000..dd93af9df64
--- /dev/null
+++ b/app/models/commit_collection.rb
@@ -0,0 +1,44 @@
+# frozen_string_literal: true
+
+# A collection of Commit instances for a specific project and Git reference.
+class CommitCollection
+ include Enumerable
+
+ attr_reader :project, :ref, :commits
+
+ # project - The project the commits belong to.
+ # commits - The Commit instances to store.
+ # ref - The name of the ref (e.g. "master").
+ def initialize(project, commits, ref = nil)
+ @project = project
+ @commits = commits
+ @ref = ref
+ end
+
+ def each(&block)
+ commits.each(&block)
+ end
+
+ # Sets the pipeline status for every commit.
+ #
+ # Setting this status ahead of time removes the need for running a query for
+ # every commit we're displaying.
+ def with_pipeline_status
+ statuses = project.pipelines.latest_status_per_commit(map(&:id), ref)
+
+ each do |commit|
+ commit.set_status_for_ref(ref, statuses[commit.id])
+ end
+
+ self
+ end
+
+ def respond_to_missing?(message, inc_private = false)
+ commits.respond_to?(message, inc_private)
+ end
+
+ # rubocop:disable GitlabSecurity/PublicSend
+ def method_missing(message, *args, &block)
+ commits.public_send(message, *args, &block)
+ end
+end
diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb
index 1eda0f9cbbd..5382f5cc627 100644
--- a/app/models/merge_request_diff.rb
+++ b/app/models/merge_request_diff.rb
@@ -284,8 +284,10 @@ class MergeRequestDiff < ActiveRecord::Base
def load_commits
commits = st_commits.presence || merge_request_diff_commits
+ commits = commits.map { |commit| Commit.from_hash(commit.to_hash, project) }
- commits.map { |commit| Commit.from_hash(commit.to_hash, project) }
+ CommitCollection
+ .new(merge_request.source_project, commits, merge_request.source_branch)
end
def save_diffs
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 6bdee538172..b7a5839f6cd 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -132,7 +132,8 @@ class Repository
commits = Gitlab::Git::Commit.where(options)
commits = Commit.decorate(commits, @project) if commits.present?
- commits
+
+ CommitCollection.new(project, commits, ref)
end
def commits_between(from, to)
@@ -148,11 +149,14 @@ class Repository
end
raw_repository.gitaly_migrate(:commits_by_message) do |is_enabled|
- if is_enabled
- find_commits_by_message_by_gitaly(query, ref, path, limit, offset)
- else
- find_commits_by_message_by_shelling_out(query, ref, path, limit, offset)
- end
+ commits =
+ if is_enabled
+ find_commits_by_message_by_gitaly(query, ref, path, limit, offset)
+ else
+ find_commits_by_message_by_shelling_out(query, ref, path, limit, offset)
+ end
+
+ CommitCollection.new(project, commits, ref)
end
end
diff --git a/changelogs/unreleased/ci-pipeline-status-query.yml b/changelogs/unreleased/ci-pipeline-status-query.yml
new file mode 100644
index 00000000000..a464e501418
--- /dev/null
+++ b/changelogs/unreleased/ci-pipeline-status-query.yml
@@ -0,0 +1,5 @@
+---
+title: Optimise getting the pipeline status of commits
+merge_request:
+author:
+type: performance
diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index 2c9e7013b77..b89b0e555d9 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -625,38 +625,29 @@ describe Ci::Pipeline, :mailer do
shared_context 'with some outdated pipelines' do
before do
- create_pipeline(:canceled, 'ref', 'A')
- create_pipeline(:success, 'ref', 'A')
- create_pipeline(:failed, 'ref', 'B')
- create_pipeline(:skipped, 'feature', 'C')
+ create_pipeline(:canceled, 'ref', 'A', project)
+ create_pipeline(:success, 'ref', 'A', project)
+ create_pipeline(:failed, 'ref', 'B', project)
+ create_pipeline(:skipped, 'feature', 'C', project)
end
- def create_pipeline(status, ref, sha)
- create(:ci_empty_pipeline, status: status, ref: ref, sha: sha)
+ def create_pipeline(status, ref, sha, project)
+ create(
+ :ci_empty_pipeline,
+ status: status,
+ ref: ref,
+ sha: sha,
+ project: project
+ )
end
end
- describe '.latest' do
+ describe '.newest_first' do
include_context 'with some outdated pipelines'
- context 'when no ref is specified' do
- let(:pipelines) { described_class.latest.all }
-
- it 'returns the latest pipeline for the same ref and different sha' do
- expect(pipelines.map(&:sha)).to contain_exactly('A', 'B', 'C')
- expect(pipelines.map(&:status))
- .to contain_exactly('success', 'failed', 'skipped')
- end
- end
-
- context 'when ref is specified' do
- let(:pipelines) { described_class.latest('ref').all }
-
- it 'returns the latest pipeline for ref and different sha' do
- expect(pipelines.map(&:sha)).to contain_exactly('A', 'B')
- expect(pipelines.map(&:status))
- .to contain_exactly('success', 'failed')
- end
+ it 'returns the pipelines from new to old' do
+ expect(described_class.newest_first.pluck(:status))
+ .to eq(%w[skipped failed success canceled])
end
end
@@ -664,20 +655,14 @@ describe Ci::Pipeline, :mailer do
include_context 'with some outdated pipelines'
context 'when no ref is specified' do
- let(:latest_status) { described_class.latest_status }
-
- it 'returns the latest status for the same ref and different sha' do
- expect(latest_status).to eq(described_class.latest.status)
- expect(latest_status).to eq('failed')
+ it 'returns the status of the latest pipeline' do
+ expect(described_class.latest_status).to eq('skipped')
end
end
context 'when ref is specified' do
- let(:latest_status) { described_class.latest_status('ref') }
-
- it 'returns the latest status for ref and different sha' do
- expect(latest_status).to eq(described_class.latest_status('ref'))
- expect(latest_status).to eq('failed')
+ it 'returns the status of the latest pipeline for the given ref' do
+ expect(described_class.latest_status('ref')).to eq('failed')
end
end
end
@@ -686,7 +671,7 @@ describe Ci::Pipeline, :mailer do
include_context 'with some outdated pipelines'
let!(:latest_successful_pipeline) do
- create_pipeline(:success, 'ref', 'D')
+ create_pipeline(:success, 'ref', 'D', project)
end
it 'returns the latest successful pipeline' do
@@ -698,8 +683,13 @@ describe Ci::Pipeline, :mailer do
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') }
+ let!(:latest_successful_pipeline1) do
+ create_pipeline(:success, 'ref1', 'D', project)
+ end
+
+ let!(:latest_successful_pipeline2) do
+ create_pipeline(:success, 'ref2', 'D', project)
+ end
it 'returns the latest successful pipeline for both refs' do
refs = %w(ref1 ref2 ref3)
@@ -708,6 +698,62 @@ describe Ci::Pipeline, :mailer do
end
end
+ describe '.latest_status_per_commit' do
+ let(:project) { create(:project) }
+
+ before do
+ pairs = [
+ %w[success ref1 123],
+ %w[manual master 123],
+ %w[failed ref 456]
+ ]
+
+ pairs.each do |(status, ref, sha)|
+ create(
+ :ci_empty_pipeline,
+ status: status,
+ ref: ref,
+ sha: sha,
+ project: project
+ )
+ end
+ end
+
+ context 'without a ref' do
+ it 'returns a Hash containing the latest status per commit for all refs' do
+ expect(described_class.latest_status_per_commit(%w[123 456]))
+ .to eq({ '123' => 'manual', '456' => 'failed' })
+ end
+
+ it 'only includes the status of the given commit SHAs' do
+ expect(described_class.latest_status_per_commit(%w[123]))
+ .to eq({ '123' => 'manual' })
+ end
+
+ context 'when there are two pipelines for a ref and SHA' do
+ it 'returns the status of the latest pipeline' do
+ create(
+ :ci_empty_pipeline,
+ status: 'failed',
+ ref: 'master',
+ sha: '123',
+ project: project
+ )
+
+ expect(described_class.latest_status_per_commit(%w[123]))
+ .to eq({ '123' => 'failed' })
+ end
+ end
+ end
+
+ context 'with a ref' do
+ it 'only includes the pipelines for the given ref' do
+ expect(described_class.latest_status_per_commit(%w[123 456], 'master'))
+ .to eq({ '123' => 'manual' })
+ end
+ end
+ end
+
describe '.internal_sources' do
subject { described_class.internal_sources }
diff --git a/spec/models/commit_collection_spec.rb b/spec/models/commit_collection_spec.rb
new file mode 100644
index 00000000000..066fe7d154e
--- /dev/null
+++ b/spec/models/commit_collection_spec.rb
@@ -0,0 +1,59 @@
+require 'spec_helper'
+
+describe CommitCollection do
+ let(:project) { create(:project, :repository) }
+ let(:commit) { project.commit }
+
+ describe '#each' do
+ it 'yields every commit' do
+ collection = described_class.new(project, [commit])
+
+ expect { |b| collection.each(&b) }.to yield_with_args(commit)
+ end
+ end
+
+ describe '#with_pipeline_status' do
+ it 'sets the pipeline status for every commit so no additional queries are necessary' do
+ create(
+ :ci_empty_pipeline,
+ ref: 'master',
+ sha: commit.id,
+ status: 'success',
+ project: project
+ )
+
+ collection = described_class.new(project, [commit])
+ collection.with_pipeline_status
+
+ recorder = ActiveRecord::QueryRecorder.new do
+ expect(commit.status).to eq('success')
+ end
+
+ expect(recorder.count).to be_zero
+ end
+ end
+
+ describe '#respond_to_missing?' do
+ it 'returns true when the underlying Array responds to the message' do
+ collection = described_class.new(project, [])
+
+ expect(collection.respond_to?(:last)).to eq(true)
+ end
+
+ it 'returns false when the underlying Array does not respond to the message' do
+ collection = described_class.new(project, [])
+
+ expect(collection.respond_to?(:foo)).to eq(false)
+ end
+ end
+
+ describe '#method_missing' do
+ it 'delegates undefined methods to the underlying Array' do
+ collection = described_class.new(project, [commit])
+
+ expect(collection.length).to eq(1)
+ expect(collection.last).to eq(commit)
+ expect(collection).not_to be_empty
+ end
+ end
+end
diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb
index e3cfa149e3a..d18a5c9dfa6 100644
--- a/spec/models/commit_spec.rb
+++ b/spec/models/commit_spec.rb
@@ -351,12 +351,19 @@ eos
end
it 'gives compound status from latest pipelines if ref is nil' do
- expect(commit.status(nil)).to eq(Ci::Pipeline.latest_status)
- expect(commit.status(nil)).to eq('failed')
+ expect(commit.status(nil)).to eq(pipeline_from_fix.status)
end
end
end
+ describe '#set_status_for_ref' do
+ it 'sets the status for a given reference' do
+ commit.set_status_for_ref('master', 'failed')
+
+ expect(commit.status('master')).to eq('failed')
+ end
+ end
+
describe '#participants' do
let(:user1) { build(:user) }
let(:user2) { build(:user) }