From b41162872c24d3f5290a25915289ab96a348f85b Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Mon, 11 Jan 2021 20:07:39 +0000 Subject: [PATCH] Bug 27380: Move get_prepped_report to object and use for svc/reports This patch moves get_prepped_report to Koha:Report->prep_report and adds some basic tests To test: 1 - Using the report created in last test, hit the report svc api like: http://localhost:8081/cgi-bin/koha/svc/report?id=6¶m_name=One&sql_params=One¶m_name=Listy|list&sql_params=2345%0D%0A423%0D%0A3%0D%0A2%0D%0A12 2 - Note the use of %0D%0A to separate list params 3 - Test with combinations with and without param_name specified http://localhost:8081/cgi-bin/koha/svc/report?id=6&sql_params=5&sql_params=2345%0D%0A423%0D%0A3%0D%0A2%0D%0A12 Signed-off-by: Owen Leonard Signed-off-by: Katrin Fischer --- C4/Reports/Guided.pm | 12 ++++++++++ Koha/Report.pm | 53 +++++++++++++++++++++++++++++++++++++++++++ opac/svc/report | 10 ++++---- reports/guided_reports.pl | 49 ++------------------------------------- svc/report | 11 ++++----- t/db_dependent/Koha/Reports.t | 23 ++++++++++++++++++- 6 files changed, 99 insertions(+), 59 deletions(-) diff --git a/C4/Reports/Guided.pm b/C4/Reports/Guided.pm index 7bc1504c53..a76d86ba44 100644 --- a/C4/Reports/Guided.pm +++ b/C4/Reports/Guided.pm @@ -557,6 +557,18 @@ sub execute_query { return (undef, { queryerr => 'Missing SELECT'} ); } + foreach my $sql_param ( @$sql_params ){ + if ( $sql_param =~ m/\n/ ){ + my @list = split /\n/, $sql_param; + my @quoted_list; + foreach my $item ( @list ){ + $item =~ s/\r//; + push @quoted_list, C4::Context->dbh->quote($item); + } + $sql_param = "(".join(",",@quoted_list).")"; + } + } + my ($useroffset, $userlimit); # Grab offset/limit from user supplied LIMIT and drop the LIMIT so we can control pagination diff --git a/Koha/Report.pm b/Koha/Report.pm index 01b7dff786..b0647bddb4 100644 --- a/Koha/Report.pm +++ b/Koha/Report.pm @@ -90,6 +90,59 @@ sub new_from_mana { Koha::Report->new($data)->store; } +=head3 prep_report + +Prep the report and return executable sql with parameters embedded and a list of header types +for building batch action links in the template + +=cut + +sub prep_report { + my ($self, $param_names, $sql_params ) = @_; + my $sql = $self->savedsql; + + # First we split out the placeholders + # This part of the code supports using [[ table.field | alias ]] in the + # query and replaces it by table.field AS alias. This is used to build + # the batch action links foir cardnumbers, itemnumbers, and biblionumbers in the template + # while allowing the library to alter the column names + my @split = split /\[\[|\]\]/,$sql; + my $headers; + for(my $i=0;$i<$#split/2;$i++){ #The placeholders are always the odd elements of the array + my ($type,$name) = split /\|/,$split[$i*2+1]; # We split them on '|' + $headers->{$name} = $type; # Store as a lookup for the template + $headers->{$name} =~ s/^\w*\.//; # strip the table name just as in $sth->{NAME} array + $split[$i*2+1] =~ s/(\||\?|\.|\*|\(|\)|\%)/\\$1/g; #Quote any special characters so we can replace the placeholders + $name = C4::Context->dbh->quote($name); + $sql =~ s/\[\[$split[$i*2+1]\]\]/$type AS $name/; # Remove placeholders from SQL + } + + my %lookup; + @lookup{@$param_names} = @$sql_params; + @split = split /<<|>>/,$sql; + for(my $i=0;$i<$#split/2;$i++) { + my $quoted = @$param_names ? $lookup{ $split[$i*2+1] } : @$sql_params[$i]; + # if there are special regexp chars, we must \ them + $split[$i*2+1] =~ s/(\||\?|\.|\*|\(|\)|\%)/\\$1/g; + if ($split[$i*2+1] =~ /\|\s*date\s*$/) { + $quoted = output_pref({ dt => dt_from_string($quoted), dateformat => 'iso', dateonly => 1 }) if $quoted; + } + unless( $split[$i*2+1] =~ /\|\s*list\s*$/ && $quoted ){ + $quoted = C4::Context->dbh->quote($quoted); + } else { + my @list = split /\n/, $quoted; + my @quoted_list; + foreach my $item ( @list ){ + $item =~ s/\r//; + push @quoted_list, C4::Context->dbh->quote($item); + } + $quoted="(".join(",",@quoted_list).")"; + } + $sql =~ s/<<$split[$i*2+1]>>/$quoted/; + } + return $sql, $headers; +} + =head3 _type Returns name of corresponding DBIC resultset diff --git a/opac/svc/report b/opac/svc/report index ef07334573..08e1ddc1f7 100755 --- a/opac/svc/report +++ b/opac/svc/report @@ -43,6 +43,7 @@ my $report_rec = $report_recs->next(); die "Sorry this report is not public\n" unless $report_rec->public; my @sql_params = $query->multi_param('sql_params'); +my @param_names = $query->multi_param('param_names'); my $cache = Koha::Caches->get_instance(); my $cache_active = $cache->is_cache_active; @@ -51,7 +52,8 @@ if ($cache_active) { $cache_key = "opac:report:" . ( $report_name ? "name:$report_name:" : "id:$report_id:" ) - . join( '-', @sql_params ); + . join( '-', @sql_params ) + . join( '_'. @param_names ); $json_text = $cache->get_from_cache($cache_key); } @@ -60,12 +62,10 @@ unless ($json_text) { my $limit = C4::Context->preference("SvcMaxReportRows") || 10; my $sql = $report_rec->savedsql; - # convert SQL parameters to placeholders - $sql =~ s/(<<.*?>>)/\?/g; - $sql =~ s/\[\[(.*?)\|(.*?)\]\]/$1 AS $2/g; + my ( $sql, undef ) = $report_rec->prep_report( \@param_names, \@sql_params ); my ( $sth, $errors ) = - execute_query( $sql, $offset, $limit, \@sql_params, $report_id ); + execute_query( $sql, $offset, $limit, undef, $report_id ); if ($sth) { my $lines; if ($report_annotation) { diff --git a/reports/guided_reports.pl b/reports/guided_reports.pl index e847d05e1c..cff8da8ffe 100755 --- a/reports/guided_reports.pl +++ b/reports/guided_reports.pl @@ -829,7 +829,7 @@ elsif ($phase eq 'Run this report'){ 'reports' => $report_id, ); } else { - my ($sql,$header_types) = get_prepped_report( $sql, \@param_names, \@sql_params); + my ($sql,$header_types) = $report->prep_report( \@param_names, \@sql_params ); $template->param(header_types => $header_types); my ( $sth, $errors ) = execute_query( $sql, $offset, $limit, undef, $report_id ); my $total = nb_rows($sql) || 0; @@ -894,7 +894,7 @@ elsif ($phase eq 'Export'){ my $reportname = $input->param('reportname'); my $reportfilename = $reportname ? "$reportname-reportresults.$format" : "reportresults.$format" ; - ($sql, undef) = get_prepped_report( $sql, \@param_names, \@sql_params ); + ($sql, undef) = $report->prep_report( \@param_names, \@sql_params ); my ($sth, $q_errors) = execute_query($sql); unless ($q_errors and @$q_errors) { my ( $type, $content ); @@ -1090,48 +1090,3 @@ sub create_non_existing_group_and_subgroup { } } -# pass $sth and sql_params, get back an executable query -sub get_prepped_report { - my ($sql, $param_names, $sql_params ) = @_; - - # First we split out the placeholders - # This part of the code supports using [[ table.field | alias ]] in the - # query and replaces it by table.field AS alias. Not sure why we would - # need it if we can type the latter (which is simpler)? - my @split = split /\[\[|\]\]/,$sql; - my $headers; - for(my $i=0;$i<$#split/2;$i++){ #The placeholders are always the odd elements of the array - my ($type,$name) = split /\|/,$split[$i*2+1]; # We split them on '|' - $headers->{$name} = $type; # Store as a lookup for the template - $headers->{$name} =~ s/^\w*\.//; # strip the table name just as in $sth->{NAME} array - $split[$i*2+1] =~ s/(\||\?|\.|\*|\(|\)|\%)/\\$1/g; #Quote any special characters so we can replace the placeholders - $name = C4::Context->dbh->quote($name); - $sql =~ s/\[\[$split[$i*2+1]\]\]/$type AS $name/; # Remove placeholders from SQL - } - - my %lookup; - @lookup{@$param_names} = @$sql_params; - @split = split /<<|>>/,$sql; - my @tmpl_parameters; - for(my $i=0;$i<$#split/2;$i++) { - my $quoted = @$param_names ? $lookup{ $split[$i*2+1] } : @$sql_params[$i]; - # if there are special regexp chars, we must \ them - $split[$i*2+1] =~ s/(\||\?|\.|\*|\(|\)|\%)/\\$1/g; - if ($split[$i*2+1] =~ /\|\s*date\s*$/) { - $quoted = output_pref({ dt => dt_from_string($quoted), dateformat => 'iso', dateonly => 1 }) if $quoted; - } - unless( $split[$i*2+1] =~ /\|\s*list\s*$/ && $quoted ){ - $quoted = C4::Context->dbh->quote($quoted); - } else { - my @list = split /\n/, $quoted; - my @quoted_list; - foreach my $item ( @list ){ - $item =~ s/\r//; - push @quoted_list, C4::Context->dbh->quote($item); - } - $quoted="(".join(",",@quoted_list).")"; - } - $sql =~ s/<<$split[$i*2+1]>>/$quoted/; - } - return $sql,$headers; -} diff --git a/svc/report b/svc/report index 0ab8fb5dcb..18b0f2bd4b 100755 --- a/svc/report +++ b/svc/report @@ -39,6 +39,7 @@ if (!$report_recs || $report_recs->count == 0 ) { die "There is no such report.\ my $report_rec = $report_recs->next(); my @sql_params = $query->multi_param('sql_params'); +my @param_names = $query->multi_param('param_names'); my ( $template, $loggedinuser, $cookie ) = get_template_and_user( { @@ -54,20 +55,18 @@ my $cache_active = $cache->is_cache_active; my ($cache_key, $json_text); if ($cache_active) { $cache_key = "intranet:report:".($report_name ? "report_name:$report_name:" : "id:$report_id:") - . join( '-', @sql_params ); + . join( '-', @sql_params ) + . join( '_'. @param_names ); $json_text = $cache->get_from_cache($cache_key); } unless ($json_text) { my $offset = 0; my $limit = C4::Context->preference("SvcMaxReportRows") || 10; - my $sql = $report_rec->savedsql; - # convert SQL parameters to placeholders - $sql =~ s/(<<.*?>>)/\?/g; - $sql =~ s/\[\[(.*?)\|(.*?)\]\]/$1 AS $2/g; + my ( $sql, undef ) = $report_rec->prep_report( \@param_names, \@sql_params ); - my ( $sth, $errors ) = execute_query( $sql, $offset, $limit, \@sql_params, $report_id ); + my ( $sth, $errors ) = execute_query( $sql, $offset, $limit, undef, $report_id ); if ($sth) { my $lines; if ($report_annotation) { diff --git a/t/db_dependent/Koha/Reports.t b/t/db_dependent/Koha/Reports.t index 45c5662247..ab982c84d7 100755 --- a/t/db_dependent/Koha/Reports.t +++ b/t/db_dependent/Koha/Reports.t @@ -17,7 +17,7 @@ use Modern::Perl; -use Test::More tests => 4; +use Test::More tests => 5; use Koha::Report; use Koha::Reports; @@ -48,4 +48,25 @@ is( $retrieved_report_1->report_name, $new_report_1->report_name, 'Find a report $retrieved_report_1->delete; is( Koha::Reports->search->count, $nb_of_reports + 1, 'Delete should have deleted the report' ); +subtest 'prep_report' => sub { + plan tests => 3; + + my $report = Koha::Report->new({ + report_name => 'report_name_for_test_1', + savedsql => 'SELECT * FROM items WHERE itemnumber IN <>', + })->store; + + my ($sql, undef) = $report->prep_report( ['Test|list'],["1\n12\n\r243"] ); + is( $sql, q{SELECT * FROM items WHERE itemnumber IN ('1','12','243')},'Expected sql generated correctly with single param and name'); + + $report->savedsql('SELECT * FROM items WHERE itemnumber IN <> AND <> AND <>')->store; + + ($sql, undef) = $report->prep_report( ['Test|list','Another'],["1\n12\n\r243",'the other'] ); + is( $sql, q{SELECT * FROM items WHERE itemnumber IN ('1','12','243') AND 'the other' AND ('1','12','243')},'Expected sql generated correctly with multiple params and names'); + + ($sql, undef) = $report->prep_report( [],["1\n12\n\r243",'the other',"42\n32\n22\n12"] ); + is( $sql, q{SELECT * FROM items WHERE itemnumber IN ('1','12','243') AND 'the other' AND ('42','32','22','12')},'Expected sql generated correctly with multiple params and no names'); + +}; + $schema->storage->txn_rollback; -- 2.11.0