Fix over-eager parenthesis unrolling (only legal in AND/OR)

diff --git a/Changes b/Changes
index 890c648..e2463f6 100644 (file)
--- a/Changes
+++ b/Changes
@@ -1,6 +1,7 @@
 Revision history for SQL::Abstract
 
     - Fix parsing of NOT EXISTS
+    - Fix over-eager parenthesis unrolling
     - Fix deep recursion warnings while parsing obnoxiously long sql statements
     - Fix incorrect comparison of malformed lists
 

diff --git a/lib/SQL/Abstract/Tree.pm b/lib/SQL/Abstract/Tree.pm
index d490a83..1a4560c 100644 (file)
--- a/lib/SQL/Abstract/Tree.pm
+++ b/lib/SQL/Abstract/Tree.pm
@@ -477,7 +477,9 @@ sub _unparse {
     return '';
   }
 
+  # FIXME - needs a config switch to disable
   $self->_parenthesis_unroll($tree);
+
   my ($car, $cdr) = @{$tree}[0,1];
 
   if (! defined $car or (! ref $car and ! defined $cdr) ) {
@@ -542,7 +544,6 @@ sub _parenthesis_unroll {
   my $self = shift;
   my $ast = shift;
 
-  #return if $self->parenthesis_significant;
   return unless (ref $ast and ref $ast->[1]);
 
   my $changes;
@@ -551,6 +552,7 @@ sub _parenthesis_unroll {
     $changes = 0;
 
     for my $child (@{$ast->[1]}) {
+
       # the current node in this loop is *always* a PAREN
       if (! ref $child or ! @$child or $child->[0] ne 'PAREN') {
         push @children, $child;
@@ -594,8 +596,8 @@ sub _parenthesis_unroll {
         $changes++;
       }
 
-      # only one element in the parenthesis which is a binary op
-      # and has exactly two grandchildren
+      # an AND/OR expression with only one binop in the parenthesis
+      # with exactly two grandchildren
       # the only time when we can *not* unroll this is when both
       # the parent and the child are mathops (in which case we'll
       # break precedence) or when the child is BETWEEN (special
@@ -603,6 +605,8 @@ sub _parenthesis_unroll {
       elsif (
         @{$child->[1]} == 1
           and
+        ($ast->[0] eq 'AND' or $ast->[0] eq 'OR')
+          and
         $child->[1][0][0] =~ SQL::Abstract::Tree::_binary_op_re()
           and
         $child->[1][0][0] ne 'BETWEEN'

diff --git a/t/14roundtrippin.t b/t/14roundtrippin.t
index 7df3010..a5e5196 100644 (file)
--- a/t/14roundtrippin.t
+++ b/t/14roundtrippin.t
@@ -17,6 +17,7 @@ my @sql = (
   "SELECT * FROM lolz WHERE ( foo.a =1 ) and foo.b LIKE 'station'",
   "SELECT [screen].[id], [screen].[name], [screen].[section_id], [screen].[xtype] FROM [users_roles] [me] JOIN [roles] [role] ON [role].[id] = [me].[role_id] JOIN [roles_permissions] [role_permissions] ON [role_permissions].[role_id] = [role].[id] JOIN [permissions] [permission] ON [permission].[id] = [role_permissions].[permission_id] JOIN [permissionscreens] [permission_screens] ON [permission_screens].[permission_id] = [permission].[id] JOIN [screens] [screen] ON [screen].[id] = [permission_screens].[screen_id] WHERE ( [me].[user_id] = ? ) GROUP BY [screen].[id], [screen].[name], [screen].[section_id], [screen].[xtype]",
   "SELECT * FROM foo WHERE NOT EXISTS (SELECT bar FROM baz)",
+  "SELECT * FROM (SELECT SUM (CASE WHEN me.artist = 'foo' THEN 1 ELSE 0 END AS artist_sum) FROM foobar) WHERE foo.a = 1 and foo.b LIKE 'station'"
 );
 
 for (@sql) {
@@ -26,7 +27,7 @@ for (@sql) {
 }
 
 # delete this test when mysql_functions gets implemented
-my $sql = 'SELECT COUNT( * ) FROM foo';
+my $sql = 'SELECT COUNT( * ), SUM( blah ) FROM foo';
 is($sqlat->format($sql), $sql, 'Roundtripping to mysql-compatible paren. syntax');
 
 lives_ok { $sqlat->unparse( $sqlat->parse( <<'EOS' ) ) } 'Able to parse/unparse grossly malformed sql';