Merge 'trunk' into 'mssql_limit_regression'
Peter Rabbitson [Sat, 9 Jan 2010 10:39:35 +0000 (10:39 +0000)]
r8277@Thesaurus (orig r8265):  ribasushi | 2010-01-09 02:16:14 +0100
 r8263@Thesaurus (orig r8251):  ribasushi | 2010-01-08 15:43:38 +0100
 New branch to find a leak
 r8264@Thesaurus (orig r8252):  ribasushi | 2010-01-08 15:52:46 +0100
 Weird test failures
 r8272@Thesaurus (orig r8260):  ribasushi | 2010-01-09 01:24:56 +0100
 Proper invocation
 r8273@Thesaurus (orig r8261):  ribasushi | 2010-01-09 01:35:34 +0100
 Test for the real leak reason
 r8274@Thesaurus (orig r8262):  ribasushi | 2010-01-09 01:37:33 +0100
 Void ctx as it should be
 r8275@Thesaurus (orig r8263):  ribasushi | 2010-01-09 02:10:13 +0100
 A "fix" for sqlt-related schema leaks
 r8276@Thesaurus (orig r8264):  ribasushi | 2010-01-09 02:15:53 +0100
 Changes

Changes
lib/DBIx/Class/Manual/Cookbook.pod
lib/DBIx/Class/ResultSet.pm
lib/DBIx/Class/ResultSetColumn.pm
lib/DBIx/Class/SQLAHacks.pm
lib/DBIx/Class/Schema.pm
lib/DBIx/Class/Storage/DBI.pm
lib/DBIx/Class/Storage/DBI/MSSQL.pm
t/746mssql.t
t/search/related_strip_prefetch.t [new file with mode: 0644]

diff --git a/Changes b/Changes
index 316308f..9bc4326 100644 (file)
--- a/Changes
+++ b/Changes
@@ -1,6 +1,7 @@
 Revision history for DBIx::Class
 
         - Perl 5.8.1 is now the minimum supported version
+        - Subqueries no longer marked experimental
         - might_have/has_one now warn if applied calling class's column
           has is_nullable set to true.
         - Fixed regression in deploy() with a {sources} table limit applied
@@ -10,6 +11,12 @@ Revision history for DBIx::Class
           parsed by SQL::Translator::Parser::DBIx::Class
         - Schema POD improvement for dclone
         - Fix regression in context sensitiveness of deployment_statements
+        - Fix regression resulting in overcomplicated query on
+          search_related from prefetching resultsets
+        - Better isolation of RNO-limited queries from the rest of a
+          prefetching resultset
+        - New MSSQL specific resultset attribute to allow hacky ordered
+          subquery suppot
         - Fix nasty schema/dbhandle leak due to SQL::Translator
 
 0.08115 2009-12-10 09:02:00 (CST)
index 7ae3630..8082ebf 100644 (file)
@@ -317,7 +317,7 @@ Please see L<DBIx::Class::ResultSet/ATTRIBUTES> documentation if you
 are in any way unsure about the use of the attributes above (C< join
 >, C< select >, C< as > and C< group_by >).
 
-=head2 Subqueries (EXPERIMENTAL)
+=head2 Subqueries
 
 You can write subqueries relatively easily in DBIC.
 
@@ -365,10 +365,6 @@ That creates the following SQL:
        WHERE artist_id = me.artist_id
       )
 
-=head3 EXPERIMENTAL
-
-Please note that subqueries are considered an experimental feature.
-
 =head2 Predefined searches
 
 You can write your own L<DBIx::Class::ResultSet> class by inheriting from it
index 13df20d..baf6be2 100644 (file)
@@ -2047,7 +2047,7 @@ sub _remove_alias {
   return \%unaliased;
 }
 
-=head2 as_query (EXPERIMENTAL)
+=head2 as_query
 
 =over 4
 
@@ -2061,8 +2061,6 @@ Returns the SQL query and bind vars associated with the invocant.
 
 This is generally used as the RHS for a subquery.
 
