From: Peter Rabbitson <ribasushi@cpan.org>
Date: Thu, 1 Dec 2011 16:50:57 +0000 (+0100)
Subject: Fix and test first_skip/skip_first limit dialects
X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=8b31f62e;p=dbsrgits%2FDBIx-Class-Historic.git

Fix and test first_skip/skip_first limit dialects

Codebase didn't take in account the fact that limit bindvals for
SELECT FIRST x SKIP y and SELECT FIRST x SKIP y need to be inserted
at the head of the bind-assembly chain. Hence introducing a new
bind-chunk position 'pre_select'. While at it move the TOP dialect
to use it as well.

Extensive tests for both dialects, and also augment the test for
the Oracle RowNum dialect (no fixes necessary)
---

diff --git a/Changes b/Changes
index 1590ce6..7076eb0 100644
--- a/Changes
+++ b/Changes
@@ -1,5 +1,8 @@
 Revision history for DBIx::Class
 
+    * Fixes
+        - Fix SkipFirst and FirstSkip limit dialects (Informix and Firebird)
+
 0.08196 2011-11-29 05:35 (UTC)
     * Fixes
         - Fix tests for DBD::SQLite >= 1.34.
diff --git a/lib/DBIx/Class/SQLMaker.pm b/lib/DBIx/Class/SQLMaker.pm
index fd42594..705c569 100644
--- a/lib/DBIx/Class/SQLMaker.pm
+++ b/lib/DBIx/Class/SQLMaker.pm
@@ -229,7 +229,7 @@ sub select {
 
 sub _assemble_binds {
   my $self = shift;
-  return map { @{ (delete $self->{"${_}_bind"}) || [] } } (qw/select from where group having order limit/);
+  return map { @{ (delete $self->{"${_}_bind"}) || [] } } (qw/pre_select select from where group having order limit/);
 }
 
 my $for_syntax = {
diff --git a/lib/DBIx/Class/SQLMaker/LimitDialects.pm b/lib/DBIx/Class/SQLMaker/LimitDialects.pm
index 56f2d53..8e85ac8 100644
--- a/lib/DBIx/Class/SQLMaker/LimitDialects.pm
+++ b/lib/DBIx/Class/SQLMaker/LimitDialects.pm
@@ -169,13 +169,13 @@ sub _SkipFirst {
   return sprintf ('SELECT %s%s%s%s',
     $offset
       ? do {
-         push @{$self->{limit_bind}}, [ $self->__offset_bindtype => $offset];
+         push @{$self->{pre_select_bind}}, [ $self->__offset_bindtype => $offset];
          'SKIP ? '
       }
       : ''
     ,
     do {
-       push @{$self->{limit_bind}}, [ $self->__rows_bindtype => $rows ];
+       push @{$self->{pre_select_bind}}, [ $self->__rows_bindtype => $rows ];
        'FIRST ? '
     },
     $sql,
@@ -199,12 +199,12 @@ sub _FirstSkip {
 
   return sprintf ('SELECT %s%s%s%s',
     do {
-       push @{$self->{limit_bind}}, [ $self->__rows_bindtype => $rows ];
+       push @{$self->{pre_select_bind}}, [ $self->__rows_bindtype => $rows ];
        'FIRST ? '
     },
     $offset
       ? do {
-         push @{$self->{limit_bind}}, [ $self->__offset_bindtype => $offset];
+         push @{$self->{pre_select_bind}}, [ $self->__offset_bindtype => $offset];
          'SKIP ? '
       }
       : ''
@@ -390,11 +390,9 @@ sub _prep_for_skimming_limit {
       $r{mid_sel} .= ', ' . $extra_order_sel->{$extra_col};
     }
 
-    # since whatever order bindvals there are, they will be realiased
-    # and need to show up in front of the entire initial inner subquery
-    # *unshift* the selector bind stack to make this happen (horrible,
-    # horrible, but we don't have another mechanism yet)
-    unshift @{$self->{select_bind}}, @{$self->{order_bind}};
+    # Whatever order bindvals there are, they will be realiased and
+    # need to show up in front of the entire initial inner subquery
+    push @{$self->{pre_select_bind}}, @{$self->{order_bind}};
   }
 
   # and this is order re-alias magic
diff --git a/lib/DBIx/Class/SQLMaker/Oracle.pm b/lib/DBIx/Class/SQLMaker/Oracle.pm
index d144113..7548c2a 100644
--- a/lib/DBIx/Class/SQLMaker/Oracle.pm
+++ b/lib/DBIx/Class/SQLMaker/Oracle.pm
@@ -25,7 +25,7 @@ sub new {
 
 sub _assemble_binds {
   my $self = shift;
-  return map { @{ (delete $self->{"${_}_bind"}) || [] } } (qw/select from where oracle_connect_by group having order limit/);
+  return map { @{ (delete $self->{"${_}_bind"}) || [] } } (qw/pre_select select from where oracle_connect_by group having order limit/);
 }
 
 
diff --git a/t/sqlmaker/limit_dialects/first_skip.t b/t/sqlmaker/limit_dialects/first_skip.t
new file mode 100644
index 0000000..c9e93bb
--- /dev/null
+++ b/t/sqlmaker/limit_dialects/first_skip.t
@@ -0,0 +1,151 @@
+use strict;
+use warnings;
+
+use Test::More;
+use lib qw(t/lib);
+use DBICTest;
+use DBIC::SqlMakerTest;
+use DBIx::Class::SQLMaker::LimitDialects;
+
+my ($LIMIT, $OFFSET) = (
+   DBIx::Class::SQLMaker::LimitDialects->__rows_bindtype,
+   DBIx::Class::SQLMaker::LimitDialects->__offset_bindtype,
+);
+
+my $schema = DBICTest->init_schema;
+
+$schema->storage->_sql_maker->limit_dialect ('FirstSkip');
+
+my $rs_selectas_col = $schema->resultset ('BooksInLibrary')->search ({}, {
+  '+select' => ['owner.name'],
+  '+as' => ['owner.name'],
+  join => 'owner',
+  rows => 1,
+  offset => 2,
+});
+
+is_same_sql_bind(
+  $rs_selectas_col->as_query,
+  '(
+    SELECT FIRST ? SKIP ? me.id, me.source, me.owner, me.title, me.price, owner.name
+      FROM books me
+      JOIN owners owner ON owner.id = me.owner
+    WHERE ( source = ? )
+  )',
+  [
+    [ $LIMIT => 1 ],
+    [ $OFFSET => 2 ],
+    [ { sqlt_datatype => 'varchar', sqlt_size => 100, dbic_colname => 'source' } => 'Library' ],
+  ],
+);
+
+$schema->storage->_sql_maker->quote_char ([qw/ [ ] /]);
+$schema->storage->_sql_maker->name_sep ('.');
+
+my $rs_selectas_rel = $schema->resultset ('BooksInLibrary')->search ({}, {
+  '+select' => ['owner.name'],
+  '+as' => ['owner_name'],
+  join => 'owner',
+  rows => 1,
+  offset => 2,
+});
+
+is_same_sql_bind(
+  $rs_selectas_rel->as_query,
+  '(
+    SELECT FIRST ? SKIP ? [me].[id], [me].[source], [me].[owner], [me].[title], [me].[price], [owner].[name]
+      FROM [books] [me]
+      JOIN [owners] [owner] ON [owner].[id] = [me].[owner]
+    WHERE ( [source] = ? )
+  )',
+  [
+    [ $LIMIT => 1 ],
+    [ $OFFSET => 2 ],
+    [ { sqlt_datatype => 'varchar', sqlt_size => 100, dbic_colname => 'source' } => 'Library' ],
+  ],
+);
+
+{
+my $subq = $schema->resultset('Owners')->search({
+   'count.id' => { -ident => 'owner.id' },
+   'count.name' => 'fail', # no one would do this in real life, the rows makes even less sense
+}, { alias => 'owner', rows => 1 })->count_rs;
+
+my $rs_selectas_rel = $schema->resultset('BooksInLibrary')->search ({}, {
+  columns => [
+     { owner_name => 'owner.name' },
+     { owner_books => $subq->as_query },
+  ],
+  join => 'owner',
+  rows => 1,
+  offset => 2,
+});
+
+is_same_sql_bind(
+  $rs_selectas_rel->as_query,
+  '(
+    SELECT FIRST ? SKIP ?
+        [owner].[name],
+        ( SELECT COUNT(*) FROM
+          ( SELECT FIRST ? [owner].[id] FROM [owners] [owner]
+            WHERE [count].[id] = [owner].[id] and [count].[name] = ? 
+          ) [owner]
+        )
+      FROM [books] [me]
+      JOIN [owners] [owner] ON [owner].[id] = [me].[owner]
+    WHERE ( [source] = ? )
+  )',
+  [
+    [ $LIMIT => 1 ],  # outer
+    [ $OFFSET => 2 ], # outer
+    [ {%$LIMIT} => 1 ],  # inner
+    [ { dbic_colname => 'count.name' } => 'fail' ],
+    [ { sqlt_datatype => 'varchar', sqlt_size => 100, dbic_colname => 'source' } => 'Library' ],
+  ],
+)
+};
+
+{
+  my $rs = $schema->resultset('Artist')->search({}, {
+    columns => 'name',
+    offset => 1,
+    order_by => 'name',
+  });
+  local $rs->result_source->{name} = "weird \n newline/multi \t \t space containing \n table";
+
+  like (
+    ${$rs->as_query}->[0],
+    qr| weird \s \n \s newline/multi \s \t \s \t \s space \s containing \s \n \s table|x,
+    'Newlines/spaces preserved in final sql',
+  );
+}
+
+{
+my $subq = $schema->resultset('Owners')->search({
+   'books.owner' => { -ident => 'owner.id' },
+}, { alias => 'owner', select => ['id'], offset => 3, rows => 4 });
+
+my $rs_selectas_rel = $schema->resultset('BooksInLibrary')->search( { -exists => $subq->as_query }, { select => ['id','owner'], rows => 1, offset => 2 } );
+
+is_same_sql_bind(
+  $rs_selectas_rel->as_query,
+  '(
+     SELECT FIRST ? SKIP ? [me].[id], [me].[owner]
+     FROM [books] [me]
+     WHERE ( ( (EXISTS (
+       SELECT FIRST ? SKIP ? [owner].[id] FROM [owners] [owner] WHERE ( [books].[owner] = [owner].[id] )
+     )) AND [source] = ? ) )
+ )',
+  [
+    [ $LIMIT => 1 ],  #outer
+    [ $OFFSET => 2 ], #outer
+    [ {%$LIMIT} => 4 ],  #inner
+    [ {%$OFFSET} => 3 ], #inner
+    [ { sqlt_datatype => 'varchar', sqlt_size => 100, dbic_colname => 'source' } => 'Library' ],
+  ],
+  'Pagination with sub-query in WHERE works'
+);
+
+}
+
+done_testing;
diff --git a/t/sqlmaker/limit_dialects/rownum.t b/t/sqlmaker/limit_dialects/rownum.t
index e792d69..14ac5cd 100644
--- a/t/sqlmaker/limit_dialects/rownum.t
+++ b/t/sqlmaker/limit_dialects/rownum.t
@@ -16,7 +16,9 @@ my ($TOTAL, $OFFSET) = (
 my $s = DBICTest->init_schema (no_deploy => 1, );
 $s->storage->sql_maker->limit_dialect ('RowNum');
 
-my $rs = $s->resultset ('CD');
+my $rs = $s->resultset ('CD')->search({ id => 1 });
+
+my $where_bind = [ { dbic_colname => 'id' }, 1 ];
 
 for my $test_set (
   {
@@ -36,11 +38,13 @@ for my $test_set (
         SELECT id, bar__id, bleh, ROWNUM rownum__index
         FROM (
           SELECT foo.id AS id, bar.id AS bar__id, TO_CHAR (foo.womble, "blah") AS bleh
-          FROM cd me
+            FROM cd me
+          WHERE id = ?
         ) me
       ) me WHERE rownum__index BETWEEN ? AND ?
     )',
     binds => [
+      $where_bind,
       [ $OFFSET => 4 ],
       [ $TOTAL => 4 ],
     ],
@@ -62,7 +66,8 @@ for my $test_set (
         SELECT id, bar__id, bleh, ROWNUM rownum__index
         FROM (
           SELECT foo.id AS id, bar.id AS bar__id, TO_CHAR(foo.womble, "blah") AS bleh
-          FROM cd me
+            FROM cd me
+          WHERE id = ?
           ORDER BY artist, title
         ) me
         WHERE ROWNUM <= ?
@@ -70,6 +75,7 @@ for my $test_set (
       WHERE rownum__index >= ?
     )',
     binds => [
+      $where_bind,
       [ $TOTAL => 4 ],
       [ $OFFSET => 4 ],
     ],
@@ -89,11 +95,13 @@ for my $test_set (
         SELECT id, ends_with_me__id, ROWNUM rownum__index
         FROM (
           SELECT foo.id AS id, ends_with_me.id AS ends_with_me__id
-          FROM cd me
+            FROM cd me
+          WHERE id = ?
         ) me
       ) me WHERE rownum__index BETWEEN ? AND ?
     )',
     binds => [
+      $where_bind,
       [ $OFFSET => 4 ],
       [ $TOTAL => 5 ],
     ],
@@ -114,7 +122,8 @@ for my $test_set (
         SELECT id, ends_with_me__id, ROWNUM rownum__index
         FROM (
           SELECT foo.id AS id, ends_with_me.id AS ends_with_me__id
-          FROM cd me
+            FROM cd me
+          WHERE id = ?
           ORDER BY artist, title
         ) me
         WHERE ROWNUM <= ?
@@ -122,6 +131,7 @@ for my $test_set (
       WHERE rownum__index >= ?
     )',
     binds => [
+      $where_bind,
       [ $TOTAL => 5 ],
       [ $OFFSET => 4 ],
     ],
diff --git a/t/sqlmaker/limit_dialects/skip_first.t b/t/sqlmaker/limit_dialects/skip_first.t
new file mode 100644
index 0000000..897ed2e
--- /dev/null
+++ b/t/sqlmaker/limit_dialects/skip_first.t
@@ -0,0 +1,152 @@
+use strict;
+use warnings;
+
+use Test::More;
+use lib qw(t/lib);
+use DBICTest;
+use DBIC::SqlMakerTest;
+use DBIx::Class::SQLMaker::LimitDialects;
+
+my ($LIMIT, $OFFSET) = (
+   DBIx::Class::SQLMaker::LimitDialects->__rows_bindtype,
+   DBIx::Class::SQLMaker::LimitDialects->__offset_bindtype,
+);
+
+my $schema = DBICTest->init_schema;
+
+$schema->storage->_sql_maker->limit_dialect ('SkipFirst');
+
+my $rs_selectas_col = $schema->resultset ('BooksInLibrary')->search ({}, {
+  '+select' => ['owner.name'],
+  '+as' => ['owner.name'],
+  join => 'owner',
+  rows => 1,
+  offset => 2,
+});
+
+is_same_sql_bind(
+  $rs_selectas_col->as_query,
+  '(
+    SELECT SKIP ? FIRST ? me.id, me.source, me.owner, me.title, me.price, owner.name
+      FROM books me
+      JOIN owners owner ON owner.id = me.owner
+    WHERE ( source = ? )
+  )',
+  [
+    [ $OFFSET => 2 ],
+    [ $LIMIT => 1 ],
+    [ { sqlt_datatype => 'varchar', sqlt_size => 100, dbic_colname => 'source' } => 'Library' ],
+  ],
+);
+
+$schema->storage->_sql_maker->quote_char ([qw/ [ ] /]);
+$schema->storage->_sql_maker->name_sep ('.');
+
+my $rs_selectas_rel = $schema->resultset ('BooksInLibrary')->search ({}, {
+  '+select' => ['owner.name'],
+  '+as' => ['owner_name'],
+  join => 'owner',
+  rows => 1,
+  offset => 2,
+});
+
+is_same_sql_bind(
+  $rs_selectas_rel->as_query,
+  '(
+    SELECT SKIP ? FIRST ? [me].[id], [me].[source], [me].[owner], [me].[title], [me].[price], [owner].[name]
+      FROM [books] [me]
+      JOIN [owners] [owner] ON [owner].[id] = [me].[owner]
+    WHERE ( [source] = ? )
+  )',
+  [
+    [ $OFFSET => 2 ],
+    [ $LIMIT => 1 ],
+    [ { sqlt_datatype => 'varchar', sqlt_size => 100, dbic_colname => 'source' } => 'Library' ],
+  ],
+);
+
+{
+my $subq = $schema->resultset('Owners')->search({
+   'count.id' => { -ident => 'owner.id' },
+   'count.name' => 'fail', # no one would do this in real life, the rows makes even less sense
+}, { alias => 'owner', rows => 1 })->count_rs;
+
+my $rs_selectas_rel = $schema->resultset('BooksInLibrary')->search ({}, {
+  columns => [
+     { owner_name => 'owner.name' },
+     { owner_books => $subq->as_query },
+  ],
+  join => 'owner',
+  rows => 1,
+  offset => 2,
+});
+
+is_same_sql_bind(
+  $rs_selectas_rel->as_query,
+  '(
+    SELECT SKIP ? FIRST ?
+        [owner].[name],
+        ( SELECT COUNT(*) FROM
+          ( SELECT FIRST ? [owner].[id] FROM [owners] [owner]
+            WHERE [count].[id] = [owner].[id] and [count].[name] = ? 
+          ) [owner]
+        )
+      FROM [books] [me]
+      JOIN [owners] [owner] ON [owner].[id] = [me].[owner]
+    WHERE ( [source] = ? )
+  )',
+  [
+    [ $OFFSET => 2 ], # outer
+    [ $LIMIT => 1 ],  # outer
+    [ {%$LIMIT} => 1 ],  # inner
+    [ { dbic_colname => 'count.name' } => 'fail' ],
+    [ { sqlt_datatype => 'varchar', sqlt_size => 100, dbic_colname => 'source' } => 'Library' ],
+  ],
+)
+};
+
+{
+  my $rs = $schema->resultset('Artist')->search({}, {
+    columns => 'name',
+    offset => 1,
+    order_by => 'name',
+  });
+  local $rs->result_source->{name} = "weird \n newline/multi \t \t space containing \n table";
+
+  like (
+    ${$rs->as_query}->[0],
+    qr| weird \s \n \s newline/multi \s \t \s \t \s space \s containing \s \n \s table|x,
+    'Newlines/spaces preserved in final sql',
+  );
+}
+
+{
+my $subq = $schema->resultset('Owners')->search({
+   'books.owner' => { -ident => 'owner.id' },
+}, { alias => 'owner', select => ['id'], offset => 3, rows => 4 });
+
+my $rs_selectas_rel = $schema->resultset('BooksInLibrary')->search( { -exists => $subq->as_query }, { select => ['id','owner'], rows => 1, offset => 2 } );
+
+is_same_sql_bind(
+  $rs_selectas_rel->as_query,
+  '(
+     SELECT SKIP ? FIRST ? [me].[id], [me].[owner]
+     FROM [books] [me]
+     WHERE ( ( (EXISTS (
+       SELECT SKIP ? FIRST ? [owner].[id] FROM [owners] [owner] WHERE ( [books].[owner] = [owner].[id] )
+     )) AND [source] = ? ) )
+ )',
+  [
+    [ $OFFSET => 2 ], #outer
+    [ $LIMIT => 1 ],  #outer
+    [ {%$OFFSET} => 3 ], #inner
+    [ {%$LIMIT} => 4 ],  #inner
+    [ { sqlt_datatype => 'varchar', sqlt_size => 100, dbic_colname => 'source' } => 'Library' ],
+  ],
+  'Pagination with sub-query in WHERE works'
+);
+
+}
+
+
+done_testing;