From: Peter Rabbitson <ribasushi@cpan.org>
Date: Fri, 22 Aug 2014 11:40:14 +0000 (+0200)
Subject: Deduplicate (and stabilize) the result of _collapse_cond
X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=5268b1da661134493695d0c8f364b2d094da616e;p=dbsrgits%2FDBIx-Class-Historic.git

Deduplicate (and stabilize) the result of _collapse_cond

Among other things set the stage to a fix of RT#98161 (later commit)
---

diff --git a/lib/DBIx/Class/ResultSet.pm b/lib/DBIx/Class/ResultSet.pm
index 9a478e4..97417fa 100644
--- a/lib/DBIx/Class/ResultSet.pm
+++ b/lib/DBIx/Class/ResultSet.pm
@@ -10,7 +10,6 @@ use DBIx::Class::_Util qw(
   fail_on_internal_wantarray fail_on_internal_call UNRESOLVABLE_CONDITION
 );
 use Try::Tiny;
-use Data::Compare (); # no imports!!! guard against insane architecture
 
 # not importing first() as it will clash with our own method
 use List::Util ();
@@ -656,26 +655,17 @@ sub _stack_cond {
     (ref $_ eq 'HASH' and ! keys %$_)
   ) and $_ = undef for ($left, $right);
 
-  # either on of the two undef or both undef
-  if ( ( (defined $left) xor (defined $right) ) or ! defined $left ) {
+  # either one of the two undef
+  if ( (defined $left) xor (defined $right) ) {
     return defined $left ? $left : $right;
   }
-
-  my $cond = $self->result_source->schema->storage->_collapse_cond({ -and => [$left, $right] });
-
-  for my $c (grep { ref $cond->{$_} eq 'ARRAY' and ($cond->{$_}[0]||'') eq '-and' } keys %$cond) {
-
-    my @vals = sort @{$cond->{$c}}[ 1..$#{$cond->{$c}} ];
-    my @fin = shift @vals;
-
-    for my $v (@vals) {
-      push @fin, $v unless Data::Compare::Compare( $fin[-1], $v );
-    }
-
-    $cond->{$c} = (@fin == 1) ? $fin[0] : [-and => @fin ];
+  # both undef
+  elsif ( ! defined $left ) {
+    return undef
+  }
+  else {
+    return $self->result_source->schema->storage->_collapse_cond({ -and => [$left, $right] });
   }
-
-  $cond;
 }
 
 =head2 search_literal
diff --git a/lib/DBIx/Class/Storage/DBIHacks.pm b/lib/DBIx/Class/Storage/DBIHacks.pm
index f21759c..d2d8f63 100644
--- a/lib/DBIx/Class/Storage/DBIHacks.pm
+++ b/lib/DBIx/Class/Storage/DBIHacks.pm
@@ -1056,6 +1056,23 @@ sub _collapse_cond {
       }
     }
 
+    # compress same-column conds found in $fin
+    for my $col ( keys %$fin ) {
+      next unless ref $fin->{$col} eq 'ARRAY' and ($fin->{$col}[0]||'') eq '-and';
+      my $val_bag = { map {
+        (! defined $_ )                   ? ( UNDEF => undef )
+      : ( ! ref $_ or is_plain_value $_ ) ? ( "VAL_$_" => $_ )
+      : ( ( 'SER_' . serialize $_ ) => $_ )
+      } @{$fin->{$col}}[1 .. $#{$fin->{$col}}] };
+
+      if (keys %$val_bag == 1 ) {
+        ($fin->{$col}) = values %$val_bag;
+      }
+      else {
+        $fin->{$col} = [ -and => map { $val_bag->{$_} } sort keys %$val_bag ];
+      }
+    }
+
     return $fin;
   }
   elsif (ref $where eq 'ARRAY') {
diff --git a/t/53lean_startup.t b/t/53lean_startup.t
index 6dd37f7..2943507 100644
--- a/t/53lean_startup.t
+++ b/t/53lean_startup.t
@@ -104,7 +104,6 @@ BEGIN {
 
     Scalar::Util
     List::Util
-    Data::Compare
 
     Class::Accessor::Grouped
     Class::C3::Componentised
@@ -123,6 +122,7 @@ BEGIN {
   register_lazy_loadable_requires(qw(
     Moo
     Context::Preserve
+    Data::Compare
   ));
 
   my $s = DBICTest::Schema->connect('dbi:SQLite::memory:');
diff --git a/t/search/stack_cond.t b/t/search/stack_cond.t
new file mode 100644
index 0000000..a68f692
--- /dev/null
+++ b/t/search/stack_cond.t
@@ -0,0 +1,79 @@
+use strict;
+use warnings;
+
+use Test::More;
+use lib qw(t/lib);
+use DBICTest ':DiffSQL';
+use SQL::Abstract qw(is_plain_value is_literal_value);
+use List::Util 'shuffle';
+use Data::Dumper;
+$Data::Dumper::Terse = 1;
+$Data::Dumper::Useqq = 1;
+$Data::Dumper::Indent = 0;
+
+my $schema = DBICTest->init_schema();
+
+for my $c (
+  { cond => undef, sql => 'IS NULL' },
+  { cond => { -value => undef }, sql => 'IS NULL' },
+  { cond => \'foo', sql => '= foo' },
+  { cond => 'foo', sql => '= ?', bind => [
+    [ { dbic_colname => "title", sqlt_datatype => "varchar", sqlt_size => 100 } => 'foo' ],
+    [ { dbic_colname => "year", sqlt_datatype => "varchar", sqlt_size => 100 } => 'foo' ],
+  ]},
+  { cond => { -value => 'foo' }, sql => '= ?', bind => [
+    [ { dbic_colname => "title", sqlt_datatype => "varchar", sqlt_size => 100 } => 'foo' ],
+    [ { dbic_colname => "year", sqlt_datatype => "varchar", sqlt_size => 100 } => 'foo' ],
+  ]},
+  { cond => \[ '?', "foo" ], sql => '= ?', bind => [
+    [ {} => 'foo' ],
+    [ {} => 'foo' ],
+  ]},
+) {
+  my $rs = $schema->resultset('CD')->search({}, { columns => 'title' });
+
+  my $bare_cond = is_literal_value($c->{cond}) ? { '=', $c->{cond} } : $c->{cond};
+
+  my @query_steps = (
+    # this is a monkey-wrench, always there
+    { title => { '!=', [ -and => \'bar' ] }, year => { '!=', [ -and => 'bar' ] } },
+
+    { title => $bare_cond, year => { '=', $c->{cond} } },
+    { -and => [ year => $bare_cond, { title => { '=', $c->{cond} } } ] },
+    [ year => $bare_cond ],
+    [ title => $bare_cond ],
+    { -and => [ { year => { '=', $c->{cond} } }, { title => { '=', $c->{cond} } } ] },
+    { -and => { -or => { year => { '=', $c->{cond} } } }, -or => { title => $bare_cond } },
+  );
+
+  if (my $v = is_plain_value($c->{cond})) {
+    push @query_steps,
+      { year => $v->[0] },
+      { title => $v->[0] },
+      { -and => [ year => $v->[0], title => $v->[0] ] },
+    ;
+  }
+
+  @query_steps = shuffle @query_steps;
+
+  $rs = $rs->search($_) for @query_steps;
+
+  my @bind = @{$c->{bind} || []};
+  {
+    no warnings 'misc';
+    splice @bind, 1, 0, [ { dbic_colname => "year", sqlt_datatype => "varchar", sqlt_size => 100 } => 'bar' ];
+  }
+
+  is_same_sql_bind (
+    $rs->as_query,
+    "(
+      SELECT me.title
+        FROM cd me
+      WHERE title != bar AND title $c->{sql} AND year != ? AND year $c->{sql}
+    )",
+    \@bind,
+    'Double condition correctly collapsed for steps' . Dumper \@query_steps,
+  );
+}
+
+done_testing;
diff --git a/t/sqlmaker/dbihacks_internals.t b/t/sqlmaker/dbihacks_internals.t
index 1f555fc..b15dc42 100644
--- a/t/sqlmaker/dbihacks_internals.t
+++ b/t/sqlmaker/dbihacks_internals.t
@@ -95,14 +95,16 @@ for my $t (
   },
   {
     where => { artistid => { '=' => [ 1 ], }, charfield => { '=' => [-and => \'1', \['?',2] ] }, rank => { '=' => [ $num, $num ] } },
-    cc_result => { artistid => 1, charfield => [-and => { '=' => \'1' }, { '=' => \['?',2] } ], rank => { '=' => [$num, $num] } },
+    cc_result => { artistid => 1, charfield => [ -and => { '=' => \['?',2] }, { '=' => \'1' } ], rank => { '=' => [$num, $num] } },
     sql => 'WHERE artistid = ? AND charfield = 1 AND charfield = ? AND ( rank = ? OR rank = ? )',
+    collapsed_sql => 'WHERE artistid = ? AND charfield = ? AND charfield = 1 AND ( rank = ? OR rank = ? )',
     efcc_result => { artistid => 1, charfield => UNRESOLVABLE_CONDITION },
   },
   {
     where => { -and => [ artistid => 1, artistid => 2 ], name => [ -and => { '!=', 1 }, 2 ], charfield => [ -or => { '=', 2 } ], rank => [-and => undef, { '=', undef }, { '!=', 2 } ] },
-    cc_result => { artistid => [ -and => 1, 2 ], name => [ -and => { '!=', 1 }, 2 ], charfield => 2, rank => [ -and => undef, undef, { '!=', 2 } ] },
+    cc_result => { artistid => [ -and => 1, 2 ], name => [ -and => { '!=', 1 }, 2 ], charfield => 2, rank => [ -and => { '!=', 2 }, undef ] },
     sql => 'WHERE artistid = ? AND artistid = ? AND charfield = ? AND name != ? AND name = ? AND rank IS NULL AND rank IS NULL AND rank != ?',
+    collapsed_sql => 'WHERE artistid = ? AND artistid = ? AND charfield = ? AND name != ? AND name = ? AND rank != ? AND rank IS NULL',
     efcc_result => {
       artistid => UNRESOLVABLE_CONDITION,
       name => 2,
@@ -194,8 +196,9 @@ for my $t (
   # batshit insanity, just to be thorough
   {
     where => { -and => [ [ 'artistid' ], [ -and => [ artistid => { '!=', 69 }, artistid => undef, artistid => { '=' => 200 } ]], artistid => [], { -or => [] }, { -and => [] }, [ 'charfield' ], { name => [] }, 'rank' ] },
-    cc_result => { artistid => [ -and => undef, { '!=', 69 }, undef, 200, [] ], charfield => undef, name => [], rank => undef },
+    cc_result => { artistid => [ -and => [], { '!=', 69 }, undef, 200  ], charfield => undef, name => [], rank => undef },
     sql => 'WHERE artistid IS NULL AND artistid != ? AND artistid IS NULL AND artistid = ? AND 0=1 AND charfield IS NULL AND 0=1 AND rank IS NULL',
+    collapsed_sql => 'WHERE 0=1 AND artistid != ? AND artistid IS NULL AND artistid = ? AND charfield IS NULL AND 0=1 AND rank IS NULL',
     efcc_result => { artistid => UNRESOLVABLE_CONDITION },
     efcc_n_result => { artistid => UNRESOLVABLE_CONDITION, charfield => undef, rank => undef },
   },
@@ -236,17 +239,17 @@ for my $t (
   ) {
     my $name = do { local ($Data::Dumper::Indent, $Data::Dumper::Terse, $Data::Dumper::Sortkeys) = (0, 1, 1); Dumper $w };
 
-    my @orig_sql_bind = $sm->where($w);
+    my ($generated_sql) = $sm->where($w);
 
-    is_same_sql ( $orig_sql_bind[0], $t->{sql}, "Expected SQL from $name" )
+    is_same_sql ( $generated_sql, $t->{sql}, "Expected SQL from $name" )
       if exists $t->{sql};
 
     my $collapsed_cond = $schema->storage->_collapse_cond($w);
 
-    is_same_sql_bind(
-      \[ $sm->where($collapsed_cond) ],
-      \\@orig_sql_bind,
-      "Collapse did not alter final SQL based on $name",
+    is_same_sql(
+      ($sm->where($collapsed_cond))[0],
+      ( $t->{collapsed_sql} || $t->{sql} || $generated_sql ),
+      "Collapse did not alter *the semantics* of the final SQL based on $name",
     );
 
     is_deeply(