From: Peter Rabbitson Date: Tue, 15 Feb 2011 11:43:33 +0000 (+0100) Subject: Revert the smart-select-ordering introduced in 36fd7f07 X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=f7f53a894c479aa594b9082ee461568fab8a81f3;p=dbsrgits%2FDBIx-Class-Historic.git Revert the smart-select-ordering introduced in 36fd7f07 While on its surface an awesome idea, it proced to be problematic: users of ->cursor and ->as_query expected that the result will have the same column-order as what they put in the arguments. Also users abuse +select as there's no tomorrow, which yielded interesting bugs. The current (and I hope final) behavior is: At the end of a chain, the resulting SQL will contain: - a merger of all columns and then all +columns attributes in the declared order. If columns is declared as a multiselect hashref i.e. { as1 => sel1, as2 => sel2 }, then the selectors for this segment are ordered alphabetically. De-duplication is also always performed on columns arguments - only the first set of identical select/as pairs is preserved - a merger of all select/as pairs, strict order, no deduplication - a merger of +select/+as pairs, same rules as above As a result of this prefetch on un-balanced resultsets now properly throws an exception --- diff --git a/Changes b/Changes index 02f436b..9b4fef0 100644 --- a/Changes +++ b/Changes @@ -5,6 +5,8 @@ Revision history for DBIx::Class sets quote_char and name_sep appropriate for your RDBMS - Add retrieve_on_insert column info flag, allowing to retrieve any column value instead of just autoinc primary keys + - Bring back strict ordering of selectors in complex search chains + (an ill-fated attempt was made in 0.08127 to order intelligently) - All limit dialects (except for the older Top and FetchFirst) are now using bind parameters for the limits/offsets, making DBI's prepare_cached useful across paged resutsets diff --git a/lib/DBIx/Class/ResultSet.pm b/lib/DBIx/Class/ResultSet.pm index 7e5a850..c4520f3 100644 --- a/lib/DBIx/Class/ResultSet.pm +++ b/lib/DBIx/Class/ResultSet.pm @@ -208,6 +208,12 @@ sub new { attrs => $attrs, }, $class; + # if there is a dark selector, this means we are already in a + # chain and the cleanup/sanification was taken care of by + # _search_rs already + $self->_normalize_selection($attrs) + unless $attrs->{_dark_selector}; + $self->result_class( $attrs->{result_class} || $source->result_class ); @@ -342,25 +348,24 @@ sub search_rs { # 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} or exists $call_attrs->{'+_trailing_select'} ); + my @selector_attrs = qw/select as columns cols +select +as +columns include_columns/; - my @selector_attrs = qw/select as columns cols +select +as +columns include_columns _trailing_select +_trailing_select/; + # reset the current selector list if new selectors are supplied + if (List::Util::first { exists $call_attrs->{$_} } qw/columns cols select as/) { + delete @{$old_attrs}{(@selector_attrs, '_dark_selector')}; + } - # Normalize the selector list (operates on the passed-in attr structure) + # Normalize the new 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 + # order to allow detection of empty vs partial 'as' + $call_attrs->{_dark_selector} = $old_attrs->{_dark_selector} + if $old_attrs->{_dark_selector}; $self->_normalize_selection ($call_attrs); # 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/) { - delete @{$old_attrs}{@selector_attrs}; - } - for (@selector_attrs) { $new_attrs->{$_} = $self->_merge_attr($old_attrs->{$_}, $call_attrs->{$_}) if ( exists $old_attrs->{$_} or exists $call_attrs->{$_} ); @@ -431,6 +436,7 @@ sub search_rs { return $rs; } +my $dark_sel_dumper; sub _normalize_selection { my ($self, $attrs) = @_; @@ -438,6 +444,8 @@ sub _normalize_selection { $attrs->{'+columns'} = $self->_merge_attr($attrs->{'+columns'}, delete $attrs->{include_columns}) if exists $attrs->{include_columns}; + # columns are always placed first, however + # 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 @@ -448,9 +456,7 @@ sub _normalize_selection { # supplied at all) - try to infer the alias, either from the -as parameter # of the selector spec, or use the parameter whole if it looks like a column # name (ugly legacy heuristic). If all fails - leave the selector bare (which - # is ok as well), but transport it over a separate attribute to make sure it is - # the last thing in the select list, thus unable to throw off the corresponding - # 'as' chain + # is ok as well), but make sure no more additions to the 'as' chain take place for my $pref ('', '+') { my ($sel, $as) = map { @@ -473,53 +479,51 @@ sub _normalize_selection { ); } elsif( ! @$as ) { - # no as part supplied at all - try to deduce + # no as part supplied at all - try to deduce (unless explicit end of named selection is declared) # if any @$as has been supplied we assume the user knows what (s)he is doing # and blindly keep stacking up pieces - my (@new_sel, @new_trailing); - for (@$sel) { - if ( ref $_ eq 'HASH' and exists $_->{-as} ) { - push @$as, $_->{-as}; - push @new_sel, $_; - } - # assume any plain no-space, no-parenthesis string to be a column spec - # FIXME - this is retarded but is necessary to support shit like 'count(foo)' - elsif ( ! ref $_ and $_ =~ /^ [^\s\(\)]+ $/x) { - push @$as, $_; - push @new_sel, $_; - } - # if all else fails - shove the selection to the trailing stack and move on - else { - push @new_trailing, $_; + unless ($attrs->{_dark_selector}) { + SELECTOR: + for (@$sel) { + if ( ref $_ eq 'HASH' and exists $_->{-as} ) { + push @$as, $_->{-as}; + } + # assume any plain no-space, no-parenthesis string to be a column spec + # FIXME - this is retarded but is necessary to support shit like 'count(foo)' + elsif ( ! ref $_ and $_ =~ /^ [^\s\(\)]+ $/x) { + push @$as, $_; + } + # if all else fails - raise a flag that no more aliasing will be allowed + else { + $attrs->{_dark_selector} = { + plus_stage => $pref, + string => ($dark_sel_dumper ||= do { + require Data::Dumper::Concise; + Data::Dumper::Concise::DumperObject()->Indent(0); + })->Values([$_])->Dump + , + }; + last SELECTOR; + } } } - - @$sel = @new_sel; - $attrs->{"${pref}_trailing_select"} = $self->_merge_attr($attrs->{"${pref}_trailing_select"}, \@new_trailing) - if @new_trailing; } elsif (@$as < @$sel) { $self->throw_exception( "Unable to handle an ${pref}as specification (@$as) with less elements than the corresponding ${pref}select" ); } - - # now see what the result for this pair looks like: - if (@$as == @$sel) { - - # if balanced - treat as a columns entry - $attrs->{"${pref}columns"} = $self->_merge_attr( - $attrs->{"${pref}columns"}, - [ map { +{ $as->[$_] => $sel->[$_] } } ( 0 .. $#$as ) ] + elsif ($pref and $attrs->{_dark_selector}) { + $self->throw_exception( + "Unable to process named '+select', resultset contains an unnamed selector $attrs->{_dark_selector}{string}" ); } - else { - # unbalanced - shove in select/as, not subject to deduplication in _resolved_attrs - $attrs->{"${pref}select"} = $self->_merge_attr($attrs->{"${pref}select"}, $sel); - $attrs->{"${pref}as"} = $self->_merge_attr($attrs->{"${pref}as"}, $as); - } - } + + # merge result + $attrs->{"${pref}select"} = $self->_merge_attr($attrs->{"${pref}select"}, $sel); + $attrs->{"${pref}as"} = $self->_merge_attr($attrs->{"${pref}as"}, $as); + } } sub _stack_cond { @@ -1432,7 +1436,7 @@ sub _count_rs { # overwrite the selector (supplied by the storage) $tmp_attrs->{select} = $rsrc->storage->_count_select ($rsrc, $attrs); $tmp_attrs->{as} = 'count'; - delete @{$tmp_attrs}{qw/columns _trailing_select/}; + delete @{$tmp_attrs}{qw/columns/}; my $tmp_rs = $rsrc->resultset_class->new($rsrc, $tmp_attrs)->get_column ('count'); @@ -1450,7 +1454,7 @@ sub _count_subq_rs { my $sub_attrs = { %$attrs }; # extra selectors do not go in the subquery and there is no point of ordering it, nor locking it - delete @{$sub_attrs}{qw/collapse columns as select _prefetch_selector_range _trailing_select order_by for/}; + delete @{$sub_attrs}{qw/collapse columns as select _prefetch_selector_range order_by for/}; # if we multi-prefetch we group_by primary keys only as this is what we would # get out of the rs via ->next/->all. We *DO WANT* to clobber old group_by regardless @@ -3235,16 +3239,13 @@ sub _resolved_attrs { my $source = $self->result_source; my $alias = $attrs->{alias}; - # 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/; + unless List::Util::first { exists $attrs->{$_} } qw/columns cols select as/; # merge selectors together - for (qw/columns select as _trailing_select/) { - $attrs->{$_} = $self->_merge_attr($attrs->{$_}, $attrs->{"+$_"}) + for (qw/columns select as/) { + $attrs->{$_} = $self->_merge_attr($attrs->{$_}, delete $attrs->{"+$_"}) if $attrs->{$_} or $attrs->{"+$_"}; } @@ -3360,11 +3361,10 @@ sub _resolved_attrs { } else { # distinct affects only the main selection part, not what prefetch may - # add below. However trailing is not yet a part of the selection as - # prefetch must insert before it + # add below. $attrs->{group_by} = $source->storage->_group_over_selection ( $attrs->{from}, - [ @{$attrs->{select}||[]}, @{$attrs->{_trailing_select}||[]} ], + $attrs->{select}, $attrs->{order_by}, ); } @@ -3372,6 +3372,10 @@ sub _resolved_attrs { $attrs->{collapse} ||= {}; if ($attrs->{prefetch}) { + + $self->throw_exception("Unable to prefetch, resultset contains an unnamed selector $attrs->{_dark_selector}{string}") + if $attrs->{_dark_selector}; + my $prefetch = $self->_merge_joinpref_attr( {}, delete $attrs->{prefetch} ); my $prefetch_ordering = []; @@ -3414,9 +3418,6 @@ sub _resolved_attrs { } - push @{ $attrs->{select} }, @{$attrs->{_trailing_select}} - if $attrs->{_trailing_select}; - # if both page and offset are specified, produce a combined offset # even though it doesn't make much sense, this is what pre 081xx has # been doing diff --git a/t/prefetch/correlated.t b/t/prefetch/correlated.t index fd5ef1d..e2b499b 100644 --- a/t/prefetch/correlated.t +++ b/t/prefetch/correlated.t @@ -85,12 +85,6 @@ is ($queries, 1, 'Only 1 query fired to retrieve everything'); $schema->storage->debug($orig_debug); $schema->storage->debugcb(undef); -# try to unbalance the select - -# first add a lone non-as-ed select -# it should be reordered to appear at the end without throwing prefetch/bind off -$c_rs = $c_rs->search({}, { '+select' => \[ 'me.cdid + ?', [ \ 'inTEger' => 1 ] ] }); - # now add an unbalanced select/as pair $c_rs = $c_rs->search ({}, { '+select' => $cdrs->search( @@ -103,7 +97,6 @@ $c_rs = $c_rs->search ({}, { '+as' => [qw/active_from active_to/], }); - is_same_sql_bind( $c_rs->as_query, '( @@ -120,8 +113,7 @@ is_same_sql_bind( WHERE siblings.artist = me.artist AND me.artist != ? ), - tracks.trackid, tracks.cd, tracks.position, tracks.title, tracks.last_updated_on, tracks.last_updated_at, - me.cdid + ? + tracks.trackid, tracks.cd, tracks.position, tracks.title, tracks.last_updated_on, tracks.last_updated_at FROM cd me LEFT JOIN track tracks ON tracks.cd = me.cdid @@ -141,10 +133,6 @@ is_same_sql_bind( [ { sqlt_datatype => 'integer', dbic_colname => 'me.artist' } => 2 ], - # the addition - [ { sqlt_datatype => 'inTEger' } - => 1 ], - # outher WHERE [ { sqlt_datatype => 'integer', dbic_colname => 'me.artist' } => 2 ], diff --git a/t/prefetch/with_limit.t b/t/prefetch/with_limit.t index 046678e..9012a9a 100644 --- a/t/prefetch/with_limit.t +++ b/t/prefetch/with_limit.t @@ -34,7 +34,7 @@ my $use_prefetch = $no_prefetch->search( } ); -# add a floating +select to make sure it does not throw things off +# add an extra +select to make sure it does not throw things off # we also expect it to appear in both selectors, as we can not know # for sure which part of the query it applies to (may be order_by, # maybe something else) @@ -43,7 +43,7 @@ my $use_prefetch = $no_prefetch->search( # is_deeply picks up this difference too (not sure if bug or # feature) $use_prefetch = $use_prefetch->search({}, { - '+select' => \[ 'me.artistid + ?', [ \ 'inTEger' => 1 ] ], + '+columns' => { monkeywrench => \[ 'me.artistid + ?', [ \ 'inTEger' => 1 ] ] }, }); my $bind_int_resolved = sub { [ { sqlt_datatype => 'inTEger' } => 1 ] }; @@ -54,12 +54,12 @@ my $bind_vc_resolved = sub { [ is_same_sql_bind ( $use_prefetch->as_query, '( - SELECT me.artistid, me.name, - cds.cdid, cds.artist, cds.title, cds.year, cds.genreid, cds.single_track, - me.artistid + ? + SELECT me.artistid + ?, + me.artistid, me.name, + cds.cdid, cds.artist, cds.title, cds.year, cds.genreid, cds.single_track FROM ( - SELECT me.artistid, me.name, - me.artistid + ? + SELECT me.artistid + ?, + me.artistid, me.name FROM artist me LEFT JOIN cd cds ON cds.artist = me.artistid @@ -69,7 +69,7 @@ is_same_sql_bind ( ON tracks.cd = cds.cdid WHERE artwork.cd_id IS NULL OR tracks.title != ? - GROUP BY me.artistid, me.name, me.artistid + ? + GROUP BY me.artistid + ?, me.artistid, me.name ORDER BY name DESC LIMIT ? ) me LEFT JOIN cd cds @@ -80,7 +80,7 @@ is_same_sql_bind ( ON tracks.cd = cds.cdid WHERE artwork.cd_id IS NULL OR tracks.title != ? - GROUP BY me.artistid, me.name, cds.cdid, cds.artist, cds.title, cds.year, cds.genreid, cds.single_track, me.artistid + ? + GROUP BY me.artistid + ?, me.artistid, me.name, cds.cdid, cds.artist, cds.title, cds.year, cds.genreid, cds.single_track ORDER BY name DESC, cds.artist, cds.year ASC )', [ diff --git a/t/search/select_chains.t b/t/search/select_chains.t index e039fb7..e57fc26 100644 --- a/t/search/select_chains.t +++ b/t/search/select_chains.t @@ -22,7 +22,7 @@ my @chain = ( '+columns' => [ { max_year => { max => 'me.year', -as => 'last_y' }}, ], '+select' => [ { count => 'me.cdid' }, ], '+as' => [ 'cnt' ], - } => 'SELECT me.cdid, LOWER( title ) AS lctitle, me.genreid, MAX( me.year ) AS last_y, COUNT( me.cdid ) FROM cd me', + } => 'SELECT me.cdid, LOWER( title ) AS lctitle, MAX( me.year ) AS last_y, me.genreid, COUNT( me.cdid ) FROM cd me', { select => [ { min => 'me.cdid' }, ], @@ -31,7 +31,7 @@ my @chain = ( { '+columns' => [ { cnt => { count => 'cdid', -as => 'cnt' } } ], - } => 'SELECT MIN( me.cdid ), COUNT ( cdid ) AS cnt FROM cd me', + } => 'SELECT COUNT ( cdid ) AS cnt, MIN( me.cdid ) FROM cd me', { columns => [ { foo => { coalesce => [qw/a b c/], -as => 'firstfound' } } ], @@ -52,15 +52,24 @@ my @chain = ( { '+select' => [ 'me.year' ], '+as' => [ 'year' ], - } => 'SELECT COALESCE( a, b, c ) AS firstfound, me.year, MAX( me.year ) AS last_y, COUNT( me.cdid ) AS cnt FROM cd me', + } => 'SELECT COALESCE( a, b, c ) AS firstfound, me.year, MAX( me.year ) AS last_y, COUNT( me.cdid ) AS cnt, me.year FROM cd me', { '+columns' => [ 'me.year' ], - } => 'SELECT COALESCE( a, b, c ) AS firstfound, me.year, MAX( me.year ) AS last_y, COUNT( me.cdid ) AS cnt FROM cd me', + } => 'SELECT COALESCE( a, b, c ) AS firstfound, me.year, MAX( me.year ) AS last_y, COUNT( me.cdid ) AS cnt, me.year FROM cd me', { '+columns' => 'me.year', - } => 'SELECT COALESCE( a, b, c ) AS firstfound, me.year, MAX( me.year ) AS last_y, COUNT( me.cdid ) AS cnt FROM cd me', + } => 'SELECT COALESCE( a, b, c ) AS firstfound, me.year, MAX( me.year ) AS last_y, COUNT( me.cdid ) AS cnt, me.year FROM cd me', + + # naked selector at the end should just work + { + '+select' => 'me.moar_stuff', + } => 'SELECT COALESCE( a, b, c ) AS firstfound, me.year, MAX( me.year ) AS last_y, COUNT( me.cdid ) AS cnt, me.year, me.moar_stuff FROM cd me', + + { + '+select' => [ { MOAR => 'f', -as => 'func' } ], + } => 'SELECT COALESCE( a, b, c ) AS firstfound, me.year, MAX( me.year ) AS last_y, COUNT( me.cdid ) AS cnt, me.year, me.moar_stuff, MOAR(f) AS func FROM cd me', ); @@ -85,6 +94,7 @@ while (@chain) { # Make sure we don't lose bits even with weird selector specs # also check that the default selector list is lazy +# and make sure that unaliased +select does not go crazy $rs = $schema->resultset('CD'); for my $attr ( { '+columns' => [ 'me.title' ] }, # this one should be de-duplicated but not the select's @@ -111,10 +121,10 @@ is_same_sql_bind ( me.year, me.genreid, me.single_track, - COUNT( artistid ) AS baz, me.year AS foo, me.year AS foo, - me.artistid AS bar + me.artistid AS bar, + COUNT( artistid ) AS baz FROM cd me )', [], @@ -137,4 +147,29 @@ is_same_sql_bind ( 'Correct order of selected columns' ); +# Test bare +select with as from root of resultset +$rs = $schema->resultset('CD')->search ({}, { + '+select' => [ + \ 'foo', + { MOAR => 'f', -as => 'func' }, + ], +}); + +is_same_sql_bind ( + $rs->as_query, + '( SELECT + me.cdid, + me.artist, + me.title, + me.year, + me.genreid, + me.single_track, + foo, + MOAR( f ) AS func + FROM cd me + )', + [], + 'Correct order of selected columns' +); + done_testing; diff --git a/t/search/select_chains_unbalanced.t b/t/search/select_chains_unbalanced.t index d602605..99dcc00 100644 --- a/t/search/select_chains_unbalanced.t +++ b/t/search/select_chains_unbalanced.t @@ -2,6 +2,7 @@ use strict; use warnings; use Test::More; +use Test::Exception; use lib qw(t/lib); use DBIC::SqlMakerTest; @@ -14,63 +15,77 @@ my $multicol_rs = $schema->resultset('Artist')->search({ artistid => \'1' }, { c my @chain = ( { - select => 'title', - as => 'title', - columns => [ 'cdid' ], + select => 'cdid', + as => 'cd_id', + columns => [ 'title' ], } => 'SELECT - me.cdid, - me.title + me.title, + me.cdid FROM cd me' - => [qw/cdid title/], + => [qw/title cd_id/], { '+select' => \ 'DISTINCT(foo, bar)', '+as' => [qw/foo bar/], } => 'SELECT - me.cdid, me.title, + me.cdid, DISTINCT(foo, bar) FROM cd me' - => [qw/cdid title foo bar/], + => [qw/title cd_id foo bar/], { - '+select' => \'unaliased randomness', - } => 'SELECT - me.cdid, - me.title, - DISTINCT(foo, bar), - unaliased randomness - FROM cd me' - => [qw/cdid title foo bar/], - { '+select' => [ 'genreid', $multicol_rs->as_query ], '+as' => [qw/genreid name rank/], } => 'SELECT - me.cdid, me.title, + me.cdid, DISTINCT(foo, bar), me.genreid, (SELECT me.name, me.rank FROM artist me WHERE ( artistid 1 )), - unaliased randomness FROM cd me' - => [qw/cdid title foo bar genreid name rank/], + => [qw/title cd_id foo bar genreid name rank/], { '+select' => { count => 'me.cdid', -as => 'cnt' }, # lack of 'as' infers from '-as' '+columns' => { len => { length => 'me.title' } }, } => 'SELECT - me.cdid, me.title, LENGTH( me.title ), + me.cdid, + DISTINCT(foo, bar), + me.genreid, + (SELECT me.name, me.rank FROM artist me WHERE ( artistid 1 )), COUNT( me.cdid ) AS cnt, + FROM cd me' + => [qw/title len cd_id foo bar genreid name rank cnt/], + { + '+select' => \'unaliased randomness', + } => 'SELECT + me.title, + LENGTH( me.title ), + me.cdid, DISTINCT(foo, bar), me.genreid, (SELECT me.name, me.rank FROM artist me WHERE ( artistid 1 )), + COUNT( me.cdid ) AS cnt, unaliased randomness FROM cd me' - => [qw/cdid title len cnt foo bar genreid name rank/], - - + => [qw/title len cd_id foo bar genreid name rank cnt/], + { + '+select' => \'MOAR unaliased randomness', + } => 'SELECT + me.title, + LENGTH( me.title ), + me.cdid, + DISTINCT(foo, bar), + me.genreid, + (SELECT me.name, me.rank FROM artist me WHERE ( artistid 1 )), + COUNT( me.cdid ) AS cnt, + unaliased randomness, + MOAR unaliased randomness + FROM cd me' + => [qw/title len cd_id foo bar genreid name rank cnt/], ); my $rs = $schema->resultset('CD'); @@ -99,4 +114,26 @@ while (@chain) { $testno++; } +# make sure proper exceptions are thrown on unbalanced use +{ + my $rs = $schema->resultset('CD')->search({}, { select => \'count(me.cdid)'}); + + lives_ok(sub { + $rs->search({}, { '+select' => 'me.cdid' })->next + }, 'Two dark selectors are ok'); + + throws_ok(sub { + $rs->search({}, { '+select' => 'me.cdid', '+as' => 'cdid' })->next + }, qr/resultset contains an unnamed selector/, 'Unnamed followed by named is not'); + + throws_ok(sub { + $rs->search_rs({}, { prefetch => 'tracks' })->next + }, qr/resultset contains an unnamed selector/, 'Throw on unaliased selector followed by prefetch'); + + throws_ok(sub { + $rs->search_rs({}, { '+select' => 'me.title', '+as' => 'title' })->next + }, qr/resultset contains an unnamed selector/, 'Throw on unaliased selector followed by +select/+as'); +} + + done_testing;