From: Peter Rabbitson Date: Sat, 25 Feb 2012 14:36:43 +0000 (+0100) Subject: Some cleanups and code dedup of Top and FetchFirst limit dialects X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=86bb5a27dac3255f4fadf5bef120edda9e17bc73;p=dbsrgits%2FDBIx-Class-Historic.git Some cleanups and code dedup of Top and FetchFirst limit dialects Relax requirement of having a primary key declared on table --- diff --git a/Changes b/Changes index 016cb09..fba35ab 100644 --- a/Changes +++ b/Changes @@ -13,6 +13,10 @@ Revision history for DBIx::Class * Fixes - Fix SkipFirst and FirstSkip limit dialects (Informix and Firebird) + - Fix "Skimming limit" dialects (Top, FetchFirst) to properly check + the order_by criteria for stability + - Fix "Skimming limit" dialects (Top, FetchFirst) to propagate + non-selected order criteria when part of a larger subquery - A number of corner case fixes of void context populate() with \[] - Fix corner case of forked children disconnecting the parents DBI handle diff --git a/lib/DBIx/Class/SQLMaker/LimitDialects.pm b/lib/DBIx/Class/SQLMaker/LimitDialects.pm index 5d9afa1..5c2c002 100644 --- a/lib/DBIx/Class/SQLMaker/LimitDialects.pm +++ b/lib/DBIx/Class/SQLMaker/LimitDialects.pm @@ -314,14 +314,30 @@ sub _prep_for_skimming_limit { my $requested_order = delete $rs_attrs->{order_by}; $r{order_by_requested} = $self->_order_by ($requested_order); - # make up an order unless supplied - my $inner_order = ($r{order_by_requested} - ? $requested_order - : [ map + # make up an order unless supplied or sanity check what we are given + my $inner_order; + if ($r{order_by_requested}) { + $self->throw_exception ( + 'Unable to safely perform "skimming type" limit with supplied unstable order criteria' + ) unless $rs_attrs->{_rsroot_rsrc}->schema->storage->_order_by_is_stable( + $rs_attrs->{from}, + $requested_order + ); + + $inner_order = $requested_order; + } + else { + $inner_order = [ map { "$rs_attrs->{alias}.$_" } - ( $rs_attrs->{_rsroot_rsrc}->_pri_cols ) - ] - ); + ( @{ + $rs_attrs->{_rsroot_rsrc}->_identifying_column_set + || + $self->throw_exception(sprintf( + 'Unable to auto-construct stable order criteria for "skimming type" limit ' + . "dialect based on source '%s'", $rs_attrs->{_rsroot_rsrc}->name) ); + } ) + ]; + } # localise as we already have all the bind values we need { @@ -361,6 +377,18 @@ sub _prep_for_skimming_limit { push @{$self->{pre_select_bind}}, @{$self->{order_bind}}; } + # if this is a part of something bigger, we need to add back all + # the extra order_by's, as they may be relied upon by the outside + # of a prefetch or something + if ($rs_attrs->{_is_internal_subuery} and keys %$extra_order_sel) { + $r{out_sel} .= sprintf ", $extra_order_sel->{$_} AS $_" + for sort + { $extra_order_sel->{$a} cmp $extra_order_sel->{$b} } + grep { $_ !~ /[^\w\-]/ } # ignore functions + keys %$extra_order_sel + ; + } + # and this is order re-alias magic for my $map ($extra_order_sel, $alias_map) { for my $col (keys %$map) { diff --git a/lib/DBIx/Class/Storage/DBIHacks.pm b/lib/DBIx/Class/Storage/DBIHacks.pm index 66790f4..3f3662b 100644 --- a/lib/DBIx/Class/Storage/DBIHacks.pm +++ b/lib/DBIx/Class/Storage/DBIHacks.pm @@ -77,7 +77,7 @@ sub _adjust_select_args_for_complex_prefetch { my $outer_attrs = { %$attrs }; delete $outer_attrs->{$_} for qw/where bind rows offset group_by having/; - my $inner_attrs = { %$attrs }; + my $inner_attrs = { %$attrs, _is_internal_subuery => 1 }; delete $inner_attrs->{$_} for qw/for collapse _prefetch_selector_range _collapse_order_by select as/; diff --git a/t/746mssql.t b/t/746mssql.t index 6557d49..12573b4 100644 --- a/t/746mssql.t +++ b/t/746mssql.t @@ -382,7 +382,7 @@ SQL }, { prefetch => 'books', - order_by => { -asc => \['name + ?', [ test => 'xxx' ]] }, # test bindvar propagation + order_by => [ { -asc => \['name + ?', [ test => 'xxx' ]] }, 'me.id' ], # test bindvar propagation group_by => [ map { "me.$_" } $schema->source('Owners')->columns ], # the literal order_by requires an explicit group_by rows => 3, # 8 results total unsafe_subselect_ok => 1, @@ -428,7 +428,7 @@ SQL having => \['1 = ?', [ test => 1 ] ], #test having propagation prefetch => 'owner', rows => 2, # 3 results total - order_by => { -desc => 'me.owner' }, + order_by => [{ -desc => 'me.owner' }, 'me.id'], unsafe_subselect_ok => 1, }, ); diff --git a/t/lib/DBICTest/Schema/Owners.pm b/t/lib/DBICTest/Schema/Owners.pm index 7b021e6..600980f 100644 --- a/t/lib/DBICTest/Schema/Owners.pm +++ b/t/lib/DBICTest/Schema/Owners.pm @@ -16,6 +16,8 @@ __PACKAGE__->add_columns( ); __PACKAGE__->set_primary_key('id'); +__PACKAGE__->add_unique_constraint(['name']); + __PACKAGE__->has_many(books => "DBICTest::Schema::BooksInLibrary", "owner"); 1; diff --git a/t/lib/sqlite.sql b/t/lib/sqlite.sql index 163a4d8..9d49210 100644 --- a/t/lib/sqlite.sql +++ b/t/lib/sqlite.sql @@ -1,7 +1,7 @@ --- +-- -- Created by SQL::Translator::Producer::SQLite --- Created on Sat Jun 11 00:39:51 2011 --- +-- Created on Fri Mar 2 18:22:33 2012 +-- -- -- Table: artist @@ -127,6 +127,8 @@ CREATE TABLE owners ( name varchar(100) NOT NULL ); +CREATE UNIQUE INDEX owners_name ON owners (name); + -- -- Table: producer -- diff --git a/t/sqlmaker/limit_dialects/fetch_first.t b/t/sqlmaker/limit_dialects/fetch_first.t index f229f4e..7a282e9 100644 --- a/t/sqlmaker/limit_dialects/fetch_first.t +++ b/t/sqlmaker/limit_dialects/fetch_first.t @@ -12,7 +12,10 @@ my $schema = DBICTest->init_schema; delete $schema->storage->_sql_maker->{_cached_syntax}; $schema->storage->_sql_maker->limit_dialect ('FetchFirst'); -my $books_45_and_owners = $schema->resultset ('BooksInLibrary')->search ({}, { prefetch => 'owner', rows => 2, offset => 3 }); +my $books_45_and_owners = $schema->resultset ('BooksInLibrary')->search ({}, { + prefetch => 'owner', rows => 2, offset => 3, + columns => [ grep { $_ ne 'title' } $schema->source('BooksInLibrary')->columns ], +}); for my $null_order ( undef, @@ -24,9 +27,9 @@ for my $null_order ( my $rs = $books_45_and_owners->search ({}, {order_by => $null_order }); is_same_sql_bind( $rs->as_query, - '(SELECT id, source, owner, title, price, owner__id, owner__name + '(SELECT id, source, owner, price, owner__id, owner__name FROM ( - SELECT me.id, me.source, me.owner, me.title, me.price, owner.id AS owner__id, owner.name AS owner__name + SELECT me.id, me.source, me.owner, me.price, owner.id AS owner__id, owner.name AS owner__name FROM books me JOIN owners owner ON owner.id = me.owner WHERE ( source = ? ) @@ -44,72 +47,72 @@ for my $null_order ( for my $ord_set ( { - order_by => \'foo DESC', - order_inner => 'foo DESC', + order_by => \'title DESC', + order_inner => 'title DESC', order_outer => 'ORDER__BY__1 ASC', order_req => 'ORDER__BY__1 DESC', exselect_outer => 'ORDER__BY__1', - exselect_inner => 'foo AS ORDER__BY__1', + exselect_inner => 'title AS ORDER__BY__1', }, { - order_by => { -asc => 'foo' }, - order_inner => 'foo ASC', + order_by => { -asc => 'title' }, + order_inner => 'title ASC', order_outer => 'ORDER__BY__1 DESC', order_req => 'ORDER__BY__1 ASC', exselect_outer => 'ORDER__BY__1', - exselect_inner => 'foo AS ORDER__BY__1', + exselect_inner => 'title AS ORDER__BY__1', }, { - order_by => { -desc => 'foo' }, - order_inner => 'foo DESC', + order_by => { -desc => 'title' }, + order_inner => 'title DESC', order_outer => 'ORDER__BY__1 ASC', order_req => 'ORDER__BY__1 DESC', exselect_outer => 'ORDER__BY__1', - exselect_inner => 'foo AS ORDER__BY__1', + exselect_inner => 'title AS ORDER__BY__1', }, { - order_by => 'foo', - order_inner => 'foo', + order_by => 'title', + order_inner => 'title', order_outer => 'ORDER__BY__1 DESC', order_req => 'ORDER__BY__1', exselect_outer => 'ORDER__BY__1', - exselect_inner => 'foo AS ORDER__BY__1', + exselect_inner => 'title AS ORDER__BY__1', }, { - order_by => [ qw{ foo me.owner} ], - order_inner => 'foo, me.owner', + order_by => [ qw{ title me.owner} ], + order_inner => 'title, me.owner', order_outer => 'ORDER__BY__1 DESC, me.owner DESC', order_req => 'ORDER__BY__1, me.owner', exselect_outer => 'ORDER__BY__1', - exselect_inner => 'foo AS ORDER__BY__1', + exselect_inner => 'title AS ORDER__BY__1', }, { - order_by => ['foo', { -desc => 'bar' } ], - order_inner => 'foo, bar DESC', + order_by => ['title', { -desc => 'bar' } ], + order_inner => 'title, bar DESC', order_outer => 'ORDER__BY__1 DESC, ORDER__BY__2 ASC', order_req => 'ORDER__BY__1, ORDER__BY__2 DESC', exselect_outer => 'ORDER__BY__1, ORDER__BY__2', - exselect_inner => 'foo AS ORDER__BY__1, bar AS ORDER__BY__2', + exselect_inner => 'title AS ORDER__BY__1, bar AS ORDER__BY__2', }, { - order_by => { -asc => [qw{ foo bar }] }, - order_inner => 'foo ASC, bar ASC', + order_by => { -asc => [qw{ title bar }] }, + order_inner => 'title ASC, bar ASC', order_outer => 'ORDER__BY__1 DESC, ORDER__BY__2 DESC', order_req => 'ORDER__BY__1 ASC, ORDER__BY__2 ASC', exselect_outer => 'ORDER__BY__1, ORDER__BY__2', - exselect_inner => 'foo AS ORDER__BY__1, bar AS ORDER__BY__2', + exselect_inner => 'title AS ORDER__BY__1, bar AS ORDER__BY__2', }, { order_by => [ - 'foo', + 'title', { -desc => [qw{bar}] }, { -asc => [qw{me.owner sensors}]}, ], - order_inner => 'foo, bar DESC, me.owner ASC, sensors ASC', + order_inner => 'title, bar DESC, me.owner ASC, sensors ASC', order_outer => 'ORDER__BY__1 DESC, ORDER__BY__2 ASC, me.owner DESC, ORDER__BY__3 DESC', order_req => 'ORDER__BY__1, ORDER__BY__2 DESC, me.owner ASC, ORDER__BY__3 ASC', exselect_outer => 'ORDER__BY__1, ORDER__BY__2, ORDER__BY__3', - exselect_inner => 'foo AS ORDER__BY__1, bar AS ORDER__BY__2, sensors AS ORDER__BY__3', + exselect_inner => 'title AS ORDER__BY__1, bar AS ORDER__BY__2, sensors AS ORDER__BY__3', }, ) { my $o_sel = $ord_set->{exselect_outer} @@ -123,11 +126,11 @@ for my $ord_set ( is_same_sql_bind( $books_45_and_owners->search ({}, {order_by => $ord_set->{order_by}})->as_query, - "(SELECT id, source, owner, title, price, owner__id, owner__name + "(SELECT id, source, owner, price, owner__id, owner__name FROM ( - SELECT id, source, owner, title, price, owner__id, owner__name$o_sel + SELECT id, source, owner, price, owner__id, owner__name$o_sel FROM ( - SELECT me.id, me.source, me.owner, me.title, me.price, owner.id AS owner__id, owner.name AS owner__name$i_sel + SELECT me.id, me.source, me.owner, me.price, owner.id AS owner__id, owner.name AS owner__name$i_sel FROM books me JOIN owners owner ON owner.id = me.owner WHERE ( source = ? ) @@ -148,13 +151,13 @@ for my $ord_set ( # with groupby is_same_sql_bind ( $books_45_and_owners->search ({}, { group_by => 'title', order_by => 'title' })->as_query, - '(SELECT me.id, me.source, me.owner, me.title, me.price, owner.id, owner.name + '(SELECT me.id, me.source, me.owner, me.price, owner.id, owner.name FROM ( - SELECT id, source, owner, title, price + SELECT id, source, owner, price, ORDER__BY__1 AS title FROM ( - SELECT id, source, owner, title, price + SELECT id, source, owner, price, ORDER__BY__1 FROM ( - SELECT me.id, me.source, me.owner, me.title, me.price + SELECT me.id, me.source, me.owner, me.price, title AS ORDER__BY__1 FROM books me JOIN owners owner ON owner.id = me.owner WHERE ( source = ? ) @@ -162,10 +165,10 @@ is_same_sql_bind ( ORDER BY title FETCH FIRST 5 ROWS ONLY ) me - ORDER BY title DESC + ORDER BY ORDER__BY__1 DESC FETCH FIRST 2 ROWS ONLY ) me - ORDER BY title + ORDER BY ORDER__BY__1 FETCH FIRST 2 ROWS ONLY ) me JOIN owners owner ON owner.id = me.owner @@ -202,9 +205,9 @@ is_same_sql_bind( $rs_selectas_top->search({})->as_query, { my $rs = $schema->resultset('Artist')->search({}, { - columns => 'name', + columns => 'artistid', offset => 1, - order_by => 'name', + order_by => 'artistid', }); local $rs->result_source->{name} = "weird \n newline/multi \t \t space containing \n table"; diff --git a/t/sqlmaker/limit_dialects/toplimit.t b/t/sqlmaker/limit_dialects/toplimit.t index bb9ef9a..61b5498 100644 --- a/t/sqlmaker/limit_dialects/toplimit.t +++ b/t/sqlmaker/limit_dialects/toplimit.t @@ -14,7 +14,10 @@ my $schema = DBICTest->init_schema; delete $schema->storage->_sql_maker->{_cached_syntax}; $schema->storage->_sql_maker->limit_dialect ('Top'); -my $books_45_and_owners = $schema->resultset ('BooksInLibrary')->search ({}, { prefetch => 'owner', rows => 2, offset => 3 }); +my $books_45_and_owners = $schema->resultset ('BooksInLibrary')->search ({}, { + prefetch => 'owner', rows => 2, offset => 3, + columns => [ grep { $_ ne 'title' } $schema->source('BooksInLibrary')->columns ], +}); for my $null_order ( undef, @@ -27,10 +30,10 @@ for my $null_order ( is_same_sql_bind( $rs->as_query, '(SELECT TOP 2 - id, source, owner, title, price, owner__id, owner__name + id, source, owner, price, owner__id, owner__name FROM ( SELECT TOP 5 - me.id, me.source, me.owner, me.title, me.price, owner.id AS owner__id, owner.name AS owner__name + me.id, me.source, me.owner, me.price, owner.id AS owner__id, owner.name AS owner__name FROM books me JOIN owners owner ON owner.id = me.owner WHERE ( source = ? ) @@ -84,72 +87,72 @@ is_same_sql_bind( for my $ord_set ( { - order_by => \'foo DESC', - order_inner => 'foo DESC', + order_by => \'title DESC', + order_inner => 'title DESC', order_outer => 'ORDER__BY__1 ASC', order_req => 'ORDER__BY__1 DESC', exselect_outer => 'ORDER__BY__1', - exselect_inner => 'foo AS ORDER__BY__1', + exselect_inner => 'title AS ORDER__BY__1', }, { - order_by => { -asc => 'foo' }, - order_inner => 'foo ASC', + order_by => { -asc => 'title' }, + order_inner => 'title ASC', order_outer => 'ORDER__BY__1 DESC', order_req => 'ORDER__BY__1 ASC', exselect_outer => 'ORDER__BY__1', - exselect_inner => 'foo AS ORDER__BY__1', + exselect_inner => 'title AS ORDER__BY__1', }, { - order_by => { -desc => 'foo' }, - order_inner => 'foo DESC', + order_by => { -desc => 'title' }, + order_inner => 'title DESC', order_outer => 'ORDER__BY__1 ASC', order_req => 'ORDER__BY__1 DESC', exselect_outer => 'ORDER__BY__1', - exselect_inner => 'foo AS ORDER__BY__1', + exselect_inner => 'title AS ORDER__BY__1', }, { - order_by => 'foo', - order_inner => 'foo', + order_by => 'title', + order_inner => 'title', order_outer => 'ORDER__BY__1 DESC', order_req => 'ORDER__BY__1', exselect_outer => 'ORDER__BY__1', - exselect_inner => 'foo AS ORDER__BY__1', + exselect_inner => 'title AS ORDER__BY__1', }, { - order_by => [ qw{ foo me.owner} ], - order_inner => 'foo, me.owner', + order_by => [ qw{ title me.owner} ], + order_inner => 'title, me.owner', order_outer => 'ORDER__BY__1 DESC, me.owner DESC', order_req => 'ORDER__BY__1, me.owner', exselect_outer => 'ORDER__BY__1', - exselect_inner => 'foo AS ORDER__BY__1', + exselect_inner => 'title AS ORDER__BY__1', }, { - order_by => ['foo', { -desc => 'bar' } ], - order_inner => 'foo, bar DESC', + order_by => ['title', { -desc => 'bar' } ], + order_inner => 'title, bar DESC', order_outer => 'ORDER__BY__1 DESC, ORDER__BY__2 ASC', order_req => 'ORDER__BY__1, ORDER__BY__2 DESC', exselect_outer => 'ORDER__BY__1, ORDER__BY__2', - exselect_inner => 'foo AS ORDER__BY__1, bar AS ORDER__BY__2', + exselect_inner => 'title AS ORDER__BY__1, bar AS ORDER__BY__2', }, { - order_by => { -asc => [qw{ foo bar }] }, - order_inner => 'foo ASC, bar ASC', + order_by => { -asc => [qw{ title bar }] }, + order_inner => 'title ASC, bar ASC', order_outer => 'ORDER__BY__1 DESC, ORDER__BY__2 DESC', order_req => 'ORDER__BY__1 ASC, ORDER__BY__2 ASC', exselect_outer => 'ORDER__BY__1, ORDER__BY__2', - exselect_inner => 'foo AS ORDER__BY__1, bar AS ORDER__BY__2', + exselect_inner => 'title AS ORDER__BY__1, bar AS ORDER__BY__2', }, { order_by => [ - 'foo', + 'title', { -desc => [qw{bar}] }, { -asc => [qw{me.owner sensors}]}, ], - order_inner => 'foo, bar DESC, me.owner ASC, sensors ASC', + order_inner => 'title, bar DESC, me.owner ASC, sensors ASC', order_outer => 'ORDER__BY__1 DESC, ORDER__BY__2 ASC, me.owner DESC, ORDER__BY__3 DESC', order_req => 'ORDER__BY__1, ORDER__BY__2 DESC, me.owner ASC, ORDER__BY__3 ASC', exselect_outer => 'ORDER__BY__1, ORDER__BY__2, ORDER__BY__3', - exselect_inner => 'foo AS ORDER__BY__1, bar AS ORDER__BY__2, sensors AS ORDER__BY__3', + exselect_inner => 'title AS ORDER__BY__1, bar AS ORDER__BY__2, sensors AS ORDER__BY__3', }, ) { my $o_sel = $ord_set->{exselect_outer} @@ -164,13 +167,13 @@ for my $ord_set ( is_same_sql_bind( $books_45_and_owners->search ({}, {order_by => $ord_set->{order_by}})->as_query, "(SELECT TOP 2 - id, source, owner, title, price, owner__id, owner__name + id, source, owner, price, owner__id, owner__name FROM ( SELECT TOP 2 - id, source, owner, title, price, owner__id, owner__name$o_sel + id, source, owner, price, owner__id, owner__name$o_sel FROM ( SELECT TOP 5 - me.id, me.source, me.owner, me.title, me.price, owner.id AS owner__id, owner.name AS owner__name$i_sel + me.id, me.source, me.owner, me.price, owner.id AS owner__id, owner.name AS owner__name$i_sel FROM books me JOIN owners owner ON owner.id = me.owner WHERE ( source = ? ) @@ -188,24 +191,24 @@ for my $ord_set ( # with groupby is_same_sql_bind ( $books_45_and_owners->search ({}, { group_by => 'title', order_by => 'title' })->as_query, - '(SELECT me.id, me.source, me.owner, me.title, me.price, owner.id, owner.name + '(SELECT me.id, me.source, me.owner, me.price, owner.id, owner.name FROM ( - SELECT TOP 2 id, source, owner, title, price + SELECT TOP 2 id, source, owner, price, ORDER__BY__1 AS title FROM ( SELECT TOP 2 - id, source, owner, title, price + id, source, owner, price, ORDER__BY__1 FROM ( SELECT TOP 5 - me.id, me.source, me.owner, me.title, me.price + me.id, me.source, me.owner, me.price, title AS ORDER__BY__1 FROM books me JOIN owners owner ON owner.id = me.owner WHERE ( source = ? ) GROUP BY title ORDER BY title ) me - ORDER BY title DESC + ORDER BY ORDER__BY__1 DESC ) me - ORDER BY title + ORDER BY ORDER__BY__1 ) me JOIN owners owner ON owner.id = me.owner WHERE ( source = ? ) @@ -240,9 +243,9 @@ is_same_sql_bind( $rs_selectas_top->search({})->as_query, { my $rs = $schema->resultset('Artist')->search({}, { - columns => 'name', + columns => 'artistid', offset => 1, - order_by => 'name', + order_by => 'artistid', }); local $rs->result_source->{name} = "weird \n newline/multi \t \t space containing \n table";