Merge pull request #481 from NickLaMuro/close-my-own-feature-request

Add CommitMetadataChecker

app/workers/commit_monitor_handlers/commit_range/github_pr_commenter.rb

@@ -7,7 +7,7 @@ class GithubPrCommenter
     include BranchWorkerMixin
 
     def self.batch_workers
-      [DiffContentChecker, DiffFilenameChecker]
+      [DiffContentChecker, DiffFilenameChecker, CommitMetadataChecker]
     end
 
     def self.handled_branch_modes

app/workers/commit_monitor_handlers/commit_range/github_pr_commenter/commit_metadata_checker.rb

@@ -0,0 +1,62 @@
+require 'rugged'
+
+module CommitMonitorHandlers::CommitRange
+  class GithubPrCommenter::CommitMetadataChecker
+    include Sidekiq::Worker
+    sidekiq_options :queue => :miq_bot_glacial
+
+    include BatchEntryWorkerMixin
+    include BranchWorkerMixin
+
+    def perform(batch_entry_id, branch_id, new_commits)
+      return unless find_batch_entry(batch_entry_id)
+      return skip_batch_entry unless find_branch(branch_id, :pr)
+
+      complete_batch_entry(:result => process_commits(new_commits))
+    end
+
+    private
+
+    def process_commits(new_commits)
+      @offenses = []
+
+      new_commits.each do |commit_sha, data|
+        check_for_usernames_in(commit_sha, data["message"])
+      end
+
+      @offenses
+    end
+
+    # From https://github.com/join
+    #
+    #     "Username may only contain alphanumeric characters or single hyphens,
+    #     and cannot begin or end with a hyphen."
+    #
+    # For the beginning and and, we do a positive lookbehind at the beginning
+    # to get the `@`, and a positive lookhead at the end to confirm their is
+    # either a period or a whitespace following the "var" (instance_variable)
+    #
+    # Since there can't be underscores in Github usernames, this makes it so we
+    # rule out partial matches of variables (@database_records having a
+    # username lookup of `database`), but still catch full variable names
+    # without underscores (`@foobarbaz`).
+    #
+    USERNAME_REGEXP = /
+      (?<=@)          # must start with a '@' (don't capture)
+      [a-zA-Z0-9]     # first character must be alphanumeric
+      [a-zA-Z0-9\-]*  # middle chars may be alphanumeric or hyphens
+      [a-zA-Z0-9]     # last character must be alphanumeric
+      (?=[\.\s])      # allow only variables without "_" (not captured)
+    /x.freeze
+
+    def check_for_usernames_in(commit, message)
+      message.scan(USERNAME_REGEXP).each do |potential_username|
+        next unless GithubService.username_lookup(potential_username)
+
+        group   = ::Branch.github_commit_uri(fq_repo_name, commit)
+        message = "Username `@#{potential_username}` detected in commit message. Consider removing."
+        @offenses << OffenseMessage::Entry.new(:low, message, group)
+      end
+    end
+  end
+end

lib/github_service.rb

@@ -113,6 +113,21 @@ def refresh_assignees(fq_name)
       assignees_cache.delete(fq_name)
     end
 
+    def username_lookup(username)
+      if username_lookup_cache.key?(username)
+        username_lookup_cache[username]
+      else
+        username_lookup_cache[username] ||= begin
+          case Net::HTTP.new("github.com", 443).tap { |h| h.use_ssl = true }.request_head("/#{username}")
+          when Net::HTTPNotFound then nil # invalid username
+          when Net::HTTPOK       then service.user(username)[:id]
+          else
+            raise "Error on GitHub with username lookup"
+          end
+        end
+      end
+    end
+
     private
 
     def service
@@ -151,6 +166,10 @@ def assignees_cache
       @assignees_cache ||= {}
     end
 
+    def username_lookup_cache
+      @username_lookup_cache ||= {}
+    end
+
     def respond_to_missing?(method_name, include_private = false)
       service.respond_to?(method_name, include_private)
     end

spec/lib/github_service_spec.rb

