[Rt-commit] rt branch, 4.4/lazy-role-groups, created. rt-4.4.4-452-gde4d823b4f
? sunnavy
sunnavy at bestpractical.com
Tue May 18 17:42:07 EDT 2021
The branch, 4.4/lazy-role-groups has been created
at de4d823b4fb45d5faada95c9c1d66aaa25d0f159 (commit)
- Log -----------------------------------------------------------------
commit 250eb2cdc082c00b95b61cc904e5428775ee7c0d
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.
Note that it's not enabled by default, for back compatibility.
diff --git a/lib/RT/Asset.pm b/lib/RT/Asset.pm
index f912f650f7..8e3ba40241 100644
--- a/lib/RT/Asset.pm
+++ b/lib/RT/Asset.pm
@@ -194,6 +194,12 @@ 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. It defaults to C<$RT::Record::Role::Roles::LAZY_ROLE_GROUPS>,
+which is false by default.
+
=back
Returns a tuple of (status, msg) on failure and (id, msg, non-fatal errors) on
@@ -214,6 +220,7 @@ sub Create {
Contact => undef,
Status => undef,
+ LazyRoleGroups => $RT::Record::Role::Roles::LAZY_ROLE_GROUPS,
@_
);
my @non_fatal_errors;
@@ -272,11 +279,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..e759818ddf 100644
--- a/lib/RT/Record/Role/Roles.pm
+++ b/lib/RT/Record/Role/Roles.pm
@@ -53,6 +53,9 @@ package RT::Record::Role::Roles;
use Role::Basic;
use Scalar::Util qw(blessed);
+# Set this to true to lazily create role groups
+our $LAZY_ROLE_GROUPS;
+
=head1 NAME
RT::Record::Role::Roles - Common methods for records which "watchers" or "roles"
@@ -752,7 +755,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..a4f3891aaa 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -192,6 +192,10 @@ 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
+
+LazyRoleGroups defaults to C<$RT::Record::Role::Roles::LAZY_ROLE_GROUPS>,
+which is false by default.
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 +243,7 @@ sub Create {
SLA => undef,
MIMEObj => undef,
_RecordTransaction => 1,
+ LazyRoleGroups => $RT::Record::Role::Roles::LAZY_ROLE_GROUPS,
@_
);
@@ -439,15 +444,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 69b846515cd7963013cf1ff319353c8d1775557c
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 8d6539a7a54ef2b5cf1ef9a05226ca1bef5f0619
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.
This is to simplify role group creation for lazy ones.
diff --git a/lib/RT/Record/Role/Roles.pm b/lib/RT/Record/Role/Roles.pm
index e759818ddf..37025cf862 100644
--- a/lib/RT/Record/Role/Roles.pm
+++ b/lib/RT/Record/Role/Roles.pm
@@ -328,12 +328,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 {
@@ -352,6 +355,12 @@ sub RoleGroup {
Object => $self,
Name => $name,
);
+
+ if ( !$group->id && $args{Create} ) {
+ if ( my $created = $self->_CreateRoleGroup($name) ) {
+ $group = $created;
+ }
+ }
}
return $group;
}
@@ -493,12 +502,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]',
@@ -757,13 +763,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 b720ec9b3c4cd864dbe1c1c7cfac374b452f42bf
Author: sunnavy <sunnavy at bestpractical.com>
Date: Tue May 18 03:32:52 2021 +0800
Optionally validate Requestor/AdminCc/Cc as they could be lazily created
See also 250eb2cdc0
diff --git a/sbin/rt-validator.in b/sbin/rt-validator.in
index 484be5f156..53f4dffb7c 100644
--- a/sbin/rt-validator.in
+++ b/sbin/rt-validator.in
@@ -334,7 +334,8 @@ 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/ ) {
+ no warnings 'once';
+ for my $role ( 'Owner', $RT::Record::Role::Roles::LAZY_ROLE_GROUPS ? () : (qw/Requestor Owner Cc AdminCc/) ) {
$res *= check_integrity(
'Tickets', 'EffectiveId' => 'Groups', 'Instance',
join_condition => 't.Domain = ? AND t.Name = ?',
commit de4d823b4fb45d5faada95c9c1d66aaa25d0f159
Author: sunnavy <sunnavy at bestpractical.com>
Date: Tue Jun 2 03:38:16 2020 +0800
Test lazy role groups
diff --git a/t/api/ticket.t b/t/api/ticket.t
index cfba3e97bf..4ce1f9a484 100644
--- a/t/api/ticket.t
+++ b/t/api/ticket.t
@@ -343,4 +343,24 @@ 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");
+{
+ local $RT::Record::Role::Roles::LAZY_ROLE_GROUPS = 1;
+ 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;
-----------------------------------------------------------------------
More information about the rt-commit
mailing list