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 + return false unless ref && commit.present? - commit = project.commit(ref) - return false unless commit - - 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 4ec8ec9c8b2..387428d90a6 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 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