From: Peter Rabbitson Date: Wed, 5 Jan 2011 23:27:05 +0000 (+0100) Subject: Fix default selection resolution - make frew happy :) X-Git-Tag: v0.08127~31 X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?p=dbsrgits%2FDBIx-Class.git;a=commitdiff_plain;h=eb37b376461257fc2faf281c905d14d8bb3a3757 Fix default selection resolution - make frew happy :) --- diff --git a/Changes b/Changes index 96478b4..99ba5c7 100644 --- a/Changes +++ b/Changes @@ -1,6 +1,8 @@ Revision history for DBIx::Class * Fixes + - Revert default selection to being lazy again (eagerness introduced + in 0.08126) - fixes DBIx::Class::Helper::ResultSet::RemoveColumns - Unaliased "dark" selectors no longer throw off prefetch - Fix proper composition of bind values across all possible SQL areas ( group_by => \[ ... ] now works properly ) diff --git a/lib/DBIx/Class/ResultSet.pm b/lib/DBIx/Class/ResultSet.pm index 554c93d..a8e468c 100644 --- a/lib/DBIx/Class/ResultSet.pm +++ b/lib/DBIx/Class/ResultSet.pm @@ -207,10 +207,6 @@ sub new { $attrs->{alias} ||= 'me'; - # default selection list - $attrs->{columns} = [ $source->resolve->columns ] - unless List::Util::first { exists $attrs->{$_} } qw/columns cols select as _trailing_select/; - # Creation of {} and bless separated to mitigate RH perl bug # see https://bugzilla.redhat.com/show_bug.cgi?id=196836 my $self = { @@ -320,9 +316,15 @@ sub search_rs { } my $call_attrs = {}; - $call_attrs = pop(@_) if ( - @_ > 1 and ( ! defined $_[-1] or ref $_[-1] eq 'HASH' ) - ); + if (@_ > 1) { + if (ref $_[-1] eq 'HASH') { + # copy for _normalize_selection + $call_attrs = { %{ pop @_ } }; + } + elsif (! defined $_[-1] ) { + pop @_; # search({}, undef) + } + } # see if we can keep the cache (no $rs changes) my $cache; @@ -343,33 +345,28 @@ sub search_rs { my $old_having = delete $old_attrs->{having}; my $old_where = delete $old_attrs->{where}; + my $new_attrs = { %$old_attrs }; - # start with blind overwriting merge - my $new_attrs = { %{$old_attrs}, %{$call_attrs} }; - - # join/prefetch use their own crazy merging heuristics - foreach my $key (qw/join prefetch/) { - $new_attrs->{$key} = $self->_merge_joinpref_attr($old_attrs->{$key}, $call_attrs->{$key}) - if exists $call_attrs->{$key}; - } - - # stack binds together - $new_attrs->{bind} = [ @{ $old_attrs->{bind} || [] }, @{ $call_attrs->{bind} || [] } ]; - - # take care of selects (only if anything changed) + # take care of call attrs (only if anything is changing) if (keys %$call_attrs) { $self->throw_exception ('_trailing_select is not a public attribute - do not use it in search()') - if exists $call_attrs->{_trailing_select}; + if ( exists $call_attrs->{_trailing_select} or exists $call_attrs->{'+_trailing_select'} ); + + my @selector_attrs = qw/select as columns cols +select +as +columns include_columns _trailing_select +_trailing_select/; + + # Normalize the selector list (operates on the passed-in attr structure) + # Need to do it on every chain instead of only once on _resolved_attrs, in + # order to separate 'as'-ed from blind 'select's + $self->_normalize_selection ($call_attrs); - my @selector_attrs = qw/select as columns cols +select +as +columns include_columns/; + # start with blind overwriting merge, exclude selector attrs + $new_attrs = { %{$old_attrs}, %{$call_attrs} }; + delete @{$new_attrs}{@selector_attrs}; # reset the current selector list if new selectors are supplied if (List::Util::first { exists $call_attrs->{$_} } qw/columns cols select as/) { - # the new/old acrobatics is because of the merger in the next loop - for ($new_attrs, $old_attrs) { - delete @{$_}{@selector_attrs, '_trailing_select'}; - } + delete @{$old_attrs}{@selector_attrs}; } for (@selector_attrs) { @@ -387,10 +384,15 @@ sub search_rs { } } - # Normalize the selector list (operates on the passed-in attr structure) - # Need to do it on every chain instead of only once on _resolved_attrs, in - # order to separate 'as'-ed from blind 'select's - $self->_normalize_selection ($new_attrs); + + # join/prefetch use their own crazy merging heuristics + foreach my $key (qw/join prefetch/) { + $new_attrs->{$key} = $self->_merge_joinpref_attr($old_attrs->{$key}, $call_attrs->{$key}) + if exists $call_attrs->{$key}; + } + + # stack binds together + $new_attrs->{bind} = [ @{ $old_attrs->{bind} || [] }, @{ $call_attrs->{bind} || [] } ]; } @@ -448,12 +450,14 @@ sub search_rs { sub _normalize_selection { my ($self, $attrs) = @_; - # merge all balanced selectors into the 'columns' stack, deleting the rest - foreach my $key (qw/+columns include_columns/) { - $attrs->{columns} = $self->_merge_attr($attrs->{columns}, delete $attrs->{$key}) - if exists $attrs->{$key}; - } + # legacy syntax + $attrs->{'+columns'} = $self->_merge_attr($attrs->{'+columns'}, delete $attrs->{include_columns}) + if exists $attrs->{include_columns}; + # Keep the X vs +X separation until _resolved_attrs time - this allows to + # delay the decision on whether to use a default select list ($rsrc->columns) + # allowing stuff like the remove_columns helper to work + # # select/as +select/+as pairs need special handling - the amount of select/as # elements in each pair does *not* have to be equal (think multicolumn # selectors like distinct(foo, bar) ). If the selector is bare (no 'as' @@ -507,7 +511,7 @@ sub _normalize_selection { } @$sel = @new_sel; - $attrs->{'_trailing_select'} = $self->_merge_attr($attrs->{'_trailing_select'}, \@new_trailing) + $attrs->{"${pref}_trailing_select"} = $self->_merge_attr($attrs->{"${pref}_trailing_select"}, \@new_trailing) if @new_trailing; } elsif (@$as < @$sel) { @@ -517,23 +521,21 @@ sub _normalize_selection { } # now see what the result for this pair looks like: - if (@$as == @$sel) { + # if balanced - treat as a columns entry - $attrs->{columns} = $self->_merge_attr( - $attrs->{columns}, + $attrs->{"${pref}columns"} = $self->_merge_attr( + $attrs->{"${pref}columns"}, { map { $as->[$_] => $sel->[$_] } ( 0 .. $#$as ) } ); } else { # unbalanced - shove in select/as, not subject to deduplication in _resolved_attrs - $attrs->{select} = $self->_merge_attr($attrs->{select}, $sel); - $attrs->{as} = $self->_merge_attr($attrs->{as}, $as); + $attrs->{"${pref}select"} = $self->_merge_attr($attrs->{"${pref}select"}, $sel); + $attrs->{"${pref}as"} = $self->_merge_attr($attrs->{"${pref}as"}, $as); } } - # simplify - delete $attrs->{$_} for grep { $attrs->{$_} and ! @{$attrs->{$_}} } qw/select as columns/; } sub _stack_cond { @@ -3214,8 +3216,18 @@ sub _resolved_attrs { my $source = $self->result_source; my $alias = $attrs->{alias}; - # take care of any selector merging - $self->_normalize_selection ($attrs); + # one last pass of normalization + $self->_normalize_selection($attrs); + + # default selection list + $attrs->{columns} = [ $source->columns ] + unless List::Util::first { exists $attrs->{$_} } qw/columns cols select as _trailing_select/; + + # merge selectors together + for (qw/columns select as _trailing_select/) { + $attrs->{$_} = $self->_merge_attr($attrs->{$_}, $attrs->{"+$_"}) + if $attrs->{$_} or $attrs->{"+$_"}; + } # disassemble columns my (@sel, @as); @@ -3383,7 +3395,7 @@ sub _resolved_attrs { } - push @sel, @{$attrs->{_trailing_select}} + push @{ $attrs->{select} }, @{$attrs->{_trailing_select}} if $attrs->{_trailing_select}; # if both page and offset are specified, produce a combined offset @@ -3543,6 +3555,7 @@ sub _merge_joinpref_attr { return [$_[0], @{$_[1]}] }, HASH => sub { + return [] if !defined $_[0] and !keys %{$_[1]}; return [ $_[1] ] if !defined $_[0]; return [ $_[0] ] if !keys %{$_[1]}; return [$_[0], $_[1]] @@ -3570,17 +3583,20 @@ sub _merge_joinpref_attr { }, HASH => { SCALAR => sub { + return [] if !keys %{$_[0]} and !defined $_[1]; return [ $_[0] ] if !defined $_[1]; return [ $_[1] ] if !keys %{$_[0]}; return [$_[0], $_[1]] }, ARRAY => sub { + return [] if !keys %{$_[0]} and !@{$_[1]}; return [ $_[0] ] if !@{$_[1]}; return $_[1] if !keys %{$_[0]}; return $_[1] if __HM_DEDUP and List::Util::first { $_ eq $_[0] } @{$_[1]}; return [ $_[0], @{$_[1]} ]; }, HASH => sub { + return [] if !keys %{$_[0]} and !keys %{$_[1]}; return [ $_[0] ] if !keys %{$_[1]}; return [ $_[1] ] if !keys %{$_[0]}; return [ $_[0] ] if $_[0] eq $_[1]; diff --git a/t/search/select_chains.t b/t/search/select_chains.t index 6adbca5..234439d 100644 --- a/t/search/select_chains.t +++ b/t/search/select_chains.t @@ -84,22 +84,36 @@ while (@chain) { } # Make sure we don't lose bits even with weird selector specs -$rs = $schema->resultset('CD')->search ({}, { - 'columns' => [ 'me.title' ], -})->search ({}, { - '+select' => \'me.year AS foo', -})->search ({}, { - '+select' => [ \'me.artistid AS bar' ], -})->search ({}, { - '+select' => { count => 'artistid', -as => 'baz' }, -}); +# also check that the default selector list is lazy +$rs = $schema->resultset('CD'); +for my $attr ( + { '+columns' => [ 'me.title' ] }, # this one should be de-duplicated but not the select's + + { '+select' => \'me.year AS foo' }, # duplication of identical select expected (FIXME ?) + { '+select' => \['me.year AS foo'] }, + + { '+select' => [ \'me.artistid AS bar' ] }, + { '+select' => { count => 'artistid', -as => 'baz' } }, +) { + for (qw/columns select as/) { + ok (! exists $rs->{attrs}{$_}, "No eager '$_' attr on fresh resultset" ); + } + + $rs = $rs->search({}, $attr); +} is_same_sql_bind ( $rs->as_query, '( SELECT + me.cdid, + me.artist, me.title, + me.year, + me.genreid, + me.single_track, COUNT( artistid ) AS baz, me.year AS foo, + me.year AS foo, me.artistid AS bar FROM cd me )',