[Rt-commit] rt branch, 4.4/tweak-groups-in-ticket-roles, created. rt-4.4.4-9-g987966888
? sunnavy
sunnavy at bestpractical.com
Fri Mar 8 15:10:56 EST 2019
The branch, 4.4/tweak-groups-in-ticket-roles has been created
at 987966888e536c38205a85b003a8ea5e9c969d75 (commit)
- Log -----------------------------------------------------------------
commit 9444ce1427bc9a110838a70d84682e93ef529d85
Author: sunnavy <sunnavy at bestpractical.com>
Date: Sat Mar 9 02:09:40 2019 +0800
Don't recursively add members to ticket role groups in CachedGroupMembers
CachedGroupMembers is used to quickly check if a user/group is a member
of another group, recursively. The table contains flat relationships
between groups and their members(including recurisve members), e.g.
- top
- sub
- user
In CachedGroupMembers, the rows are like(simplified and also excluded
self to self rows):
Group Member
top sub
sub user
top user
You can see there is a direct relationship between "top" to "user" in
CachedGroupMembers.
The performance becomes quite bad when processing updates of groups that
are in roles of a huge list of tickets. e.g.
- AdminCc(for ticket 1)
- group1
- AdminCc(for ticket 2)
- group1
- AdminCc(for ticket 3)
- group1
...
If we add a new member to "group1", we need to create "AdminCc <-> the
new member" rows in CachedGroupMembers for all the tickets above, which
could be quite time consuming: as the complexity is O(n), it will be
linearly slower as the ticket list grows.
To get around it, let's not create those rows in CachedGroupMembers and
tweak logic in RT instead.
diff --git a/lib/RT/CachedGroupMember.pm b/lib/RT/CachedGroupMember.pm
index c7c130937..f148e2bc1 100644
--- a/lib/RT/CachedGroupMember.pm
+++ b/lib/RT/CachedGroupMember.pm
@@ -156,7 +156,7 @@ sub Create {
return $id if $args{'Member'}->id == $args{'Group'}->id;
- if ( $args{'Member'}->IsGroup() ) {
+ if ( $args{'Group'}->Object->Domain ne 'RT::Ticket-Role' && $args{'Member'}->IsGroup() ) {
my $GroupMembers = $args{'Member'}->Object->MembersObj();
while ( my $member = $GroupMembers->Next() ) {
my $cached_member =
diff --git a/lib/RT/GroupMember.pm b/lib/RT/GroupMember.pm
index d7331aea5..b59acd625 100644
--- a/lib/RT/GroupMember.pm
+++ b/lib/RT/GroupMember.pm
@@ -123,6 +123,20 @@ sub _InsertCGM {
QUOTEVALUE => 0,
ENTRYAGGREGATOR => 'AND',
);
+ my $groups = $cgm->Join(
+ ALIAS1 => 'main',
+ FIELD1 => 'GroupId',
+ TABLE2 => 'Groups',
+ FIELD2 => 'id',
+ );
+ $cgm->Limit(
+ ALIAS => $groups,
+ FIELD => 'Domain',
+ OPERATOR => '!=',
+ VALUE => 'RT::Ticket-Role',
+ ENTRYAGGREGATOR => 'AND',
+ CASESENSITIVE => 0,
+ );
while ( my $parent_member = $cgm->Next ) {
my $parent_id = $parent_member->MemberId;
commit 89c2d94fee4b027174f6d975d96097915059544d
Author: sunnavy <sunnavy at bestpractical.com>
Date: Sat Mar 9 02:48:56 2019 +0800
Check direct group members in recursive member methods for ticket role groups
As we don't maintain a full flat relationship in CachedGroupMembers for
ticket role groups(in 9444ce14), we need to check all their direct group
members too, which have full flat relationships themselves.
Note Domain is set for all the rows, the expression of "$self->Domain //
''" is to eliminate uninitialized warnings for cases where $self hasn't
loaded a row yet(it happens in tests).
diff --git a/lib/RT/Group.pm b/lib/RT/Group.pm
index 19a41ef7a..bb0049708 100644
--- a/lib/RT/Group.pm
+++ b/lib/RT/Group.pm
@@ -738,6 +738,12 @@ sub DeepMembersObj {
#If we don't have rights, don't include any results
# TODO XXX WHY IS THERE NO ACL CHECK HERE?
$members_obj->LimitToMembersOfGroup( $self->PrincipalId );
+ if ( ( $self->Domain // '' ) eq 'RT::Ticket-Role' ) {
+ my $groups = $self->GroupMembersObj( Recursively => 0 );
+ while ( my $group = $groups->Next ) {
+ $members_obj->LimitToMembersOfGroup( $group->PrincipalId );
+ }
+ }
return ( $members_obj );
@@ -790,9 +796,16 @@ sub GroupMembersObj {
ALIAS2 => $groups->PrincipalsAlias, FIELD2 => 'id',
);
$groups->Limit(
- ALIAS => $members_alias,
- FIELD => 'GroupId',
- VALUE => $self->PrincipalId,
+ ALIAS => $members_alias,
+ FIELD => 'GroupId',
+ $args{Recursively} && ( $self->Domain // '' ) eq 'RT::Ticket-Role'
+ ? ( OPERATOR => 'IN',
+ VALUE => [
+ $self->PrincipalId,
+ map { $_->PrincipalId } @{ $self->GroupMembersObj( Recursively => 0 )->ItemsArrayRef }
+ ]
+ )
+ : ( VALUE => $self->PrincipalId, )
);
$groups->Limit(
ALIAS => $members_alias,
@@ -832,7 +845,14 @@ sub UserMembersObj {
$users->Limit(
ALIAS => $members_alias,
FIELD => 'GroupId',
- VALUE => $self->PrincipalId,
+ $args{Recursively} && ( $self->Domain // '' ) eq 'RT::Ticket-Role'
+ ? ( OPERATOR => 'IN',
+ VALUE => [
+ $self->PrincipalId,
+ map { $_->PrincipalId } @{ $self->GroupMembersObj( Recursively => 0 )->ItemsArrayRef }
+ ]
+ )
+ : ( VALUE => $self->PrincipalId, )
);
$users->Limit(
ALIAS => $members_alias,
@@ -1109,9 +1129,14 @@ sub HasMemberRecursively {
if ( my $member_id = $member_obj->id ) {
return $member_id;
}
- else {
- return (undef);
+ elsif ( ( $self->Domain // '' ) eq 'RT::Ticket-Role' ) {
+ my $groups = $self->GroupMembersObj( Recursively => 0 );
+ while ( my $group = $groups->Next ) {
+ my $ret = $group->HasMemberRecursively($principal);
+ return $ret if $ret;
+ }
}
+ return (undef);
}
commit f26dafbd0cb5bec312b4d5e86badf6f51d1562c2
Author: sunnavy <sunnavy at bestpractical.com>
Date: Sat Mar 9 03:01:37 2019 +0800
Check direct group members in rights check for ticket role groups
It covers both ticket search and rights check on a single ticket.
See also 9444ce14 and 89c2d94f
diff --git a/lib/RT/Principal.pm b/lib/RT/Principal.pm
index aec96f7a2..f5c395588 100644
--- a/lib/RT/Principal.pm
+++ b/lib/RT/Principal.pm
@@ -574,6 +574,10 @@ sub _HasRoleRightQuery {
@_
);
+ my $groups = RT::Groups->new( RT->SystemUser );
+ $groups->LimitToUserDefinedGroups;
+ $groups->WithMember( PrincipalId => $self->id, Recursively => 1 );
+
my $query =
" FROM Groups, Principals, CachedGroupMembers WHERE "
@@ -589,7 +593,7 @@ sub _HasRoleRightQuery {
# also, check to see if the right is being granted _directly_ to this principal,
# as is the case when we want to look up group rights
. "AND Principals.id = CachedGroupMembers.GroupId "
- . "AND CachedGroupMembers.MemberId = " . $self->Id . " "
+ . "AND CachedGroupMembers.MemberId IN (" . ( join ',', $self->Id, map { $_->id } @{ $groups->ItemsArrayRef } ) . ") "
;
if ( $args{'Roles'} ) {
diff --git a/lib/RT/Tickets.pm b/lib/RT/Tickets.pm
index 5dcd50f07..3cccf2048 100644
--- a/lib/RT/Tickets.pm
+++ b/lib/RT/Tickets.pm
@@ -2693,11 +2693,16 @@ sub CurrentUserCanSee {
if ( $join_roles ) {
$role_group_alias = $self->_RoleGroupsJoin( New => 1 );
$cgm_alias = $self->_GroupMembersJoin( GroupsAlias => $role_group_alias );
+
+ my $groups = RT::Groups->new( RT->SystemUser );
+ $groups->LimitToUserDefinedGroups;
+ $groups->WithMember( PrincipalId => $id, Recursively => 1 );
+
$self->Limit(
LEFTJOIN => $cgm_alias,
FIELD => 'MemberId',
- OPERATOR => '=',
- VALUE => $id,
+ OPERATOR => 'IN',
+ VALUE => [ $id, map { $_->id } @{ $groups->ItemsArrayRef } ],
);
}
my $limit_queues = sub {
commit 987966888e536c38205a85b003a8ea5e9c969d75
Author: sunnavy <sunnavy at bestpractical.com>
Date: Sat Mar 9 03:11:50 2019 +0800
Exclude ticket role groups for recursive validity check in CachedGroupMembers
See also 9444ce14
diff --git a/sbin/rt-validator.in b/sbin/rt-validator.in
index 33767827f..51479bd21 100644
--- a/sbin/rt-validator.in
+++ b/sbin/rt-validator.in
@@ -573,9 +573,13 @@ FROM
AND cgm3.MemberId = gm2.MemberId
AND cgm3.Via = cgm1.id
AND cgm3.ImmediateParentId = cgm1.MemberId )
+ LEFT JOIN Groups g ON (
+ cgm1.GroupId = g.id
+ )
WHERE cgm1.GroupId != cgm1.MemberId
AND gm2.GroupId = cgm1.MemberId
AND cgm3.id IS NULL
+AND g.Domain != 'RT::Ticket-Role'
END
my $action = sub {
-----------------------------------------------------------------------
More information about the rt-commit
mailing list