Commit d95f91c3 authored by Vasilii Iakliushin's avatar Vasilii Iakliushin

Optimize protected branches/tags matching

Contributes to https://gitlab.com/gitlab-org/gitlab/-/issues/345479

**Problem**

We request branches and tags object from Gitaly, however we need only
their names to match against provided reference.

**Solution**

Use `branch_names` and `tag_names` calls that support caching and
faster.

Changelog: performance
parent c3fb4845
...@@ -81,8 +81,7 @@ module Projects ...@@ -81,8 +81,7 @@ module Projects
@protected_branch = @project.protected_branches.new @protected_branch = @project.protected_branches.new
@protected_tag = @project.protected_tags.new @protected_tag = @project.protected_tags.new
@protected_branches_count = @protected_branches.reduce(0) { |sum, branch| sum + branch.matching(@project.repository.branches).size } @protected_tags_count = @protected_tags.reduce(0) { |sum, tag| sum + tag.matching(@project.repository.tag_names).size }
@protected_tags_count = @protected_tags.reduce(0) { |sum, tag| sum + tag.matching(@project.repository.tags).size }
load_gon_index load_gon_index
end end
......
...@@ -2,6 +2,10 @@ ...@@ -2,6 +2,10 @@
class ProtectableDropdown class ProtectableDropdown
REF_TYPES = %i[branches tags].freeze REF_TYPES = %i[branches tags].freeze
REF_NAME_METHODS = {
branches: :branch_names,
tags: :tag_names
}.freeze
def initialize(project, ref_type) def initialize(project, ref_type)
raise ArgumentError, "invalid ref type `#{ref_type}`" unless ref_type.in?(REF_TYPES) raise ArgumentError, "invalid ref type `#{ref_type}`" unless ref_type.in?(REF_TYPES)
...@@ -23,12 +27,12 @@ class ProtectableDropdown ...@@ -23,12 +27,12 @@ class ProtectableDropdown
private private
def refs def ref_names
@project.repository.public_send(@ref_type) # rubocop:disable GitlabSecurity/PublicSend @project.repository.public_send(ref_name_method) # rubocop:disable GitlabSecurity/PublicSend
end end
def ref_names def ref_name_method
refs.map(&:name) REF_NAME_METHODS[@ref_type]
end end
def protections def protections
......
...@@ -5,10 +5,10 @@ class RefMatcher ...@@ -5,10 +5,10 @@ class RefMatcher
@ref_name_or_pattern = ref_name_or_pattern @ref_name_or_pattern = ref_name_or_pattern
end end
# Returns all branches/tags (among the given list of refs [`Gitlab::Git::Branch`]) # Returns all branches/tags (among the given list of refs [`Gitlab::Git::Branch`] or their names [`String`])
# that match the current protected ref. # that match the current protected ref.
def matching(refs) def matching(refs)
refs.select { |ref| matches?(ref.name) } refs.select { |ref| ref.is_a?(String) ? matches?(ref) : matches?(ref.name) }
end end
# Checks if the protected ref matches the given ref name. # Checks if the protected ref matches the given ref name.
......
.protected-branches-list.js-protected-branches-list.qa-protected-branches-list .protected-branches-list.js-protected-branches-list.qa-protected-branches-list
- if @protected_branches.empty? - if @protected_branches.empty?
.card-header.bg-white .card-header.bg-white
= s_("ProtectedBranch|Protected branch (%{protected_branches_count})") % { protected_branches_count: @protected_branches_count } = s_("ProtectedBranch|Protected branch (%{protected_branches_count})") % { protected_branches_count: 0 }
%p.settings-message.text-center %p.settings-message.text-center
= s_("ProtectedBranch|There are currently no protected branches, protect a branch with the form above.") = s_("ProtectedBranch|There are currently no protected branches, protect a branch with the form above.")
- else - else
......
...@@ -9,7 +9,7 @@ ...@@ -9,7 +9,7 @@
%div %div
- if protected_branch.wildcard? - if protected_branch.wildcard?
- matching_branches = protected_branch.matching(repository.branches) - matching_branches = protected_branch.matching(repository.branch_names)
= link_to pluralize(matching_branches.count, "matching branch"), namespace_project_protected_branch_path(@project.namespace, @project, protected_branch) = link_to pluralize(matching_branches.count, "matching branch"), namespace_project_protected_branch_path(@project.namespace, @project, protected_branch)
- elsif !protected_branch.commit - elsif !protected_branch.commit
%span.text-muted Branch was deleted. %span.text-muted Branch was deleted.
......
...@@ -6,7 +6,7 @@ ...@@ -6,7 +6,7 @@
= gl_badge_tag s_('ProtectedTags|default'), variant: :info, class: 'gl-ml-2' = gl_badge_tag s_('ProtectedTags|default'), variant: :info, class: 'gl-ml-2'
%td %td
- if protected_tag.wildcard? - if protected_tag.wildcard?
- matching_tags = protected_tag.matching(repository.tags) - matching_tags = protected_tag.matching(repository.tag_names)
= link_to pluralize(matching_tags.count, "matching tag"), project_protected_tag_path(@project, protected_tag) = link_to pluralize(matching_tags.count, "matching tag"), project_protected_tag_path(@project, protected_tag)
- else - else
- if commit = protected_tag.commit - if commit = protected_tag.commit
......
.protected-tags-list.js-protected-tags-list .protected-tags-list.js-protected-tags-list
- if @protected_tags.empty? - if @protected_tags.empty?
.card-header .card-header
Protected tags (#{@protected_tags_count}) Protected tags (0)
%p.settings-message.text-center %p.settings-message.text-center
No tags are protected. No tags are protected.
- else - else
......
...@@ -3,8 +3,9 @@ ...@@ -3,8 +3,9 @@
require 'spec_helper' require 'spec_helper'
RSpec.describe ProtectableDropdown do RSpec.describe ProtectableDropdown do
subject(:dropdown) { described_class.new(project, ref_type) }
let(:project) { create(:project, :repository) } let(:project) { create(:project, :repository) }
let(:subject) { described_class.new(project, :branches) }
describe 'initialize' do describe 'initialize' do
it 'raises ArgumentError for invalid ref type' do it 'raises ArgumentError for invalid ref type' do
...@@ -13,34 +14,75 @@ RSpec.describe ProtectableDropdown do ...@@ -13,34 +14,75 @@ RSpec.describe ProtectableDropdown do
end end
end end
describe '#protectable_ref_names' do shared_examples 'protectable_ref_names' do
context 'when project repository is not empty' do context 'when project repository is not empty' do
before do it 'includes elements matching a protected ref wildcard' do
create(:protected_branch, project: project, name: 'master') is_expected.to include(matching_ref)
end
it { expect(subject.protectable_ref_names).to include('feature') }
it { expect(subject.protectable_ref_names).not_to include('master') }
it "includes branches matching a protected branch wildcard" do factory = ref_type == :branches ? :protected_branch : :protected_tag
expect(subject.protectable_ref_names).to include('feature')
create(:protected_branch, name: 'feat*', project: project) create(factory, name: "#{matching_ref[0]}*", project: project)
subject = described_class.new(project.reload, :branches) subject = described_class.new(project.reload, ref_type)
expect(subject.protectable_ref_names).to include('feature') expect(subject.protectable_ref_names).to include(matching_ref)
end end
end end
context 'when project repository is empty' do context 'when project repository is empty' do
let(:project) { create(:project) } let(:project) { create(:project) }
it "returns empty list" do it 'returns empty list' do
subject = described_class.new(project, :branches) is_expected.to be_empty
end
end
end
describe '#protectable_ref_names' do
subject { dropdown.protectable_ref_names }
context 'for branches' do
let(:ref_type) { :branches }
let(:matching_ref) { 'feature' }
expect(subject.protectable_ref_names).to be_empty before do
create(:protected_branch, project: project, name: 'master')
end end
it { is_expected.to include(matching_ref) }
it { is_expected.not_to include('master') }
it_behaves_like 'protectable_ref_names'
end
context 'for tags' do
let(:ref_type) { :tags }
let(:matching_ref) { 'v1.0.0' }
before do
create(:protected_tag, project: project, name: 'v1.1.0')
end
it { is_expected.to include(matching_ref) }
it { is_expected.not_to include('v1.1.0') }
it_behaves_like 'protectable_ref_names'
end
end
describe '#hash' do
subject { dropdown.hash }
context 'for branches' do
let(:ref_type) { :branches }
it { is_expected.to include(id: 'feature', text: 'feature', title: 'feature') }
end
context 'for tags' do
let(:ref_type) { :tags }
it { is_expected.to include(id: 'v1.0.0', text: 'v1.0.0', title: 'v1.0.0') }
end end
end end
end end
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe RefMatcher do
subject(:ref_matcher) { described_class.new(ref_pattern) }
let(:ref_pattern) { 'v1.0' }
shared_examples 'matching_refs' do
context 'when there is no match' do
let(:ref_pattern) { 'unknown' }
it { is_expected.to match_array([]) }
end
context 'when ref pattern is a wildcard' do
let(:ref_pattern) { 'v*' }
it { is_expected.to match_array(refs) }
end
end
describe '#matching' do
subject { ref_matcher.matching(refs) }
context 'when refs are strings' do
let(:refs) { ['v1.0', 'v1.1'] }
it { is_expected.to match_array([ref_pattern]) }
it_behaves_like 'matching_refs'
end
context 'when refs are ref objects' do
let(:matching_ref) { double('tag', name: 'v1.0') }
let(:not_matching_ref) { double('tag', name: 'v1.1') }
let(:refs) { [matching_ref, not_matching_ref] }
it { is_expected.to match_array([matching_ref]) }
it_behaves_like 'matching_refs'
end
end
describe '#matches?' do
subject { ref_matcher.matches?(ref_name) }
let(:ref_name) { 'v1.0' }
it { is_expected.to be_truthy }
context 'when ref_name is empty' do
let(:ref_name) { '' }
it { is_expected.to be_falsey }
end
context 'when ref pattern matches wildcard' do
let(:ref_pattern) { 'v*' }
it { is_expected.to be_truthy }
end
context 'when ref pattern does not match wildcard' do
let(:ref_pattern) { 'v2.*' }
it { is_expected.to be_falsey }
end
end
describe '#wildcard?' do
subject { ref_matcher.wildcard? }
it { is_expected.to be_falsey }
context 'when pattern is a wildcard' do
let(:ref_pattern) { 'v*' }
it { is_expected.to be_truthy }
end
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment