From: Peter Rabbitson Date: Fri, 19 Sep 2014 08:32:10 +0000 (+0200) Subject: Properly handle empty group_by/order_by X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=d16df2398243321f1bd43fcc625d2e14852af0c9;p=dbsrgits%2FDBIx-Class-Historic.git Properly handle empty group_by/order_by --- diff --git a/lib/DBIx/Class/ResultSet.pm b/lib/DBIx/Class/ResultSet.pm index 97417fa..a699745 100644 --- a/lib/DBIx/Class/ResultSet.pm +++ b/lib/DBIx/Class/ResultSet.pm @@ -3614,18 +3614,18 @@ sub _resolved_attrs { ]; } - if ( defined $attrs->{order_by} ) { - $attrs->{order_by} = ( - ref( $attrs->{order_by} ) eq 'ARRAY' - ? [ @{ $attrs->{order_by} } ] - : [ $attrs->{order_by} || () ] - ); - } + for my $attr (qw(order_by group_by)) { - if ($attrs->{group_by} and ref $attrs->{group_by} ne 'ARRAY') { - $attrs->{group_by} = [ $attrs->{group_by} ]; - } + if ( defined $attrs->{$attr} ) { + $attrs->{$attr} = ( + ref( $attrs->{$attr} ) eq 'ARRAY' + ? [ @{ $attrs->{$attr} } ] + : [ $attrs->{$attr} || () ] + ); + delete $attrs->{$attr} unless @{$attrs->{$attr}}; + } + } # generate selections based on the prefetch helper my ($prefetch, @prefetch_select, @prefetch_as); diff --git a/t/search/empty_attrs.t b/t/search/empty_attrs.t new file mode 100644 index 0000000..3b52487 --- /dev/null +++ b/t/search/empty_attrs.t @@ -0,0 +1,51 @@ +use strict; +use warnings; + +use Test::More; + +use lib qw(t/lib); +use DBICTest ':DiffSQL'; + +my $schema = DBICTest->init_schema(); + +my $rs = $schema->resultset('Artist')->search( + [ -and => [ {}, [] ], -or => [ {}, [] ] ], + { + select => [], + columns => {}, + '+columns' => 'artistid', + join => [ {}, [ [ {}, {} ] ], {} ], + prefetch => [ [ [ {}, [] ], {} ], {}, [ {} ] ], + order_by => [], + group_by => [], + offset => 0, + } +); + +is_same_sql_bind( + $rs->as_query, + '(SELECT me.artistid FROM artist me)', + [], +); + +is_same_sql_bind( + $rs->count_rs->as_query, + '(SELECT COUNT(*) FROM artist me)', + [], +); + +is_same_sql_bind( + $rs->as_subselect_rs->search({}, { columns => 'artistid' })->as_query, + '(SELECT me.artistid FROM (SELECT me.artistid FROM artist me) me)', + [], +); + +{ + local $TODO = 'Stupid misdesigned as_subselect_rs'; + is_same_sql_bind( + $rs->as_subselect_rs->as_query, + $rs->as_subselect_rs->search({}, { columns => 'artistid' })->as_query, + ); +} + +done_testing; diff --git a/t/sqlmaker/limit_dialects/torture.t b/t/sqlmaker/limit_dialects/torture.t index c14ea60..9d8d23d 100644 --- a/t/sqlmaker/limit_dialects/torture.t +++ b/t/sqlmaker/limit_dialects/torture.t @@ -37,6 +37,12 @@ my @order_bind = ( my $tests = { LimitOffset => { + limit_plain => [ + "( SELECT me.artistid FROM artist me LIMIT ? )", + [ + [ { sqlt_datatype => 'integer' } => 5 ] + ], + ], limit => [ "( SELECT me.id, owner.id, owner.name, ? * ?, ? @@ -140,6 +146,12 @@ my $tests = { }, LimitXY => { + limit_plain => [ + "( SELECT me.artistid FROM artist me LIMIT ? )", + [ + [ { sqlt_datatype => 'integer' } => 5 ] + ], + ], ordered_limit_offset => [ "( SELECT me.id, owner.id, owner.name, ? * ?, ? @@ -181,6 +193,12 @@ my $tests = { }, SkipFirst => { + limit_plain => [ + "( SELECT FIRST ? me.artistid FROM artist me )", + [ + [ { sqlt_datatype => 'integer' } => 5 ] + ], + ], ordered_limit_offset => [ "( SELECT SKIP ? FIRST ? me.id, owner.id, owner.name, ? * ?, ? @@ -220,6 +238,12 @@ my $tests = { }, FirstSkip => { + limit_plain => [ + "( SELECT FIRST ? me.artistid FROM artist me )", + [ + [ { sqlt_datatype => 'integer' } => 5 ] + ], + ], ordered_limit_offset => [ "( SELECT FIRST ? SKIP ? me.id, owner.id, owner.name, ? * ?, ? @@ -295,6 +319,23 @@ my $tests = { )"; { + limit_plain => [ + "( + SELECT me.artistid + FROM ( + SELECT me.artistid, ROW_NUMBER() OVER( ) AS rno__row__index + FROM ( + SELECT me.artistid + FROM artist me + ) me + ) me + WHERE rno__row__index >= ? AND rno__row__index <= ? + )", + [ + [ { sqlt_datatype => 'integer' } => 1 ], + [ { sqlt_datatype => 'integer' } => 5 ], + ], + ], limit => [$unordered_sql, [ @select_bind, @@ -380,6 +421,19 @@ my $tests = { }; { + limit_plain => [ + "( + SELECT me.artistid + FROM ( + SELECT me.artistid + FROM artist me + ) me + WHERE ROWNUM <= ? + )", + [ + [ { sqlt_datatype => 'integer' } => 5 ], + ], + ], limit => [ $limit_sql->(), [ @select_bind, @@ -479,6 +533,10 @@ my $tests = { }, FetchFirst => { + limit_plain => [ + "( SELECT me.artistid FROM artist me FETCH FIRST 5 ROWS ONLY )", + [], + ], limit => [ "( SELECT me.id, owner.id, owner.name, ? * ?, ? @@ -593,6 +651,10 @@ my $tests = { }, Top => { + limit_plain => [ + "( SELECT TOP 5 me.artistid FROM artist me )", + [], + ], limit => [ "( SELECT TOP 4 me.id, owner.id, owner.name, ? * ?, ? @@ -699,6 +761,25 @@ my $tests = { }, GenericSubQ => { + limit_plain => [ + "( + SELECT me.artistid + FROM ( + SELECT me.artistid + FROM artist me + ) me + WHERE + ( + SELECT COUNT(*) + FROM artist rownum__emulation + WHERE rownum__emulation.artistid < me.artistid + ) < ? + ORDER BY me.artistid ASC + )", + [ + [ { sqlt_datatype => 'integer' } => 5 ] + ], + ], ordered_limit => [ "( SELECT me.id, owner__id, owner__name, bar, baz @@ -836,7 +917,25 @@ for my $limtype (sort keys %$tests) { delete $schema->storage->_sql_maker->{_cached_syntax}; $schema->storage->_sql_maker->limit_dialect ($limtype); - my $can_run = ($limtype eq $native_limit_dialect or $limtype eq 'GenericSubQ'); + # do the simplest thing possible first + if ($tests->{$limtype}{limit_plain}) { + is_same_sql_bind( + $schema->resultset('Artist')->search( + [ -and => [ {}, [] ], -or => [ {}, [] ] ], + { + columns => 'artistid', + join => [ {}, [ [ {}, {} ] ], {} ], + prefetch => [ [ [ {}, [] ], {} ], {}, [ {} ] ], + order_by => ( $limtype eq 'GenericSubQ' ? 'artistid' : [] ), + group_by => [], + rows => 5, + offset => 0, + } + )->as_query, + @{$tests->{$limtype}{limit_plain}}, + "$limtype: Plain unordered ungrouped select with limit and no offset", + ) + } # chained search is necessary to exercise the recursive {where} parser my $rs = $schema->resultset('BooksInLibrary')->search( @@ -856,6 +955,7 @@ for my $limtype (sort keys %$tests) { # # not all tests run on all dialects (somewhere impossible, somewhere makes no sense) # + my $can_run = ($limtype eq $native_limit_dialect or $limtype eq 'GenericSubQ'); # only limit, no offset, no order if ($tests->{$limtype}{limit}) {