Mailing List Archive

This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "rt".

The branch, 5.0/use-bind-values has been created
at 9583be3c6799374f9bf431ff5a5d46d8a0884644 (commit)

- Log -----------------------------------------------------------------
commit 9583be3c6799374f9bf431ff5a5d46d8a0884644
Author: sunnavy <sunnavy@bestpractical.com>
Date: Thu Sep 23 03:46:30 2021 +0800

Bump DBIx::SearchBuilder to 1.70 to use bind parameters for searches

diff --git a/etc/cpanfile b/etc/cpanfile
index a3c8324fe9..4c42edac8c 100644
--- a/etc/cpanfile
+++ b/etc/cpanfile
@@ -22,7 +22,7 @@ requires 'DateTime', '>= 0.44';
requires 'DateTime::Format::Natural', '>= 0.67';
requires 'DateTime::Locale', '>= 0.40, != 1.00, != 1.01';
requires 'DBI', '>= 1.37';
-requires 'DBIx::SearchBuilder', '>= 1.68';
+requires 'DBIx::SearchBuilder', '>= 1.70';
requires 'Devel::GlobalDestruction';
requires 'Devel::StackTrace', '>= 1.19, != 1.28, != 1.29';
requires 'Digest::base';

commit db40026431414ea82dbc385bcb04f64f39c2093a
Author: sunnavy <sunnavy@bestpractical.com>
Date: Wed Sep 8 17:35:47 2021 +0800

Give Pg a hint about the data type of the argument

As the string will be converted to a placeholder(?), Pg won't know its
type and thus complain about it, the error is like:

could not determine data type of parameter $2

