Filter project hooks by branch

Allow specificying a branch filter for a project hook and only trigger
a project hook if either the branch filter is blank or the branch matches.
Only supported for push_events for now.

7 files changed, 158 insertions, 4 deletions

diff --git a/spec/features/projects/settings/integration_settings_spec.rb b/spec/features/projects/settings/integration_settings_spec.rb
index 8745ff72df0..32959969f54 100644
--- a/spec/features/projects/settings/integration_settings_spec.rb
+++ b/spec/features/projects/settings/integration_settings_spec.rb
@@ -51,6 +51,7 @@ describe 'Projects > Settings > Integration settings' do
 
         fill_in 'hook_url', with: url
         check 'Tag push events'
+        fill_in 'hook_push_events_branch_filter', with: 'master'
         check 'Enable SSL verification'
         check 'Job events'
 
diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml
index 0a1e3eb83d3..579f175c4a8 100644
--- a/spec/lib/gitlab/import_export/safe_model_attributes.yml
+++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml
@@ -416,6 +416,7 @@ ProjectHook:
 - type
 - service_id
 - push_events
+- push_events_branch_filter
 - issues_events
 - merge_requests_events
 - tag_push_events
diff --git a/spec/models/hooks/active_hook_filter_spec.rb b/spec/models/hooks/active_hook_filter_spec.rb
new file mode 100644
index 00000000000..c97003eb542
--- /dev/null
+++ b/spec/models/hooks/active_hook_filter_spec.rb
@@ -0,0 +1,67 @@
+require 'spec_helper'
+
+describe ActiveHookFilter do
+  subject(:filter) { described_class.new(hook) }
+
+  describe '#matches?' do
+    context 'for push event hooks' do
+      let(:hook) do
+        create(:project_hook, push_events: true, push_events_branch_filter: branch_filter)
+      end
+
+      context 'branch filter is specified' do
+        let(:branch_filter) { 'master' }
+        it 'returns true if branch matches' do
+          expect(filter.matches?(:push_hooks, { ref: 'refs/heads/master' })).to eq(true)
+        end
+
+        it 'returns false if branch does not match' do
+          expect(filter.matches?(:push_hooks, { ref: 'refs/heads/my_branch' })).to eq(false)
+        end
+
+        it 'returns false if ref is nil' do
+          expect(filter.matches?(:push_hooks, {})).to eq(false)
+        end
+
+        context 'branch filter contains wildcard' do
+          let(:branch_filter) { 'features/*' }
+
+          it 'returns true if branch matches' do
+            expect(filter.matches?(:push_hooks, { ref: 'refs/heads/features/my-branch' })).to eq(true)
+            expect(filter.matches?(:push_hooks, { ref: 'refs/heads/features/my-branch/something' })).to eq(true)
+          end
+
+          it 'returns false if branch does not match' do
+            expect(filter.matches?(:push_hooks, { ref: 'refs/heads/master' })).to eq(false)
+          end
+        end
+      end
+
+      context 'branch filter is not specified' do
+        let(:branch_filter) { nil }
+
+        it 'returns true' do
+          expect(filter.matches?(:push_hooks, { ref: 'refs/heads/master' })).to eq(true)
+        end
+      end
+
+      context 'branch filter is empty string' do
+        let(:branch_filter) { '' }
+
+        it 'acts like branch is not specified' do
+          expect(filter.matches?(:push_hooks, { ref: 'refs/heads/master' })).to eq(true)
+        end
+      end
+    end
+
+    context 'for non-push-events hooks' do
+      let(:hook) do
+        create(:project_hook, issues_events: true, push_events: false, push_events_branch_filter: '')
+      end
+
+      it 'returns true as branch filters are not yet supported for these' do
+        expect(filter.matches?(:issues_events, { ref: 'refs/heads/master' })).to eq(true)
+      end
+    end
+  end
+end
diff --git a/spec/models/hooks/web_hook_spec.rb b/spec/models/hooks/web_hook_spec.rb
index ea6d6e53ef5..a4181631f01 100644
--- a/spec/models/hooks/web_hook_spec.rb
+++ b/spec/models/hooks/web_hook_spec.rb
@@ -35,6 +35,26 @@ describe WebHook do
 
       it { is_expected.not_to allow_values("foo\nbar", "foo\r\nbar").for(:token) }
     end
+
+    describe 'push_events_branch_filter' do
+      it { is_expected.to allow_values("good_branch_name", "another/good-branch_name").for(:push_events_branch_filter) }
+      it { is_expected.to allow_values("").for(:push_events_branch_filter) }
+      it { is_expected.not_to allow_values("bad branch name", "bad~branchname").for(:push_events_branch_filter) }
+
+      it 'gets rid of whitespace' do
+        hook.push_events_branch_filter = ' branch '
+        hook.save
+
+        expect(hook.push_events_branch_filter).to eq('branch')
+      end
+
+      it 'stores whitespace only as empty' do
+        hook.push_events_branch_filter = ' '
+        hook.save
+
+        expect(hook.push_events_branch_filter).to eq('')
+      end
+    end
   end
 
   describe 'execute' do
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 2ec030daa67..cea7fa31b16 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -3671,7 +3671,10 @@ describe Project do
 
   describe '#execute_hooks' do
     let(:data) { { ref: 'refs/heads/master', data: 'data' } }
