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 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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";