From: Peter Rabbitson Date: Fri, 22 Aug 2014 11:40:14 +0000 (+0200) Subject: Deduplicate (and stabilize) the result of _collapse_cond X-Git-Tag: v0.082800~72 X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=5268b1da;p=dbsrgits%2FDBIx-Class.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(