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 <gmc@esilibrary.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes all tests and QA script.
No regressions found.

Signed-off-by: Galen Charlton <gmc@esilibrary.com>

diff --git a/C4/Reports/Guided.pm b/C4/Reports/Guided.pm
index 0a4294a..2381483 100644 (file)
--- 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 (executable)
--- 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 (executable)
--- 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;