diff --git a/app/models/issue_collection.rb b/app/models/issue_collection.rb index df36252a5b1959603481f61c699e156bb40bc175..f0b7d9914c80167f9c5aa94cbcaa3dd9e9b019d7 100644 --- a/app/models/issue_collection.rb +++ b/app/models/issue_collection.rb @@ -32,6 +32,8 @@ class IssueCollection end end + alias_method :visible_to, :updatable_by_user + private def project_ids diff --git a/app/policies/issue_policy.rb b/app/policies/issue_policy.rb index f3ede58a001416392821f636beabcd3630c95f16..52fa33bc4b032fc9c2239815badc60517365b5e3 100644 --- a/app/policies/issue_policy.rb +++ b/app/policies/issue_policy.rb @@ -18,6 +18,6 @@ class IssuePolicy < IssuablePolicy def can_read_confidential? return false unless @user - IssueCollection.new([@subject]).updatable_by_user(@user).any? + IssueCollection.new([@subject]).visible_to(@user).any? end end diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb index e8415862de51be3daf703dc1e5584f6b2a961188..de313095bedb85b03c950b45a331a6140724be9b 100644 --- a/app/services/git_push_service.rb +++ b/app/services/git_push_service.rb @@ -105,35 +105,11 @@ class GitPushService < BaseService # Extract any GFM references from the pushed commit messages. If the configured issue-closing regex is matched, # close the referenced Issue. Create cross-reference Notes corresponding to any other referenced Mentionables. def process_commit_messages - is_default_branch = is_default_branch? - - authors = Hash.new do |hash, commit| - email = commit.author_email - next hash[email] if hash.has_key?(email) - - hash[email] = commit_user(commit) - end + default = is_default_branch? @push_commits.each do |commit| - # Keep track of the issues that will be actually closed because they are on a default branch. - # Hence, when creating cross-reference notes, the not-closed issues (on non-default branches) - # will also have cross-reference. - closed_issues = [] - - if is_default_branch - # Close issues if these commits were pushed to the project's default branch and the commit message matches the - # closing regex. Exclude any mentioned Issues from cross-referencing even if the commits are being pushed to - # a different branch. - closed_issues = commit.closes_issues(current_user) - closed_issues.each do |issue| - if can?(current_user, :update_issue, issue) - Issues::CloseService.new(project, authors[commit], {}).execute(issue, commit: commit) - end - end - end - - commit.create_cross_references!(authors[commit], closed_issues) - update_issue_metrics(commit, authors) + ProcessCommitWorker. + perform_async(project.id, current_user.id, commit.id, default) end end @@ -176,11 +152,4 @@ class GitPushService < BaseService def branch_name @branch_name ||= Gitlab::Git.ref_name(params[:ref]) end - - def update_issue_metrics(commit, authors) - mentioned_issues = commit.all_references(authors[commit]).issues - - Issue::Metrics.where(issue_id: mentioned_issues.map(&:id), first_mentioned_in_commit_at: nil). - update_all(first_mentioned_in_commit_at: commit.committed_date) - end end diff --git a/app/services/issues/close_service.rb b/app/services/issues/close_service.rb index 45cca216ccce68016b13a423295f3854c9c1f020..ab4c51386a42973cbe9f3d1ae3291fd7dbf48de6 100644 --- a/app/services/issues/close_service.rb +++ b/app/services/issues/close_service.rb @@ -1,8 +1,21 @@ module Issues class CloseService < Issues::BaseService + # Closes the supplied issue if the current user is able to do so. def execute(issue, commit: nil, notifications: true, system_note: true) return issue unless can?(current_user, :update_issue, issue) + close_issue(issue, + commit: commit, + notifications: notifications, + system_note: system_note) + end + + # Closes the supplied issue without checking if the user is authorized to + # do so. + # + # The code calling this method is responsible for ensuring that a user is + # allowed to close the given issue. + def close_issue(issue, commit: nil, notifications: true, system_note: true) if project.jira_tracker? && project.jira_service.active project.jira_service.execute(commit, issue) todo_service.close_issue(issue, current_user) diff --git a/app/workers/process_commit_worker.rb b/app/workers/process_commit_worker.rb new file mode 100644 index 0000000000000000000000000000000000000000..071741fbacd2cd6e6aca6860b9731f9a3040cc31 --- /dev/null +++ b/app/workers/process_commit_worker.rb @@ -0,0 +1,67 @@ +# Worker for processing individiual commit messages pushed to a repository. +# +# Jobs for this worker are scheduled for every commit that is being pushed. As a +# result of this the workload of this worker should be kept to a bare minimum. +# Consider using an extra worker if you need to add any extra (and potentially +# slow) processing of commits. +class ProcessCommitWorker + include Sidekiq::Worker + include DedicatedSidekiqQueue + + # project_id - The ID of the project this commit belongs to. + # user_id - The ID of the user that pushed the commit. + # commit_sha - The SHA1 of the commit to process. + # default - The data was pushed to the default branch. + def perform(project_id, user_id, commit_sha, default = false) + project = Project.find_by(id: project_id) + + return unless project + + user = User.find_by(id: user_id) + + return unless user + + commit = find_commit(project, commit_sha) + + return unless commit + + author = commit.author || user + + process_commit_message(project, commit, user, author, default) + + update_issue_metrics(commit, author) + end + + def process_commit_message(project, commit, user, author, default = false) + closed_issues = default ? commit.closes_issues(user) : [] + + unless closed_issues.empty? + close_issues(project, user, author, commit, closed_issues) + end + + commit.create_cross_references!(author, closed_issues) + end + + def close_issues(project, user, author, commit, issues) + # We don't want to run permission related queries for every single issue, + # therefor we use IssueCollection here and skip the authorization check in + # Issues::CloseService#execute. + IssueCollection.new(issues).updatable_by_user(user).each do |issue| + Issues::CloseService.new(project, author). + close_issue(issue, commit: commit) + end + end + + def update_issue_metrics(commit, author) + mentioned_issues = commit.all_references(author).issues + + Issue::Metrics.where(issue_id: mentioned_issues.map(&:id), first_mentioned_in_commit_at: nil). + update_all(first_mentioned_in_commit_at: commit.committed_date) + end + + private + + def find_commit(project, sha) + project.commit(sha) + end +end diff --git a/changelogs/unreleased/process-commits-using-sidekiq.yml b/changelogs/unreleased/process-commits-using-sidekiq.yml new file mode 100644 index 0000000000000000000000000000000000000000..9f596e6a584c3ff8236b980daf03261de8a1293c --- /dev/null +++ b/changelogs/unreleased/process-commits-using-sidekiq.yml @@ -0,0 +1,4 @@ +--- +title: Process commits using a dedicated Sidekiq worker +merge_request: 6802 +author: diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index f36fe893fd0cf9c9bb508d6f016d7d5c768b5996..0aec8aedf724ffd979ef88ac6043bdc9d63cb97c 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -21,6 +21,7 @@ - [post_receive, 5] - [merge, 5] - [update_merge_requests, 3] + - [process_commit, 2] - [new_note, 2] - [build, 2] - [pipeline, 2] diff --git a/spec/models/issue_collection_spec.rb b/spec/models/issue_collection_spec.rb index d9ab397c302daa64cd5c8a3f1708201be0c5b775..d742c8146802c2848ca9495662310fa714daf243 100644 --- a/spec/models/issue_collection_spec.rb +++ b/spec/models/issue_collection_spec.rb @@ -55,4 +55,13 @@ describe IssueCollection do end end end + + describe '#visible_to' do + it 'is an alias for updatable_by_user' do + updatable_by_user = described_class.instance_method(:updatable_by_user) + visible_to = described_class.instance_method(:visible_to) + + expect(visible_to).to eq(updatable_by_user) + end + end end diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb index 1c7a3d04b09fdfdd167b51daba890d555f7a130e..cea7e6429f953da7f9601edd1fb8f0ad69c78c3a 100644 --- a/spec/services/git_push_service_spec.rb +++ b/spec/services/git_push_service_spec.rb @@ -302,6 +302,9 @@ describe GitPushService, services: true do author_email: commit_author.email ) + allow_any_instance_of(ProcessCommitWorker).to receive(:find_commit). + and_return(commit) + allow(project.repository).to receive(:commits_between).and_return([commit]) end @@ -357,6 +360,9 @@ describe GitPushService, services: true do committed_date: commit_time ) + allow_any_instance_of(ProcessCommitWorker).to receive(:find_commit). + and_return(commit) + allow(project.repository).to receive(:commits_between).and_return([commit]) end @@ -393,6 +399,9 @@ describe GitPushService, services: true do allow(project.repository).to receive(:commits_between). and_return([closing_commit]) + allow_any_instance_of(ProcessCommitWorker).to receive(:find_commit). + and_return(closing_commit) + project.team << [commit_author, :master] end diff --git a/spec/services/issues/close_service_spec.rb b/spec/services/issues/close_service_spec.rb index 5dfb33f4b28d5499f9e0f61dfbd6fa700bc76aa9..4465f22a001ac48873168bff0daf5ce252475970 100644 --- a/spec/services/issues/close_service_spec.rb +++ b/spec/services/issues/close_service_spec.rb @@ -15,10 +15,39 @@ describe Issues::CloseService, services: true do end describe '#execute' do + let(:service) { described_class.new(project, user) } + + it 'checks if the user is authorized to update the issue' do + expect(service).to receive(:can?).with(user, :update_issue, issue). + and_call_original + + service.execute(issue) + end + + it 'does not close the issue when the user is not authorized to do so' do + allow(service).to receive(:can?).with(user, :update_issue, issue). + and_return(false) + + expect(service).not_to receive(:close_issue) + expect(service.execute(issue)).to eq(issue) + end + + it 'closes the issue when the user is authorized to do so' do + allow(service).to receive(:can?).with(user, :update_issue, issue). + and_return(true) + + expect(service).to receive(:close_issue). + with(issue, commit: nil, notifications: true, system_note: true) + + service.execute(issue) + end + end + + describe '#close_issue' do context "valid params" do before do perform_enqueued_jobs do - described_class.new(project, user).execute(issue) + described_class.new(project, user).close_issue(issue) end end @@ -41,24 +70,12 @@ describe Issues::CloseService, services: true do end end - context 'current user is not authorized to close issue' do - before do - perform_enqueued_jobs do - described_class.new(project, guest).execute(issue) - end - end - - it 'does not close the issue' do - expect(issue).to be_open - end - end - context 'when issue is not confidential' do it 'executes issue hooks' do expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :issue_hooks) expect(project).to receive(:execute_services).with(an_instance_of(Hash), :issue_hooks) - described_class.new(project, user).execute(issue) + described_class.new(project, user).close_issue(issue) end end @@ -69,14 +86,14 @@ describe Issues::CloseService, services: true do expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :confidential_issue_hooks) expect(project).to receive(:execute_services).with(an_instance_of(Hash), :confidential_issue_hooks) - described_class.new(project, user).execute(issue) + described_class.new(project, user).close_issue(issue) end end context 'external issue tracker' do before do allow(project).to receive(:default_issues_tracker?).and_return(false) - described_class.new(project, user).execute(issue) + described_class.new(project, user).close_issue(issue) end it { expect(issue).to be_valid } diff --git a/spec/workers/process_commit_worker_spec.rb b/spec/workers/process_commit_worker_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..3e4fee422409e3eea2cf079b409141db77189ca5 --- /dev/null +++ b/spec/workers/process_commit_worker_spec.rb @@ -0,0 +1,109 @@ +require 'spec_helper' + +describe ProcessCommitWorker do + let(:worker) { described_class.new } + let(:user) { create(:user) } + let(:project) { create(:project, :public) } + let(:issue) { create(:issue, project: project, author: user) } + let(:commit) { project.commit } + + describe '#perform' do + it 'does not process the commit when the project does not exist' do + expect(worker).not_to receive(:close_issues) + + worker.perform(-1, user.id, commit.id) + end + + it 'does not process the commit when the user does not exist' do + expect(worker).not_to receive(:close_issues) + + worker.perform(project.id, -1, commit.id) + end + + it 'does not process the commit when the commit no longer exists' do + expect(worker).not_to receive(:close_issues) + + worker.perform(project.id, user.id, 'this-should-does-not-exist') + end + + it 'processes the commit message' do + expect(worker).to receive(:process_commit_message).and_call_original + + worker.perform(project.id, user.id, commit.id) + end + + it 'updates the issue metrics' do + expect(worker).to receive(:update_issue_metrics).and_call_original + + worker.perform(project.id, user.id, commit.id) + end + end + + describe '#process_commit_message' do + context 'when pushing to the default branch' do + it 'closes issues that should be closed per the commit message' do + allow(commit).to receive(:safe_message). + and_return("Closes #{issue.to_reference}") + + expect(worker).to receive(:close_issues). + with(project, user, user, commit, [issue]) + + worker.process_commit_message(project, commit, user, user, true) + end + end + + context 'when pushing to a non-default branch' do + it 'does not close any issues' do + allow(commit).to receive(:safe_message). + and_return("Closes #{issue.to_reference}") + + expect(worker).not_to receive(:close_issues) + + worker.process_commit_message(project, commit, user, user, false) + end + end + + it 'creates cross references' do + expect(commit).to receive(:create_cross_references!) + + worker.process_commit_message(project, commit, user, user) + end + end + + describe '#close_issues' do + context 'when the user can update the issues' do + it 'closes the issues' do + worker.close_issues(project, user, user, commit, [issue]) + + issue.reload + + expect(issue.closed?).to eq(true) + end + end + + context 'when the user can not update the issues' do + it 'does not close the issues' do + other_user = create(:user) + + worker.close_issues(project, other_user, other_user, commit, [issue]) + + issue.reload + + expect(issue.closed?).to eq(false) + end + end + end + + describe '#update_issue_metrics' do + it 'updates any existing issue metrics' do + allow(commit).to receive(:safe_message). + and_return("Closes #{issue.to_reference}") + + worker.update_issue_metrics(commit, user) + + metric = Issue::Metrics.first + + expect(metric.first_mentioned_in_commit_at).to eq(commit.committed_date) + end + end +end