Merge branch '28359-skip-process-commit-worker-unless-issues-referenced' into 'master'

Use regex to skip unnecessary reference processing in ProcessCommitWorker

Closes #28359

See merge request !10867

app/models/concerns/mentionable.rb

@@ -78,6 +78,8 @@ module Mentionable
 
   # Extract GFM references to other Mentionables from this Mentionable. Always excludes its #local_reference.
   def referenced_mentionables(current_user = self.author)
+    return [] unless matches_cross_reference_regex?
+
     refs = all_references(current_user)
     refs = (refs.issues + refs.merge_requests + refs.commits)
 
@@ -87,6 +89,20 @@ module Mentionable
     refs.reject { |ref| ref == local_reference }
   end
 
+  # Uses regex to quickly determine if mentionables might be referenced
+  # Allows heavy processing to be skipped
+  def matches_cross_reference_regex?
+    reference_pattern = if project.default_issues_tracker?
+                          ReferenceRegexes::DEFAULT_PATTERN
+                        else
+                          ReferenceRegexes::EXTERNAL_PATTERN
+                        end
+
+    self.class.mentionable_attrs.any? do |attr, _|
+      __send__(attr) =~ reference_pattern
+    end
+  end
+
   # Create a cross-reference Note for each GFM reference to another Mentionable found in the +mentionable_attrs+.
   def create_cross_references!(author = self.author, without = [])
     refs = referenced_mentionables(author)

app/models/concerns/mentionable/reference_regexes.rb

@@ -0,0 +1,22 @@
+module Mentionable
+  module ReferenceRegexes
+    def self.reference_pattern(link_patterns, issue_pattern)
+      Regexp.union(link_patterns,
+                   issue_pattern,
+                   Commit.reference_pattern,
+                   MergeRequest.reference_pattern)
+    end
+
+    DEFAULT_PATTERN = begin
+      issue_pattern = Issue.reference_pattern
+      link_patterns = Regexp.union([Issue, Commit, MergeRequest].map(&:link_reference_pattern))
+      reference_pattern(link_patterns, issue_pattern)
+    end
+
+    EXTERNAL_PATTERN = begin
+      issue_pattern = ExternalIssue.reference_pattern
+      link_patterns = URI.regexp(%w(http https))
+      reference_pattern(link_patterns, issue_pattern)
+    end
+  end
+end

app/services/git_push_service.rb

@@ -85,8 +85,10 @@ class GitPushService < BaseService
     default = is_default_branch?
 
     push_commits.last(PROCESS_COMMIT_LIMIT).each do |commit|
-      ProcessCommitWorker.
-        perform_async(project.id, current_user.id, commit.to_hash, default)
+      if commit.matches_cross_reference_regex?
+        ProcessCommitWorker.
+          perform_async(project.id, current_user.id, commit.to_hash, default)
+      end
     end
   end
 

app/workers/process_commit_worker.rb

@@ -23,6 +23,9 @@ class ProcessCommitWorker
     return unless user
 
     commit = build_commit(project, commit_hash)
+
+    return unless commit.matches_cross_reference_regex?
+
     author = commit.author || user
 
     process_commit_message(project, commit, user, author, default)

spec/features/cycle_analytics_spec.rb

@@ -6,7 +6,7 @@ feature 'Cycle Analytics', feature: true, js: true do
   let(:project) { create(:project, :repository) }
   let(:issue) { create(:issue, project: project, created_at: 2.days.ago) }
   let(:milestone) { create(:milestone, project: project) }
-  let(:mr) { create_merge_request_closing_issue(issue) }
+  let(:mr) { create_merge_request_closing_issue(issue, commit_message: "References #{issue.to_reference}") }
   let(:pipeline) { create(:ci_empty_pipeline, status: 'created', project: project, ref: mr.source_branch, sha: mr.source_branch_sha) }
 
   context 'as an allowed user' do
@@ -32,7 +32,6 @@ feature 'Cycle Analytics', feature: true, js: true do
       before do
         project.team << [user, :master]
 
-        allow_any_instance_of(Gitlab::ReferenceExtractor).to receive(:issues).and_return([issue])
         create_cycle
         deploy_master
 

spec/lib/gitlab/cycle_analytics/events_spec.rb

@@ -11,8 +11,6 @@ describe 'cycle analytics events' do
   end
 
   before do
-    allow_any_instance_of(Gitlab::ReferenceExtractor).to receive(:issues).and_return([context])
-
     setup(context)
   end
 
@@ -332,7 +330,7 @@ describe 'cycle analytics events' do
   def setup(context)
     milestone = create(:milestone, project: project)
     context.update(milestone: milestone)
-    mr = create_merge_request_closing_issue(context)
+    mr = create_merge_request_closing_issue(context, commit_message: "References #{context.to_reference}")
 
     ProcessCommitWorker.new.perform(project.id, user.id, mr.commits.last.to_hash)
   end

