From f51af496769f2fe181d4633f810b85103efd181e Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Wed, 15 Jun 2016 11:15:01 +0530 Subject: 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. --- spec/models/project_spec.rb | 64 +++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 62 insertions(+), 2 deletions(-) (limited to 'spec/models/project_spec.rb') 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 -- cgit v1.2.1 From b1c81f849e5e5b03f56e89cdcefba029ed5c0543 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Wed, 6 Jul 2016 15:07:30 +0530 Subject: Have `Project#open_branches` return branches that are matched by a wildcard protected branch. 1. The `open_branches` method is used to provide a list of branches while creating a protected branch. 2. It makes sense to include branches which are matched by one or more wildcard protected branches, since the user might want to make exact protected branches from these as well. 3. This also provides a large performance improvement. On my machine, in a project with 5000 branches and 2000 protected branches, the `ProtectedBranches#index` page went from a 40 seconds load time to 4 seconds (10x speedup). --- spec/models/project_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'spec/models/project_spec.rb') diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 117ffd551e4..cd126027c95 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -438,12 +438,12 @@ 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 + it "includes 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') + expect(Project.find(project.id).open_branches.map(&:name)).to include('feature') end end -- cgit v1.2.1