From d24bdf969a2d6d8997dbae41f6ac7d4b713e9858 Mon Sep 17 00:00:00 2001 From: Robert May Date: Fri, 13 Oct 2023 13:22:13 +0100 Subject: [PATCH 1/6] Replace cross-joins on approval rules Changelog: changed --- ee/app/models/concerns/approval_rule_like.rb | 18 ++++++++++++++++++ ee/app/models/ee/project.rb | 6 ++---- ee/app/serializers/ee/user_entity.rb | 2 +- 3 files changed, 21 insertions(+), 5 deletions(-) diff --git a/ee/app/models/concerns/approval_rule_like.rb b/ee/app/models/concerns/approval_rule_like.rb index e8c800a36cae62..30ad8b0a0b9d6c 100644 --- a/ee/app/models/concerns/approval_rule_like.rb +++ b/ee/app/models/concerns/approval_rule_like.rb @@ -22,6 +22,8 @@ module ApprovalRuleLike has_and_belongs_to_many :groups, class_name: 'Group', join_table: "#{self.table_name}_groups", after_add: :audit_add, after_remove: :audit_remove + + has_many :group_members, through: :groups has_many :group_users, -> { distinct.allow_cross_joins_across_databases(url: "https://gitlab.com/gitlab-org/gitlab/-/issues/417457") }, through: :groups, source: :users @@ -81,6 +83,14 @@ def approvers @approvers ||= filter_inactive_approvers(with_role_approvers) end + def approvers_include_user_id?(user) + # To match filter_inactive_approvers + return false unless user.active? + + deprecated_approvers_with_cross_join.pluck(:id).include?(user.id) || + group_members.exists?(user_id: user.id) + end + def code_owner? raise NotImplementedError end @@ -128,6 +138,14 @@ def direct_approvers end end + def deprecated_approvers_with_cross_join + if users.loaded? + users | role_approvers + else + User.from_union([users, role_approvers]) + end + end + def with_role_approvers if users.loaded? && group_users.loaded? users | group_users | role_approvers diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb index 536e848525f041..ed55f1645f9af2 100644 --- a/ee/app/models/ee/project.rb +++ b/ee/app/models/ee/project.rb @@ -744,11 +744,9 @@ def approvals_before_merge super end - def applicable_approval_rules_for_user(user_id, target_branch = nil) + def applicable_approval_rules_for_user(user, target_branch = nil) visible_approval_rules(target_branch: target_branch).select do |rule| - rule.approvers - .allow_cross_joins_across_databases(url: "https://gitlab.com/gitlab-org/gitlab/-/issues/417461") - .pluck(:id).include?(user_id) + rule.approvers_include_user_id?(user) end end diff --git a/ee/app/serializers/ee/user_entity.rb b/ee/app/serializers/ee/user_entity.rb index 578ae86e922cf5..ada28b68131dac 100644 --- a/ee/app/serializers/ee/user_entity.rb +++ b/ee/app/serializers/ee/user_entity.rb @@ -6,7 +6,7 @@ module UserEntity prepended do expose :applicable_approval_rules, if: proc { |_, options| options[:project]&.feature_available?(:merge_request_approvers) && options[:approval_rules] }, using: ::EE::API::Entities::ApprovalRuleShort do |user, options| - options[:project]&.applicable_approval_rules_for_user(user.id, options[:target_branch]) + options[:project]&.applicable_approval_rules_for_user(user, options[:target_branch]) end end end -- GitLab From 68291a269487838fe08671e80ce78435a01ea404 Mon Sep 17 00:00:00 2001 From: Thong Kuah Date: Mon, 16 Oct 2023 09:08:55 +0000 Subject: [PATCH 2/6] Apply 3 suggestion(s) to 1 file(s) --- ee/app/models/concerns/approval_rule_like.rb | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/ee/app/models/concerns/approval_rule_like.rb b/ee/app/models/concerns/approval_rule_like.rb index 30ad8b0a0b9d6c..35de3666bada62 100644 --- a/ee/app/models/concerns/approval_rule_like.rb +++ b/ee/app/models/concerns/approval_rule_like.rb @@ -84,11 +84,11 @@ def approvers end def approvers_include_user_id?(user) - # To match filter_inactive_approvers - return false unless user.active? + return false unless filter_inactive_approvers([user]).present? - deprecated_approvers_with_cross_join.pluck(:id).include?(user.id) || - group_members.exists?(user_id: user.id) + relation_exists?(users, :id, user.id) || + relation_exists?(role_approvers, :id, user.id) || + relation_exists?(group_members, :user_id, user.id) end def code_owner? @@ -138,12 +138,10 @@ def direct_approvers end end - def deprecated_approvers_with_cross_join - if users.loaded? - users | role_approvers - else - User.from_union([users, role_approvers]) - end + def relation_exists?(relation, column_name, id) + return relation.exists?({column_name => id}) unless relation.loaded? + + relation.detect { |item| item.read_attribute(column_name) == id } end def with_role_approvers -- GitLab From 8b624db97ce6cf31253dc0b631f7453431bdd36a Mon Sep 17 00:00:00 2001 From: Robert May Date: Mon, 16 Oct 2023 12:47:03 +0100 Subject: [PATCH 3/6] Add spec coverage --- .../concerns/approval_rule_like_spec.rb | 36 +++++++++++++------ 1 file changed, 26 insertions(+), 10 deletions(-) diff --git a/ee/spec/models/concerns/approval_rule_like_spec.rb b/ee/spec/models/concerns/approval_rule_like_spec.rb index 0c0ee82a0667a7..3bf8502b61df1a 100644 --- a/ee/spec/models/concerns/approval_rule_like_spec.rb +++ b/ee/spec/models/concerns/approval_rule_like_spec.rb @@ -15,20 +15,36 @@ let(:subject_traits) { [] } shared_examples 'approval rule like' do - describe '#approvers' do - let(:group1_user) { create(:user) } - let(:group2_user) { create(:user) } + let(:group1_user) { create(:user) } + let(:group2_user) { create(:user) } + + before do + subject.users << user1 + subject.users << user2 + subject.groups << group1 + subject.groups << group2 + + group1.add_guest(group1_user) + group2.add_guest(group2_user) + end + + describe '#approvers_include_user_id?' do + let(:rule) { subject.class.find(subject.id) } - before do - subject.users << user1 - subject.users << user2 - subject.groups << group1 - subject.groups << group2 + it 'returns true for a contained user' do + expect(rule.approvers_include_user_id?(user1)).to be_truthy + end - group1.add_guest(group1_user) - group2.add_guest(group2_user) + it 'returns true for a group user' do + expect(rule.approvers_include_user_id?(group1_user)).to be_truthy end + it 'returns false for a missing user' do + expect(rule.approvers_include_user_id?(user3)).to be_falsey + end + end + + describe '#approvers' do shared_examples 'approvers contains the right users' do it 'contains users as direct members and group members' do rule = subject.class.find(subject.id) -- GitLab From 31f3c836325f91a513a72a322f330fabfcec1684 Mon Sep 17 00:00:00 2001 From: Robert May Date: Mon, 16 Oct 2023 12:48:38 +0100 Subject: [PATCH 4/6] Fix rubocop fault --- ee/app/models/concerns/approval_rule_like.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/models/concerns/approval_rule_like.rb b/ee/app/models/concerns/approval_rule_like.rb index 35de3666bada62..997d09eccc840b 100644 --- a/ee/app/models/concerns/approval_rule_like.rb +++ b/ee/app/models/concerns/approval_rule_like.rb @@ -139,7 +139,7 @@ def direct_approvers end def relation_exists?(relation, column_name, id) - return relation.exists?({column_name => id}) unless relation.loaded? + return relation.exists?({ column_name => id }) unless relation.loaded? relation.detect { |item| item.read_attribute(column_name) == id } end -- GitLab From db05a6e450de60c13156c5ff48793154b40d2c81 Mon Sep 17 00:00:00 2001 From: Robert May Date: Tue, 17 Oct 2023 10:45:51 +0100 Subject: [PATCH 5/6] Add new relationship to all_models.yml --- spec/lib/gitlab/import_export/all_models.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index 4a6924b43a89d0..a5b26acbf6e315 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -1060,6 +1060,7 @@ approval_rules: - users - groups - group_users + - group_members - security_orchestration_policy_configuration - protected_branches - approval_merge_request_rule_sources -- GitLab From c83591cad9bcce8de118c978a6397651f2390746 Mon Sep 17 00:00:00 2001 From: Robert May Date: Wed, 25 Oct 2023 10:48:26 +0100 Subject: [PATCH 6/6] Adjustments from reviews --- ee/app/models/concerns/approval_rule_like.rb | 16 +++++------ ee/app/models/ee/project.rb | 2 +- .../concerns/approval_rule_like_spec.rb | 28 ++++++++++++++++--- 3 files changed, 33 insertions(+), 13 deletions(-) diff --git a/ee/app/models/concerns/approval_rule_like.rb b/ee/app/models/concerns/approval_rule_like.rb index 997d09eccc840b..c8819d27da8b77 100644 --- a/ee/app/models/concerns/approval_rule_like.rb +++ b/ee/app/models/concerns/approval_rule_like.rb @@ -83,12 +83,12 @@ def approvers @approvers ||= filter_inactive_approvers(with_role_approvers) end - def approvers_include_user_id?(user) - return false unless filter_inactive_approvers([user]).present? + def approvers_include_user?(user) + return false if filter_inactive_approvers([user]).empty? - relation_exists?(users, :id, user.id) || - relation_exists?(role_approvers, :id, user.id) || - relation_exists?(group_members, :user_id, user.id) + relation_exists?(users, column: :id, value: user.id) || + relation_exists?(role_approvers, column: :id, value: user.id) || + relation_exists?(group_members, column: :user_id, value: user.id) end def code_owner? @@ -138,10 +138,10 @@ def direct_approvers end end - def relation_exists?(relation, column_name, id) - return relation.exists?({ column_name => id }) unless relation.loaded? + def relation_exists?(relation, column:, value:) + return relation.exists?({ column => value }) unless relation.loaded? - relation.detect { |item| item.read_attribute(column_name) == id } + relation.detect { |item| item.read_attribute(column) == value } end def with_role_approvers diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb index ed55f1645f9af2..1bc4bf199916ff 100644 --- a/ee/app/models/ee/project.rb +++ b/ee/app/models/ee/project.rb @@ -746,7 +746,7 @@ def approvals_before_merge def applicable_approval_rules_for_user(user, target_branch = nil) visible_approval_rules(target_branch: target_branch).select do |rule| - rule.approvers_include_user_id?(user) + rule.approvers_include_user?(user) end end diff --git a/ee/spec/models/concerns/approval_rule_like_spec.rb b/ee/spec/models/concerns/approval_rule_like_spec.rb index 3bf8502b61df1a..8d228d31c53f3f 100644 --- a/ee/spec/models/concerns/approval_rule_like_spec.rb +++ b/ee/spec/models/concerns/approval_rule_like_spec.rb @@ -28,19 +28,39 @@ group2.add_guest(group2_user) end - describe '#approvers_include_user_id?' do + describe '#approvers_include_user?' do let(:rule) { subject.class.find(subject.id) } it 'returns true for a contained user' do - expect(rule.approvers_include_user_id?(user1)).to be_truthy + expect(rule.approvers_include_user?(user1)).to be_truthy end it 'returns true for a group user' do - expect(rule.approvers_include_user_id?(group1_user)).to be_truthy + expect(rule.approvers_include_user?(group1_user)).to be_truthy end it 'returns false for a missing user' do - expect(rule.approvers_include_user_id?(user3)).to be_falsey + expect(rule.approvers_include_user?(user3)).to be_falsey + end + + context 'when the user relations are already loaded' do + it 'returns true for a contained user' do + rule.users.to_a + + expect(rule.approvers_include_user?(user1)).to be_truthy + end + + it 'returns true for a group user' do + rule.group_members.to_a + + expect(rule.approvers_include_user?(group1_user)).to be_truthy + end + + it 'returns false for a missing user' do + rule.users.to_a + + expect(rule.approvers_include_user?(user3)).to be_falsey + end end end -- GitLab