spec/models/concerns/mentionable_spec.rb

@@ -163,3 +163,52 @@ describe Issue, "Mentionable" do
     end
   end
 end
+
+describe Commit, 'Mentionable' do
+  let(:project) { create(:project, :public, :repository) }
+  let(:commit)  { project.commit }
+
+  describe '#matches_cross_reference_regex?' do
+    it "is false when message doesn't reference anything" do
+      allow(commit.raw).to receive(:message).and_return "WIP: Do something"
+
+      expect(commit.matches_cross_reference_regex?).to be false
+    end
+
+    it 'is true if issue #number mentioned in title' do
+      allow(commit.raw).to receive(:message).and_return "#1"
+
+      expect(commit.matches_cross_reference_regex?).to be true
+    end
+
+    it 'is true if references an MR' do
+      allow(commit.raw).to receive(:message).and_return "See merge request !12"
+
+      expect(commit.matches_cross_reference_regex?).to be true
+    end
+
+    it 'is true if references a commit' do
+      allow(commit.raw).to receive(:message).and_return "a1b2c3d4"
+
+      expect(commit.matches_cross_reference_regex?).to be true
+    end
+
+    it 'is true if issue referenced by url' do
+      issue = create(:issue, project: project)
+
+      allow(commit.raw).to receive(:message).and_return Gitlab::UrlBuilder.build(issue)
+
+      expect(commit.matches_cross_reference_regex?).to be true
+    end
+
+    context 'with external issue tracker' do
+      let(:project) { create(:jira_project) }
+
+      it 'is true if external issues referenced' do
+        allow(commit.raw).to receive(:message).and_return 'JIRA-123'
+
+        expect(commit.matches_cross_reference_regex?).to be true
+      end
+    end
+  end
+end

spec/requests/projects/cycle_analytics_events_spec.rb

@@ -9,8 +9,6 @@ describe 'cycle analytics events', api: true do
     before do
       project.team << [user, :developer]
 
-      allow_any_instance_of(Gitlab::ReferenceExtractor).to receive(:issues).and_return([issue])
-
       3.times do |count|
         Timecop.freeze(Time.now + count.days) do
           create_cycle
@@ -121,7 +119,7 @@ describe 'cycle analytics events', api: true do
   def create_cycle
     milestone = create(:milestone, project: project)
     issue.update(milestone: milestone)
-    mr = create_merge_request_closing_issue(issue)
+    mr = create_merge_request_closing_issue(issue, commit_message: "References #{issue.to_reference}")
 
     pipeline = create(:ci_empty_pipeline, status: 'created', project: project, ref: mr.source_branch, sha: mr.source_branch_sha)
     pipeline.run

spec/services/git_push_service_spec.rb

@@ -622,12 +622,21 @@ describe GitPushService, services: true do
 
     it 'only schedules a limited number of commits' do
       allow(service).to receive(:push_commits).
-        and_return(Array.new(1000, double(:commit, to_hash: {})))
+        and_return(Array.new(1000, double(:commit, to_hash: {}, matches_cross_reference_regex?: true)))
 
       expect(ProcessCommitWorker).to receive(:perform_async).exactly(100).times
 
       service.process_commit_messages
     end
+
+    it "skips commits which don't include cross-references" do
+      allow(service).to receive(:push_commits).
+        and_return([double(:commit, to_hash: {}, matches_cross_reference_regex?: false)])
+
+      expect(ProcessCommitWorker).not_to receive(:perform_async)
+
+      service.process_commit_messages
+    end
   end
 
   def execute_service(project, user, oldrev, newrev, ref)

spec/support/cycle_analytics_helpers.rb

@@ -20,7 +20,7 @@ module CycleAnalyticsHelpers
                        ref: 'refs/heads/master').execute
   end
 
-  def create_merge_request_closing_issue(issue, message: nil, source_branch: nil)
+  def create_merge_request_closing_issue(issue, message: nil, source_branch: nil, commit_message: 'commit message')
     if !source_branch || project.repository.commit(source_branch).blank?
       source_branch = generate(:branch)
       project.repository.add_branch(user, source_branch, 'master')
@@ -30,7 +30,7 @@ module CycleAnalyticsHelpers
       user,
       generate(:branch),
       'content',
-      message: 'commit message',
+      message: commit_message,
       branch_name: source_branch)
     project.repository.commit(sha)
 

spec/workers/process_commit_worker_spec.rb

@@ -20,6 +20,14 @@ describe ProcessCommitWorker do
       worker.perform(project.id, -1, commit.to_hash)
     end
 
+    it 'does not process the commit when no issues are referenced' do
+      allow(worker).to receive(:build_commit).and_return(double(matches_cross_reference_regex?: false))
+
+      expect(worker).not_to receive(:process_commit_message)
+
+      worker.perform(project.id, user.id, commit.to_hash)
+    end
+
     it 'processes the commit message' do
       expect(worker).to receive(:process_commit_message).and_call_original