From db683b285cfb31deb9dbdafbc5a18e0b471ffa65 Mon Sep 17 00:00:00 2001
From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Date: Fri, 21 Sep 2018 09:15:08 +0200
Subject: [PATCH] Bug 21281: (QA follow-up) Introduce _add_backtics

Added a (initially trivial) test in Creators/Lib.t too.
Sometimes you start something, but you end somewhere else ;)

The test seems a bit slower now (regex, lookahead, etc). But this should
not hurt label creators in daily use.
Advantages: code is even more solid, consolidated in one place and can be
tested on its own.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
---
 C4/Creators/Lib.pm            | 33 +++++++++++++++++----------------
 t/db_dependent/Creators/Lib.t | 12 +++++++++++-
 2 files changed, 28 insertions(+), 17 deletions(-)

diff --git a/C4/Creators/Lib.pm b/C4/Creators/Lib.pm
index f5616a5c05..fc40c7b701 100644
--- a/C4/Creators/Lib.pm
+++ b/C4/Creators/Lib.pm
@@ -67,11 +67,8 @@ C4::Creators::Lib
 
 sub _SELECT {
     my @params = @_;
-    my $fields_list = $params[0];
-    if (index($fields_list, ' ')==-1 && index($fields_list,',')==-1 && $fields_list ne '*') {
-        $fields_list = "`$fields_list`";
-    }
-    my $query = "SELECT $fields_list FROM $params[1]";
+    my $fieldname = _add_backtics($params[0]);
+    my $query = "SELECT $fieldname FROM $params[1]";
     $params[2] ? $query .= " WHERE $params[2];" : $query .= ';';
     my $sth = C4::Context->dbh->prepare($query);
 #    $sth->{'TraceLevel'} = 3;
@@ -87,6 +84,19 @@ sub _SELECT {
     return $record_set;
 }
 
+sub _add_backtics {
+    my ( @args ) = @_;
+    s/(?:^|\b)(\w+)(?:\b|$)/`$1`/g for @args;
+    # Too bad that we need to correct a few exceptions: aggregate functions
+    my @aggregates = ( 'COUNT', 'MAX', 'MIN', 'SUM' ); # add when needed..
+    foreach my $aggr (@aggregates) {
+        s/`$aggr`\(/$aggr\(/gi for @args;
+    }
+    # And correct aliases
+    s/(`|\))\s+`AS`\s+`/$1 AS `/gi for @args;
+    return wantarray ? @args : $args[0];
+}
+
 my $barcode_types = [
     {type => 'CODE39',          name => 'Code 39',              desc => 'Translates the characters 0-9, A-Z, \'-\', \'*\', \'+\', \'$\', \'%\', \'/\', \'.\' and \' \' to a barcode pattern.',                                  selected => 0},
     {type => 'CODE39MOD',       name => 'Code 39 + Modulo43',   desc => 'Translates the characters 0-9, A-Z, \'-\', \'*\', \'+\', \'$\', \'%\', \'/\', \'.\' and \' \' to a barcode pattern. Encodes Mod 43 checksum.',         selected => 0},
@@ -148,17 +158,8 @@ my $output_formats = [
 
 sub _build_query {
     my ( $params, $table ) = @_;
-    my @fields = exists $params->{fields} ? @{ $params->{fields} } : ();
-    my @fields2 = ();
-    foreach my $field_name (@fields) {
-        if (index($field_name,' ')==-1 && $field_name ne '*') {
-            push @fields2, "`$field_name`";
-        } else {
-            push @fields2, $field_name;
-        }
-    }
-    @fields = @fields2;
-    my $query = "SELECT " . ( @fields ? join(', ', @fields ) : '*' ) . " FROM $table";
+    my @fields = exists $params->{fields} ? _add_backtics( @{ $params->{fields} } ) : ('*');
+    my $query = "SELECT " . join(', ', @fields ) . " FROM $table";
     my @where_args;
     if ( exists $params->{filters} ) {
         $query .= ' WHERE 1 ';
diff --git a/t/db_dependent/Creators/Lib.t b/t/db_dependent/Creators/Lib.t
index d1808508ea..288257e7f8 100644
--- a/t/db_dependent/Creators/Lib.t
+++ b/t/db_dependent/Creators/Lib.t
@@ -18,7 +18,7 @@
 
 use Modern::Perl;
 use Graphics::Magick;
-use Test::More tests => 645;
+use Test::More tests => 646;
 use Test::MockModule;
 use t::lib::Mocks;
 use t::lib::TestBuilder;
@@ -1437,6 +1437,16 @@ is( $records->[0]->{cardnumber},   $cardnumber1,  'cardnumber   is good' );
 is( $records->[0]->{branchcode},   $branchcode,   'branchcode   is good' );
 is( $records->[0]->{categorycode}, $categorycode, 'categorycode is good' );
 
+subtest '_add_backtics' => sub {
+    plan tests => 7;
+    my @a = ( 'rows', 'rows ', ' rows ', 'table.rows,field2', 'table.field1, table.field_2_a', 'table1.*, *', 'COUNT(id) AS mycount, count(*) as mycount2' );
+    my @a_exp = ( '`rows`', '`rows` ', ' `rows` ', '`table`.`rows`,`field2`', '`table`.`field1`, `table`.`field_2_a`', '`table1`.*, *', 'COUNT(`id`) AS `mycount`, COUNT(*) AS `mycount2`' ); # expected results
+    my @b = C4::Creators::Lib::_add_backtics(@a);
+    while( my $f = shift @a_exp ) {
+        is( shift @b, $f, (shift @a). ' became '. $f );
+    }
+};
+
 # ---------- Sub ------------------------------------------
 my %preferences;
 
-- 
2.39.5