diff --git a/lib/RT/Assets.pm b/lib/RT/Assets.pm
index 8cd4d4c2f8..dad7d054ca 100644
--- a/lib/RT/Assets.pm
+++ b/lib/RT/Assets.pm
@@ -983,6 +983,9 @@ sub _LinkLimit {
if ( RT->Config->Get('DatabaseType') eq 'SQLite' ) {
$join_expression = qq{'$local_prefix' || main.id};
}
+ elsif ( RT->Config->Get('DatabaseType') eq 'Pg' ) {
+ $join_expression = qq{CONCAT( '$local_prefix'::text, main.id )};;
+ }
else {
$join_expression = qq{CONCAT( '$local_prefix', main.id )};;
}

commit 96875e34fcef40359bf7384baaab4e5cd6a19f47
Author: sunnavy <sunnavy@bestpractical.com>
Date: Wed Sep 1 05:23:32 2021 +0800

Refactor bare select queries to use bind values

diff --git a/lib/RT/Principal.pm b/lib/RT/Principal.pm
index a49f63aa0e..953867ad70 100644
--- a/lib/RT/Principal.pm
+++ b/lib/RT/Principal.pm
@@ -418,12 +418,10 @@ sub HasRights {

my %res = ();
{
- my $query
- = "SELECT DISTINCT ACL.RightName "
- . $self->_HasGroupRightQuery(
- EquivObjects => $args{'EquivObjects'}
- );
- my $rights = $RT::Handle->dbh->selectcol_arrayref($query);
+ my ( $query, @bind_values ) = $self->_HasGroupRightQuery( EquivObjects => $args{'EquivObjects'} );
+ $query = "SELECT DISTINCT ACL.RightName $query";
+
+ my $rights = $RT::Handle->dbh->selectcol_arrayref($query, undef, @bind_values);
unless ($rights) {
$RT::Logger->warning( $RT::Handle->dbh->errstr );
return ();
@@ -432,12 +430,9 @@ sub HasRights {
}
my $roles;
{
- my $query
- = "SELECT DISTINCT Groups.Name "
- . $self->_HasRoleRightQuery(
- EquivObjects => $args{'EquivObjects'}
- );
- $roles = $RT::Handle->dbh->selectcol_arrayref($query);
+ my ( $query, @bind_values ) = $self->_HasRoleRightQuery( EquivObjects => $args{'EquivObjects'} );
+ $query = "SELECT DISTINCT Groups.Name $query";
+ $roles = $RT::Handle->dbh->selectcol_arrayref($query, undef, @bind_values);
unless ($roles) {
$RT::Logger->warning( $RT::Handle->dbh->errstr );
return ();
@@ -468,14 +463,11 @@ sub HasRights {

if ( @enabled_roles ) {

- my $query
- = "SELECT DISTINCT ACL.RightName "
- . $self->_RolesWithRightQuery(
- EquivObjects => $args{'EquivObjects'}
- )
- . ' AND ('. join( ' OR ', map "PrincipalType = '$_'", @enabled_roles ) .')'
- ;
- my $rights = $RT::Handle->dbh->selectcol_arrayref($query);
+ my ( $query, @bind_values ) = $self->_RolesWithRightQuery( EquivObjects => $args{'EquivObjects'} );
+ $query = "SELECT DISTINCT ACL.RightName $query";
+ $query .= ' AND ('. join( ' OR ', ("PrincipalType = ?") x @enabled_roles ) .')';
+ push @bind_values, @enabled_roles;
+ my $rights = $RT::Handle->dbh->selectcol_arrayref($query, undef, @bind_values);
unless ($rights) {
$RT::Logger->warning( $RT::Handle->dbh->errstr );
return ();
@@ -520,12 +512,11 @@ sub _HasGroupRight {
@_
);

- my $query
- = "SELECT ACL.id, ACL.ObjectType, ACL.ObjectId "
- . $self->_HasGroupRightQuery( %args );
+ my ( $query, @bind_values ) = $self->_HasGroupRightQuery(%args);
+ $query = "SELECT ACL.id, ACL.ObjectType, ACL.ObjectId $query";

$self->_Handle->ApplyLimits( \$query, 1 );
- my ( $hit, $obj, $id ) = $self->_Handle->FetchResult($query);
+ my ( $hit, $obj, $id ) = $self->_Handle->FetchResult($query, @bind_values);
return (0) unless $hit;

$obj .= "-$id" if $id;
@@ -540,30 +531,33 @@ sub _HasGroupRightQuery {
@_
);

+ my @bind_values;
my $query
= "FROM ACL, Principals, CachedGroupMembers WHERE "

# Never find disabled groups.
. "Principals.id = ACL.PrincipalId "
- . "AND Principals.PrincipalType = 'Group' "
- . "AND Principals.Disabled = 0 "
-
+ . "AND Principals.PrincipalType = ? " # 'Group'
+ . "AND Principals.Disabled = ? " # 0
# See if the principal is a member of the group recursively or _is the rightholder_
# never find recursively disabled group members
# 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 CachedGroupMembers.GroupId = ACL.PrincipalId "
. "AND CachedGroupMembers.GroupId = Principals.id "
- . "AND CachedGroupMembers.MemberId = ". $self->Id . " "
- . "AND CachedGroupMembers.Disabled = 0 ";
+ . "AND CachedGroupMembers.MemberId = ? " # $self->Id
+ . "AND CachedGroupMembers.Disabled = ? "; # 0
+ push @bind_values, 'Group', 0, $self->Id, 0;

my @clauses;
foreach my $obj ( @{ $args{'EquivObjects'} } ) {
my $type = ref($obj) || $obj;
- my $clause = "ACL.ObjectType = '$type'";
+ my $clause = "ACL.ObjectType = ?"; # $type
+ push @bind_values, $type;

if ( defined eval { $obj->id } ) { # it might be 0
- $clause .= " AND ACL.ObjectId = " . $obj->id;
+ $clause .= " AND ACL.ObjectId = ?"; # $obj->id;
+ push @bind_values, $obj->id;
}

push @clauses, "($clause)";
@@ -574,13 +568,15 @@ sub _HasGroupRightQuery {
if ( my $right = $args{'Right'} ) {
# Only find superuser or rights with the name $right
if ( $right eq 'SuperUser' ) {
- $query .= " AND ACL.RightName = 'SuperUser' "
+ $query .= " AND ACL.RightName = ? "; # 'SuperUser'
+ push @bind_values, 'SuperUser';
}
else {
- $query .= " AND ACL.RightName IN ('SuperUser', '$right')";
+ $query .= " AND ACL.RightName IN (?, ?)"; # 'SuperUser', $right
+ push @bind_values, 'SuperUser', $right;
}
}
- return $query;
+ return ( $query, @bind_values );
}

sub _HasRoleRight {
@@ -593,11 +589,11 @@ sub _HasRoleRight {
my @roles = $self->RolesWithRight(%args);
return 0 unless @roles;

- my $query = "SELECT Groups.id "
- . $self->_HasRoleRightQuery( %args, Roles => \@roles );
+ my ( $query, @bind_values ) = $self->_HasRoleRightQuery( %args, Roles => \@roles );
+ $query = "SELECT Groups.id $query";

$self->_Handle->ApplyLimits( \$query, 1 );
- my ($hit) = $self->_Handle->FetchResult($query);
+ my ($hit) = $self->_Handle->FetchResult($query, @bind_values);
return (1) if $hit;

return 0;
@@ -619,30 +615,44 @@ sub _HasRoleRightQuery {
" FROM Groups, Principals, CachedGroupMembers WHERE "

# Never find disabled things
- . "Principals.Disabled = 0 " . "AND CachedGroupMembers.Disabled = 0 "
+ . "Principals.Disabled = ? " . "AND CachedGroupMembers.Disabled = ? " # 0, 0

# We always grant rights to Groups
. "AND Principals.id = Groups.id "
- . "AND Principals.PrincipalType = 'Group' "
+ . "AND Principals.PrincipalType = ? " # 'Group'

# See if the principal is a member of the group recursively or _is the rightholder_
# never find recursively disabled group members
# 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 IN (" . ( join ',', $self->Id, map { $_->id } @{ $groups->ItemsArrayRef } ) . ") "
+ . "AND CachedGroupMembers.MemberId IN (" . ( join ',', ('?') x (1 + @{ $groups->ItemsArrayRef } ) ) . ") "
;
+ my @bind_values = ( 0, 0, 'Group', $self->Id, map { $_->id } @{ $groups->ItemsArrayRef } );

if ( $args{'Roles'} ) {
- $query .= "AND (" . join( ' OR ',
- map $RT::Handle->__MakeClauseCaseInsensitive('Groups.Name', '=', "'$_'"),
- @{ $args{'Roles'} }
- ) . ")";
+ $query .= "AND ("
+ . join( ' OR ',
+ ( $RT::Handle->__MakeClauseCaseInsensitive( 'Groups.Name', '=', '?' ) )
+ x @{ $args{'Roles'} } )
+ . ")";
+ push @bind_values, map { lc $_ } @{ $args{'Roles'} };
}

- my @object_clauses = RT::Users->_RoleClauses( Groups => @{ $args{'EquivObjects'} } );
+ my @object_clauses;
+ foreach my $obj ( @{ $args{'EquivObjects'} } ) {
+ my $type = ref($obj) ? ref($obj) : $obj;
+
+ my $role_clause = $RT::Handle->__MakeClauseCaseInsensitive( "Groups.Domain", '=', '?' );
+ push @bind_values, lc "$type-Role";
+ if ( my $id = eval { $obj->id } ) {
+ $role_clause .= " AND Groups.Instance = ?";
+ push @bind_values, $id;
+ }
+ push @object_clauses, "($role_clause)";
+ }
$query .= " AND (" . join( ' OR ', @object_clauses ) . ")";
- return $query;
+ return ( $query, @bind_values );
}

=head2 RolesWithRight
@@ -672,10 +682,10 @@ sub RolesWithRight {
return () if $args{'Right'} eq 'ExecuteCode'
and RT->Config->Get('DisallowExecuteCode');

- my $query = "SELECT DISTINCT PrincipalType "
- . $self->_RolesWithRightQuery( %args );
+ my ($query, @bind_values) = $self->_RolesWithRightQuery( %args );
+ $query = "SELECT DISTINCT PrincipalType $query";

- my $roles = $RT::Handle->dbh->selectcol_arrayref($query);
+ my $roles = $RT::Handle->dbh->selectcol_arrayref($query, undef, @bind_values);
unless ($roles) {
$RT::Logger->warning( $RT::Handle->dbh->errstr );
return ();
@@ -715,32 +725,39 @@ sub _RolesWithRightQuery {
@_
);

+ my @bind_values;
my $query = " FROM ACL WHERE"

# we need only roles
- . " PrincipalType != 'Group'";
+ . " PrincipalType != ?"; # 'Group'
+ push @bind_values, 'Group';

if ( my $right = $args{'Right'} ) {
if ( $args{'IncludeSuperusers'} && $right ne 'SuperUser' ) {
- $query .= " AND RightName IN ('SuperUser', '$right')";
+ $query .= " AND RightName IN (?, ?)"; # 'SuperUser', $right
+ push @bind_values, 'SuperUser', $right;
}
else {
- $query .= " AND RightName = '$right'";
+ $query .= " AND RightName = ?"; # $right
+ push @bind_values, $right;
}
}

# skip rights granted on system level if we were asked to
unless ( $args{'IncludeSystemRights'} ) {
- $query .= " AND ObjectType != 'RT::System'";
+ $query .= " AND ObjectType != ?"; # 'RT::System'
+ push @bind_values, 'RT::System';
}

my (@object_clauses);
foreach my $obj ( @{ $args{'EquivObjects'} } ) {
my $type = ref($obj) ? ref($obj) : $obj;

- my $object_clause = "ObjectType = '$type'";
+ my $object_clause = "ObjectType = ?"; # $type
+ push @bind_values, $type;
if ( my $id = eval { $obj->id } ) {
- $object_clause .= " AND ObjectId = $id";
+ $object_clause .= " AND ObjectId = ?"; # $id
+ push @bind_values, $id;
}
push @object_clauses, "($object_clause)";
}
@@ -749,7 +766,7 @@ sub _RolesWithRightQuery {
$query .= " AND (" . join( ' OR ', @object_clauses ) . ")"
if @object_clauses;

- return $query;
+ return ( $query, @bind_values );
}



commit ef84c1bad3a01edfdbaaf43050c9276634e185d5
Author: sunnavy <sunnavy@bestpractical.com>
Date: Tue Aug 31 05:58:20 2021 +0800

Update tests to force BuildSelectQuery to not use bind values

Bind values are enabled in RT by default now.

diff --git a/t/api/sql.t b/t/api/sql.t
index 7241e6fb3c..7a39cd65f9 100644
--- a/t/api/sql.t
+++ b/t/api/sql.t
@@ -7,7 +7,7 @@ use RT::Test tests => undef;
my $users = RT::Users->new( RT->SystemUser );
$users->WhoHaveGroupRight( Right => 'OwnTicket', Object => RT->System, IncludeSuperusers => 1 );
like(
- $users->BuildSelectQuery,
+ $users->BuildSelectQuery(PreferBind => 0),
qr{RightName IN \('SuperUser', 'OwnTicket'\)},
'RightName check in WhoHaveGroupRight uses IN'
);
@@ -31,7 +31,7 @@ my %ticketsql = (
my $tickets = RT::Tickets->new( RT->SystemUser );
for my $query ( sort keys %ticketsql ) {
$tickets->FromSQL($query);
- like( $tickets->BuildSelectQuery, $ticketsql{$query}, qq{TicketSQL "$query" uses IN} );
+ like( $tickets->BuildSelectQuery(PreferBind => 0), $ticketsql{$query}, qq{TicketSQL "$query" uses IN} );
}

done_testing;
diff --git a/t/ticket/priority.t b/t/ticket/priority.t
index 6a40a5b7cb..327a968e2b 100644
--- a/t/ticket/priority.t
+++ b/t/ticket/priority.t
@@ -91,11 +91,11 @@ diag "Ticket/Transaction search";
for my $field (qw/Priority InitialPriority FinalPriority/) {
my $tickets = RT::Tickets->new( RT->SystemUser );
$tickets->FromSQL("Queue = 'General' AND $field = 'Low'");
- like( $tickets->BuildSelectQuery, qr/$field = '20'/, "$field is translated properly" );
+ like( $tickets->BuildSelectQuery(PreferBind => 0), qr/$field = '20'/, "$field is translated properly" );

my $txns = RT::Transactions->new( RT->SystemUser );
$txns->FromSQL("TicketQueue = 'General' AND Ticket$field = 'Low'");
- like( $txns->BuildSelectQuery, qr/$field = '20'/, "Ticket$field is translated properly" );
+ like( $txns->BuildSelectQuery(PreferBind => 0), qr/$field = '20'/, "Ticket$field is translated properly" );
}

my $tickets = RT::Tickets->new( RT->SystemUser );

commit f36902d607776faa4fff78100729dde4f3fd6b8d
Author: sunnavy <sunnavy@bestpractical.com>
Date: Thu Sep 9 01:45:46 2021 +0800

Use bind variables in DBIx::SearchBuilder by default

This should improve performance, specifically for Oracle, which very
strongly recommends using bind parameters for all queries.

diff --git a/etc/RT_Config.pm.in b/etc/RT_Config.pm.in
index 6576ec8a78..d1725a0b51 100644
--- a/etc/RT_Config.pm.in
+++ b/etc/RT_Config.pm.in
@@ -1625,6 +1625,16 @@ Admin -> Tools menu for SuperUsers.

Set($StatementLog, undef);

+=item SQL bind parameters
+
+RT enables SQL bind parameters for all searches by default, which improves
+performance especially for Oracle. If you need to disable this for some
+reason, add this to config:
+
+ $ENV{SB_PREFER_BIND} = 0;
+
+See also L<DBIx::SearchBuilder/BuildSelectQuery>.
+
=back


diff --git a/lib/RT/SearchBuilder.pm b/lib/RT/SearchBuilder.pm
index ee16cf42ca..caf1d035b1 100644
--- a/lib/RT/SearchBuilder.pm
+++ b/lib/RT/SearchBuilder.pm
@@ -69,6 +69,7 @@ use warnings;
use 5.010;

use base qw(DBIx::SearchBuilder RT::Base);
+$DBIx::SearchBuilder::PREFER_BIND = 1 unless defined $ENV{SB_PREFER_BIND};

use RT::Base;
use DBIx::SearchBuilder "1.40";

-----------------------------------------------------------------------


hooks/post-receive
--
rt
_______________________________________________
rt-commit mailing list
rt-commit@lists.bestpractical.com
https://lists.bestpractical.com/mailman/listinfo/rt-commit