Bug 24695: Move to Koha::Report->is_sql_valid

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

C4/Reports/Guided.pm

@@ -552,8 +552,8 @@ sub execute_query {
     $limit  = 999999 unless $limit;
     $debug and print STDERR "execute_query($sql, $offset, $limit)\n";
 
-    my $errors = Koha::Reports->validate_sql($sql);
-    return (undef, @{$errors}[0]) if (scalar(@$errors));
+    my ( $is_sql_valid, $errors ) = Koha::Report->new({ savedsql => $sql })->is_sql_valid;
+    return (undef, @{$errors}[0]) unless $is_sql_valid;
 
     foreach my $sql_param ( @$sql_params ){
         if ( $sql_param =~ m/\n/ ){

Koha/Report.pm

@@ -36,6 +36,36 @@ Koha::Report - Koha Report Object class
 
 =cut
 
+# FIXME We could only return an error code instead of the arrayref
+# Only 1 error is returned
+# TODO Koha::Report->store should check this before saving
+=head3 is_sql_valid
+
+my ( $is_sql_valid, $errors ) = $report->is_sql_valid;
+
+$errors is a arrayref of hashrefs, keys can be sqlerr or queryerr.
+
+Validate SQL query string so it only contains a select,
+not any of the harmful queries.
+
+=cut
+
+sub is_sql_valid {
+    my ($self) = @_;
+
+    my $sql = $self->savedsql;
+    $sql //= '';
+    my @errors = ();
+
+    if ($sql =~ /;?\W?(UPDATE|DELETE|DROP|INSERT|SHOW|CREATE)\W/i) {
+        push @errors, { sqlerr => $1 };
+    } elsif ($sql !~ /^\s*SELECT\b\s*/i) {
+        push @errors, { queryerr => 'Missing SELECT' };
+    }
+
+    return ( @errors ? 0 : 1, \@errors );
+}
+
 =head3 get_search_info
 
 Return search info

Koha/Reports.pm

@@ -35,28 +35,6 @@ Koha::Reports - Koha Report Object set class
 
 =cut
 
-=head3 validate_sql
-
-Validate SQL query string so it only contains a select,
-not any of the harmful queries.
-
-=cut
-
-sub validate_sql {
-    my ($self, $sql) = @_;
-
-    $sql //= '';
-    my @errors = ();
-
-    if ($sql =~ /;?\W?(UPDATE|DELETE|DROP|INSERT|SHOW|CREATE)\W/i) {
-        push @errors, { sqlerr => $1 };
-    } elsif ($sql !~ /^\s*SELECT\b\s*/i) {
-        push @errors, { queryerr => 'Missing SELECT' };
-    }
-
-    return \@errors;
-}
-
 =head3 _type
 
 Returns name of corresponding DBIC resultset

reports/guided_reports.pl

@@ -239,7 +239,8 @@ elsif ( $phase eq 'Update SQL'){
 
     create_non_existing_group_and_subgroup($input, $group, $subgroup);
 
-    push(@errors, @{Koha::Reports->validate_sql($sql)});
+    my ( $is_sql_valid, $validation_errors ) = Koha::Report->new({ savedsql => $sql })->is_sql_valid;
+    push(@errors, @$validation_errors) unless $is_sql_valid;
 
     if (@errors) {
         $template->param(
@@ -595,7 +596,8 @@ elsif ( $phase eq 'Save Report' ) {
 
     create_non_existing_group_and_subgroup($input, $group, $subgroup);
     ## FIXME this is AFTER entering a name to save the report under
-    push(@errors, @{Koha::Reports->validate_sql($sql)});
+    my ( $is_sql_valid, $validation_errors ) = Koha::Report->new({ savedsql => $sql })->is_sql_valid;
+    push(@errors, @$validation_errors) unless $is_sql_valid;
 
     if (@errors) {
         $template->param(

t/db_dependent/Koha/Reports.t

@@ -71,14 +71,41 @@ subtest 'prep_report' => sub {
 
 $schema->storage->txn_rollback;
 
-subtest 'validate_sql' => sub {
-    plan tests => 3 + 6*2;
-    my @badwords = ('UPDATE', 'DELETE', 'DROP', 'INSERT', 'SHOW', 'CREATE');
-    is_deeply( Koha::Reports->validate_sql(), [{ queryerr => 'Missing SELECT'}], 'Empty sql is missing SELECT' );
-    is_deeply( Koha::Reports->validate_sql('FOO'), [{ queryerr => 'Missing SELECT'}], 'Nonsense sql is missing SELECT' );
-    is_deeply( Koha::Reports->validate_sql('select FOO'), [], 'select FOO is good' );
+subtest 'is_sql_valid' => sub {
+    plan tests => 3 + 6 * 2;
+    my @badwords = ( 'UPDATE', 'DELETE', 'DROP', 'INSERT', 'SHOW', 'CREATE' );
+    is_deeply(
+        [ Koha::Report->new( { savedsql => '' } )->is_sql_valid ],
+        [ 0, [ { queryerr => 'Missing SELECT' } ] ],
+        'Empty sql is missing SELECT'
+    );
+    is_deeply(
+        [ Koha::Report->new( { savedsql => 'FOO' } )->is_sql_valid ],
+        [ 0, [ { queryerr => 'Missing SELECT' } ] ],
+        'Nonsense sql is missing SELECT'
+    );
+    is_deeply(
+        [ Koha::Report->new( { savedsql => 'select FOO' } )->is_sql_valid ],
+        [ 1, [] ],
+        'select FOO is good'
+    );
     foreach my $word (@badwords) {
-        is_deeply( Koha::Reports->validate_sql('select FOO;'.$word.' BAR'), [{ sqlerr => $word}], 'select FOO with '.$word.' BAR' );
-        is_deeply( Koha::Reports->validate_sql($word.' qux'), [{ sqlerr => $word}], $word.' qux' );
+        is_deeply(
+            [
+                Koha::Report->new(
+                    { savedsql => 'select FOO;' . $word . ' BAR' }
+                )->is_sql_valid
+            ],
+            [ 0, [ { sqlerr => $word } ] ],
+            'select FOO with ' . $word . ' BAR'
+        );
+        is_deeply(
+            [
+                Koha::Report->new( { savedsql => $word . ' qux' } )
+                  ->is_sql_valid
+            ],
+            [ 0, [ { sqlerr => $word } ] ],
+            $word . ' qux'
+        );
     }
-}
+  }