From 95ebc77eaa2baf2e8828dea68fcf70fa15425dbd Mon Sep 17 00:00:00 2001 From: Galen Charlton Date: Sun, 20 Apr 2014 16:04:27 +0000 Subject: [PATCH] Bug 9915: (follow-up) use SQL placeholders This patch teaches C4::Reports::Guided::execute_query() how to accept a list of query parameter values. It then follows-up on the main patch by simplifying how it converts report parameters to a complete SQL query, and removes the use of DBI->quote() and complicated regexes. To test: [1] Verify that using the OPAC svc/report service with sql_params continues to work. [2] Verify that there are no regressions with running reports from the staff interface, both via the web service and the reports interface. [3] Verify that prove -v /db_dependent/Reports_Guided.t passes. Signed-off-by: Galen Charlton --- C4/Reports/Guided.pm | 33 +++++++++++++++++++-------------- opac/svc/report | 15 +++------------ t/db_dependent/Reports_Guided.t | 23 ++++++++++++++++++++--- 3 files changed, 42 insertions(+), 29 deletions(-) diff --git a/C4/Reports/Guided.pm b/C4/Reports/Guided.pm index 0a4294a..2381483 100644 --- a/C4/Reports/Guided.pm +++ b/C4/Reports/Guided.pm @@ -400,22 +400,25 @@ sub nb_rows { =head2 execute_query - ($results, $error) = execute_query($sql, $offset, $limit) + ($sth, $error) = execute_query($sql, $offset, $limit[, \@sql_params]) -When passed C<$sql>, this function returns an array ref containing a result set -suitably formatted for display in html or for output as a flat file when passed in -C<$format> and C<$id>. It also returns the C<$total> records available for the -supplied query. If passed any query other than a SELECT, or if there is a db error, -C<$errors> an array ref is returned containing the error after this manner: +This function returns a DBI statement handler from which the caller can +fetch the results of the SQL passed via C<$sql>. + +If passed any query other than a SELECT, or if there is a db error, +C<$errors> an array ref is returned containing the error after this +manner: C<$error->{'sqlerr'}> contains the offending SQL keyword. -C<$error->{'queryerr'}> contains the native db engine error returned for the query. +C<$error->{'queryerr'}> contains the native db engine error returned +for the query. + +C<$offset>, and C<$limit> are required parameters. -Valid values for C<$format> are 'text,' 'tab,' 'csv,' or 'url. C<$sql>, C<$type>, -C<$offset>, and C<$limit> are required parameters. If a valid C<$format> is passed -in, C<$offset> and C<$limit> are ignored for obvious reasons. A LIMIT specified by -the user in a user-supplied SQL query WILL apply in any case. +C<\@sql_params> is an optional list of parameter values to paste in. +The caller is reponsible for making sure that C<$sql> has placeholders +and that the number placeholders matches the number of parameters. =cut @@ -470,9 +473,11 @@ sub strip_limit { } } -sub execute_query ($;$$$) { +sub execute_query { + + my ( $sql, $offset, $limit, $sql_params ) = @_; - my ( $sql, $offset, $limit, $no_count ) = @_; + $sql_params = [] unless defined $sql_params; # check parameters unless ($sql) { @@ -506,7 +511,7 @@ sub execute_query ($;$$$) { $sql .= " LIMIT ?, ?"; my $sth = C4::Context->dbh->prepare($sql); - $sth->execute($offset, $limit); + $sth->execute(@$sql_params, $offset, $limit); return ( $sth ); # my @xmlarray = ... ; # my $url = "/cgi-bin/koha/reports/guided_reports.pl?phase=retrieve%20results&id=$id"; diff --git a/opac/svc/report b/opac/svc/report index fb50ba2..1ded9c9 100755 --- a/opac/svc/report +++ b/opac/svc/report @@ -56,21 +56,12 @@ unless ($json_text) { my $offset = 0; my $limit = C4::Context->preference("SvcMaxReportRows") || 10; my $sql = $report_rec->{savedsql}; - if (@sql_params) { - # we have sql params need to fix the sql - my @split = split /<<|>>/, $sql; - my @tmpl_parameters; - for ( my $i = 0 ; $i < $#split / 2 ; $i++ ) { - my $quoted = C4::Context->dbh->quote( $sql_params[$i] ); + # convert SQL parameters to placeholders + $sql =~ s/(<<.*?>>)/\?/g; - # if there are special regexp chars, we must \ them - $split[ $i * 2 + 1 ] =~ s/(\||\?|\.|\*|\(|\)|\%)/\\$1/g; - $sql =~ s/<<$split[$i*2+1]>>/$quoted/; - } - } my ( $sth, $errors ) = - execute_query( $sql, $offset, $limit ); + execute_query( $sql, $offset, $limit, \@sql_params ); if ($sth) { my $lines; if ($report_annotation) { diff --git a/t/db_dependent/Reports_Guided.t b/t/db_dependent/Reports_Guided.t index 273ee84..ad34dbd 100755 --- a/t/db_dependent/Reports_Guided.t +++ b/t/db_dependent/Reports_Guided.t @@ -5,7 +5,7 @@ use Modern::Perl; -use Test::More tests => 12; +use Test::More tests => 14; use C4::Context; @@ -14,8 +14,7 @@ BEGIN { } can_ok( 'C4::Reports::Guided', - qw(save_report - delete_report) + qw(save_report delete_report execute_query) ); #Start transaction @@ -68,6 +67,24 @@ $count -= 2; is( scalar( @{ get_saved_reports() } ), $count, "Report2 and report3 have been deleted" ); +my $sth = execute_query('SELECT COUNT(*) FROM systempreferences', 0, 10); +my $results = $sth->fetchall_arrayref; +is(scalar(@$results), 1, 'running a query returned a result'); + +my $version = C4::Context->preference('Version'); +$sth = execute_query( + 'SELECT value FROM systempreferences WHERE variable = ?', + 0, + 10, + [ 'Version' ], +); +$results = $sth->fetchall_arrayref; +is_deeply( + $results, + [ [ $version ] ], + 'running a query with a parameter returned the expected result' +); + #End transaction $dbh->rollback; -- 1.7.10.4