From: Peter Rabbitson Date: Sat, 12 Jul 2014 10:32:20 +0000 (+0200) Subject: Refactor ::DBIHacks::_extract_fixed_condition_columns (sequel to 8d005ad9) X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=8e40a627f;p=dbsrgits%2FDBIx-Class-Historic.git Refactor ::DBIHacks::_extract_fixed_condition_columns (sequel to 8d005ad9) Instead of just returning an array of column names, switch to a hashref that can double as an inferred-value bag. As a bonus deduplicates and folds-in another codepath from ::ResultSet::_merge_with_rscond In the process fixup _collapse_cond to be even more robust in some arrayref corner-cases. Now it is guaranteed to return a hashref at all times --- diff --git a/lib/DBIx/Class/ResultSet.pm b/lib/DBIx/Class/ResultSet.pm index f817d3a..f71bf38 100644 --- a/lib/DBIx/Class/ResultSet.pm +++ b/lib/DBIx/Class/ResultSet.pm @@ -7,7 +7,7 @@ use DBIx::Class::Carp; use DBIx::Class::ResultSetColumn; use Scalar::Util qw/blessed weaken reftype/; use DBIx::Class::_Util qw( - fail_on_internal_wantarray is_plain_value is_literal_value UNRESOLVABLE_CONDITION + fail_on_internal_wantarray UNRESOLVABLE_CONDITION ); use Try::Tiny; use Data::Compare (); # no imports!!! guard against insane architecture @@ -2485,7 +2485,7 @@ sub new_result { sub _merge_with_rscond { my ($self, $data) = @_; - my (%new_data, @cols_from_relations); + my ($implied_data, @cols_from_relations); my $alias = $self->{attrs}{alias}; @@ -2493,43 +2493,25 @@ sub _merge_with_rscond { # just massage $data below } elsif ($self->{cond} eq UNRESOLVABLE_CONDITION) { - %new_data = %{ $self->{attrs}{related_objects} || {} }; # nothing might have been inserted yet - @cols_from_relations = keys %new_data; - } - elsif (ref $self->{cond} ne 'HASH') { - $self->throw_exception( - "Can't abstract implicit construct, resultset condition not a hash" - ); + $implied_data = $self->{attrs}{related_objects}; # nothing might have been inserted yet + @cols_from_relations = keys %{ $implied_data || {} }; } else { - if ($self->{cond}) { - my $implied = $self->_remove_alias( - $self->result_source->schema->storage->_collapse_cond($self->{cond}), - $alias, - ); - - for my $c (keys %$implied) { - my $v = $implied->{$c}; - if ( ! length ref $v or is_plain_value($v) ) { - $new_data{$c} = $v; - } - elsif ( - ref $v eq 'HASH' and keys %$v == 1 and exists $v->{'='} and is_literal_value($v->{'='}) - ) { - $new_data{$c} = $v->{'='}; - } - } - } + my $eqs = $self->result_source->schema->storage->_extract_fixed_condition_columns($self->{cond}, 'consider_nulls'); + $implied_data = { map { + ( ($eqs->{$_}||'') eq UNRESOLVABLE_CONDITION ) ? () : ( $_ => $eqs->{$_} ) + } keys %$eqs }; } - # precedence must be given to passed values over values inherited from - # the cond, so the order here is important. - %new_data = ( - %new_data, - %{ $self->_remove_alias($data, $alias) }, + return ( + { map + { %{ $self->_remove_alias($_, $alias) } } + # precedence must be given to passed values over values inherited from + # the cond, so the order here is important. + ( $implied_data||(), $data) + }, + \@cols_from_relations ); - - return (\%new_data, \@cols_from_relations); } # _has_resolved_attr diff --git a/lib/DBIx/Class/ResultSource.pm b/lib/DBIx/Class/ResultSource.pm index 81ce7d8..886f47e 100644 --- a/lib/DBIx/Class/ResultSource.pm +++ b/lib/DBIx/Class/ResultSource.pm @@ -9,7 +9,7 @@ use DBIx::Class::ResultSet; use DBIx::Class::ResultSourceHandle; use DBIx::Class::Carp; -use DBIx::Class::_Util qw(is_literal_value UNRESOLVABLE_CONDITION); +use DBIx::Class::_Util 'UNRESOLVABLE_CONDITION'; use Devel::GlobalDestruction; use Try::Tiny; use List::Util 'first'; @@ -1834,14 +1834,13 @@ sub _resolve_relationship_condition { } # see which parts of the joinfree cond are *NOT* foreign-source-column equalities - my $joinfree_cond_equality_columns = { map - {( $_ => 1 )} - @{ $self->schema->storage->_extract_fixed_condition_columns($joinfree_cond) } - }; + my $joinfree_cond_equality_columns = + $self->schema->storage->_extract_fixed_condition_columns($joinfree_cond, 'consider_nulls'); + @nonvalue_cols = map { $_ =~ /^\Q$joinfree_alias.\E(.+)/ } grep - { ! $joinfree_cond_equality_columns->{$_} } + { ! exists $joinfree_cond_equality_columns->{$_} } keys %$joinfree_cond; return ($joinfree_cond, 0, (@nonvalue_cols ? \@nonvalue_cols : undef)); diff --git a/lib/DBIx/Class/Storage/DBIHacks.pm b/lib/DBIx/Class/Storage/DBIHacks.pm index d78ab74..ae04942 100644 --- a/lib/DBIx/Class/Storage/DBIHacks.pm +++ b/lib/DBIx/Class/Storage/DBIHacks.pm @@ -16,7 +16,7 @@ use mro 'c3'; use List::Util 'first'; use Scalar::Util 'blessed'; use Sub::Name 'subname'; -use DBIx::Class::_Util qw(is_plain_value is_literal_value); +use DBIx::Class::_Util qw(is_plain_value is_literal_value UNRESOLVABLE_CONDITION); use namespace::clean; # @@ -887,7 +887,7 @@ sub _order_by_is_stable { my @cols = ( ( map { $_->[0] } $self->_extract_order_criteria($order_by) ), - ( $where ? @{ $self->_extract_fixed_condition_columns($where) || [] } : () ), + ( $where ? keys %{ $self->_extract_fixed_condition_columns($where) } : () ), ) or return 0; my $colinfo = $self->_resolve_column_info($ident, \@cols); @@ -965,7 +965,7 @@ sub _extract_colinfo_of_stable_main_source_order_by_portion { ? $colinfos->{$_}{-colname} : () } - @{$self->_extract_fixed_condition_columns($attrs->{where}) || []} + keys %{ $self->_extract_fixed_condition_columns($attrs->{where}) } ) : () ]; @@ -1074,10 +1074,7 @@ sub _collapse_cond { : { $w[0] => undef } ; } - elsif ( ref $w[0] ) { - return \@w; - } - elsif ( @w == 2 ) { + elsif ( @w == 2 and ! ref $w[0]) { if ( ( $w[0]||'' ) =~ /^\-and$/i ) { return (ref $w[1] eq 'HASH' or ref $w[1] eq 'ARRAY') ? $self->_collapse_cond($w[1], (ref $w[1] eq 'ARRAY') ) @@ -1088,14 +1085,16 @@ sub _collapse_cond { return $self->_collapse_cond({ @w }); } } + else { + return { -or => \@w }; + } } else { # not a hash not an array return { '' => $where }; } - # catchall, some of the things above fall through - return $where; + die 'should not get here'; } sub _collapse_cond_unroll_pairs { @@ -1179,48 +1178,72 @@ sub _collapse_cond_unroll_pairs { return @conds; } - -# returns an arrayref of column names which *definitely* have some -# sort of non-nullable *single* equality requested in the given condition -# specification. This is used to figure out if a resultset is -# constrained to a column which is part of a unique constraint, -# which in turn allows us to better predict how ordering will behave -# etc. +# Analyzes a given condition and attempts to extract all columns +# with a definitive fixed-condition criteria. Returns a hashref +# of k/v pairs suitable to be passed to set_columns(), with a +# MAJOR CAVEAT - multi-value (contradictory) equalities are still +# represented as a reference to the UNRESOVABLE_CONDITION constant +# The reason we do this is that some codepaths only care about the +# codition being stable, as opposed to actually making sense # -# this is a rudimentary, incomplete, and error-prone extractor -# however this is OK - it is conservative, and if we can not find -# something that is in fact there - the stack will recover gracefully +# The normal mode is used to figure out if a resultset is constrained +# to a column which is part of a unique constraint, which in turn +# allows us to better predict how ordering will behave etc. +# +# With the optional "consider_nulls" boolean argument, the function +# is instead used to infer inambiguous values from conditions +# (e.g. the inheritance of resultset conditions on new_result) +# +my $undef_marker = \ do{ my $x = 'undef' }; sub _extract_fixed_condition_columns { - my $self = shift; - my $where_hash = $self->_collapse_cond(shift); - - my $res; - for my $c (keys %$where_hash) { - if (defined (my $v = $where_hash->{$c}) ) { - if ( - ! length ref $v - or - is_plain_value ($v) - or - ( - ref $v eq 'HASH' - and - keys %$v == 1 - and - ref $v->{'='} - and - is_literal_value($v->{'='}) - ) - ) { - $res->{$c} = 1; - } - elsif (ref $v eq 'ARRAY' and ($v->[0]||'') eq '-and') { - $res->{$_} = 1 for map { @{ $self->_extract_fixed_condition_columns({ $c => $_ }) } } @{$v}[1..$#$v]; + my ($self, $where, $consider_nulls) = @_; + my $where_hash = $self->_collapse_cond($_[1]); + + my $res = {}; + my ($c, $v); + for $c (keys %$where_hash) { + my $vals; + + if (!defined ($v = $where_hash->{$c}) ) { + $vals->{$undef_marker} = $v if $consider_nulls + } + elsif ( + ! length ref $v + or + is_plain_value ($v) + ) { + $vals->{$v} = $v; + } + elsif ( + ref $v eq 'HASH' + and + keys %$v == 1 + and + ref $v->{'='} + and + # do not need to check for plain values - _collapse_cond did it for us + is_literal_value($v->{'='}) + ) { + $vals->{$v->{'='}} = $v->{'='}; + } + elsif (ref $v eq 'ARRAY' and ($v->[0]||'') eq '-and') { + for ( @{$v}[1..$#$v] ) { + my $subval = $self->_extract_fixed_condition_columns({ $c => $_ }, 'consider nulls'); # always fish nulls out on recursion + next unless exists $subval->{$c}; # didn't find anything + $vals->{defined $subval->{$c} ? $subval->{$c} : $undef_marker} = $subval->{$c}; } } + + if (keys %$vals == 1) { + ($res->{$c}) = (values %$vals) + unless !$consider_nulls and exists $vals->{$undef_marker}; + } + elsif (keys %$vals > 1) { + $res->{$c} = UNRESOLVABLE_CONDITION; + } } - return [ sort keys %$res ]; + $res; } 1; diff --git a/t/sqlmaker/dbihacks_internals.t b/t/sqlmaker/dbihacks_internals.t index 81deb89..66f0148 100644 --- a/t/sqlmaker/dbihacks_internals.t +++ b/t/sqlmaker/dbihacks_internals.t @@ -5,6 +5,7 @@ use Test::Warn; use lib qw(t/lib); use DBICTest ':DiffSQL'; +use DBIx::Class::_Util 'UNRESOLVABLE_CONDITION'; use Data::Dumper; @@ -30,67 +31,89 @@ for my $t ( where => { artistid => 1, charfield => undef }, cc_result => { artistid => 1, charfield => undef }, sql => 'WHERE artistid = ? AND charfield IS NULL', - efcc_result => [qw( artistid )], + efcc_result => { artistid => 1 }, + efcc_n_result => { artistid => 1, charfield => undef }, }, { where => { -and => [ artistid => 1, charfield => undef, { rank => 13 } ] }, cc_result => { artistid => 1, charfield => undef, rank => 13 }, sql => 'WHERE artistid = ? AND charfield IS NULL AND rank = ?', - efcc_result => [qw( artistid rank )], + efcc_result => { artistid => 1, rank => 13 }, + efcc_n_result => { artistid => 1, charfield => undef, rank => 13 }, }, { where => { -and => [ { artistid => 1, charfield => undef}, { rank => 13 } ] }, cc_result => { artistid => 1, charfield => undef, rank => 13 }, sql => 'WHERE artistid = ? AND charfield IS NULL AND rank = ?', - efcc_result => [qw( artistid rank )], + efcc_result => { artistid => 1, rank => 13 }, + efcc_n_result => { artistid => 1, charfield => undef, rank => 13 }, }, { where => { -and => [ -or => { name => 'Caterwauler McCrae' }, 'rank' ] }, cc_result => { name => 'Caterwauler McCrae', rank => undef }, sql => 'WHERE name = ? AND rank IS NULL', - efcc_result => [qw( name )], + efcc_result => { name => 'Caterwauler McCrae' }, + efcc_n_result => { name => 'Caterwauler McCrae', rank => undef }, }, { where => { -and => [ [ [ artist => {'=' => \'foo' } ] ], { name => \[ '= ?', 'bar' ] } ] }, cc_result => { artist => {'=' => \'foo' }, name => \[ '= ?', 'bar' ] }, sql => 'WHERE artist = foo AND name = ?', - efcc_result => [qw( artist )], + efcc_result => { artist => \'foo' }, }, { where => { -and => [ -or => { name => 'Caterwauler McCrae', artistid => 2 } ] }, cc_result => { -or => [ artistid => 2, name => 'Caterwauler McCrae' ] }, sql => 'WHERE artistid = ? OR name = ?', - efcc_result => [], + efcc_result => {}, + }, + { + where => { -or => { name => 'Caterwauler McCrae', artistid => 2 } }, + cc_result => { -or => [ artistid => 2, name => 'Caterwauler McCrae' ] }, + sql => 'WHERE artistid = ? OR name = ?', + efcc_result => {}, }, { where => { -and => [ \'foo=bar', [ { artistid => { '=', $num } } ], { name => 'Caterwauler McCrae'} ] }, cc_result => { '' => \'foo=bar', name => 'Caterwauler McCrae', artistid => $num }, sql => 'WHERE foo=bar AND artistid = ? AND name = ?', - efcc_result => [qw( artistid name )], + efcc_result => { name => 'Caterwauler McCrae', artistid => $num }, }, { where => { artistid => [ $num ], rank => [ 13, 2, 3 ], charfield => [ undef ] }, cc_result => { artistid => $num, charfield => undef, rank => [13, 2, 3] }, sql => 'WHERE artistid = ? AND charfield IS NULL AND ( rank = ? OR rank = ? OR rank = ? )', - efcc_result => [qw( artistid )], + efcc_result => { artistid => $num }, + efcc_n_result => { artistid => $num, charfield => undef }, }, { where => { artistid => { '=' => 1 }, rank => { '>' => 12 }, charfield => { '=' => undef } }, cc_result => { artistid => 1, charfield => undef, rank => { '>' => 12 } }, sql => 'WHERE artistid = ? AND charfield IS NULL AND rank > ?', - efcc_result => [qw( artistid )], + efcc_result => { artistid => 1 }, + efcc_n_result => { artistid => 1, charfield => undef }, }, { where => { artistid => { '=' => [ 1 ], }, charfield => { '=' => [-and => \'1', \['?',2] ] }, rank => { '=' => [ $num, $num ] } }, cc_result => { artistid => 1, charfield => [-and => { '=' => \'1' }, { '=' => \['?',2] } ], rank => { '=' => [$num, $num] } }, sql => 'WHERE artistid = ? AND charfield = 1 AND charfield = ? AND ( rank = ? OR rank = ? )', - efcc_result => [qw( artistid charfield )], + 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 } ] }, sql => 'WHERE artistid = ? AND artistid = ? AND charfield = ? AND name != ? AND name = ? AND rank IS NULL AND rank IS NULL AND rank != ?', - efcc_result => [qw( artistid charfield name )], + efcc_result => { + artistid => UNRESOLVABLE_CONDITION, + name => 2, + charfield => 2, + }, + efcc_n_result => { + artistid => UNRESOLVABLE_CONDITION, + name => 2, + charfield => 2, + rank => undef, + }, }, { where => { -and => [ @@ -106,24 +129,24 @@ for my $t ( ], }, sql => 'WHERE ( _macro.to LIKE ? OR _wc_macros.to LIKE ? ) AND group.is_active = ? AND me.is_active = ?', - efcc_result => [qw( group.is_active me.is_active )], + efcc_result => { 'group.is_active' => 1, 'me.is_active' => 1 }, }, { where => { artistid => [] }, cc_result => { artistid => [] }, - efcc_result => [], + efcc_result => {}, }, (map { { where => { -and => $_ }, cc_result => undef, - efcc_result => [], + efcc_result => {}, sql => '', }, { where => { -or => $_ }, cc_result => undef, - efcc_result => [], + efcc_result => {}, sql => '', }, } ( @@ -138,14 +161,15 @@ for my $t ( )), # FIXME legacy compat crap, possibly worth undef/dieing in SQLMaker - { where => { artistid => {} }, sql => '', cc_result => undef, efcc_result => [] }, + { where => { artistid => {} }, sql => '', cc_result => undef, efcc_result => {}, efcc_n_result => {} }, # 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 }, 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', - efcc_result => [qw( artistid )], + efcc_result => { artistid => UNRESOLVABLE_CONDITION }, + efcc_n_result => { artistid => UNRESOLVABLE_CONDITION, charfield => undef, rank => undef }, }, # original test from RT#93244 @@ -166,14 +190,21 @@ for my $t ( 'me.title' => 'Spoonful of bees', }, sql => 'WHERE LOWER(me.title) LIKE ? AND me.title = ?', - efcc_result => [qw( me.title )], + efcc_result => { 'me.title' => 'Spoonful of bees' }, } ) { for my $w ( $t->{where}, [ -and => $t->{where} ], - ( keys %{$t->{where}} <= 1 ) ? [ %{$t->{where}} ] : () + ( keys %{$t->{where}} <= 1 ? [ %{$t->{where}} ] : () ), + ( (keys %{$t->{where}} == 1 and $t->{where}{-or}) + ? ( ref $t->{where}{-or} eq 'HASH' + ? [ map { $_ => $t->{where}{-or}{$_} } sort keys %{$t->{where}{-or}} ] + : $t->{where}{-or} + ) + : () + ), ) { my $name = do { local ($Data::Dumper::Indent, $Data::Dumper::Terse, $Data::Dumper::Sortkeys) = (0, 1, 1); Dumper $w }; @@ -201,6 +232,12 @@ for my $t ( $t->{efcc_result}, "Expected fixed_condition produced on $name", ); + + is_deeply( + $schema->storage->_extract_fixed_condition_columns($w, 'consider_nulls'), + $t->{efcc_n_result}, + "Expected fixed_condition including NULLs produced on $name", + ) if $t->{efcc_n_result}; } }