[Rt-commit] rt branch, 4.4-trunk, updated. rt-4.4.4-520-g46afb50b29
? sunnavy
sunnavy at bestpractical.com
Mon Jun 7 09:24:04 EDT 2021
The branch, 4.4-trunk has been updated
via 46afb50b290d80892d3e619dabdfc4da2a7ca768 (commit)
via 0fc5300b727fe515e3f63f8caa1a08c134d66357 (commit)
via 72579cd5475c7c0f34acded7902f554684735abf (commit)
via f720701ee3aba5a9377b11fee2848524e4a1a3c4 (commit)
via 542dfc95ff81c1a34c25e8a405cd2d7ed21d7a65 (commit)
via 1792f97b5e623607d63fc34f6f82d405fbe6cc30 (commit)
via 5ab6360868212697ee02ccb38191abc8939a2389 (commit)
from 14af0211a5d60a66c6b6962a4c921d3e99ce26b2 (commit)
Summary of changes:
lib/RT/Asset.pm | 19 +++++++++----
lib/RT/Group.pm | 2 +-
lib/RT/Record/Role/Roles.pm | 58 ++++++++++++++++++++++++++++++++------
lib/RT/SearchBuilder/Role/Roles.pm | 3 +-
lib/RT/Ticket.pm | 25 ++++++++++------
sbin/rt-validator.in | 3 +-
t/api/ticket.t | 20 +++++++++++++
7 files changed, 105 insertions(+), 25 deletions(-)
- Log -----------------------------------------------------------------
commit 5ab6360868212697ee02ccb38191abc8939a2389
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 e913a9a0e4..822fafcb03 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 = 0;
+
=head1 NAME
RT::Record::Role::Roles - Common methods for records which "watchers" or "roles"
@@ -761,7 +764,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)) {
@@ -804,5 +817,31 @@ sub LabelForRole {
return $role->{Name};
}
+=head1 OPTIONS
+
+=head2 Lazy Role Groups
+
+Role groups are typically created for all roles on a ticket or asset when
+that object is created. If you are creating a large number of tickets or
+assets automatically (e.g., with an automated import process) and you use
+custom roles in addition to core roles, this requires many additional rows
+to be created for each base ticket or asset. This adds time to the create
+process for each ticket or asset.
+
+Roles support a lazy option that will defer creating the underlying role
+groups until the object is accessed later. This speeds up the initial
+create process with minimal impact if tickets or assets are accessed
+individually later (like a user loading a ticket and working on it).
+
+This lazy behavior is off by default for backward compatibility. To
+enable it, set this package variable:
+
+ $RT::Record::Role::Roles::LAZY_ROLE_GROUPS = 1;
+
+If you are evaluating this option for performance, it's worthwhile to
+benchmark your ticket or asset create process before and after to confirm
+you see faster create times.
+
+=cut
1;
diff --git a/lib/RT/SearchBuilder/Role/Roles.pm b/lib/RT/SearchBuilder/Role/Roles.pm
index 98fb469f3c..6050d648d4 100644
--- a/lib/RT/SearchBuilder/Role/Roles.pm
+++ b/lib/RT/SearchBuilder/Role/Roles.pm
@@ -393,7 +393,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;
@@ -430,6 +430,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 2cd940cea0..3d131aa9d7 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 1792f97b5e623607d63fc34f6f82d405fbe6cc30
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 542dfc95ff81c1a34c25e8a405cd2d7ed21d7a65
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 822fafcb03..0950a96620 100644
--- a/lib/RT/Record/Role/Roles.pm
+++ b/lib/RT/Record/Role/Roles.pm
@@ -337,12 +337,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 {
@@ -361,6 +364,12 @@ sub RoleGroup {
Object => $self,
Name => $name,
);
+
+ if ( !$group->id && $args{Create} ) {
+ if ( my $created = $self->_CreateRoleGroup($name) ) {
+ $group = $created;
+ }
+ }
}
return $group;
}
@@ -502,12 +511,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]',
@@ -766,13 +772,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 f720701ee3aba5a9377b11fee2848524e4a1a3c4
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 72579cd5475c7c0f34acded7902f554684735abf
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;
commit 0fc5300b727fe515e3f63f8caa1a08c134d66357
Author: Jim Brandt <jbrandt at bestpractical.com>
Date: Fri May 28 09:21:24 2021 -0400
Convert pod link to private method to code format
diff --git a/lib/RT/Record/Role/Roles.pm b/lib/RT/Record/Role/Roles.pm
index 0950a96620..ef00dfbb7b 100644
--- a/lib/RT/Record/Role/Roles.pm
+++ b/lib/RT/Record/Role/Roles.pm
@@ -140,7 +140,7 @@ as such is not managed by the core codebase or an extension.
=item CreateGroupPredicate
Optional. A subroutine whose return value indicates whether the group for this
-role should be created as part of L</_CreateRoleGroups>. When this subroutine
+role should be created as part of C<_CreateRoleGroups>. When this subroutine
is not provided, the group will be created. The same parameters that will be
passed to L<RT::Group/CreateRoleGroup> are passed to your predicate (including
C<Object>)
commit 46afb50b290d80892d3e619dabdfc4da2a7ca768
Merge: 14af0211a5 0fc5300b72
Author: sunnavy <sunnavy at bestpractical.com>
Date: Mon Jun 7 21:23:18 2021 +0800
Merge branch '4.4/lazy-role-groups' into 4.4-trunk
-----------------------------------------------------------------------
More information about the rt-commit
mailing list