-B<NOTE>: This feature is still experimental.
-
 =cut
 
 sub as_query {
@@ -2640,10 +2638,19 @@ sub _chain_relationship {
       ||
     $self->_has_resolved_attr (@force_subq_attrs)
   ) {
+    # Nuke the prefetch (if any) before the new $rs attrs
+    # are resolved (prefetch is useless - we are wrapping
+    # a subquery anyway).
+    my $rs_copy = $self->search;
+    $rs_copy->{attrs}{join} = $self->_merge_attr (
+      $rs_copy->{attrs}{join},
+      delete $rs_copy->{attrs}{prefetch},
+    );
+
     $from = [{
       -source_handle => $source->handle,
       -alias => $attrs->{alias},
-      $attrs->{alias} => $self->as_query,
+      $attrs->{alias} => $rs_copy->as_query,
     }];
     delete @{$attrs}{@force_subq_attrs, 'where'};
     $seen->{-relation_chain_depth} = 0;
index 430e35b..0057abf 100644 (file)
@@ -98,7 +98,7 @@ sub new {
   return $new;
 }
 
-=head2 as_query (EXPERIMENTAL)
+=head2 as_query
 
 =over 4
 
@@ -112,8 +112,6 @@ Returns the SQL query and bind vars associated with the invocant.
 
 This is generally used as the RHS for a subquery.
 
-B<NOTE>: This feature is still experimental.
-
 =cut
 
 sub as_query { return shift->_resultset->as_query(@_) }
index 02c336c..52a7af6 100644 (file)
@@ -51,19 +51,27 @@ sub new {
 sub _RowNumberOver {
   my ($self, $sql, $order, $rows, $offset ) = @_;
 
+  # get the select to make the final amount of columns equal the original one
+  my ($select) = $sql =~ /^ \s* SELECT \s+ (.+?) \s+ FROM/ix
+    or croak "Unrecognizable SELECT: $sql";
+
   # get the order_by only (or make up an order if none exists)
   my $order_by = $self->_order_by(
     (delete $order->{order_by}) || $self->_rno_default_order
   );
 
-  # whatever is left
+  # whatever is left of the order_by
   my $group_having = $self->_order_by($order);
 
-  $sql = sprintf (<<'EOS', $order_by, $sql, $group_having, $offset + 1, $offset + $rows, );
+  my $qalias = $self->_quote ($self->{_dbic_rs_attrs}{alias});
+
+  $sql = sprintf (<<EOS, $offset + 1, $offset + $rows, );
 
-SELECT * FROM (
-  SELECT orig_query.*, ROW_NUMBER() OVER(%s ) AS rno__row__index FROM (%s%s) orig_query
-) rno_subq WHERE rno__row__index BETWEEN %d AND %d
+SELECT $select FROM (
+  SELECT $qalias.*, ROW_NUMBER() OVER($order_by ) AS rno__row__index FROM (
+    ${sql}${group_having}
+  ) $qalias
+) $qalias WHERE rno__row__index BETWEEN %d AND %d
 
 EOS
 
index 1121682..2021151 100644 (file)
@@ -1083,7 +1083,7 @@ sub deployment_statements {
   $self->storage->deployment_statements($self, @_);
 }
 
-=head2 create_ddl_dir (EXPERIMENTAL)
+=head2 create_ddl_dir
 
 =over 4
 
index 2939f4a..4aa88d8 100644 (file)
@@ -1360,7 +1360,6 @@ sub insert {
   return $updated_cols;
 }
 
-## Still not quite perfect, and EXPERIMENTAL
 ## Currently it is assumed that all values passed will be "normal", i.e. not
 ## scalar refs, or at least, all the same type as the first set, the statement is
 ## only prepped once.
@@ -1768,11 +1767,24 @@ sub _select_args {
 
   my @limit;
 
-  # see if we need to tear the prefetch apart (either limited has_many or grouped prefetch)
-  # otherwise delegate the limiting to the storage, unless software limit was requested
+  # see if we need to tear the prefetch apart otherwise delegate the limiting to the
+  # storage, unless software limit was requested
   if (
+    #limited has_many
     ( $attrs->{rows} && keys %{$attrs->{collapse}} )
        ||
+    # limited prefetch with RNO subqueries
+    (
+      $attrs->{rows}
+        &&
+      $sql_maker->limit_dialect eq 'RowNumberOver'
+        &&
+      $attrs->{_prefetch_select}
+        &&
+      @{$attrs->{_prefetch_select}}
+    )
+      ||
+    # grouped prefetch
     ( $attrs->{group_by}
         &&
       @{$attrs->{group_by}}
@@ -1782,7 +1794,6 @@ sub _select_args {
       @{$attrs->{_prefetch_select}}
     )
   ) {
-
     ($ident, $select, $where, $attrs)
       = $self->_adjust_select_args_for_complex_prefetch ($ident, $select, $where, $attrs);
   }
@@ -2136,7 +2147,7 @@ sub is_datatype_numeric {
 }
 
 
-=head2 create_ddl_dir (EXPERIMENTAL)
+=head2 create_ddl_dir
 
 =over 4
 
@@ -2188,10 +2199,8 @@ hashref like the following
  { ignore_constraint_names => 0, # ... other options }
 
 
-Note that this feature is currently EXPERIMENTAL and may not work correctly
-across all databases, or fully handle complex relationships.
-
-WARNING: Please check all SQL files created, before applying them.
+WARNING: You are strongly advised to check all SQL files created, before applying
+them.
 
 =cut
 
index cc831e8..0331180 100644 (file)
@@ -179,8 +179,9 @@ sub _execute {
 sub last_insert_id { shift->_identity }
 
 #
-# MSSQL is retarded wrt ordered subselects. One needs to add a TOP 100%
-# to *all* subqueries, do it here.
+# MSSQL is retarded wrt ordered subselects. One needs to add a TOP
+# to *all* subqueries, but one also can't use TOP 100 PERCENT
+# http://sqladvice.com/forums/permalink/18496/22931/ShowThread.aspx#22931
 #
 sub _select_args_to_query {
   my $self = shift;
@@ -190,7 +191,11 @@ sub _select_args_to_query {
   # see if this is an ordered subquery
   my $attrs = $_[3];
   if ( scalar $self->sql_maker->_order_by_chunks ($attrs->{order_by}) ) {
-    $sql =~ s/^ \s* SELECT \s/SELECT TOP 100 PERCENT /xi;
+    $self->throw_exception(
+      'An ordered subquery encountered. Please see "Ordered Subqueries" in DBIx::Class::Storage::DBI::MSSQL
+    ') unless $attrs->{unsafe_subquery};
+    my $max = 2 ** 32;
+    $sql =~ s/^ \s* SELECT \s/SELECT TOP $max /xi;
   }
 
   return wantarray
@@ -303,6 +308,45 @@ $table_name ON>. Unfortunately this operation in MSSQL requires the
 C<db_ddladmin> privilege, which is normally not included in the standard
 write-permissions.
 
+=head2 Ordered Subqueries
+
+ # this is deemed unsafe and throws under MSSQL
+ $rs->search ({}, {
+  prefetch => 'relation',
+  rows => 2,
+  offset => 3,
+ });
+
+ # however this should work (but please check what comes back from the db)
+ $rs->search ({}, {
+  unsafe_subquery => 1,
+  prefetch => 'relation',
+  rows => 2,
+  offset => 3,
+ });
+
+DBIC can do truly wonderful things with the aid of subqueries, and does so
+automatically when necessary. Especially useful are ordered subqueries,
+which allow searches like "Give me things number 4 to 6 (ordered by name), and
+prefetch all their relations, no matter how many". In its pursuit of standards
+Microsft SQL Server goes to great lengths to forbid the use of ordered
+subqueries. While there is a hack which fools the syntax checker, the optimizer
+may B<still elect to break the subquery>. Testing has determined that while
+such breakage does occur (the test suite contains an explicit test which
+demonstrates the problem), it is relative rare. The benefits of ordered
+subqueries are on the other hand too great to be outright disabled for MSSQL.
+
+Thus compromise between usability and perfection is the MSSQL-specific
+L<resultset attribute|DBIx::Class::ResultSet/ATTRIBUTES> C<unsafe_subquery>.
+It is deliberately not possible to set this on the Storage level, as the user
+should inspect (and preferrably regression-test) the return of every such
+ResultSet individually.
+
+If it is possible to rewrite the search() in a way that will avoid the need
+for this flag - you are urged to do so. If DBIC internals insist that an
+ordered subquery is necessary for an operation, and you believe there is a
+differnt/better way to get the same result - please file a bugreport.
+
 =head1 AUTHOR
 
 See L<DBIx::Class/CONTRIBUTORS>.
index e34d0f0..f73f5ff 100644 (file)
@@ -178,10 +178,10 @@ is $rs->find($row->id)->amount, undef,'updated money value to NULL round-trip';
 
 $schema->storage->dbh_do (sub {
     my ($storage, $dbh) = @_;
-    eval { $dbh->do("DROP TABLE Owners") };
-    eval { $dbh->do("DROP TABLE Books") };
+    eval { $dbh->do("DROP TABLE owners") };
+    eval { $dbh->do("DROP TABLE books") };
     $dbh->do(<<'SQL');
-CREATE TABLE Books (
+CREATE TABLE books (
    id INT IDENTITY (1, 1) NOT NULL,
    source VARCHAR(100),
    owner INT,
@@ -189,7 +189,7 @@ CREATE TABLE Books (
    price INT NULL
 )
 
-CREATE TABLE Owners (
+CREATE TABLE owners (
    id INT IDENTITY (1, 1) NOT NULL,
    name VARCHAR(100),
 )
@@ -205,10 +205,10 @@ lives_ok ( sub {
     [qw/1   wiggle/],
     [qw/2   woggle/],
     [qw/3   boggle/],
-    [qw/4   fREW/],
-    [qw/5   fRIOUX/],
-    [qw/6   fROOH/],
-    [qw/7   fRUE/],
+    [qw/4   fRIOUX/],
+    [qw/5   fRUE/],
+    [qw/6   fREW/],
+    [qw/7   fROOH/],
     [qw/8   fISMBoC/],
     [qw/9   station/],
     [qw/10   mirror/],
@@ -220,11 +220,12 @@ lives_ok ( sub {
   ]);
 }, 'populate with PKs supplied ok' );
 
+
 lives_ok (sub {
   # start a new connection, make sure rebless works
   # test an insert with a supplied identity, followed by one without
   my $schema = DBICTest::Schema->connect($dsn, $user, $pass);
-  for (1..2) {
+  for (2, 1) {
     my $id = $_ * 20 ;
     $schema->resultset ('Owners')->create ({ id => $id, name => "troglodoogle $id" });
     $schema->resultset ('Owners')->create ({ name => "troglodoogle " . ($id + 1) });
@@ -254,20 +255,122 @@ lives_ok ( sub {
   ]);
 }, 'populate without PKs supplied ok' );
 
-# make sure ordered subselects work
+# plain ordered subqueries throw
+throws_ok (sub {
+  $schema->resultset('Owners')->search ({}, { order_by => 'name' })->as_query
+}, qr/ordered subquery encountered/, 'Ordered Subquery detection throws ok');
+
+# make sure ordered subselects *somewhat* work
 {
+  my $owners = $schema->resultset ('Owners')->search ({}, { order_by => 'name', offset => 2, rows => 3, unsafe_subquery => 1 });
+
+  my $al = $owners->current_source_alias;
+  my $sealed_owners = $owners->result_source->resultset->search (
+    {},
+    {
+      alias => $al,
+      from => [{
+        -alias => $al,
+        -source_handle => $owners->result_source->handle,
+        $al => $owners->as_query,
+      }],
+    },
+  );
+
+  is_deeply (
+    [ map { $_->name } ($sealed_owners->all) ],
+    [ map { $_->name } ($owners->all) ],
+    'Sort preserved from within a subquery',
+  );
+}
+
+TODO: {
+  local $TODO = "This porbably will never work, but it isn't critical either afaik";
+
   my $book_owner_ids = $schema->resultset ('BooksInLibrary')
-                               ->search ({}, { join => 'owner', distinct => 1, order_by => { -desc => 'owner'} })
+                               ->search ({}, { join => 'owner', distinct => 1, order_by => 'owner.name', unsafe_subquery => 1 })
                                 ->get_column ('owner');
 
-  my $owners = $schema->resultset ('Owners')->search ({
+  my $book_owners = $schema->resultset ('Owners')->search ({
     id => { -in => $book_owner_ids->as_query }
   });
 
-  is ($owners->count, 8, 'Correct amount of book owners');
-  is ($owners->all, 8, 'Correct amount of book owner objects');
+  is_deeply (
+    [ map { $_->id } ($book_owners->all) ],
+    [ $book_owner_ids->all ],
+    'Sort is preserved across IN subqueries',
+  );
+}
+
+# This is known not to work - thus the negative test
+{
+  my $owners = $schema->resultset ('Owners')->search ({}, { order_by => 'name', offset => 2, rows => 3, unsafe_subquery => 1 });
+  my $corelated_owners = $owners->result_source->resultset->search (
+    {
+      id => { -in => $owners->get_column('id')->as_query },
+    },
+    {
+      order_by => 'name' #reorder because of what is shown above
+    },
+  );
+
+  cmp_ok (
+    join ("\x00", map { $_->name } ($corelated_owners->all) ),
+      'ne',
+    join ("\x00", map { $_->name } ($owners->all) ),
+    'Sadly sort not preserved from within a corelated subquery',
+  );
 }
 
+
+# make sure right-join-side single-prefetch ordering limit works
+{
+  my $rs = $schema->resultset ('BooksInLibrary')->search (
+    {
+      'owner.name' => { '!=', 'woggle' },
+    },
+    {
+      prefetch => 'owner',
+      order_by => 'owner.name',
+    }
+  );
+  # this is the order in which they should come from the above query
+  my @owner_names = qw/boggle fISMBoC fREW fRIOUX fROOH fRUE wiggle wiggle/;
+
+  is ($rs->all, 8, 'Correct amount of objects from right-sorted joined resultset');
+  is_deeply (
+    [map { $_->owner->name } ($rs->all) ],
+    \@owner_names,
+    'Rows were properly ordered'
+  );
+
+  my $limited_rs = $rs->search ({}, {rows => 7, offset => 2, unsafe_subquery => 1});
+  is ($limited_rs->count, 6, 'Correct count of limited right-sorted joined resultset');
+  is ($limited_rs->count_rs->next, 6, 'Correct count_rs of limited right-sorted joined resultset');
+
+  my $queries;
+  $schema->storage->debugcb(sub { $queries++; });
+  $schema->storage->debug(1);
+
+  is_deeply (
+    [map { $_->owner->name } ($limited_rs->all) ],
+    [@owner_names[2 .. 7]],
+    'Limited rows were properly ordered'
+  );
+  is ($queries, 1, 'Only one query with prefetch');
+
+  $schema->storage->debugcb(undef);
+  $schema->storage->debug(0);
+
+
+  is_deeply (
+    [map { $_->name } ($limited_rs->search_related ('owner')->all) ],
+    [@owner_names[2 .. 7]],
+    'Rows are still properly ordered after search_related'
+  );
+}
+
+
 #
 # try a prefetch on tables with identically named columns
 #
@@ -287,6 +390,7 @@ $schema->storage->_sql_maker->{name_sep} = '.';
       prefetch => 'books',
       order_by => { -asc => \['name + ?', [ test => 'xxx' ]] }, # test bindvar propagation
       rows     => 3,  # 8 results total
+      unsafe_subquery => 1,
     },
   );
 
@@ -315,6 +419,7 @@ $schema->storage->_sql_maker->{name_sep} = '.';
       prefetch => 'owner',
       rows     => 2,  # 3 results total
       order_by => { -desc => 'owner' },
+      unsafe_subquery => 1,
     },
   );
 
@@ -337,38 +442,13 @@ $schema->storage->_sql_maker->{name_sep} = '.';
   is ($books->page(2)->count_rs->next, 1, 'Prefetched grouped search returns correct count_rs');
 }
 
-# make sure right-join-side ordering limit works
-{
-  my $rs = $schema->resultset ('BooksInLibrary')->search (
-    {
-      'owner.name' => [qw/wiggle woggle/],
-    },
-    {
-      join => 'owner',
-      order_by => { -desc => 'owner.name' },
-    }
-  );
-
-  is ($rs->all, 3, 'Correct amount of objects from right-sorted joined resultset');
-  my $limited_rs = $rs->search ({}, {rows => 3, offset => 1});
-  is ($limited_rs->count, 2, 'Correct count of limited right-sorted joined resultset');
-  is ($limited_rs->count_rs->next, 2, 'Correct count_rs of limited right-sorted joined resultset');
-  is ($limited_rs->all, 2, 'Correct amount of objects from limited right-sorted joined resultset');
-
-  is_deeply (
-    [map { $_->name } ($limited_rs->search_related ('owner')->all) ],
-    [qw/woggle wiggle/],    # there is 1 woggle library book and 2 wiggle books, the limit gets us one of each
-    'Rows were properly ordered'
-  );
-}
-
 done_testing;
 
 # clean up our mess
 END {
   if (my $dbh = eval { $schema->storage->_dbh }) {
     eval { $dbh->do("DROP TABLE $_") }
-      for qw/artist money_test Books Owners/;
+      for qw/artist money_test books owners/;
   }
 }
 # vim:sw=2 sts=2
diff --git a/t/search/related_strip_prefetch.t b/t/search/related_strip_prefetch.t
new file mode 100644 (file)
index 0000000..10621ae
--- /dev/null
@@ -0,0 +1,43 @@
+use strict;
+use warnings;
+
+use Test::More;
+use Test::Exception;
+
+use lib qw(t/lib);
+use DBIC::SqlMakerTest;
+use DBICTest;
+
+my $schema = DBICTest->init_schema();
+
+my $rs = $schema->resultset('CD')->search (
+  { 'tracks.id' => { '!=', 666 }},
+  { join => 'artist', prefetch => 'tracks', rows => 2 }
+);
+
+my $rel_rs = $rs->search_related ('tags', { 'tags.tag' => { '!=', undef }}, { distinct => 1});
+
+is_same_sql_bind (
+  $rel_rs->as_query,
+  '(
+    SELECT tags.tagid, tags.cd, tags.tag
+      FROM (
+        SELECT me.cdid, me.artist, me.title, me.year, me.genreid, me.single_track
+          FROM cd me
+          JOIN artist artist ON artist.artistid = me.artist
+          LEFT JOIN track tracks ON tracks.cd = me.cdid
+        WHERE ( tracks.id != ? )
+        LIMIT 2
+      ) me
+      JOIN artist artist ON artist.artistid = me.artist
+      LEFT JOIN track tracks ON tracks.cd = me.cdid
+      LEFT JOIN tags tags ON tags.cd = me.cdid
+    WHERE ( tags.tag IS NOT NULL )
+    GROUP BY tags.tagid, tags.cd, tags.tag
+  )',
+
+  [ [ 'tracks.id' => 666 ] ],
+  'Prefetch spec successfully stripped on search_related'
+);
+
+done_testing;