Support wildcard matches for protected branches at the model level.
1. The main implementation is in the `ProtectedBranch` model. The wildcard is converted to a Regex and compared. This has been tested thoroughly. - While `Project#protected_branch?` is the main entry point, `project#open_branches` and `project#developers_can_push_to_protected_branch?` have also been modified to work with wildcard protected branches. - The regex is memoized (within the `ProtectedBranch` instance) 2. Improve the performance of `Project#protected_branch?` - This method is called from `Project#open_branches` once _per branch_ in the project, to check if that branch is protected or not. - Before, `#protected_branch?` was making a database call every time it was invoked (in the above case, that amounts to once per branch), which is expensive. - This commit caches the list of protected branches in memory, which reduces the number of database calls down to 1. - A downside to this approach is that `#protected_branch?` _could_ return a stale value (due to the caching), but this is an acceptable tradeoff. 3. Remove the (now) unused `Project#protected_branch_names` method. - This was previously used to check for protected branch status.
This commit is contained in:
parent
ba9ef7f393
commit
f51af49676
4 changed files with 227 additions and 15 deletions
|
@ -803,17 +803,7 @@ class Project < ActiveRecord::Base
|
|||
end
|
||||
|
||||
def open_branches
|
||||
# We're using a Set here as checking values in a large Set is faster than
|
||||
# checking values in a large Array.
|
||||
protected_set = Set.new(protected_branch_names)
|
||||
|
||||
repository.branches.reject do |branch|
|
||||
protected_set.include?(branch.name)
|
||||
end
|
||||
end
|
||||
|
||||
def protected_branch_names
|
||||
@protected_branch_names ||= protected_branches.pluck(:name)
|
||||
repository.branches.reject { |branch| self.protected_branch?(branch.name) }
|
||||
end
|
||||
|
||||
def root_ref?(branch)
|
||||
|
@ -830,11 +820,12 @@ class Project < ActiveRecord::Base
|
|||
|
||||
# Check if current branch name is marked as protected in the system
|
||||
def protected_branch?(branch_name)
|
||||
protected_branch_names.include?(branch_name)
|
||||
@protected_branches ||= self.protected_branches.to_a
|
||||
ProtectedBranch.matching(branch_name, protected_branches: @protected_branches).present?
|
||||
end
|
||||
|
||||
def developers_can_push_to_protected_branch?(branch_name)
|
||||
protected_branches.any? { |pb| pb.name == branch_name && pb.developers_can_push }
|
||||
protected_branches.matching(branch_name).any?(&:developers_can_push)
|
||||
end
|
||||
|
||||
def forked?
|
||||
|
|
|
@ -8,4 +8,40 @@ class ProtectedBranch < ActiveRecord::Base
|
|||
def commit
|
||||
project.commit(self.name)
|
||||
end
|
||||
|
||||
# Returns all protected branches that match the given branch name.
|
||||
# This realizes all records from the scope built up so far, and does
|
||||
# _not_ return a relation.
|
||||
#
|
||||
# This method optionally takes in a list of `protected_branches` to search
|
||||
# through, to avoid calling out to the database.
|
||||
def self.matching(branch_name, protected_branches: nil)
|
||||
(protected_branches || all).select { |protected_branch| protected_branch.matches?(branch_name) }
|
||||
end
|
||||
|
||||
# Checks if the protected branch matches the given branch name.
|
||||
def matches?(branch_name)
|
||||
return false if self.name.blank?
|
||||
|
||||
exact_match?(branch_name) || wildcard_match?(branch_name)
|
||||
end
|
||||
|
||||
protected
|
||||
|
||||
def exact_match?(branch_name)
|
||||
self.name == branch_name
|
||||
end
|
||||
|
||||
def wildcard_match?(branch_name)
|
||||
wildcard_regex === branch_name
|
||||
end
|
||||
|
||||
def wildcard_regex
|
||||
@wildcard_regex ||= begin
|
||||
name = self.name.gsub('*', 'STAR_DONT_ESCAPE')
|
||||
quoted_name = Regexp.quote(name)
|
||||
regex_string = quoted_name.gsub('STAR_DONT_ESCAPE', '.*?')
|
||||
/\A#{regex_string}\z/
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
@ -437,6 +437,14 @@ describe Project, models: true do
|
|||
|
||||
it { expect(project.open_branches.map(&:name)).to include('feature') }
|
||||
it { expect(project.open_branches.map(&:name)).not_to include('master') }
|
||||
|
||||
it "does not include branches matching a protected branch wildcard" do
|
||||
expect(project.open_branches.map(&:name)).to include('feature')
|
||||
|
||||
create(:protected_branch, name: 'feat*', project: project)
|
||||
|
||||
expect(Project.find(project.id).open_branches.map(&:name)).not_to include('feature')
|
||||
end
|
||||
end
|
||||
|
||||
describe '#star_count' do
|
||||
|
@ -937,15 +945,67 @@ describe Project, models: true do
|
|||
describe '#protected_branch?' do
|
||||
let(:project) { create(:empty_project) }
|
||||
|
||||
it 'returns true when a branch is a protected branch' do
|
||||
it 'returns true when the branch matches a protected branch via direct match' do
|
||||
project.protected_branches.create!(name: 'foo')
|
||||
|
||||
expect(project.protected_branch?('foo')).to eq(true)
|
||||
end
|
||||
|
||||
it 'returns false when a branch is not a protected branch' do
|
||||
it 'returns true when the branch matches a protected branch via wildcard match' do
|
||||
project.protected_branches.create!(name: 'production/*')
|
||||
|
||||
expect(project.protected_branch?('production/some-branch')).to eq(true)
|
||||
end
|
||||
|
||||
it 'returns false when the branch does not match a protected branch via direct match' do
|
||||
expect(project.protected_branch?('foo')).to eq(false)
|
||||
end
|
||||
|
||||
it 'returns false when the branch does not match a protected branch via wildcard match' do
|
||||
project.protected_branches.create!(name: 'production/*')
|
||||
|
||||
expect(project.protected_branch?('staging/some-branch')).to eq(false)
|
||||
end
|
||||
end
|
||||
|
||||
describe "#developers_can_push_to_protected_branch?" do
|
||||
let(:project) { create(:empty_project) }
|
||||
|
||||
context "when the branch matches a protected branch via direct match" do
|
||||
it "returns true if 'Developers can Push' is turned on" do
|
||||
create(:protected_branch, name: "production", project: project, developers_can_push: true)
|
||||
|
||||
expect(project.developers_can_push_to_protected_branch?('production')).to be true
|
||||
end
|
||||
|
||||
it "returns false if 'Developers can Push' is turned off" do
|
||||
create(:protected_branch, name: "production", project: project, developers_can_push: false)
|
||||
|
||||
expect(project.developers_can_push_to_protected_branch?('production')).to be false
|
||||
end
|
||||
end
|
||||
|
||||
context "when the branch matches a protected branch via wilcard match" do
|
||||
it "returns true if 'Developers can Push' is turned on" do
|
||||
create(:protected_branch, name: "production/*", project: project, developers_can_push: true)
|
||||
|
||||
expect(project.developers_can_push_to_protected_branch?('production/some-branch')).to be true
|
||||
end
|
||||
|
||||
it "returns false if 'Developers can Push' is turned off" do
|
||||
create(:protected_branch, name: "production/*", project: project, developers_can_push: false)
|
||||
|
||||
expect(project.developers_can_push_to_protected_branch?('production/some-branch')).to be false
|
||||
end
|
||||
end
|
||||
|
||||
context "when the branch does not match a protected branch" do
|
||||
it "returns false" do
|
||||
create(:protected_branch, name: "production/*", project: project, developers_can_push: true)
|
||||
|
||||
expect(project.developers_can_push_to_protected_branch?('staging/some-branch')).to be false
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
describe '#container_registry_path_with_namespace' do
|
||||
|
|
|
@ -1,6 +1,8 @@
|
|||
require 'spec_helper'
|
||||
|
||||
describe ProtectedBranch, models: true do
|
||||
subject { build_stubbed(:protected_branch) }
|
||||
|
||||
describe 'Associations' do
|
||||
it { is_expected.to belong_to(:project) }
|
||||
end
|
||||
|
@ -12,4 +14,127 @@ describe ProtectedBranch, models: true do
|
|||
it { is_expected.to validate_presence_of(:project) }
|
||||
it { is_expected.to validate_presence_of(:name) }
|
||||
end
|
||||
|
||||
describe "#matches?" do
|
||||
context "when the protected branch setting is not a wildcard" do
|
||||
let(:protected_branch) { build(:protected_branch, name: "production/some-branch") }
|
||||
|
||||
it "returns true for branch names that are an exact match" do
|
||||
expect(protected_branch.matches?("production/some-branch")).to be true
|
||||
end
|
||||
|
||||
it "returns false for branch names that are not an exact match" do
|
||||
expect(protected_branch.matches?("staging/some-branch")).to be false
|
||||
end
|
||||
end
|
||||
|
||||
context "when the protected branch name contains wildcard(s)" do
|
||||
context "when there is a single '*'" do
|
||||
let(:protected_branch) { build(:protected_branch, name: "production/*") }
|
||||
|
||||
it "returns true for branch names matching the wildcard" do
|
||||
expect(protected_branch.matches?("production/some-branch")).to be true
|
||||
expect(protected_branch.matches?("production/")).to be true
|
||||
end
|
||||
|
||||
it "returns false for branch names not matching the wildcard" do
|
||||
expect(protected_branch.matches?("staging/some-branch")).to be false
|
||||
expect(protected_branch.matches?("production")).to be false
|
||||
end
|
||||
end
|
||||
|
||||
context "when the wildcard contains regex symbols other than a '*'" do
|
||||
let(:protected_branch) { build(:protected_branch, name: "pro.duc.tion/*") }
|
||||
|
||||
it "returns true for branch names matching the wildcard" do
|
||||
expect(protected_branch.matches?("pro.duc.tion/some-branch")).to be true
|
||||
end
|
||||
|
||||
it "returns false for branch names not matching the wildcard" do
|
||||
expect(protected_branch.matches?("production/some-branch")).to be false
|
||||
expect(protected_branch.matches?("proXducYtion/some-branch")).to be false
|
||||
end
|
||||
end
|
||||
|
||||
context "when there are '*'s at either end" do
|
||||
let(:protected_branch) { build(:protected_branch, name: "*/production/*") }
|
||||
|
||||
it "returns true for branch names matching the wildcard" do
|
||||
expect(protected_branch.matches?("gitlab/production/some-branch")).to be true
|
||||
expect(protected_branch.matches?("/production/some-branch")).to be true
|
||||
expect(protected_branch.matches?("gitlab/production/")).to be true
|
||||
expect(protected_branch.matches?("/production/")).to be true
|
||||
end
|
||||
|
||||
it "returns false for branch names not matching the wildcard" do
|
||||
expect(protected_branch.matches?("gitlabproductionsome-branch")).to be false
|
||||
expect(protected_branch.matches?("production/some-branch")).to be false
|
||||
expect(protected_branch.matches?("gitlab/production")).to be false
|
||||
expect(protected_branch.matches?("production")).to be false
|
||||
end
|
||||
end
|
||||
|
||||
context "when there are arbitrarily placed '*'s" do
|
||||
let(:protected_branch) { build(:protected_branch, name: "pro*duction/*/gitlab/*") }
|
||||
|
||||
it "returns true for branch names matching the wildcard" do
|
||||
expect(protected_branch.matches?("production/some-branch/gitlab/second-branch")).to be true
|
||||
expect(protected_branch.matches?("proXYZduction/some-branch/gitlab/second-branch")).to be true
|
||||
expect(protected_branch.matches?("proXYZduction/gitlab/gitlab/gitlab")).to be true
|
||||
expect(protected_branch.matches?("proXYZduction//gitlab/")).to be true
|
||||
expect(protected_branch.matches?("proXYZduction/some-branch/gitlab/")).to be true
|
||||
expect(protected_branch.matches?("proXYZduction//gitlab/some-branch")).to be true
|
||||
end
|
||||
|
||||
it "returns false for branch names not matching the wildcard" do
|
||||
expect(protected_branch.matches?("production/some-branch/not-gitlab/second-branch")).to be false
|
||||
expect(protected_branch.matches?("prodXYZuction/some-branch/gitlab/second-branch")).to be false
|
||||
expect(protected_branch.matches?("proXYZduction/gitlab/some-branch/gitlab")).to be false
|
||||
expect(protected_branch.matches?("proXYZduction/gitlab//")).to be false
|
||||
expect(protected_branch.matches?("proXYZduction/gitlab/")).to be false
|
||||
expect(protected_branch.matches?("proXYZduction//some-branch/gitlab")).to be false
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
describe "#matching" do
|
||||
context "for direct matches" do
|
||||
it "returns a list of protected branches matching the given branch name" do
|
||||
production = create(:protected_branch, name: "production")
|
||||
staging = create(:protected_branch, name: "staging")
|
||||
|
||||
expect(ProtectedBranch.matching("production")).to include(production)
|
||||
expect(ProtectedBranch.matching("production")).not_to include(staging)
|
||||
end
|
||||
|
||||
it "accepts a list of protected branches to search from, so as to avoid a DB call" do
|
||||
production = build(:protected_branch, name: "production")
|
||||
staging = build(:protected_branch, name: "staging")
|
||||
|
||||
expect(ProtectedBranch.matching("production")).to be_empty
|
||||
expect(ProtectedBranch.matching("production", protected_branches: [production, staging])).to include(production)
|
||||
expect(ProtectedBranch.matching("production", protected_branches: [production, staging])).not_to include(staging)
|
||||
end
|
||||
end
|
||||
|
||||
context "for wildcard matches" do
|
||||
it "returns a list of protected branches matching the given branch name" do
|
||||
production = create(:protected_branch, name: "production/*")
|
||||
staging = create(:protected_branch, name: "staging/*")
|
||||
|
||||
expect(ProtectedBranch.matching("production/some-branch")).to include(production)
|
||||
expect(ProtectedBranch.matching("production/some-branch")).not_to include(staging)
|
||||
end
|
||||
|
||||
it "accepts a list of protected branches to search from, so as to avoid a DB call" do
|
||||
production = build(:protected_branch, name: "production/*")
|
||||
staging = build(:protected_branch, name: "staging/*")
|
||||
|
||||
expect(ProtectedBranch.matching("production/some-branch")).to be_empty
|
||||
expect(ProtectedBranch.matching("production/some-branch", protected_branches: [production, staging])).to include(production)
|
||||
expect(ProtectedBranch.matching("production/some-branch", protected_branches: [production, staging])).not_to include(staging)
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
Loading…
Reference in a new issue