diff options
author | Yorick Peterse <yorickpeterse@gmail.com> | 2017-11-10 22:57:11 +0300 |
---|---|---|
committer | Yorick Peterse <yorickpeterse@gmail.com> | 2017-11-16 18:01:14 +0300 |
commit | ab16a6fb34c0f3e4d9afed3332c559868201e606 (patch) | |
tree | e878395b0f198b3db1cc014bad02361628f6206e | |
parent | 81e94ce1761b48b73be2a8d71938dfe934921e35 (diff) |
Optimise getting the pipeline status of commits
This adds an optimised way of getting the latest pipeline status for a
list of Commit objects (or just a single one).
-rw-r--r-- | app/controllers/projects/commits_controller.rb | 1 | ||||
-rw-r--r-- | app/controllers/projects/merge_requests_controller.rb | 3 | ||||
-rw-r--r-- | app/models/ci/pipeline.rb | 66 | ||||
-rw-r--r-- | app/models/commit.rb | 9 | ||||
-rw-r--r-- | app/models/commit_collection.rb | 44 | ||||
-rw-r--r-- | app/models/merge_request_diff.rb | 4 | ||||
-rw-r--r-- | app/models/repository.rb | 16 | ||||
-rw-r--r-- | changelogs/unreleased/ci-pipeline-status-query.yml | 5 | ||||
-rw-r--r-- | spec/models/ci/pipeline_spec.rb | 122 | ||||
-rw-r--r-- | spec/models/commit_collection_spec.rb | 59 | ||||
-rw-r--r-- | spec/models/commit_spec.rb | 11 |
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 19814864e50..c77c837ff3f 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 3a89fa9264b..35ee12bdfa1 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) } |