diff options
author | Timothy Andrew <mail@timothyandrew.net> | 2016-06-15 11:15:01 +0530 |
---|---|---|
committer | Timothy Andrew <mail@timothyandrew.net> | 2016-07-05 10:50:34 +0530 |
commit | f51af496769f2fe181d4633f810b85103efd181e (patch) | |
tree | b95805a0f52389d752174948434f0196d2194ba6 /spec/models | |
parent | ba9ef7f3935cfaa42fcdb2317567cc383c7e9c22 (diff) | |
download | gitlab-ce-f51af496769f2fe181d4633f810b85103efd181e.tar.gz |
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.
Diffstat (limited to 'spec/models')
-rw-r--r-- | spec/models/project_spec.rb | 64 | ||||
-rw-r--r-- | spec/models/protected_branch_spec.rb | 125 |
2 files changed, 187 insertions, 2 deletions
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index a8c777d1e3e..117ffd551e4 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -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 diff --git a/spec/models/protected_branch_spec.rb b/spec/models/protected_branch_spec.rb index b523834c6e9..8bf0d24a128 100644 --- a/spec/models/protected_branch_spec.rb +++ b/spec/models/protected_branch_spec.rb @@ -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 |