-    it 'executes the projects hooks with the specified scope' do
+    it 'executes active projects hooks with the specified scope' do
+      expect_any_instance_of(ActiveHookFilter).to receive(:matches?)
+        .with(:tag_push_hooks, data)
+        .and_return(true)
       hook = create(:project_hook, merge_requests_events: false, tag_push_events: true)
       project = create(:project, hooks: [hook])
 
@@ -3689,6 +3692,18 @@ describe Project do
       project.execute_hooks(data, :tag_push_hooks)
     end
 
+    it 'does not execute project hooks which are not active' do
+      expect_any_instance_of(ActiveHookFilter).to receive(:matches?)
+        .with(:tag_push_hooks, data)
+        .and_return(false)
+      hook = create(:project_hook, tag_push_events: true)
+      project = create(:project, hooks: [hook])
+
+      expect_any_instance_of(ProjectHook).not_to receive(:async_execute).once
+
+      project.execute_hooks(data, :tag_push_hooks)
+    end
+
     it 'executes the system hooks with the specified scope' do
       expect_any_instance_of(SystemHooksService).to receive(:execute_hooks).with(data, :merge_request_hooks)
 
diff --git a/spec/requests/api/project_hooks_spec.rb b/spec/requests/api/project_hooks_spec.rb
index bc45a63d9f1..a0816322b10 100644
--- a/spec/requests/api/project_hooks_spec.rb
+++ b/spec/requests/api/project_hooks_spec.rb
@@ -9,7 +9,8 @@ describe API::ProjectHooks, 'ProjectHooks' do
            :all_events_enabled,
            project: project,
            url: 'http://example.com',
-           enable_ssl_verification: true)
+           enable_ssl_verification: true,
+           push_events_branch_filter: 'master')
   end
 
   before do
@@ -38,6 +39,7 @@ describe API::ProjectHooks, 'ProjectHooks' do
         expect(json_response.first['pipeline_events']).to eq(true)
         expect(json_response.first['wiki_page_events']).to eq(true)
         expect(json_response.first['enable_ssl_verification']).to eq(true)
+        expect(json_response.first['push_events_branch_filter']).to eq('master')
       end
     end
 
@@ -95,7 +97,7 @@ describe API::ProjectHooks, 'ProjectHooks' do
       expect do
         post api("/projects/#{project.id}/hooks", user),
           url: "http://example.com", issues_events: true, confidential_issues_events: true, wiki_page_events: true,
-          job_events: true
+          job_events: true, push_events_branch_filter: 'some-feature-branch'
       end.to change {project.hooks.count}.by(1)
 
       expect(response).to have_gitlab_http_status(201)
@@ -111,6 +113,7 @@ describe API::ProjectHooks, 'ProjectHooks' do
       expect(json_response['pipeline_events']).to eq(false)
       expect(json_response['wiki_page_events']).to eq(true)
       expect(json_response['enable_ssl_verification']).to eq(true)
+      expect(json_response['push_events_branch_filter']).to eq('some-feature-branch')
       expect(json_response).not_to include('token')
     end
 
@@ -137,7 +140,12 @@ describe API::ProjectHooks, 'ProjectHooks' do
     end
 
     it "returns a 422 error if url not valid" do
-      post api("/projects/#{project.id}/hooks", user), "url" => "ftp://example.com"
+      post api("/projects/#{project.id}/hooks", user), url: "ftp://example.com"
+      expect(response).to have_gitlab_http_status(422)
+    end
+
+    it "returns a 422 error if branch filter is not valid" do
+      post api("/projects/#{project.id}/hooks", user), url: "http://example.com", push_events_branch_filter: '~badbranchname/'
       expect(response).to have_gitlab_http_status(422)
     end
   end
diff --git a/spec/validators/branch_filter_validator_spec.rb b/spec/validators/branch_filter_validator_spec.rb
new file mode 100644
index 00000000000..3be54827431
--- /dev/null
+++ b/spec/validators/branch_filter_validator_spec.rb
@@ -0,0 +1,42 @@
+require 'spec_helper'
+
+describe BranchFilterValidator do
+  let(:validator) { described_class.new(attributes: [:push_events_branch_filter]) }
+  let(:hook) { build(:project_hook) }
+
+  describe '#validates_each' do
+    it 'allows valid branch names' do
+      validator.validate_each(hook, :push_events_branch_filter, "good_branch_name")
+      validator.validate_each(hook, :push_events_branch_filter, "another/good_branch_name")
+      expect(hook.errors.empty?).to be true
+    end
+
+    it 'disallows bad branch names' do
+      validator.validate_each(hook, :push_events_branch_filter, "bad branch~name")
+      expect(hook.errors[:push_events_branch_filter].empty?).to be false
+    end
+
+    it 'allows wildcards' do
+      validator.validate_each(hook, :push_events_branch_filter, "features/*")
+      validator.validate_each(hook, :push_events_branch_filter, "features/*/bla")
+      validator.validate_each(hook, :push_events_branch_filter, "*-stable")
+      expect(hook.errors.empty?).to be true
+    end
+
+    it 'gets rid of whitespace' do
+      filter = ' master '
+      validator.validate_each(hook, :push_events_branch_filter, filter)
+
+      expect(filter).to eq 'master'
+    end
+
+    # Branch names can be quite long but in practice aren't over 255 so 4000 should
+    # be enough space for a list of branch names but we can increase if needed.
+    it 'limits length to 4000 chars' do
+      filter = 'a' * 4001
+      validator.validate_each(hook, :push_events_branch_filter, filter)
+
+      expect(hook.errors[:push_events_branch_filter].empty?).to be false
+    end
+  end
+end