diff --git a/ee/config/feature_flags/wip/validate_codeowner_users.yml b/ee/config/feature_flags/wip/validate_codeowner_users.yml
new file mode 100644
index 0000000000000000000000000000000000000000..d401f8a4feb2ca8314b41168acb141e855e0fc91
--- /dev/null
+++ b/ee/config/feature_flags/wip/validate_codeowner_users.yml
@@ -0,0 +1,9 @@
+---
+name: validate_codeowner_users
+feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/502583
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/179310
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/519201
+milestone: '17.9'
+group: group::source code
+type: wip
+default_enabled: false
diff --git a/ee/lib/gitlab/code_owners/entry.rb b/ee/lib/gitlab/code_owners/entry.rb
index b1e9d3eb1f34d00658c9be882e2b0fef19c1bbe3..f3f8b4b843be271d9d8d88da7df28d02bebc04ea 100644
--- a/ee/lib/gitlab/code_owners/entry.rb
+++ b/ee/lib/gitlab/code_owners/entry.rb
@@ -5,20 +5,18 @@ module CodeOwners
class Entry
include ::Gitlab::Utils::StrongMemoize
- Data = Struct.new(:pattern, :owner_line, :section, :optional, :approvals_required, :exclusion)
+ Data = Struct.new(:pattern, :owner_line, :section, :optional, :approvals_required, :exclusion, :line_number)
attr_reader :data
protected :data
- delegate :pattern, :hash, :owner_line, :section, :approvals_required, :exclusion, to: :data
+ delegate :pattern, :hash, :owner_line, :section, :approvals_required, :exclusion, :line_number, to: :data
- # rubocop:disable Metrics/ParameterLists -- TODO: Reduce number of parameters in this method
def initialize(
- pattern, owner_line, section = Section::DEFAULT, optional = false, approvals_required = 0,
- exclusion = false)
- @data = Data.new(pattern, owner_line, section, optional, approvals_required, exclusion)
+ pattern, owner_line, section: Section::DEFAULT, optional: false, approvals_required: 0,
+ line_number: nil, exclusion: false)
+ @data = Data.new(pattern, owner_line, section, optional, approvals_required, exclusion, line_number)
end
- # rubocop:enable Metrics/ParameterLists
def all_users(project)
strong_memoize(:all_users) do
diff --git a/ee/lib/gitlab/code_owners/error.rb b/ee/lib/gitlab/code_owners/error.rb
index a2392ac57a2e520c806b9c25b62e2823d06717bb..87a115da5e1855a8ddae85e5f8ffc8a1e8d5cd4a 100644
--- a/ee/lib/gitlab/code_owners/error.rb
+++ b/ee/lib/gitlab/code_owners/error.rb
@@ -9,6 +9,7 @@ class Error
MISSING_SECTION_NAME = :missing_section_name
INVALID_APPROVAL_REQUIREMENT = :invalid_approval_requirement
INVALID_SECTION_FORMAT = :invalid_section_format
+ OWNER_WITHOUT_PERMISSION = :owner_without_permission
def initialize(message:, line_number:, path:)
@message = message
diff --git a/ee/lib/gitlab/code_owners/file.rb b/ee/lib/gitlab/code_owners/file.rb
index 7f5b64347c8eea3096cd9bc9ca8675a3e9ac1b82..3c5fcde04309296727393763eab05a0e00d4df9d 100644
--- a/ee/lib/gitlab/code_owners/file.rb
+++ b/ee/lib/gitlab/code_owners/file.rb
@@ -9,6 +9,10 @@ class File
# `FNM_PATHNAME` makes sure ** matches path separators
FNMATCH_FLAGS = (::File::FNM_DOTMATCH | ::File::FNM_PATHNAME).freeze
+ # Maxmimum number of references to validate
+ # This maximum is currently not based on any benchmark
+ MAX_REFERENCES = 200
+
def initialize(blob)
@blob = blob
@errors = []
@@ -61,7 +65,7 @@ def entries_for_path(path)
matches = []
parsed_data.each do |_, section_entries|
- if Feature.enabled?(:codeowners_file_exclusions, @blob.repository.project)
+ if Feature.enabled?(:codeowners_file_exclusions, project)
matching_patterns = section_entries.keys.reverse.select { |pattern| path_matches?(pattern, path) }
matching_entries = matching_patterns.map { |pattern| section_entries[pattern] }
@@ -82,12 +86,30 @@ def entries_for_path(path)
def valid?
parsed_data
+ validate_users
errors.none?
end
private
+ def project
+ @blob&.repository&.project
+ end
+
+ def validate_users
+ # Avoids querying the database for users if there are still syntax errors
+ return if errors.present?
+
+ return unless project && Feature.enabled?(:validate_codeowner_users, project)
+
+ entries = parsed_data.values.flat_map(&:values)
+ validation_errors = UserPermissionCheck.new(project, entries, limit: MAX_REFERENCES).errors
+ validation_errors.each do |e|
+ add_error(e[:error], e[:line_number])
+ end
+ end
+
def data
return "" if @blob.nil? || @blob.binary?
@@ -130,7 +152,7 @@ def get_parsed_data
def parse_entry(line, parsed, section, line_number)
pattern, _separator, entry_owners = line.partition(/(?<!\\)\s+/)
- if Feature.enabled?(:codeowners_file_exclusions, @blob.repository.project)
+ if Feature.enabled?(:codeowners_file_exclusions, project)
is_exclusion = pattern.start_with?('!')
pattern = pattern[1..] if is_exclusion
end
@@ -145,17 +167,15 @@ def parse_entry(line, parsed, section, line_number)
add_error(Error::MISSING_ENTRY_OWNER, line_number) if owners.blank?
- entry_args = [
+ parsed[section.name][normalized_pattern] = Entry.new(
pattern,
owners,
- section.name,
- section.optional,
- section.approvals
- ]
-
- entry_args << is_exclusion if Feature.enabled?(:codeowners_file_exclusions, @blob.repository.project)
-
- parsed[section.name][normalized_pattern] = Entry.new(*entry_args)
+ section: section.name,
+ optional: section.optional,
+ approvals_required: section.approvals,
+ exclusion: Feature.enabled?(:codeowners_file_exclusions, project) && is_exclusion,
+ line_number: line_number
+ )
end
def skip?(line)
diff --git a/ee/lib/gitlab/code_owners/user_permission_check.rb b/ee/lib/gitlab/code_owners/user_permission_check.rb
new file mode 100644
index 0000000000000000000000000000000000000000..8ffac4dc5ca554e33b0be58f9d35ee4214723dff
--- /dev/null
+++ b/ee/lib/gitlab/code_owners/user_permission_check.rb
@@ -0,0 +1,47 @@
+# frozen_string_literal: true
+
+module Gitlab
+ module CodeOwners
+ class UserPermissionCheck
+ def initialize(project, code_owners_entries, limit:)
+ @project = project
+ @entries = code_owners_entries
+ @limit = limit
+ end
+
+ def errors
+ groups = GroupsLoader.new(project, names: extracted_names).groups
+ possible_usernames = extracted_names - groups.map(&:full_path)
+
+ users = UsersLoader.new(project, names: possible_usernames).members
+
+ # Preload for ability check
+ ActiveRecord::Associations::Preloader.new(records: users, associations: :namespace_bans).call
+ project.team.max_member_access_for_user_ids(users.map(&:id))
+
+ users_with_permission = users.select do |user|
+ user.can?(:update_merge_request, project)
+ end
+
+ error_usernames = possible_usernames - users_with_permission.map(&:username)
+ entries.each_with_object([]) do |entry, errors|
+ next unless Gitlab::CodeOwners::ReferenceExtractor.new(entry.owner_line).names.intersect?(error_usernames)
+
+ errors << {
+ error: Error::OWNER_WITHOUT_PERMISSION,
+ line_number: entry.line_number
+ }
+ end
+ end
+
+ private
+
+ attr_reader :entries, :project, :limit
+
+ def extracted_names
+ owner_lines = entries.map(&:owner_line)
+ ReferenceExtractor.new(owner_lines).names.first(limit)
+ end
+ end
+ end
+end
diff --git a/ee/spec/graphql/api/validate_code_owner_file_spec.rb b/ee/spec/graphql/api/validate_code_owner_file_spec.rb
index 758693248f9830c8abd7590672d28bd2b5a33115..c8c176a10019f4c5ca0b4fd696cf18720a3e7243 100644
--- a/ee/spec/graphql/api/validate_code_owner_file_spec.rb
+++ b/ee/spec/graphql/api/validate_code_owner_file_spec.rb
@@ -203,12 +203,19 @@
context 'when code owners file is correct' do
let_it_be(:ref) { 'good-branch' }
-
+ let_it_be(:documentation_owner) { create(:user, developer_of: project) }
+ let_it_be(:nested_group) { create(:group, :nested) }
+ let_it_be(:test_owner) { create(:user, developer_of: project) }
+ let_it_be(:owner) { create(:user, developer_of: project) }
+ let_it_be(:owner2) { create(:user, developer_of: project) }
+
+ let!(:project_group_link) { create(:project_group_link, project: project, group: nested_group.parent) }
+ let!(:project_group_link2) { create(:project_group_link, project: project, group: nested_group) }
let_it_be(:code_owners_content) do
<<~CODEOWNERS
- docs/* @documentation-owner
- docs/CODEOWNERS @owner-1 owner2@gitlab.org @owner-3 @documentation-owner
- spec/* @test-owner @test-group @test-group/nested-group
+ docs/* @#{documentation_owner.username}
+ docs/CODEOWNERS @#{owner.username} owner2@gitlab.org @#{owner2.username} @#{documentation_owner.username}
+ spec/* @#{test_owner.username} @#{nested_group.parent.full_path} @#{nested_group.full_path}
CODEOWNERS
end
diff --git a/ee/spec/lib/gitlab/code_owners/entry_spec.rb b/ee/spec/lib/gitlab/code_owners/entry_spec.rb
index 74f3840d5b6ce639daaf1c781389292c6d01dc43..1e218da84a93ec3a8f4a568d759897df710b31ac 100644
--- a/ee/spec/lib/gitlab/code_owners/entry_spec.rb
+++ b/ee/spec/lib/gitlab/code_owners/entry_spec.rb
@@ -6,7 +6,7 @@
described_class.new(
"/**/file",
"@user jane@gitlab.org @group @group/nested-group @@maintainer @@developers @@maintainer",
- "Documentation"
+ section: "Documentation"
)
end
@@ -155,9 +155,9 @@
described_class.new(
"/**/file",
"@user jane@gitlab.org @group @group/nested-group",
- "Documentation",
- false,
- 2)
+ section: "Documentation",
+ optional: false,
+ approvals_required: 2)
end
it 'returns 2' do
@@ -170,7 +170,7 @@
described_class.new(
"/**/file",
"@user jane@gitlab.org @group @group/nested-group",
- "Documentation")
+ section: "Documentation")
end
it 'returns 0' do
diff --git a/ee/spec/lib/gitlab/code_owners/file_spec.rb b/ee/spec/lib/gitlab/code_owners/file_spec.rb
index fd667b70b83e324512e3e80df7ef7359418f10b2..d469aff400eb25a332b9eafdca12f069243eedf9 100644
--- a/ee/spec/lib/gitlab/code_owners/file_spec.rb
+++ b/ee/spec/lib/gitlab/code_owners/file_spec.rb
@@ -7,7 +7,7 @@
# 'project' is required for the #fake_blob helper
#
- let(:project) { build(:project) }
+ let_it_be_with_reload(:project) { create(:project, :in_group) }
let(:blob) { fake_blob(path: 'CODEOWNERS', data: file_content) }
let(:file_content) do
@@ -556,14 +556,7 @@ def owner_line(pattern)
end
describe '#valid?' do
- context 'when codeowners file is correct' do
- it 'does not detect errors' do
- expect(file.valid?).to eq(true)
- expect(file.errors).to be_empty
- end
- end
-
- context 'when codeowners file has errors' do
+ context 'when codeowners file has syntax errors' do
let(:file_content) do
<<~CONTENT
*.rb
@@ -595,6 +588,48 @@ def owner_line(pattern)
]
)
end
+
+ it 'does not call UserPermissionCheck' do
+ expect(Gitlab::CodeOwners::UserPermissionCheck).not_to receive(:new)
+
+ file.valid?
+ end
+ end
+
+ context 'when the codeowners file does not have syntax errors' do
+ let(:file_content) do
+ <<~CONTENT
+ file1 @reference
+
+ README.md @other_reference
+
+ CONTENT
+ end
+
+ it 'calls UserPermissionCheck' do
+ expect(Gitlab::CodeOwners::UserPermissionCheck).to receive(:new).with(
+ project,
+ [
+ a_kind_of(Gitlab::CodeOwners::Entry),
+ a_kind_of(Gitlab::CodeOwners::Entry)
+ ],
+ limit: described_class::MAX_REFERENCES
+ ).and_return(instance_double(Gitlab::CodeOwners::UserPermissionCheck, errors: []))
+
+ file.valid?
+ end
+
+ context 'when the validate_codeowner_users feature flag is not enabled' do
+ before do
+ stub_feature_flags(validate_codeowner_users: false)
+ end
+
+ it 'does not call UserPermissionCheck' do
+ expect(Gitlab::CodeOwners::UserPermissionCheck).not_to receive(:new)
+
+ file.valid?
+ end
+ end
end
end
end
diff --git a/ee/spec/lib/gitlab/code_owners/loader_spec.rb b/ee/spec/lib/gitlab/code_owners/loader_spec.rb
index 6116cf68523820d5d3386616f017bb570f366afc..ace81592bddaf06f468d483b7f4fd8326bf995d0 100644
--- a/ee/spec/lib/gitlab/code_owners/loader_spec.rb
+++ b/ee/spec/lib/gitlab/code_owners/loader_spec.rb
@@ -39,7 +39,11 @@
end
describe '#entries' do
- let(:expected_entry) { Gitlab::CodeOwners::Entry.new('docs/CODEOWNERS', '@owner-1 owner2@gitlab.org @owner-3 @documentation-owner') }
+ let(:expected_entry) { Gitlab::CodeOwners::Entry.new('docs/CODEOWNERS', '@owner-1 owner2@gitlab.org @owner-3 @documentation-owner', line_number: 2) }
+ let(:spec_entry) do
+ Gitlab::CodeOwners::Entry.new('spec/*', '@test-owner @test-group @test-group/nested-group', line_number: 3)
+ end
+
let(:first_entry) { loader.entries.first }
it 'returns entries for the matched line' do
@@ -71,9 +75,7 @@
let(:paths) { ['docs/CODEOWNERS', 'spec/loader_spec.rb', 'spec/entry_spec.rb'] }
it 'loads 2 entries' do
- other_entry = Gitlab::CodeOwners::Entry.new('spec/*', '@test-owner @test-group @test-group/nested-group')
-
- expect(loader.entries).to contain_exactly(expected_entry, other_entry)
+ expect(loader.entries).to contain_exactly(expected_entry, spec_entry)
end
it 'performs 10 queries for users and groups' do
@@ -100,7 +102,6 @@
context 'group as a code owner' do
let(:paths) { ['spec/loader_spec.rb'] }
- let(:expected_entry) { Gitlab::CodeOwners::Entry.new('spec/*', '@test-owner @test-group @test-group/nested-group') }
it 'loads group members as code owners' do
test_group = create(:group, path: 'test-group')
@@ -111,7 +112,7 @@
test_group.add_developer(group_user)
test_group.add_developer(test_owner)
- expect(loader.entries).to contain_exactly(expected_entry)
+ expect(loader.entries).to contain_exactly(spec_entry)
expect(loader.members).to contain_exactly(group_user, test_owner)
entry = loader.entries.first
@@ -121,7 +122,7 @@
context 'role as a code owner' do
let(:paths) { ['app/accounts_helper.rb'] }
- let(:expected_entry) { Gitlab::CodeOwners::Entry.new('app/*', '@@developer') }
+ let(:expected_entry) { Gitlab::CodeOwners::Entry.new('app/*', '@@developer', line_number: 4) }
it 'loads users with developer role as code owners' do
expect(loader.entries).to contain_exactly(expected_entry)
@@ -135,12 +136,12 @@
context 'for multiple paths' do
let(:paths) { ['app/accounts_helper.rb', 'spec/entry_spec.rb', 'config/boot.rb'] }
+ let(:other_entry_2) do
+ Gitlab::CodeOwners::Entry.new('config/*', '@@maintainers', line_number: 5)
+ end
it 'loads 3 entries' do
- other_entry_1 = Gitlab::CodeOwners::Entry.new('spec/*', '@test-owner @test-group @test-group/nested-group')
- other_entry_2 = Gitlab::CodeOwners::Entry.new('config/*', '@@maintainers')
-
- expect(loader.entries).to contain_exactly(expected_entry, other_entry_1, other_entry_2)
+ expect(loader.entries).to contain_exactly(expected_entry, spec_entry, other_entry_2)
end
end
end
@@ -293,10 +294,24 @@
subject { loader.track_file_validation }
context 'when file has no linting error' do
- it 'sends no snowplow event' do
+ context 'when the validate_codeowner_users flag is not enabled' do
+ before do
+ stub_feature_flags(validate_codeowner_users: false)
+ end
+
+ it 'sends no snowplow event' do
+ subject
+
+ expect_no_snowplow_event(category: described_class.name)
+ end
+ end
+
+ it 'sends errors for invalid user references' do
subject
- expect_no_snowplow_event(category: described_class.name)
+ expect_snowplow_event(category: described_class.name, action: 'file_validation', label: 'owner_without_permission', project: project, extra: { file_path: "CODEOWNERS", line_number: 2 })
+ expect_snowplow_event(category: described_class.name, action: 'file_validation', label: 'owner_without_permission', project: project, extra: { file_path: "CODEOWNERS", line_number: 3 })
+ expect_snowplow_event(category: described_class.name, action: 'file_validation', label: 'owner_without_permission', project: project, extra: { file_path: "CODEOWNERS", line_number: 3 })
end
end
diff --git a/ee/spec/lib/gitlab/code_owners/user_permission_check_spec.rb b/ee/spec/lib/gitlab/code_owners/user_permission_check_spec.rb
new file mode 100644
index 0000000000000000000000000000000000000000..6c01b96a2728e144e4ad55edaf9470e91be20045
--- /dev/null
+++ b/ee/spec/lib/gitlab/code_owners/user_permission_check_spec.rb
@@ -0,0 +1,221 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::CodeOwners::UserPermissionCheck, feature_category: :source_code_management do
+ let_it_be_with_reload(:project) { create(:project, :in_group) }
+ let(:limit) { Gitlab::CodeOwners::File::MAX_REFERENCES }
+ let(:section) { Gitlab::CodeOwners::Section::DEFAULT }
+ let(:group_entry) do
+ Gitlab::CodeOwners::Entry.new(
+ 'file1',
+ "@#{project.group.full_path}",
+ section: section,
+ optional: true,
+ approvals_required: 0,
+ exclusion: false,
+ line_number: 1
+ )
+ end
+
+ let(:entry) do
+ Gitlab::CodeOwners::Entry.new(
+ 'file2',
+ "@#{user.username}",
+ section: section,
+ optional: true,
+ approvals_required: 0,
+ exclusion: false,
+ line_number: 3
+ )
+ end
+
+ let(:entries) { [group_entry, entry] }
+ let_it_be(:guest) { create(:user, guest_of: project) }
+ let_it_be(:developer) { create(:user, developer_of: project) }
+ let_it_be(:group_developer) { create(:user, developer_of: project.group) }
+ let(:not_persisted_user) { build(:user) }
+ let(:non_member) { create(:user) }
+
+ subject(:errors) { described_class.new(project, entries, limit: limit).errors }
+
+ context 'when the entries mention a username' do
+ shared_examples_for 'validation error for name without permissions' do
+ it 'has warnings' do
+ expect(errors).to contain_exactly({
+ error: Gitlab::CodeOwners::Error::OWNER_WITHOUT_PERMISSION,
+ line_number: entry.line_number
+ })
+ end
+ end
+
+ shared_examples_for 'valid entries' do
+ it { is_expected.to be_blank }
+ end
+
+ context 'when the user does not exist' do
+ let(:user) { not_persisted_user }
+
+ it_behaves_like 'validation error for name without permissions'
+ end
+
+ context 'when the user is not a member' do
+ let(:user) { non_member }
+
+ it_behaves_like 'validation error for name without permissions'
+ end
+
+ context 'when the user is directly invited' do
+ let(:user) { guest }
+
+ it_behaves_like 'validation error for name without permissions'
+
+ context 'with sufficient access' do
+ let(:user) { developer }
+
+ it_behaves_like 'valid entries'
+ end
+ end
+
+ context 'when the user inherits access from the parent group' do
+ let(:user) { group_developer }
+
+ it_behaves_like 'valid entries'
+ end
+
+ context 'when the user is from an invited group' do
+ let(:access_level) { :guest }
+ let(:project_group_link) { create :project_group_link, access_level, project: project }
+ let(:user) { create(:user, developer_of: project_group_link.group) }
+
+ it_behaves_like 'validation error for name without permissions'
+
+ context 'with sufficient access' do
+ let(:access_level) { :developer }
+
+ it_behaves_like 'valid entries'
+ end
+ end
+ end
+
+ describe 'queries' do
+ let(:multiple_reference_entry) do
+ Gitlab::CodeOwners::Entry.new(
+ 'file1',
+ "@#{project.group.full_path} @#{group_developer.username}",
+ section: section,
+ optional: true,
+ approvals_required: 0,
+ exclusion: false,
+ line_number: 1
+ )
+ end
+
+ let(:single_reference_entry) do
+ Gitlab::CodeOwners::Entry.new(
+ 'file2',
+ "@#{project.group.full_path}",
+ section: section,
+ optional: true,
+ approvals_required: 0,
+ exclusion: false,
+ line_number: 1
+ )
+ end
+
+ let(:invalid_entry_with_multiple_references) do
+ Gitlab::CodeOwners::Entry.new(
+ 'file4',
+ "@#{developer.username} @#{non_member.username} @#{guest.username}",
+ section: section,
+ optional: true,
+ approvals_required: 0,
+ exclusion: false,
+ line_number: 2
+ )
+ end
+
+ let(:invalid_entry) do
+ Gitlab::CodeOwners::Entry.new(
+ 'file3',
+ "@#{not_persisted_user.username}",
+ section: section,
+ optional: true,
+ approvals_required: 0,
+ exclusion: false,
+ line_number: 3
+ )
+ end
+
+ let!(:entries) do
+ [
+ multiple_reference_entry,
+ single_reference_entry,
+ invalid_entry_with_multiple_references,
+ invalid_entry
+ ]
+ end
+
+ context 'when limiting the number of references' do
+ context 'when the number of references is within the limit' do
+ it 'validates all the references' do
+ expect(Gitlab::CodeOwners::UsersLoader).to receive(:new).with(
+ project, names: [group_developer, developer, non_member, guest, not_persisted_user].map(&:username)
+ ).and_call_original
+ expect(errors.all? { |e| e[:error] == Gitlab::CodeOwners::Error::OWNER_WITHOUT_PERMISSION }).to be(true)
+ expect(errors.pluck(:line_number)).to match_array([
+ invalid_entry_with_multiple_references.line_number,
+ invalid_entry.line_number
+ ])
+ end
+ end
+
+ context 'when there are more references than the limit' do
+ before do
+ stub_const('Gitlab::CodeOwners::File::MAX_REFERENCES', 4)
+ end
+
+ it 'only validates up to the maximum' do
+ expect(Gitlab::CodeOwners::UsersLoader).to receive(:new).with(
+ project, names: [group_developer, developer, non_member].map(&:username)
+ ).and_call_original
+ error = errors.first
+
+ expect(errors.size).to eq(1)
+ expect(error[:error]).to eq(Gitlab::CodeOwners::Error::OWNER_WITHOUT_PERMISSION)
+ expect(error[:line_number]).to eq(invalid_entry_with_multiple_references.line_number)
+ end
+ end
+ end
+
+ it 'does not perform N+1 queries', :request_store, :use_sql_query_cache do
+ control = ActiveRecord::QueryRecorder.new(query_recorder_debug: true, skip_cached: false) do
+ described_class.new(project, [invalid_entry_with_multiple_references, multiple_reference_entry],
+ limit: limit).errors
+ end
+
+ extra_developer = create(:user, developer_of: project)
+ extra_guest = create(:user, guest_of: project)
+ extra_non_member = create(:user)
+ extra_invalid_entry = Gitlab::CodeOwners::Entry.new(
+ 'extra_file',
+ "@#{extra_developer.username} @#{extra_non_member.username} @#{extra_guest.username}",
+ section: section,
+ optional: true,
+ approvals_required: 0,
+ exclusion: false,
+ line_number: 10
+ )
+
+ project.reload
+
+ expect do
+ described_class.new(project, [
+ invalid_entry_with_multiple_references,
+ multiple_reference_entry,
+ extra_invalid_entry
+ ], limit: limit).errors
+ end.to issue_same_number_of_queries_as(control)
+ end
+ end
+end
diff --git a/ee/spec/models/approval_merge_request_rule_spec.rb b/ee/spec/models/approval_merge_request_rule_spec.rb
index 4877ede6642244af39f448c2936699a1fa62f639..847dd427b07c43b93ad91b63b8dfb121dff4b12c 100644
--- a/ee/spec/models/approval_merge_request_rule_spec.rb
+++ b/ee/spec/models/approval_merge_request_rule_spec.rb
@@ -260,7 +260,7 @@
end
context "when the existing rule matches name but not section" do
- let(:entry) { Gitlab::CodeOwners::Entry.new("*.rb", "@user", "example_section") }
+ let(:entry) { Gitlab::CodeOwners::Entry.new("*.rb", "@user", section: "example_section") }
it "creates a new rule" do
expect(rule).not_to eq(existing_code_owner_rule)
@@ -273,14 +273,14 @@
create(:code_owner_rule, name: '*.rb', merge_request: merge_request, approvals_required: 2)
end
- let(:entry) { Gitlab::CodeOwners::Entry.new("*.rb", "@user", "codeowners", false, 2) }
+ let(:entry) { Gitlab::CodeOwners::Entry.new("*.rb", "@user", section: "codeowners", optional: false, approvals_required: 2) }
it 'finds the existing rule' do
expect(rule).to eq(existing_code_owner_rule)
end
context "when the existing rule matches name but not section" do
- let(:entry) { Gitlab::CodeOwners::Entry.new("*.rb", "@user", "example_section", false, 2) }
+ let(:entry) { Gitlab::CodeOwners::Entry.new("*.rb", "@user", section: "example_section", optional: false, approvals_required: 2) }
it "creates a new rule" do
expect(rule).not_to eq(existing_code_owner_rule)
@@ -313,7 +313,7 @@
end
context "when section is present" do
- let(:entry) { Gitlab::CodeOwners::Entry.new("*.js", "@user", "Test Section") }
+ let(:entry) { Gitlab::CodeOwners::Entry.new("*.js", "@user", section: "Test Section") }
it "creates a new rule and saves section when present" do
expect(subject.section).to eq(entry.section)
diff --git a/ee/spec/services/merge_requests/sync_code_owner_approval_rules_spec.rb b/ee/spec/services/merge_requests/sync_code_owner_approval_rules_spec.rb
index b34daeb00f2f5ad9267d7f8db70cdbaf2ae864b6..b07bcbe74a0397cd81c0253b78ad07434eb47b29 100644
--- a/ee/spec/services/merge_requests/sync_code_owner_approval_rules_spec.rb
+++ b/ee/spec/services/merge_requests/sync_code_owner_approval_rules_spec.rb
@@ -28,7 +28,8 @@
def build_entry(pattern, users, groups, roles, section = Gitlab::CodeOwners::Section::DEFAULT, approvals_required = 0)
text = "#{(users + groups).map(&:to_reference).join(' ')} #{roles}"
- entry = Gitlab::CodeOwners::Entry.new(pattern, text, section, false, approvals_required)
+ entry = Gitlab::CodeOwners::Entry.new(pattern, text, section: section, optional: false,
+ approvals_required: approvals_required)
entry.add_matching_users_from(users)
entry.add_matching_groups_from(groups)