From c4e91dbc781c73fb3feb8dbe5fb48ecf2a95c374 Mon Sep 17 00:00:00 2001 From: Sashi Kumar Kumaresan Date: Wed, 8 Oct 2025 22:58:02 +0200 Subject: [PATCH 1/2] Refactor member role query to consider hierarchy This change updates the query to get the users from member role to consider the hierarchy of membership. EE: true Changelog: fixed --- ee/app/models/ee/project_team.rb | 66 +++++++++++++++++++++++++++++--- 1 file changed, 61 insertions(+), 5 deletions(-) diff --git a/ee/app/models/ee/project_team.rb b/ee/app/models/ee/project_team.rb index 57f8f75a820a6b..0c0e72a8130256 100644 --- a/ee/app/models/ee/project_team.rb +++ b/ee/app/models/ee/project_team.rb @@ -9,16 +9,20 @@ def members_with_access_level_or_custom_roles(levels: [], member_role_ids: []) return ::User.none unless levels.any? || member_role_ids.any? users = project.authorized_users + applicable_members = distinct_applicable_members if levels.any? && member_role_ids.any? - users = users - .where(project_authorizations: { access_level: levels }) - .or(users.where(members: { member_role_id: member_role_ids })) - .joins(:members) + users = users.where(project_authorizations: { access_level: levels }).or( + users.joins( + "INNER JOIN (#{applicable_members.to_sql}) AS applicable_members ON applicable_members.user_id = users.id" + ).where('applicable_members.member_role_id' => member_role_ids) + ) elsif levels.any? users = users.where(project_authorizations: { access_level: levels }) elsif member_role_ids.any? - users = users.joins(:members).where(members: { member_role_id: member_role_ids }) + users = users.joins( + "INNER JOIN (#{applicable_members.to_sql}) AS applicable_members ON applicable_members.user_id = users.id" + ).where('applicable_members.member_role_id' => member_role_ids) end users @@ -54,6 +58,58 @@ def add_member(user, access_level, current_user: nil, expires_at: nil) private + def distinct_applicable_members + union_members = build_union_members + return ::Member.none if union_members.empty? + + union = ::Gitlab::SQL::Union.new(union_members, remove_duplicates: false) + sql = distinct_on_user_id(union) + + # Enumerate columns to avoid column caching issues + ::Member.select(*::Member.column_names) + .from([Arel.sql("(#{sql}) AS #{::Member.table_name}")]) + end + + def build_union_members + union_members = [] + + # Project members + union_members << project.project_members.select(::Member.column_names) + + # Group members from ancestors (single query for all ancestor groups) + if project.group + union_members << ::GroupMember + .where(source_id: project.group.self_and_ancestor_ids) + .select(::Member.column_names) + end + + union_members + end + + def distinct_on_user_id(union) + # Get one member per user_id, preferring ProjectMember over GroupMember + <<~SQL + SELECT DISTINCT ON (user_id) #{member_columns} + FROM (#{union.to_sql}) AS #{member_union_table} + ORDER BY user_id, + CASE + WHEN type = 'ProjectMember' THEN 1 + WHEN type = 'GroupMember' THEN 2 + ELSE 3 + END + SQL + end + + def member_union_table + 'member_union' + end + + def member_columns + ::Member.column_names.map do |column_name| + "#{member_union_table}.#{column_name}" + end.join(', ') + end + def group_member_lock group && group.membership_lock end -- GitLab From 171affe23c54979f87cbb961252d20de4e990cfd Mon Sep 17 00:00:00 2001 From: Sashi Kumar Kumaresan Date: Fri, 10 Oct 2025 13:10:06 +0200 Subject: [PATCH 2/2] Refactor spec and fix failing spec --- ee/app/models/ee/project_team.rb | 12 +- ee/spec/models/ee/project_team_spec.rb | 302 ++++++++++++++++++++----- 2 files changed, 259 insertions(+), 55 deletions(-) diff --git a/ee/app/models/ee/project_team.rb b/ee/app/models/ee/project_team.rb index 0c0e72a8130256..1e6b3dabf7b943 100644 --- a/ee/app/models/ee/project_team.rb +++ b/ee/app/models/ee/project_team.rb @@ -12,10 +12,14 @@ def members_with_access_level_or_custom_roles(levels: [], member_role_ids: []) applicable_members = distinct_applicable_members if levels.any? && member_role_ids.any? - users = users.where(project_authorizations: { access_level: levels }).or( - users.joins( - "INNER JOIN (#{applicable_members.to_sql}) AS applicable_members ON applicable_members.user_id = users.id" - ).where('applicable_members.member_role_id' => member_role_ids) + # Add the join to the base query first so both sides of .or() have the same join structure + users_with_join = users.joins( + "INNER JOIN (#{applicable_members.to_sql}) AS applicable_members ON applicable_members.user_id = users.id" + ) + + # Now both sides of .or() have the same join structure + users = users_with_join.where(project_authorizations: { access_level: levels }).or( + users_with_join.where('applicable_members.member_role_id' => member_role_ids) ) elsif levels.any? users = users.where(project_authorizations: { access_level: levels }) diff --git a/ee/spec/models/ee/project_team_spec.rb b/ee/spec/models/ee/project_team_spec.rb index b9a52f611e0217..52e3a295521194 100644 --- a/ee/spec/models/ee/project_team_spec.rb +++ b/ee/spec/models/ee/project_team_spec.rb @@ -109,22 +109,37 @@ end describe '#members_with_access_level_or_custom_roles' do - let_it_be(:group) { create(:group) } + let_it_be(:parent_group) { create(:group) } + let_it_be(:group) { create(:group, parent: parent_group) } let_it_be(:project) { create(:project, group: group) } - let_it_be(:custom_role) { create(:member_role) } - - let_it_be(:developer) { create(:user) } - let_it_be(:maintainer) { create(:user) } - let_it_be(:reporter) { create(:user) } - - let_it_be(:levels) { [] } - let_it_be(:member_role_ids) { [] } - - before do - create(:project_member, :developer, project: project, user: developer, member_role: custom_role) - create(:project_member, :maintainer, project: project, user: maintainer) - create(:project_member, :reporter, project: project, user: reporter) + let_it_be(:custom_role) { create(:member_role, namespace: parent_group) } + let_it_be(:another_custom_role) { create(:member_role, namespace: parent_group) } + + let_it_be(:project_developer_with_custom_role) { create(:user) } + let_it_be(:project_maintainer) { create(:user) } + let_it_be(:project_reporter) { create(:user) } + let_it_be(:group_developer_with_custom_role) { create(:user) } + let_it_be(:parent_group_developer_with_custom_role) { create(:user) } + let_it_be(:group_maintainer) { create(:user) } + let_it_be(:user_in_both_project_and_group) { create(:user) } + + let(:levels) { [] } + let(:member_role_ids) { [] } + + before_all do + create(:project_member, :developer, project: project, user: project_developer_with_custom_role, + member_role: custom_role) + create(:project_member, :maintainer, project: project, user: project_maintainer) + create(:project_member, :reporter, project: project, user: project_reporter) + create(:project_member, :developer, project: project, user: user_in_both_project_and_group) + + create(:group_member, :developer, source: group, user: group_developer_with_custom_role, member_role: custom_role) + create(:group_member, :maintainer, source: group, user: group_maintainer) + create(:group_member, :reporter, source: group, user: user_in_both_project_and_group) + + create(:group_member, :developer, source: parent_group, user: parent_group_developer_with_custom_role, + member_role: another_custom_role) end subject(:members_with_access_level_or_custom_roles) do @@ -136,48 +151,181 @@ end context 'when filtering by access level' do - let_it_be(:levels) { [Gitlab::Access::MAINTAINER] } + let(:levels) { [Gitlab::Access::MAINTAINER] } + + it 'returns all maintainers from project and group hierarchy' do + expect(members_with_access_level_or_custom_roles).to contain_exactly(project_maintainer, group_maintainer) + end - it { is_expected.to contain_exactly(maintainer) } + context 'with multiple access levels' do + let(:levels) { [Gitlab::Access::MAINTAINER, Gitlab::Access::REPORTER] } + + it 'returns users with any of the specified access levels' do + expect(members_with_access_level_or_custom_roles).to contain_exactly( + project_maintainer, + group_maintainer, + project_reporter + ) + end + end end context 'when filtering by custom roles' do - let_it_be(:member_role_ids) { [custom_role.id] } + let(:member_role_ids) { [custom_role.id] } - it { is_expected.to contain_exactly(developer) } + it 'returns users with the specified custom role from project and group' do + expect(members_with_access_level_or_custom_roles).to contain_exactly( + project_developer_with_custom_role, + group_developer_with_custom_role + ) + end + + context 'with a different custom role' do + let(:member_role_ids) { [another_custom_role.id] } + + it 'returns users with the different custom role from parent group' do + expect(members_with_access_level_or_custom_roles).to contain_exactly( + parent_group_developer_with_custom_role + ) + end + end + + context 'with multiple custom roles' do + let(:member_role_ids) { [custom_role.id, another_custom_role.id] } + + it 'returns users with any of the specified custom roles' do + expect(members_with_access_level_or_custom_roles).to contain_exactly( + project_developer_with_custom_role, + group_developer_with_custom_role, + parent_group_developer_with_custom_role + ) + end + end end context 'when filtering by both access level and custom roles' do - let_it_be(:levels) { [Gitlab::Access::MAINTAINER] } - let_it_be(:member_role_ids) { [custom_role.id] } + let(:levels) { [Gitlab::Access::MAINTAINER] } + let(:member_role_ids) { [custom_role.id] } + + it 'returns users matching either access level or custom role' do + expect(members_with_access_level_or_custom_roles).to contain_exactly( + project_developer_with_custom_role, + group_developer_with_custom_role, + project_maintainer, + group_maintainer + ) + end + + context 'with multiple access levels and custom roles' do + let(:levels) { [Gitlab::Access::MAINTAINER, Gitlab::Access::REPORTER] } + let(:member_role_ids) { [custom_role.id, another_custom_role.id] } - it { is_expected.to contain_exactly(developer, maintainer) } + it 'returns users matching any of the criteria' do + expect(members_with_access_level_or_custom_roles).to contain_exactly( + project_developer_with_custom_role, + group_developer_with_custom_role, + parent_group_developer_with_custom_role, + project_maintainer, + group_maintainer, + project_reporter + ) + end + end end context 'when filtering with non-existent custom role' do - let_it_be(:member_roles_id) { [non_existing_record_id] } + let(:member_role_ids) { [non_existing_record_id] } it { is_expected.to be_empty } end + + context 'when user is member of both project and group' do + let(:member_role_ids) { [custom_role.id] } + + it 'returns user only once (deduplication)' do + expect(members_with_access_level_or_custom_roles).to contain_exactly( + project_developer_with_custom_role, + group_developer_with_custom_role + ) + end + + context 'when filtering by access level' do + let(:levels) { [Gitlab::Access::DEVELOPER] } + let(:member_role_ids) { [] } + + it 'returns all users with Developer access level' do + expect(members_with_access_level_or_custom_roles).to contain_exactly( + project_developer_with_custom_role, + group_developer_with_custom_role, + parent_group_developer_with_custom_role, + user_in_both_project_and_group + ) + end + end + end + + context 'when project is in a standalone group without parent groups' do + let_it_be(:standalone_group) { create(:group) } + let_it_be(:standalone_project) { create(:project, group: standalone_group) } + let_it_be(:standalone_custom_role) { create(:member_role, namespace: standalone_group) } + let_it_be(:standalone_developer) { create(:user) } + let_it_be(:standalone_maintainer) { create(:user) } + + let(:member_role_ids) { [standalone_custom_role.id] } + + before_all do + create(:project_member, :developer, project: standalone_project, user: standalone_developer, + member_role: standalone_custom_role) + create(:project_member, :maintainer, project: standalone_project, user: standalone_maintainer) + end + + subject(:members_with_access_level_or_custom_roles) do + standalone_project.team.members_with_access_level_or_custom_roles(levels: levels, + member_role_ids: member_role_ids) + end + + it 'returns project members with custom role' do + expect(members_with_access_level_or_custom_roles).to contain_exactly(standalone_developer) + end + + context 'when filtering by access level' do + let(:levels) { [Gitlab::Access::MAINTAINER] } + let(:member_role_ids) { [] } + + it 'returns project members with access level' do + expect(members_with_access_level_or_custom_roles).to contain_exactly(standalone_maintainer) + end + end + end end describe '#user_exists_with_access_level_or_custom_roles?' do - let_it_be(:group) { create(:group) } + let_it_be(:parent_group) { create(:group) } + let_it_be(:group) { create(:group, parent: parent_group) } let_it_be(:project) { create(:project, group: group) } - let_it_be(:custom_role) { create(:member_role) } - let_it_be(:another_custom_role) { create(:member_role) } - - let_it_be(:developer) { create(:user) } - let_it_be(:maintainer) { create(:user) } - let_it_be(:reporter) { create(:user) } - let_it_be(:guest) { create(:user) } + let_it_be(:custom_role) { create(:member_role, namespace: parent_group) } + let_it_be(:another_custom_role) { create(:member_role, namespace: parent_group) } + + let_it_be(:project_developer) { create(:user) } + let_it_be(:project_maintainer) { create(:user) } + let_it_be(:project_reporter) { create(:user) } + let_it_be(:project_guest) { create(:user) } + let_it_be(:group_developer_with_custom_role) { create(:user) } + let_it_be(:parent_group_developer_with_custom_role) { create(:user) } + let_it_be(:group_maintainer) { create(:user) } let_it_be(:non_member) { create(:user) } - before do - create(:project_member, :developer, project: project, user: developer, member_role: custom_role) - create(:project_member, :maintainer, project: project, user: maintainer) - create(:project_member, :reporter, project: project, user: reporter) - create(:project_member, :guest, project: project, user: guest) + before_all do + create(:project_member, :developer, project: project, user: project_developer, member_role: custom_role) + create(:project_member, :maintainer, project: project, user: project_maintainer) + create(:project_member, :reporter, project: project, user: project_reporter) + create(:project_member, :guest, project: project, user: project_guest) + + create(:group_member, :developer, source: group, user: group_developer_with_custom_role, member_role: custom_role) + create(:group_member, :maintainer, source: group, user: group_maintainer) + + create(:group_member, :developer, source: parent_group, user: parent_group_developer_with_custom_role, + member_role: another_custom_role) end subject(:user_exists) do @@ -188,7 +336,7 @@ context 'when no parameters are provided' do let(:levels) { [] } let(:member_role_ids) { [] } - let(:user) { developer } + let(:user) { project_developer } it 'returns false' do expect(user_exists).to be false @@ -199,8 +347,16 @@ let(:levels) { [Gitlab::Access::MAINTAINER] } let(:member_role_ids) { [] } - context 'when user has the specified access level' do - let(:user) { maintainer } + context 'when user has the specified access level as project member' do + let(:user) { project_maintainer } + + it 'returns true' do + expect(user_exists).to be true + end + end + + context 'when user has the specified access level as group member' do + let(:user) { group_maintainer } it 'returns true' do expect(user_exists).to be true @@ -208,7 +364,7 @@ end context 'when user does not have the specified access level' do - let(:user) { developer } + let(:user) { project_developer } it 'returns false' do expect(user_exists).to be false @@ -228,7 +384,7 @@ let(:member_role_ids) { [] } context 'when user has one of the specified access levels' do - let(:user) { maintainer } + let(:user) { project_maintainer } it 'returns true' do expect(user_exists).to be true @@ -236,7 +392,7 @@ end context 'when user has another of the specified access levels' do - let(:user) { reporter } + let(:user) { project_reporter } it 'returns true' do expect(user_exists).to be true @@ -244,7 +400,7 @@ end context 'when user does not have any of the specified access levels' do - let(:user) { guest } + let(:user) { project_guest } it 'returns false' do expect(user_exists).to be false @@ -257,8 +413,16 @@ let(:levels) { [] } let(:member_role_ids) { [custom_role.id] } - context 'when user has the specified custom role' do - let(:user) { developer } + context 'when user has the specified custom role as project member' do + let(:user) { project_developer } + + it 'returns true' do + expect(user_exists).to be true + end + end + + context 'when user has the specified custom role as group member' do + let(:user) { group_developer_with_custom_role } it 'returns true' do expect(user_exists).to be true @@ -266,7 +430,7 @@ end context 'when user does not have the specified custom role' do - let(:user) { maintainer } + let(:user) { project_maintainer } it 'returns false' do expect(user_exists).to be false @@ -281,11 +445,39 @@ end end + context 'when filtering by a different custom role' do + let(:member_role_ids) { [another_custom_role.id] } + + context 'when user has the different custom role' do + let(:user) { parent_group_developer_with_custom_role } + + it 'returns true' do + expect(user_exists).to be true + end + end + + context 'when user does not have the different custom role' do + let(:user) { project_developer } + + it 'returns false' do + expect(user_exists).to be false + end + end + end + context 'when filtering by multiple custom roles' do let(:member_role_ids) { [custom_role.id, another_custom_role.id] } context 'when user has one of the specified custom roles' do - let(:user) { developer } + let(:user) { project_developer } + + it 'returns true' do + expect(user_exists).to be true + end + end + + context 'when user has the other specified custom role' do + let(:user) { parent_group_developer_with_custom_role } it 'returns true' do expect(user_exists).to be true @@ -293,7 +485,7 @@ end context 'when user does not have any of the specified custom roles' do - let(:user) { maintainer } + let(:user) { project_maintainer } it 'returns false' do expect(user_exists).to be false @@ -305,7 +497,7 @@ let(:member_role_ids) { [non_existing_record_id] } context 'when user is a member' do - let(:user) { developer } + let(:user) { project_developer } it 'returns false' do expect(user_exists).to be false @@ -319,7 +511,7 @@ let(:member_role_ids) { [custom_role.id] } context 'when user has the specified access level' do - let(:user) { maintainer } + let(:user) { project_maintainer } it 'returns true' do expect(user_exists).to be true @@ -327,7 +519,15 @@ end context 'when user has the specified custom role' do - let(:user) { developer } + let(:user) { project_developer } + + it 'returns true' do + expect(user_exists).to be true + end + end + + context 'when user has the specified custom role from group hierarchy' do + let(:user) { group_developer_with_custom_role } it 'returns true' do expect(user_exists).to be true @@ -335,7 +535,7 @@ end context 'when user has neither the access level nor the custom role' do - let(:user) { guest } + let(:user) { project_guest } it 'returns false' do expect(user_exists).to be false -- GitLab