Attachment 27330 Details for Bug 9915 – [PASSED QA] Bug 9915: (follow-up) use SQL placeholders

[patch] [PASSED QA] Bug 9915: (follow-up) use SQL placeholders

PASSED-QA-Bug-9915-follow-up-use-SQL-placeholders.patch (text/plain), 5.83 KB, created by Katrin Fischer on 2014-04-20 20:55:30 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Katrin Fischer

Created: 2014-04-20 20:55:30 UTC

Size: 5.83 KB

patch

obsolete

>From 7709aff02c1c66fd535542c7479d99908325d673 Mon Sep 17 00:00:00 2001
>From: Galen Charlton <gmc@esilibrary.com>
>Date: Sun, 20 Apr 2014 16:04:27 +0000
>Subject: [PATCH] [PASSED QA] 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.
>---
> 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.8.3.2

Actions: View | Diff | Splinter Review

Attachments on bug 9915: 16830 | 25918 | 25957 | 27316 | 27327 | 27328 | 27330 | 27331 | 27332