diff --git a/ee/app/controllers/ee/search_controller.rb b/ee/app/controllers/ee/search_controller.rb
index 52b8b043fbd2e46c986ab4ef6a1ec49d227061be..1109b56ee06aab0bf36c8b44cf819527d7e81b2f 100644
--- a/ee/app/controllers/ee/search_controller.rb
+++ b/ee/app/controllers/ee/search_controller.rb
@@ -101,6 +101,8 @@ def payload_metadata
super.merge(
'meta.search.filters.source_branch' => filter_params[:source_branch],
'meta.search.filters.not_source_branch' => filter_params.dig(:not, :source_branch),
+ 'meta.search.filters.target_branch' => filter_params[:target_branch],
+ 'meta.search.filters.not_target_branch' => filter_params.dig(:not, :target_branch),
'meta.search.filters.author_username' => filter_params[:author_username],
'meta.search.filters.not_author_username' => filter_params.dig(:not, :author_username))
end
@@ -125,7 +127,11 @@ def run_index_integrity_worker
override :filter_params
def filter_params
- super.merge(params.permit(:source_branch, :author_username, not: [:source_branch, :author_username]))
+ permitted_filter_params = [:source_branch, :target_branch, :author_username]
+ super.merge(params.permit(
+ *permitted_filter_params,
+ not: permitted_filter_params
+ ))
end
end
end
diff --git a/ee/app/services/concerns/ee/search/filter.rb b/ee/app/services/concerns/ee/search/filter.rb
index 8f11da561019f6ecd24838d1d27080e0f0d6362b..de8997f5dce24fd212168e4f5a5f42b7b37a8eb6 100644
--- a/ee/app/services/concerns/ee/search/filter.rb
+++ b/ee/app/services/concerns/ee/search/filter.rb
@@ -15,6 +15,8 @@ def filters
label_name: params[:label_name],
source_branch: params[:source_branch],
not_source_branch: params[:not_source_branch],
+ target_branch: params[:target_branch],
+ not_target_branch: params[:not_target_branch],
author_username: params[:author_username],
not_author_username: params[:not_author_username]
)
diff --git a/ee/config/feature_flags/gitlab_com_derisk/search_mr_filter_target_branch.yml b/ee/config/feature_flags/gitlab_com_derisk/search_mr_filter_target_branch.yml
new file mode 100644
index 0000000000000000000000000000000000000000..0b12c480fe23fd8adcadd27ae83535a05e778ba4
--- /dev/null
+++ b/ee/config/feature_flags/gitlab_com_derisk/search_mr_filter_target_branch.yml
@@ -0,0 +1,9 @@
+---
+name: search_mr_filter_target_branch
+feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/463375
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/160210
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/473699
+milestone: '17.3'
+group: group::global search
+type: gitlab_com_derisk
+default_enabled: false
diff --git a/ee/lib/gitlab/elastic/search_results.rb b/ee/lib/gitlab/elastic/search_results.rb
index 605da5263984e1274f76d5d8be7e20a4cf543684..8c56879b9bfb756ecc29855861f1ca469f9b06f8 100644
--- a/ee/lib/gitlab/elastic/search_results.rb
+++ b/ee/lib/gitlab/elastic/search_results.rb
@@ -326,9 +326,7 @@ def scope_options(scope)
when :projects, :notes, :commits
base_options.merge(filters.slice(:include_archived))
when :merge_requests
- base_options.merge(
- filters.slice(:order_by, :sort, :state, :include_archived, :source_branch, :not_source_branch,
- :author_username, :not_author_username, :label_name))
+ base_options.merge(merge_request_scope_options)
when :issues
base_options.merge(
filters.slice(:order_by, :sort, :confidential, :state, :labels, :label_name, :include_archived),
@@ -488,6 +486,22 @@ def issue_aggregations
end
strong_memoize_attr :issue_aggregations
+ def merge_request_scope_options
+ filters.slice(
+ :order_by,
+ :sort,
+ :state,
+ :include_archived,
+ :source_branch,
+ :not_source_branch,
+ :target_branch,
+ :not_target_branch,
+ :author_username,
+ :not_author_username,
+ :label_name
+ )
+ end
+
def allowed_to_read_users?
Ability.allowed?(current_user, :read_users_list)
end
diff --git a/ee/lib/search/elastic/filters.rb b/ee/lib/search/elastic/filters.rb
index 1206fbae736fc2c76bf9175bf5f20b62d23094e7..40b9fc5adcd3908d20c225c39d9e5f6beb705bc3 100644
--- a/ee/lib/search/elastic/filters.rb
+++ b/ee/lib/search/elastic/filters.rb
@@ -34,6 +34,34 @@ def by_source_branch(query_hash:, options:)
end
end
+ def by_target_branch(query_hash:, options:)
+ target_branch = options[:target_branch]
+ not_target_branch = options[:not_target_branch]
+
+ return query_hash unless target_branch || not_target_branch
+
+ context.name(:filters) do
+ should = []
+ if target_branch
+ should << { term: { target_branch: { _name: context.name(:target_branch), value: target_branch } } }
+ end
+
+ if not_target_branch
+ should << {
+ bool: {
+ must_not: {
+ term: { target_branch: { _name: context.name(:not_target_branch), value: not_target_branch } }
+ }
+ }
+ }
+ end
+
+ add_filter(query_hash, :query, :bool, :filter) do
+ { bool: { should: should, minimum_should_match: 1 } }
+ end
+ end
+ end
+
def by_author(query_hash:, options:)
author_username = options[:author_username]
not_author_username = options[:not_author_username]
diff --git a/ee/lib/search/elastic/merge_request_query_builder.rb b/ee/lib/search/elastic/merge_request_query_builder.rb
index 7eeb36ae221868d880832f6284eee60f1ebfc8fa..450d3a11108caf4b662ea9df85109b05f506b08a 100644
--- a/ee/lib/search/elastic/merge_request_query_builder.rb
+++ b/ee/lib/search/elastic/merge_request_query_builder.rb
@@ -8,22 +8,7 @@ class MergeRequestQueryBuilder < QueryBuilder
DOC_TYPE = 'merge_request'
def build
- query_hash =
- if query =~ /!(\d+)\z/
- ::Search::Elastic::Queries.by_iid(iid: Regexp.last_match(1), doc_type: DOC_TYPE)
- else
- # iid field can be added here as lenient option will
- # pardon format errors, like integer out of range.
- fields = %w[iid^3 title^2 description]
-
- if !::Search::Elastic::Queries::ADVANCED_QUERY_SYNTAX_REGEX.match?(query) &&
- Feature.enabled?(:search_uses_match_queries, options[:current_user])
- ::Search::Elastic::Queries.by_multi_match_query(fields: fields, query: query, options: options)
- else
- ::Search::Elastic::Queries.by_simple_query_string(fields: fields, query: query, options: options)
- end
- end
-
+ query_hash = build_query_hash(query: query, options: options)
query_hash = ::Search::Elastic::Filters.by_authorization(query_hash: query_hash, options: options)
query_hash = ::Search::Elastic::Filters.by_state(query_hash: query_hash, options: options)
query_hash = ::Search::Elastic::Filters.by_archived(query_hash: query_hash, options: options)
@@ -32,6 +17,10 @@ def build
query_hash = ::Search::Elastic::Filters.by_source_branch(query_hash: query_hash, options: options)
end
+ if Feature.enabled?(:search_mr_filter_target_branch, options[:current_user])
+ query_hash = ::Search::Elastic::Filters.by_target_branch(query_hash: query_hash, options: options)
+ end
+
if Feature.enabled?(:search_mr_filter_author, options[:current_user])
query_hash = ::Search::Elastic::Filters.by_author(query_hash: query_hash, options: options)
end
@@ -62,6 +51,23 @@ def extra_options
authorization_use_traversal_ids: false # https://gitlab.com/gitlab-org/gitlab/-/issues/351279
}
end
+
+ def build_query_hash(query:, options:)
+ if query =~ /!(\d+)\z/
+ ::Search::Elastic::Queries.by_iid(iid: Regexp.last_match(1), doc_type: DOC_TYPE)
+ else
+ # iid field can be added here as lenient option will
+ # pardon format errors, like integer out of range.
+ fields = %w[iid^3 title^2 description]
+
+ if !::Search::Elastic::Queries::ADVANCED_QUERY_SYNTAX_REGEX.match?(query) &&
+ Feature.enabled?(:search_uses_match_queries, options[:current_user])
+ ::Search::Elastic::Queries.by_multi_match_query(fields: fields, query: query, options: options)
+ else
+ ::Search::Elastic::Queries.by_simple_query_string(fields: fields, query: query, options: options)
+ end
+ end
+ end
end
end
end
diff --git a/ee/spec/controllers/ee/search_controller_spec.rb b/ee/spec/controllers/ee/search_controller_spec.rb
index 575429516c98f52765c2bb7ad959fe8983fc22ba..414789c2b8514ebf701a8356ee7629666ed9d2d4 100644
--- a/ee/spec/controllers/ee/search_controller_spec.rb
+++ b/ee/spec/controllers/ee/search_controller_spec.rb
@@ -300,6 +300,8 @@ def request
expect(payload[:metadata]['meta.search.filters.source_branch']).to eq('included-branch')
expect(payload[:metadata]['meta.search.filters.not_source_branch']).to eq('excluded-branch')
+ expect(payload[:metadata]['meta.search.filters.target_branch']).to eq('included-branch')
+ expect(payload[:metadata]['meta.search.filters.not_target_branch']).to eq('excluded-branch')
expect(payload[:metadata]['meta.search.filters.author_username']).to eq('included-username')
expect(payload[:metadata]['meta.search.filters.not_author_username']).to eq('excluded-username')
end
@@ -308,9 +310,11 @@ def request
scope: 'issues',
search: 'hello world',
source_branch: 'included-branch',
+ target_branch: 'included-branch',
author_username: 'included-username',
not: {
source_branch: 'excluded-branch',
+ target_branch: 'excluded-branch',
author_username: 'excluded-username'
}
}
diff --git a/ee/spec/lib/search/elastic/filters_spec.rb b/ee/spec/lib/search/elastic/filters_spec.rb
index 3c50fb72d6c32ef74152ae11c97dd9ae6e0ed2a3..734f88cb59228cd94b95cc3cf3b8532a066223ef 100644
--- a/ee/spec/lib/search/elastic/filters_spec.rb
+++ b/ee/spec/lib/search/elastic/filters_spec.rb
@@ -12,7 +12,7 @@
end
end
- describe '#by_source_branch' do
+ describe '.by_source_branch' do
subject(:by_source_branch) { described_class.by_source_branch(query_hash: query_hash, options: options) }
context 'when options[:source_branch] and options[:not_source_branch] are empty' do
@@ -83,7 +83,78 @@
end
end
- describe '#by_author' do
+ describe '.by_target_branch' do
+ subject(:by_target_branch) { described_class.by_target_branch(query_hash: query_hash, options: options) }
+
+ context 'when options[:target_branch] and options[:not_target_branch] are empty' do
+ let(:options) { {} }
+
+ it_behaves_like 'does not modify the query_hash'
+ end
+
+ context 'when options[:target_branch] and options[:not_target_branch] are both provided' do
+ let(:options) { { target_branch: 'branch-1', not_target_branch: 'branch-2' } }
+
+ it 'adds the target branch filter to query_hash' do
+ expected_filter = [
+ { bool:
+ { should: [{ term: { target_branch: { _name: 'filters:target_branch', value: 'branch-1' } } },
+ { bool: {
+ must_not: {
+ term: { target_branch: { _name: 'filters:not_target_branch', value: 'branch-2' } }
+ }
+ } }],
+ minimum_should_match: 1 } }
+ ]
+
+ expect(by_target_branch.dig(:query, :bool, :filter)).to eq(expected_filter)
+ expect(by_target_branch.dig(:query, :bool, :must)).to be_empty
+ expect(by_target_branch.dig(:query, :bool, :must_not)).to be_empty
+ expect(by_target_branch.dig(:query, :bool, :should)).to be_empty
+ end
+ end
+
+ context 'when options[:target_branch] is provided' do
+ let(:options) { { target_branch: 'foo-bar-branch' } }
+
+ it 'adds the target branch filter to query_hash' do
+ expected_filter = [
+ { bool:
+ { should: [{ term: { target_branch: { _name: 'filters:target_branch', value: 'foo-bar-branch' } } }],
+ minimum_should_match: 1 } }
+ ]
+
+ expect(by_target_branch.dig(:query, :bool, :filter)).to eq(expected_filter)
+ expect(by_target_branch.dig(:query, :bool, :must)).to be_empty
+ expect(by_target_branch.dig(:query, :bool, :must_not)).to be_empty
+ expect(by_target_branch.dig(:query, :bool, :should)).to be_empty
+ end
+ end
+
+ context 'when options[:not_target_branch] is provided' do
+ let(:options) { { not_target_branch: 'hello-branch' } }
+
+ it 'adds the target branch filter to query_hash' do
+ expected_filter = [
+ { bool:
+ { should:
+ [{ bool: {
+ must_not: {
+ term: { target_branch: { _name: 'filters:not_target_branch', value: 'hello-branch' } }
+ }
+ } }],
+ minimum_should_match: 1 } }
+ ]
+
+ expect(by_target_branch.dig(:query, :bool, :filter)).to eq(expected_filter)
+ expect(by_target_branch.dig(:query, :bool, :must)).to be_empty
+ expect(by_target_branch.dig(:query, :bool, :must_not)).to be_empty
+ expect(by_target_branch.dig(:query, :bool, :should)).to be_empty
+ end
+ end
+ end
+
+ describe '.by_author' do
let_it_be(:included_user) { create(:user) }
let_it_be(:excluded_user) { create(:user) }
@@ -157,7 +228,7 @@
end
end
- describe '#by_not_hidden' do
+ describe '.by_not_hidden' do
subject(:by_not_hidden) { described_class.by_not_hidden(query_hash: query_hash, options: options) }
context 'when options[:current_user] is empty' do
@@ -197,7 +268,7 @@
end
end
- describe '#by_state' do
+ describe '.by_state' do
subject(:by_state) { described_class.by_state(query_hash: query_hash, options: options) }
context 'when options[:state] is empty' do
@@ -232,7 +303,7 @@
end
end
- describe '#by_archived' do
+ describe '.by_archived' do
subject(:by_archived) { described_class.by_archived(query_hash: query_hash, options: options) }
context 'when options[:include_archived] is empty or false' do
@@ -285,7 +356,7 @@
end
end
- describe '#by_label_ids' do
+ describe '.by_label_ids' do
let_it_be(:label_title) { 'My label' }
let_it_be(:group) { create(:group) }
let_it_be(:project) { create(:project, group: group) }
@@ -506,7 +577,7 @@
end
end
- describe '#by_confidentiality' do
+ describe '.by_confidentiality' do
let_it_be(:authorized_project) { create(:project, developers: [user]) }
let_it_be(:private_project) { create(:project, :private) }
@@ -652,7 +723,7 @@
end
end
- describe '#by_authorization' do
+ describe '.by_authorization' do
let_it_be(:public_group) { create(:group, :public) }
let_it_be(:authorized_project) { create(:project, group: public_group, developers: [user]) }
let_it_be(:private_project) { create(:project, :private, group: public_group) }
diff --git a/ee/spec/lib/search/elastic/merge_request_query_builder_spec.rb b/ee/spec/lib/search/elastic/merge_request_query_builder_spec.rb
index c03da822b41e351a793aae9e44a76016406168b8..67d2982f3af608da00556273f9bf1e10776d7df1 100644
--- a/ee/spec/lib/search/elastic/merge_request_query_builder_spec.rb
+++ b/ee/spec/lib/search/elastic/merge_request_query_builder_spec.rb
@@ -87,46 +87,80 @@
end
describe 'source_branch' do
+ it 'does not apply filters by default' do
+ assert_names_in_query(build, without: %w[filters:source_branch filters:not_source_branch])
+ end
+
+ context 'when source_branch option is provided' do
+ let(:options) { base_options.merge(source_branch: 'hello') }
+
+ it 'applies the filter' do
+ assert_names_in_query(build, with: %w[filters:source_branch])
+ end
+ end
+
+ context 'when not_source_branch option is provided' do
+ let(:options) { base_options.merge(not_source_branch: 'world') }
+
+ it 'applies the filter' do
+ assert_names_in_query(build, with: %w[filters:not_source_branch])
+ end
+ end
+
context 'when search_mr_filter_source_branch flag is false' do
before do
stub_feature_flags(search_mr_filter_source_branch: false)
end
- it 'does not apply filters by default' do
+ it 'does not apply filters' do
assert_names_in_query(build, without: %w[filters:source_branch filters:not_source_branch])
end
context 'when source_branch options are provided' do
let(:options) { base_options.merge(source_branch: 'hello', not_source_branch: 'world') }
- it 'does not apply filters by default' do
+ it 'does not apply filters' do
assert_names_in_query(build, without: %w[filters:source_branch filters:not_source_branch])
end
end
end
+ end
- context 'when search_mr_filter_source_branch flag is true' do
- before do
- stub_feature_flags(search_mr_filter_source_branch: true)
+ describe 'target_branch' do
+ it 'does not apply filters by default' do
+ assert_names_in_query(build, without: %w[filters:target_branch filters:not_target_branch])
+ end
+
+ context 'when target_branch option is provided' do
+ let(:options) { base_options.merge(target_branch: 'hello') }
+
+ it 'applies the filter' do
+ assert_names_in_query(build, with: %w[filters:target_branch])
end
+ end
- it 'does not apply filters by default' do
- assert_names_in_query(build, without: %w[filters:source_branch filters:not_source_branch])
+ context 'when not_target_branch option is provided' do
+ let(:options) { base_options.merge(not_target_branch: 'world') }
+
+ it 'applies the filter' do
+ assert_names_in_query(build, with: %w[filters:not_target_branch])
end
+ end
- context 'when source_branch option is provided' do
- let(:options) { base_options.merge(source_branch: 'hello') }
+ context 'when search_mr_filter_target_branch flag is false' do
+ before do
+ stub_feature_flags(search_mr_filter_target_branch: false)
+ end
- it 'does not apply filters by default' do
- assert_names_in_query(build, with: %w[filters:source_branch])
- end
+ it 'does not apply filters' do
+ assert_names_in_query(build, without: %w[filters:target_branch filters:not_target_branch])
end
- context 'when not_source_branch option is provided' do
- let(:options) { base_options.merge(not_source_branch: 'world') }
+ context 'when target_branch options are provided' do
+ let(:options) { base_options.merge(target_branch: 'hello', not_target_branch: 'world') }
- it 'does not apply filters by default' do
- assert_names_in_query(build, with: %w[filters:not_source_branch])
+ it 'does not apply filters' do
+ assert_names_in_query(build, without: %w[filters:target_branch filters:not_target_branch])
end
end
end
@@ -135,48 +169,42 @@
describe 'author' do
let_it_be(:user) { create(:user) }
- context 'when search_mr_filter_author flag is false' do
- before do
- stub_feature_flags(search_mr_filter_author: false)
- end
+ it 'does not apply filters by default' do
+ assert_names_in_query(build, without: %w[filters:author filters:not_author])
+ end
- it 'does not apply filters by default' do
- assert_names_in_query(build, without: %w[filters:author filters:not_author])
+ context 'when author_username option is provided' do
+ let(:options) { base_options.merge(author_username: user.username) }
+
+ it 'applies the filter' do
+ assert_names_in_query(build, with: %w[filters:author])
end
+ end
- context 'when author_username options are provided' do
- let(:options) do
- base_options.merge(author_username: user.username, not_author_username: user.username)
- end
+ context 'when not_author_username option is provided' do
+ let(:options) { base_options.merge(not_author_username: user.username) }
- it 'does not apply filters by default' do
- assert_names_in_query(build, without: %w[filters:author filters:not_author])
- end
+ it 'applies the filter' do
+ assert_names_in_query(build, with: %w[filters:not_author])
end
end
- context 'when search_mr_filter_author flag is true' do
+ context 'when search_mr_filter_author flag is false' do
before do
- stub_feature_flags(search_mr_filter_author: true)
+ stub_feature_flags(search_mr_filter_author: false)
end
- it 'does not apply filters by default' do
+ it 'does not apply filters' do
assert_names_in_query(build, without: %w[filters:author filters:not_author])
end
- context 'when author_username option is provided' do
- let(:options) { base_options.merge(author_username: user.username) }
-
- it 'does not apply filters by default' do
- assert_names_in_query(build, with: %w[filters:author])
+ context 'when author_username options are provided' do
+ let(:options) do
+ base_options.merge(author_username: user.username, not_author_username: user.username)
end
- end
- context 'when not_author_username option is provided' do
- let(:options) { base_options.merge(not_author_username: user.username) }
-
- it 'does not apply filters by default' do
- assert_names_in_query(build, with: %w[filters:not_author])
+ it 'does not apply filters' do
+ assert_names_in_query(build, without: %w[filters:author filters:not_author])
end
end
end