Skip to content
Snippets Groups Projects
Commit 9ffecb10 authored by Adam Hegyi's avatar Adam Hegyi
Browse files

Merge branch 'id-by-approvers-finder-cross-joins' into 'master'

Rewrite SQL query for ByApproversFinder

See merge request !136614



Merged-by: Adam Hegyi's avatarAdam Hegyi <ahegyi@gitlab.com>
Approved-by: Thong Kuah's avatarThong Kuah <tkuah@gitlab.com>
Approved-by: Adam Hegyi's avatarAdam Hegyi <ahegyi@gitlab.com>
Reviewed-by: Igor Drozdov's avatarIgor Drozdov <idrozdov@gitlab.com>
Reviewed-by: Thong Kuah's avatarThong Kuah <tkuah@gitlab.com>
Co-authored-by: Igor Drozdov's avatarIgor Drozdov <idrozdov@gitlab.com>
parents b3acff0f 72fcd7fe
No related branches found
No related tags found
2 merge requests!136855Draft: Revert saml-auth-flow-for-mr-approval from 16-6-stable-ee,!136614Rewrite SQL query for ByApproversFinder
Pipeline #1071649864 passed
......@@ -19,7 +19,7 @@ def execute(items)
elsif by_any_approvers?
with_any_approvers(items)
elsif ids.present?
find_approvers_by_ids(items)
find_approvers_by_ids(items, ids)
elsif usernames.present?
find_approvers_by_names(items)
else
......@@ -59,32 +59,47 @@ def with_any_approvers(items)
end
def find_approvers_by_names(items)
with_users_filtered_by_criteria(items) do |items_with_users|
find_approvers_by_query(items_with_users, :username, usernames)
end
find_approvers_by_ids(items, User.where(username: usernames).ids)
end
def find_approvers_by_ids(items)
with_users_filtered_by_criteria(items) do |items_with_users|
find_approvers_by_query(items_with_users, :id, ids)
# It is possible to filter merge requests by passing multiple approvers.
# This method looks for merge requests that contain an approver with an id
# included in the passed list.
# Then it's checked that the merge requests are duplicated exactly the number of
# times as the number of passed ids in order to find the merge requests that
# contain all the approvers from the passed list.
def find_approvers_by_ids(items, user_ids)
with_users_filtered_by_criteria(items) do |items_with_users, users_association|
items_with_users
.where(users_association => { user_id: user_ids })
.group('merge_requests.id')
.having("COUNT(#{users_association}.user_id) = ?", user_ids.size)
end
end
def find_approvers_by_query(items, field, values)
items
.where(users: { field => values })
.group('merge_requests.id')
.having("COUNT(users.id) = ?", values.size)
.allow_cross_joins_across_databases(url: "https://gitlab.com/gitlab-org/gitlab/-/issues/417459")
end
# This method iterates over all possible sources of approvers and applies the logic
# from the block passed in #find_approvers_by_ids
def with_users_filtered_by_criteria(items)
users_mrs = yield(items.joins(approval_rules: :users))
group_users_mrs = yield(items.joins(approval_rules: { groups: :users }))
mrs_without_overridden_rules = items.left_outer_joins(:approval_rules).where(approval_merge_request_rules: { id: nil })
project_users_mrs = yield(mrs_without_overridden_rules.joins(target_project: { approval_rules: :users }))
project_group_users_mrs = yield(mrs_without_overridden_rules.joins(target_project: { approval_rules: { groups: :users } }))
# Users added directly to the merge request approval rules
users_mrs = yield(
items.joins(approval_rules: :approval_merge_request_rules_users), :approval_merge_request_rules_users
)
# Users added via groups to the merge request approval rules
group_users_mrs = yield(items.joins(approval_rules: :group_members), :members)
mrs_without_overridden_rules =
items.left_outer_joins(:approval_rules).where(approval_merge_request_rules: { id: nil })
# Users added to a project approval rule that are used by a merge request
project_users_mrs = yield(
mrs_without_overridden_rules.joins(target_project: {
approval_rules: :approval_project_rules_users
}), :approval_project_rules_users
)
# Users added via groups to a project approval rule that are used by a merge request
project_group_users_mrs = yield(
mrs_without_overridden_rules.joins(target_project: { approval_rules: :group_members }), :members
)
items.select_from_union([users_mrs, group_users_mrs, project_users_mrs, project_group_users_mrs])
end
......
......@@ -5,8 +5,6 @@ class ApprovalMergeRequestRule < ApplicationRecord
include ApprovalRuleLike
include UsageStatistics
has_many :scan_result_policy_violations, through: :scan_result_policy_read, source: :violations
scope :not_matching_pattern, -> (pattern) { code_owner.where.not(name: pattern) }
scope :matching_pattern, -> (pattern) { code_owner.where(name: pattern) }
......@@ -37,6 +35,8 @@ class ApprovalMergeRequestRule < ApplicationRecord
# approved_approvers is only populated after MR is merged
has_and_belongs_to_many :approved_approvers, class_name: 'User', join_table: :approval_merge_request_rules_approved_approvers
has_many :approval_merge_request_rules_users
has_many :scan_result_policy_violations, through: :scan_result_policy_read, source: :violations
has_one :approval_merge_request_rule_source
has_one :approval_project_rule, through: :approval_merge_request_rule_source
has_one :approval_project_rule_project, through: :approval_project_rule, source: :project
......
# frozen_string_literal: true
# Model for join table between ApprovalMergeRequestRule and User
class ApprovalMergeRequestRulesUser < ApplicationRecord # rubocop:disable Gitlab/NamespacedClass -- Conventional name for a join class
end
......@@ -2,7 +2,7 @@
require 'spec_helper'
RSpec.describe ApprovalMergeRequestRule, factory_default: :keep do
RSpec.describe ApprovalMergeRequestRule, factory_default: :keep, feature_category: :code_review do
let_it_be_with_reload(:project) { create_default(:project, :repository) }
let_it_be_with_reload(:merge_request) { create_default(:merge_request) }
......@@ -12,6 +12,14 @@
subject { build_stubbed(:approval_merge_request_rule) }
it { is_expected.to have_one(:approval_project_rule_project).through(:approval_project_rule) }
it { is_expected.to have_one(:approval_project_rule).through(:approval_merge_request_rule_source) }
it { is_expected.to have_many(:approval_merge_request_rules_users) }
it do
is_expected.to have_many(:scan_result_policy_violations)
.through(:scan_result_policy_read)
.source(:violations)
end
end
describe 'validations' do
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment