[Rt-commit] rt branch, 4.4/lazy-role-groups, created. rt-4.4.4-452-gde27f385cb
? sunnavy
sunnavy at bestpractical.com
Mon May 17 17:20:38 EDT 2021
The branch, 4.4/lazy-role-groups has been created
at de27f385cb1a28a10c6ad9f709e6db592fbeeaae (commit)
- Log -----------------------------------------------------------------
commit d1c2ebfc1796a3850cc2ea33203d9b914f03e75e
Author: sunnavy <sunnavy at bestpractical.com>
Date: Tue Jun 2 03:26:33 2020 +0800
Support to create ticket/asset role groups lazily
For objects with a large number of custom roles, creating all necessary
role groups at object create time can add a large number of additional
create operations in the DB, impacting performance. Creating them later
when needed reduces time to create the object.
Since role groups may not exist, we need to use left join in searches.
diff --git a/lib/RT/Asset.pm b/lib/RT/Asset.pm
index f912f650f7..d20a8efb60 100644
--- a/lib/RT/Asset.pm
+++ b/lib/RT/Asset.pm
@@ -194,6 +194,10 @@ by ID.
Any of these link types accept either a single value or arrayref of values
parseable by L<RT::URI>.
+=item LazyRoleGroups
+
+A boolean to control if to create role groups immediately or just when necessary
+
=back
Returns a tuple of (status, msg) on failure and (id, msg, non-fatal errors) on
@@ -214,6 +218,7 @@ sub Create {
Contact => undef,
Status => undef,
+ LazyRoleGroups => 1,
@_
);
my @non_fatal_errors;
@@ -272,11 +277,13 @@ sub Create {
# Let users who just created an asset see it until the end of this method.
$self->{_object_is_readable} = 1;
- # Create role groups
- unless ($self->_CreateRoleGroups()) {
- RT->Logger->error("Couldn't create role groups for asset ". $self->id);
- RT->DatabaseHandle->Rollback();
- return (0, $self->loc("Couldn't create role groups for asset"));
+ if ( !$args{LazyRoleGroups} ) {
+ # Create role groups
+ unless ($self->_CreateRoleGroups()) {
+ RT->Logger->error("Couldn't create role groups for asset ". $self->id);
+ RT->DatabaseHandle->Rollback();
+ return (0, $self->loc("Couldn't create role groups for asset"));
+ }
}
# Figure out users for roles
diff --git a/lib/RT/Record/Role/Roles.pm b/lib/RT/Record/Role/Roles.pm
index 4d4723a66a..9cf818fd3c 100644
--- a/lib/RT/Record/Role/Roles.pm
+++ b/lib/RT/Record/Role/Roles.pm
@@ -752,7 +752,17 @@ sub _AddRolesOnCreate {
my $changed = 0;
for my $role (keys %{$roles}) {
+ next unless @{$roles->{$role}};
+
my $group = $self->RoleGroup($role);
+ if ( !$group->id ) {
+ $group = $self->_CreateRoleGroup($role);
+ if ( !$group || !$group->id ) {
+ push @errors, $self->loc( "Couldn't create role group '[_1]'", $role );
+ next;
+ }
+ }
+
my @left;
for my $principal (@{$roles->{$role}}) {
if ($acls{$role}->($principal)) {
diff --git a/lib/RT/SearchBuilder/Role/Roles.pm b/lib/RT/SearchBuilder/Role/Roles.pm
index b8fbaf1c31..958c42599b 100644
--- a/lib/RT/SearchBuilder/Role/Roles.pm
+++ b/lib/RT/SearchBuilder/Role/Roles.pm
@@ -374,7 +374,7 @@ sub RoleLimit {
# positive condition case
$group_members ||= $self->_GroupMembersJoin(
- GroupsAlias => $groups, New => 1, Left => 0
+ GroupsAlias => $groups, New => 1,
);
if ($args{FIELD} eq "id") {
my @ids;
@@ -411,6 +411,7 @@ sub RoleLimit {
else {
my $cgm_2 = $self->NewAlias('CachedGroupMembers');
my $group_members_2 = $self->Join(
+ TYPE => 'LEFT',
ALIAS1 => $group_members,
FIELD1 => 'MemberId',
ALIAS2 => $cgm_2,
diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index d1b83e42d8..64c658348c 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -192,6 +192,7 @@ Arguments: ARGS is a hash of named parameters. Valid parameters are:
Due -- an ISO date describing the ticket's due date and time in GMT
MIMEObj -- a MIME::Entity object with the content of the initial ticket request.
CustomField-<n> -- a scalar or array of values for the customfield with the id <n>
+ LazyRoleGroups -- a boolean to control if to create role groups immediately or just when necessary
Ticket links can be set up during create by passing the link type as a hask key and
the ticket id to be linked to as a value (or a URI when linking to other objects).
@@ -239,6 +240,7 @@ sub Create {
SLA => undef,
MIMEObj => undef,
_RecordTransaction => 1,
+ LazyRoleGroups => 1,
@_
);
@@ -439,15 +441,17 @@ sub Create {
}
# Create (empty) role groups
- my $create_groups_ret = $self->_CreateRoleGroups();
- unless ($create_groups_ret) {
- $RT::Logger->crit( "Couldn't create ticket groups for ticket "
- . $self->Id
- . ". aborting Ticket creation." );
- $RT::Handle->Rollback();
- return ( 0, 0,
- $self->loc("Ticket could not be created due to an internal error")
- );
+ if ( !$args{LazyRoleGroups} ) {
+ my $create_groups_ret = $self->_CreateRoleGroups();
+ unless ($create_groups_ret) {
+ $RT::Logger->crit( "Couldn't create ticket groups for ticket "
+ . $self->Id
+ . ". aborting Ticket creation." );
+ $RT::Handle->Rollback();
+ return ( 0, 0,
+ $self->loc("Ticket could not be created due to an internal error")
+ );
+ }
}
# Codify what it takes to add each kind of group
commit 1568c9dd887ca3446b0f835d0e82d8c779bf9e8a
Author: sunnavy <sunnavy at bestpractical.com>
Date: Tue Jun 2 03:32:58 2020 +0800
Default PrincipalId to 0 to avoid SQL error for not-existing role groups
The RoleGroup method returns an empty group object when current user
doesn't have right or the corresponding role group doesn't exist. This
could result in SQL syntax errors when calling methods like MembersObj:
SELECT main.* FROM GroupMembers main WHERE (main.GroupId = ) ORDER BY main.id ASC
Defaulting PrincipalId to 0 could avoid SQL errors like above and return
correct empty results.
diff --git a/lib/RT/Group.pm b/lib/RT/Group.pm
index 0e4cb973dd..706c3efd29 100644
--- a/lib/RT/Group.pm
+++ b/lib/RT/Group.pm
@@ -1320,7 +1320,7 @@ Returns this user's PrincipalId
sub PrincipalId {
my $self = shift;
- return $self->Id;
+ return $self->Id || 0;
}
sub InstanceObj {
commit 529e5dcbc6ab95ae47760419abd13f3da4dd64aa
Author: sunnavy <sunnavy at bestpractical.com>
Date: Tue May 18 02:07:00 2021 +0800
Improve RoleGroup method to create the group if asked
We have quite a few code that calls RoleGroup just to check the members
of the group, in which case we don't want to create the group
implicitly, for efficiency and performance. Thus we add "Create"
parameter to make the creation optional.
diff --git a/lib/RT/Record/Role/Roles.pm b/lib/RT/Record/Role/Roles.pm
index 9cf818fd3c..5011607a0a 100644
--- a/lib/RT/Record/Role/Roles.pm
+++ b/lib/RT/Record/Role/Roles.pm
@@ -325,12 +325,15 @@ sub HasRole {
return scalar grep { $type eq $_ } $self->Roles;
}
-=head2 RoleGroup
+=head2 RoleGroup NAME, CheckRight => RIGHT_NAME, Create => 1|0
Expects a role name as the first parameter which is used to load the
L<RT::Group> for the specified role on this record. Returns an unloaded
L<RT::Group> object on failure.
+If the group is not created yet and C<Create> parameter is true(default is
+false), it will create the group accordingly.
+
=cut
sub RoleGroup {
@@ -349,6 +352,12 @@ sub RoleGroup {
Object => $self,
Name => $name,
);
+
+ if ( !$group->id && $args{Create} ) {
+ if ( my $created = $self->_CreateRoleGroup($name) ) {
+ $group = $created;
+ }
+ }
}
return $group;
}
@@ -490,12 +499,9 @@ sub AddRoleMember {
return (0, $self->loc("Permission denied"))
if $acl and not $acl->($type => $principal);
- my $group = $self->RoleGroup( $type );
+ my $group = $self->RoleGroup( $type, Create => 1 );
if (!$group->id) {
- $group = $self->_CreateRoleGroup($type);
- if (!$group || !$group->id) {
- return (0, $self->loc("Role group '[_1]' not found", $type));
- }
+ return (0, $self->loc("Role group '[_1]' not found", $type));
}
return (0, $self->loc('[_1] is already [_2]',
@@ -754,13 +760,10 @@ sub _AddRolesOnCreate {
for my $role (keys %{$roles}) {
next unless @{$roles->{$role}};
- my $group = $self->RoleGroup($role);
+ my $group = $self->RoleGroup($role, Create => 1);
if ( !$group->id ) {
- $group = $self->_CreateRoleGroup($role);
- if ( !$group || !$group->id ) {
- push @errors, $self->loc( "Couldn't create role group '[_1]'", $role );
- next;
- }
+ push @errors, $self->loc( "Couldn't create role group '[_1]'", $role );
+ next;
}
my @left;
commit 2f3b4f4ee744649f11ab5c99ff43bd6087b3b9de
Author: sunnavy <sunnavy at bestpractical.com>
Date: Tue May 18 03:32:52 2021 +0800
Remove ticket Requestor/AdminCc/Cc validations as they are lazily created now
See also d1c2ebfc17
diff --git a/sbin/rt-validator.in b/sbin/rt-validator.in
index 484be5f156..436531b9ab 100644
--- a/sbin/rt-validator.in
+++ b/sbin/rt-validator.in
@@ -334,7 +334,7 @@ push @CHECKS, 'Queues <-> Role Groups' => sub {
push @CHECKS, 'Tickets <-> Role Groups' => sub {
# from queue to group
my $res = 1;
- for my $role (qw/Requestor Owner Cc AdminCc/ ) {
+ for my $role (qw/Owner/ ) {
$res *= check_integrity(
'Tickets', 'EffectiveId' => 'Groups', 'Instance',
join_condition => 't.Domain = ? AND t.Name = ?',
commit de27f385cb1a28a10c6ad9f709e6db592fbeeaae
Author: sunnavy <sunnavy at bestpractical.com>
Date: Tue Jun 2 03:38:16 2020 +0800
Update tests for the change of lazy role groups
diff --git a/t/api/group.t b/t/api/group.t
index 7ec3a341ef..4d654133df 100644
--- a/t/api/group.t
+++ b/t/api/group.t
@@ -120,7 +120,7 @@ diag "Ticket role group members";
{
RT::Test->load_or_create_queue( Name => 'General' );
my $ticket = RT::Test->create_ticket( Queue => 'General', Subject => 'test ticket role group' );
- my $admincc = $ticket->RoleGroup('AdminCc');
+ my $admincc = $ticket->RoleGroup( 'AdminCc', Create => 1 );
my $delegates = RT::Test->load_or_create_group('delegates');
my $core = RT::Test->load_or_create_group('core team');
my $alice = RT::Test->load_or_create_user( Name => 'alice' );
diff --git a/t/api/rights.t b/t/api/rights.t
index def221dc0a..5e99ec8835 100644
--- a/t/api/rights.t
+++ b/t/api/rights.t
@@ -179,7 +179,7 @@ diag "Ticket role rights for users in groups that are added in ticket roles";
{
RT::Test->load_or_create_queue( Name => 'General' );
my $ticket = RT::Test->create_ticket( Queue => 'General', Subject => 'test ticket role group' );
- my $admincc = $ticket->RoleGroup('AdminCc');
+ my $admincc = $ticket->RoleGroup( 'AdminCc', Create => 1 );
ok( $admincc->PrincipalObj->GrantRight( Right => 'ShowTicket', Object => $ticket->QueueObj ),
'Grant AdminCc ShowTicket right' );
diff --git a/t/api/ticket.t b/t/api/ticket.t
index cfba3e97bf..fbd18b1a16 100644
--- a/t/api/ticket.t
+++ b/t/api/ticket.t
@@ -112,7 +112,8 @@ my ($id, $msg) = $ticket->Create(Subject => "Foo",
Owner => RT->SystemUser->Id,
Status => 'open',
Requestor => ['jesse at example.com'],
- Queue => '1'
+ Queue => '1',
+ LazyRoleGroups => 0,
);
ok ($id, "Ticket $id was created");
ok(my $group = $ticket->RoleGroup('Requestor'));
@@ -343,4 +344,23 @@ diag("Test ticket types with different cases");
is($t->Type, "approval", "Approvals, the third and final internal type, are also lc'd during Create");
}
+diag("Test LazyRoleGroups");
+{
+ my $t = RT::Ticket->new( RT->SystemUser );
+ my ($ok) = $t->Create(
+ Queue => 'General',
+ Subject => 'LazyRoleGroup test',
+ Requestor => 'root at localhost',
+ );
+ ok( $t->RoleGroup($_)->Id, "$_ role group is created" ) for qw/Requestor Owner/;
+
+ for my $role (qw/Cc AdminCc/) {
+ ok( !$t->RoleGroup($role)->Id, "$role role group is not created yet" );
+ ok( $t->AddWatcher( Type => $role, Email => 'root at localhost' ) );
+ my $role_group = $t->RoleGroup($role);
+ ok( $role_group->Id, "$role role group is created" );
+ is( $role_group->MemberEmailAddressesAsString, 'root at localhost', "$role role group has the member root" );
+ }
+}
+
done_testing;
diff --git a/t/api/tickets.t b/t/api/tickets.t
index facaefa7d1..6f903a7438 100644
--- a/t/api/tickets.t
+++ b/t/api/tickets.t
@@ -164,7 +164,7 @@ ok( $unlimittickets->Count > 0, "UnLimited tickets object should return tickets"
diag "Ticket role group members";
{
my $ticket = RT::Test->create_ticket( Queue => 'General', Subject => 'test ticket role group' );
- my $admincc = $ticket->RoleGroup('AdminCc');
+ my $admincc = $ticket->RoleGroup( 'AdminCc', Create => 1 );
my $delegates = RT::Test->load_or_create_group('delegates');
my $core = RT::Test->load_or_create_group('core team');
@@ -212,7 +212,7 @@ diag "Ticket role group members";
my $abc = RT::Test->load_or_create_user( Name => 'abc' ); # so there are multiple users to search
my $abc_ticket = RT::Test->create_ticket( Queue => 'General', Subject => 'test ticket role group' );
- ok( $abc_ticket->RoleGroup('AdminCc')->AddMember( $abc->PrincipalId ), 'Add abc to AdminCc' );
+ ok( $abc_ticket->RoleGroup( 'AdminCc', Create => 1 )->AddMember( $abc->PrincipalId ), 'Add abc to AdminCc' );
my $tickets = RT::Tickets->new( RT->SystemUser );
$tickets->FromSQL("Subject = 'test ticket role group' AND AdminCc.Name LIKE 'a'");
diff --git a/t/assets/roles.t b/t/assets/roles.t
index 1d8a647aa3..b6baa6cd65 100644
--- a/t/assets/roles.t
+++ b/t/assets/roles.t
@@ -4,7 +4,7 @@ use warnings;
use RT::Test::Assets tests => undef;
my $catalog = create_catalog( Name => "A catalog" );
-my $asset = create_asset( Name => "Test asset", Catalog => $catalog->id );
+my $asset = create_asset( Name => "Test asset", Catalog => $catalog->id, LazyRoleGroups => 0 );
ok $asset && $asset->id, "Created asset";
for my $object ($asset, $catalog, RT->System) {
diff --git a/t/validator/group_members.t b/t/validator/group_members.t
index ada0815a16..a455ac5322 100644
--- a/t/validator/group_members.t
+++ b/t/validator/group_members.t
@@ -128,7 +128,7 @@ RT::Test->db_is_valid;
diag "CGM recurisve check for ticket role groups";
{
my $ticket = RT::Test->create_ticket( Queue => 'General', Subject => 'test ticket role group' );
- my $admincc = $ticket->RoleGroup('AdminCc');
+ my $admincc = $ticket->RoleGroup( 'AdminCc', Create => 1 );
my $delegates = RT::Test->load_or_create_group('delegates');
my $core = RT::Test->load_or_create_group('core team');
my $alice = RT::Test->load_or_create_user( Name => 'alice' );
diff --git a/t/validator/role_groups.t b/t/validator/role_groups.t
index a32ddcf138..580f2f4d2f 100644
--- a/t/validator/role_groups.t
+++ b/t/validator/role_groups.t
@@ -25,7 +25,9 @@ like( $res, qr/Tickets references a nonexistent record in Groups/, 'Found/Fixed
RT::Test->db_is_valid;
for my $object ( $ticket, $ticket->QueueObj ) {
- for my $type (qw/Requestor AdminCc Cc Owner/) {
+ my @roles = 'Owner';
+ push @roles, qw/Requestor AdminCc Cc/ if $object->isa('RT::Queue');
+ for my $type (@roles) {
ok( $object->RoleGroup($type)->id, "Recreated group $type for " . ref $object );
}
}
diff --git a/t/web/download_user_info.t b/t/web/download_user_info.t
index 44155d49a7..bf41a899ba 100644
--- a/t/web/download_user_info.t
+++ b/t/web/download_user_info.t
@@ -62,9 +62,9 @@ EOF
my $transaction_info_tsv = <<EOF;
Ticket Id\tid\tCreated\tDescription\tOldValue\tNewValue\tContent
-1\t30\t$date_created\tTicket created\t\t\tThis transaction appears to have no content
-1\t32\t$date_commented\tComments added\t\t\tTest - Comment
-1\t33\t$date_correspondence\tCorrespondence added\t\t\tTest - Reply
+1\t28\t$date_created\tTicket created\t\t\tThis transaction appears to have no content
+1\t30\t$date_commented\tComments added\t\t\tTest - Comment
+1\t31\t$date_correspondence\tCorrespondence added\t\t\tTest - Reply
EOF
is $agent->content, $transaction_info_tsv,
-----------------------------------------------------------------------
More information about the rt-commit
mailing list