@@ -0,0 +1,63 @@
+describe GithubService do
+  describe "#username_lookup" do
+    let(:lookup_username) { "NickLaMuro" }
+    let(:lookup_status)   { 200 }
+
+    before do
+      # HTTP lookup
+      stub_request(:head, "https://github.com/#{lookup_username}")
+        .with(:headers => {'Accept' => '*/*', 'User-Agent' => 'Ruby'})
+        .to_return(:status => lookup_status, :body => "", :headers => {})
+    end
+
+    after do
+      lookup_cache.delete(lookup_username)
+    end
+
+    def lookup_cache
+      described_class.send(:username_lookup_cache)
+    end
+
+    context "for a valid user" do
+      before do
+        github_service_add_stub :url           => "/users/#{lookup_username}",
+                                :response_body => {'id' => 123}.to_json
+      end
+
+      it "looks up a user and stores the user's ID in the cache" do
+        expect(described_class.username_lookup(lookup_username)).to eq(123)
+        expect(lookup_cache).to eq("NickLaMuro" => 123)
+      end
+    end
+
+    context "for a user that is not found" do
+      let(:lookup_status) { 404 }
+
+      it "looks up a user and stores the user's ID in the cache" do
+        expect(described_class.username_lookup(lookup_username)).to eq(nil)
+        expect(lookup_cache).to eq("NickLaMuro" => nil)
+      end
+
+      it "does a lookup call only once" do
+        http_instance  = Net::HTTP.new("github.com", 443)
+        fake_not_found = Net::HTTPNotFound.new(nil, nil, nil)
+        expect(Net::HTTP).to     receive(:new).and_return(http_instance)
+        expect(http_instance).to receive(:request_head).once.and_return(fake_not_found)
+
+        expect(described_class.username_lookup(lookup_username)).to eq(nil)
+        expect(described_class.username_lookup(lookup_username)).to eq(nil)
+      end
+    end
+
+    context "when GitHub is having a bad time..." do
+      let(:lookup_status) { 500 }
+
+      it "looks up a user and does not stores the username in the cache" do
+        expect do
+          described_class.username_lookup(lookup_username)
+        end.to raise_error(RuntimeError, "Error on GitHub with username lookup")
+        expect(lookup_cache).to eq({})
+      end
+    end
+  end
+end

spec/workers/commit_monitor_handlers/commit_range/github_pr_commenter/commit_metadata_checker_spec.rb

@@ -0,0 +1,98 @@
+describe CommitMonitorHandlers::CommitRange::GithubPrCommenter::CommitMetadataChecker do
+  let(:batch_entry)        { BatchEntry.create!(:job => BatchJob.create!) }
+  let(:branch)             { create(:pr_branch) }
+  let(:commit_message_1)   { "Adds SUPER FEATURE.  BREAKS NOTHING!" }
+  let(:commit_message_2)   { "fix tests" }
+  let(:commit_message_3)   { "fix moar tests" }
+
+  let(:commits) do
+    {
+      "abcd123" => {"message" => commit_message_1, "files" => ["app/models/super.rb"]},
+      "abcd234" => {"message" => commit_message_2, "files" => ["spec/models/super_spec.rb"]},
+      "abcd345" => {"message" => commit_message_3, "files" => ["spec/models/normal_spec.rb"]}
+    }
+  end
+
+  let(:username_lookup_cache) do
+    {
+      "Fryguy"          => 123, # valid user
+      "NickLaMuro"      => 234, # valid user
+      "NickLaMura"      => nil, # mispelled invalid user
+      "Nick-LaMuro"     => nil, # invalid user with hyphen
+      "booksandauthors" => nil  # not a user (surprisingly)
+    }
+  end
+
+  before do
+    stub_sidekiq_logger
+    stub_job_completion
+
+    allow(GithubService).to receive(:username_lookup_cache).and_return(username_lookup_cache)
+  end
+
+  context "with basic commit messages" do
+    it "doesn't create any offenses" do
+      described_class.new.perform(batch_entry.id, branch.id, commits)
+
+      batch_entry.reload
+      expect(batch_entry.result.length).to eq(0)
+    end
+  end
+
+  context "with multiline commit messages" do
+    let(:commit_message_1) do
+      <<~COMMIT_MSG
+        In which the hero adds the greatest feature...
+
+        With this change, I made it so that by changing this:
+
+            @database_records = Books.all
+
+        Avoids a N+1 on authors by adding an .includes call
+
+            @booksandauthors = Books.all.includes(:author)
+
+        Saving us 100s of queries for our tiny bookstore app.
+
+        Thanks to @Fryguy and @Nick-LaMuro for this suggestion!
+      COMMIT_MSG
+    end
+
+    let(:commit_message_2) do
+      <<~COMMIT_MSG
+        fixes tests
+
+        Forgot that we stubbed things...
+
+        cc @Fryguy @NickLaMuro
+      COMMIT_MSG
+    end
+
+    let(:commit_message_3) do
+      <<~COMMIT_MSG
+        fixes moar tests
+
+        I forget how to rebase... #dealWithIt @NickLaMura
+      COMMIT_MSG
+    end
+
+    it "returns one offense for each valid username" do
+      described_class.new.perform(batch_entry.id, branch.id, commits)
+
+      batch_entry.reload
+      expect(batch_entry.result.length).to eq(3)
+      expect(batch_entry.result.first).to have_attributes(
+        :group   => "https://github.com/#{branch.fq_repo_name}/commit/abcd123",
+        :message => "Username `@Fryguy` detected in commit message. Consider removing."
+      )
+      expect(batch_entry.result.second).to have_attributes(
+        :group   => "https://github.com/#{branch.fq_repo_name}/commit/abcd234",
+        :message => "Username `@Fryguy` detected in commit message. Consider removing."
+      )
+      expect(batch_entry.result.third).to have_attributes(
+        :group   => "https://github.com/#{branch.fq_repo_name}/commit/abcd234",
+        :message => "Username `@NickLaMuro` detected in commit message. Consider removing."
+      )
+    end
+  end
+end