From: Peter Rabbitson <ribasushi@cpan.org>
Date: Fri, 3 Jan 2014 16:57:16 +0000 (+0100)
Subject: Ensure that multi-nested parenthesis are never unrolled after IN
X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?p=scpubgit%2FQ-Branch.git;a=commitdiff_plain;h=7d27345242d34aa0ad38e3a5f54d0bd09b558d32

Ensure that multi-nested parenthesis are never unrolled after IN

This lets us remove parenthesis_significant from many tests that do
not need it, including reversal of 1ba9d0f03
---

diff --git a/lib/SQL/Abstract/Tree.pm b/lib/SQL/Abstract/Tree.pm
index 519cae4..c6faef9 100644
--- a/lib/SQL/Abstract/Tree.pm
+++ b/lib/SQL/Abstract/Tree.pm
@@ -659,7 +659,7 @@ sub _parenthesis_unroll {
       }
 
       # unroll nested parenthesis
-      while ( @{$child->[1]} == 1 and $child->[1][0][0] eq '-PAREN') {
+      while ( $ast->[0] ne 'IN' and @{$child->[1]} == 1 and $child->[1][0][0] eq '-PAREN') {
         $child = $child->[1][0];
         $changes++;
       }
diff --git a/t/05in_between.t b/t/05in_between.t
index d3ede2f..832b7a1 100644
--- a/t/05in_between.t
+++ b/t/05in_between.t
@@ -141,46 +141,40 @@ my @in_between_tests = (
   },
 
   {
-    parenthesis_significant => 1,
     where => { x => { -in => [ 1 .. 3] } },
-    stmt => "WHERE ( x IN (?, ?, ?) )",
-    bind => [ 1 .. 3],
+    stmt => "WHERE x IN (?, ?, ?)",
+    bind => [ 1 .. 3 ],
     test => '-in with an array of scalars',
   },
   {
-    parenthesis_significant => 1,
     where => { x => { -in => [] } },
-    stmt => "WHERE ( 0=1 )",
+    stmt => "WHERE 0=1",
     bind => [],
     test => '-in with an empty array',
   },
   {
-    parenthesis_significant => 1,
     where => { x => { -in => \'( 1,2,lower(y) )' } },
-    stmt => "WHERE ( x IN ( 1,2,lower(y) ) )",
+    stmt => "WHERE x IN ( 1,2,lower(y) )",
     bind => [],
     test => '-in with a literal scalarref',
   },
 
   # note that outer parens are opened even though literal was requested below
   {
-    parenthesis_significant => 1,
     where => { x => { -in => \['( ( ?,?,lower(y) ) )', 1, 2] } },
-    stmt => "WHERE ( x IN ( ?,?,lower(y) ) )",
+    stmt => "WHERE x IN ( ?,?,lower(y) )",
     bind => [1, 2],
     test => '-in with a literal arrayrefref',
   },
   {
-    parenthesis_significant => 1,
     where => {
       status => { -in => \"(SELECT status_codes\nFROM states)" },
     },
-    stmt => " WHERE ( status IN ( SELECT status_codes FROM states )) ",
+    stmt => " WHERE status IN ( SELECT status_codes FROM states )",
     bind => [],
     test => '-in multi-line subquery test',
   },
   {
-    parenthesis_significant => 1,
     where => {
       customer => { -in => \[
         'SELECT cust_id FROM cust WHERE balance > ?',
@@ -189,17 +183,15 @@ my @in_between_tests = (
       status => { -in => \'SELECT status_codes FROM states' },
     },
     stmt => "
-      WHERE ((
+      WHERE
             customer IN ( SELECT cust_id FROM cust WHERE balance > ? )
         AND status IN ( SELECT status_codes FROM states )
-      ))
     ",
     bind => [2000],
     test => '-in POD test',
   },
 
   {
-    parenthesis_significant => 1,
     where => { x => { -in => [ \['LOWER(?)', 'A' ], \'LOWER(b)', { -lower => 'c' } ] } },
     stmt => " WHERE ( x IN ( LOWER(?), LOWER(b), LOWER ? ) )",
     bind => [qw/A c/],
@@ -213,7 +205,6 @@ my @in_between_tests = (
       \Qversion of SQL::Abstract will emit the logically correct SQL \E
       \Qinstead of raising this exception)\E
     /x,
-    parenthesis_significant => 1,
     where => { x => { -in => [ 1, undef ] } },
     stmt => " WHERE ( x IN ( ? ) OR x IS NULL )",
     bind => [ 1 ],
@@ -227,16 +218,14 @@ my @in_between_tests = (
       \Qversion of SQL::Abstract will emit the logically correct SQL \E
       \Qinstead of raising this exception)\E
     /x,
-    parenthesis_significant => 1,
     where => { x => { -in => [ 1, undef, 2, 3, undef ] } },
     stmt => " WHERE ( x IN ( ?, ?, ? ) OR x IS NULL )",
     bind => [ 1, 2, 3 ],
     test => '-in with multiple undef elements',
   },
   {
-    parenthesis_significant => 1,
     where => { a => { -in => 42 }, b => { -not_in => 42 } },
-    stmt => ' WHERE ( ( a IN ( ? ) AND b NOT IN ( ? ) ) )',
+    stmt => ' WHERE a IN ( ? ) AND b NOT IN ( ? )',
     bind => [ 42, 42 ],
     test => '-in, -not_in with scalar',
   },
@@ -254,7 +243,6 @@ my @in_between_tests = (
       \Qversion of SQL::Abstract will emit the logically correct SQL \E
       \Qinstead of raising this exception)\E
     /x,
-    parenthesis_significant => 1,
     where => { a => { -in => [42, undef] }, b => { -not_in => [42, undef] } },
     stmt => ' WHERE ( ( a IN ( ? ) OR a IS NULL ) AND b NOT IN ( ? ) AND b IS NOT NULL )',
     bind => [ 42, 42 ],
diff --git a/t/10test.t b/t/10test.t
index 8d02f4e..60d10d7 100644
--- a/t/10test.t
+++ b/t/10test.t
@@ -257,10 +257,10 @@ my @sql_tests = (
       },
       {
         equal => 0,
-        opts => { parenthesis_significant => 1 },
         statements => [
-          q/SELECT foo FROM bar WHERE a IN (1,2,3)/,
           q/SELECT foo FROM bar WHERE a IN (1,3,2)/,
+          q/SELECT foo FROM bar WHERE a IN 1,2,3/,
+          q/SELECT foo FROM bar WHERE a IN (1,2,3)/,
           q/SELECT foo FROM bar WHERE a IN ((1,2,3))/,
         ]
       },
diff --git a/t/11parser.t b/t/11parser.t
index 7cf6509..3c60c95 100644
--- a/t/11parser.t
+++ b/t/11parser.t
@@ -811,7 +811,7 @@ is_deeply($sqlat->parse('SELECT foo FROM bar ORDER BY x + ? DESC, oomph, y - ? D
   ]
 ], 'Crazy ORDER BY parsed correctly');
 
-is_deeply( $sqlat->parse("META SELECT * * FROM (SELECT *, FROM foobar baz buzz) foo bar WHERE NOT NOT NOT EXISTS (SELECT 'cr,ap') AND foo.a = ? STUFF moar(stuff) and not (foo.b LIKE 'station') and x = y and a = b and GROUP BY , ORDER BY x x1 x2 y asc, max(y) desc x z desc"), [
+is_deeply( $sqlat->parse("META SELECT * * FROM (SELECT *, FROM foobar baz buzz) foo bar WHERE NOT NOT NOT EXISTS (SELECT 'cr,ap') AND foo.a = ? STUFF moar(stuff) and not (foo.b LIKE 'station') and x = y and z in ((1, 2)) and a = b and GROUP BY , ORDER BY x x1 x2 y asc, max(y) desc x z desc"), [
   [
     "-LITERAL",
     [
@@ -1047,6 +1047,44 @@ is_deeply( $sqlat->parse("META SELECT * * FROM (SELECT *, FROM foobar baz buzz)
             ]
           ],
           [
+            'IN',
+            [
+              [
+                '-LITERAL',
+                [
+                  'z',
+                ],
+              ],
+              [
+                '-PAREN',
+                [
+                  [
+                    '-PAREN',
+                    [
+                      [
+                        '-LIST',
+                        [
+                          [
+                            '-LITERAL',
+                            [
+                              '1'
+                            ]
+                          ],
+                          [
+                            '-LITERAL',
+                            [
+                              '2'
+                            ]
+                          ],
+                        ],
+                      ],
+                    ],
+                  ],
+                ],
+              ],
+            ],
+          ],
+          [
             "=",
             [
               [
diff --git a/t/14roundtrippin.t b/t/14roundtrippin.t
index 981297e..9dc581e 100644
--- a/t/14roundtrippin.t
+++ b/t/14roundtrippin.t
@@ -53,8 +53,8 @@ for my $orig (@sql) {
     $sqlat->unparse($ast);
   };
 
-  # deal with parenthesis readjustment
-  $_ =~ s/\s*([\(\)])\s*/$1 /g
+  # deal with whitespace around parenthesis readjustment
+  $_ =~ s/ \s* ( [ \(\) ] ) \s* /$1/gx
     for ($orig, $reassembled);
 
   is (
@@ -69,6 +69,15 @@ for my $orig (@sql) {
   };
 }
 
+# this is invalid SQL, we are just checking that the parser
+# does not inadvertently make it right
+my $sql = 'SELECT * FROM foo WHERE x IN ( ( 1 ) )';
+is(
+  $sqlat->unparse($sqlat->parse($sql)),
+  $sql,
+  'Multi-parens around IN survive',
+);
+
 lives_ok { $sqlat->unparse( $sqlat->parse( <<'EOS' ) ) } 'Able to parse/unparse grossly malformed sql';
 SELECT
   (