Fix proper handling of composite resultset heads (e.g. as_subselect_rs)

Solves http://lists.scsys.co.uk/pipermail/dbix-class/2012-July/010650.html

diff --git a/Changes b/Changes
index c42606c..353bbdf 100644 (file)
--- a/Changes
+++ b/Changes
@@ -9,6 +9,8 @@ Revision history for DBIx::Class
     * Fixes
         - When performing resultset update/delete only strip condition
           qualifiers - leave the source name alone (RT#80015, RT#78844)
+        - Fix incorrect behavior on resultset update/delete invoked on
+          composite resultsets (e.g. as_subselect_rs)
         - More robust behavior under heavily threaded environments - make
           sure we do not have refaddr reuse in the global storage registry
         - Fix failing test on 5.8 under Win32 (RT#81114)

diff --git a/lib/DBIx/Class/ResultSet.pm b/lib/DBIx/Class/ResultSet.pm
index 18ff813..91ae0bb 100644 (file)
--- a/lib/DBIx/Class/ResultSet.pm
+++ b/lib/DBIx/Class/ResultSet.pm
@@ -1761,37 +1761,47 @@ sub _rs_update_delete {
 
   my $attrs = { %{$self->_resolved_attrs} };
 
+  my $join_classifications;
   my $existing_group_by = delete $attrs->{group_by};
-  my $needs_subq = defined $existing_group_by;
 
-  # simplify the joinmap and maybe decide if a subquery is necessary
-  my $relation_classifications = {};
+  # do we need a subquery for any reason?
+  my $needs_subq = (
+    defined $existing_group_by
+      or
+    # if {from} is unparseable wrap a subq
+    ref($attrs->{from}) ne 'ARRAY'
+      or
+    # limits call for a subq
+    $self->_has_resolved_attr(qw/rows offset/)
+  );
 
-  if (ref($attrs->{from}) eq 'ARRAY') {
-    # if we already know we need a subq, no point of classifying relations
-    if (!$needs_subq and @{$attrs->{from}} > 1) {
-      $attrs->{from} = $storage->_prune_unused_joins ($attrs->{from}, $attrs->{select}, $cond, $attrs);
+  # simplify the joinmap, so we can further decide if a subq is necessary
+  if (!$needs_subq and @{$attrs->{from}} > 1) {
+    $attrs->{from} = $storage->_prune_unused_joins ($attrs->{from}, $attrs->{select}, $cond, $attrs);
 
-      $relation_classifications = $storage->_resolve_aliastypes_from_select_args (
+    # check if there are any joins left after the prune
+    if ( @{$attrs->{from}} > 1 ) {
+      $join_classifications = $storage->_resolve_aliastypes_from_select_args (
         [ @{$attrs->{from}}[1 .. $#{$attrs->{from}}] ],
         $attrs->{select},
         $cond,
         $attrs
       );
+
+      # any non-pruneable joins imply subq
+      $needs_subq = scalar keys %{ $join_classifications->{restricting} || {} };
     }
   }
-  else {
-    $needs_subq ||= 1; # if {from} is unparseable assume the worst
-  }
+
+  # check if the head is composite (by now all joins are thrown out unless $needs_subq)
+  $needs_subq ||= (
+    (ref $attrs->{from}[0]) ne 'HASH'
+      or
+    ref $attrs->{from}[0]{ $attrs->{from}[0]{-alias} }
+  );
 
   # do we need anything like a subquery?
-  if (
-    ! $needs_subq
-      and
-    ! keys %{ $relation_classifications->{restricting} || {} }
-      and
-    ! $self->_has_resolved_attr(qw/rows offset/) # limits call for a subq
-  ) {
+  unless ($needs_subq) {
     # Most databases do not allow aliasing of tables in UPDATE/DELETE. Thus
     # a condition containing 'me' or other table prefixes will not work
     # at all. Tell SQLMaker to dequalify idents via a gross hack.
@@ -1800,6 +1810,7 @@ sub _rs_update_delete {
       local $sqla->{_dequalify_idents} = 1;
       \[ $sqla->_recurse_where($self->{cond}) ];
     };
+
     return $rsrc->storage->$op(
       $rsrc,
       $op eq 'update' ? $values : (),
@@ -1845,11 +1856,11 @@ sub _rs_update_delete {
 
     # if all else fails - get all primary keys and operate over a ORed set
     # wrap in a transaction for consistency
-    # this is where the group_by starts to matter
+    # this is where the group_by/multiplication starts to matter
     if (
       $existing_group_by
         or
-      keys %{ $relation_classifications->{multiplying} || {} }
+      keys %{ $join_classifications->{multiplying} || {} }
     ) {
       # make sure if there is a supplied group_by it matches the columns compiled above
       # perfectly. Anything else can not be sanely executed on most databases so croak

diff --git a/t/resultset/update_delete.t b/t/resultset/update_delete.t
index 62081bf..a5217ae 100644 (file)
--- a/t/resultset/update_delete.t
+++ b/t/resultset/update_delete.t
@@ -4,6 +4,13 @@ use warnings;
 use lib qw(t/lib);
 use Test::More;
 use Test::Exception;
+
+use DBICTest::Schema::CD;
+BEGIN {
+  # the default scalarref table name will not work well for this test
+  DBICTest::Schema::CD->table('cd');
+}
+
 use DBICTest;
 use DBIC::DebugObj;
 use DBIC::SqlMakerTest;
@@ -267,6 +274,7 @@ $schema->storage->debug (1);
     'delete with fully qualified table name and subquery correct'
   );
 
+  # this is expected to fail - we only want to collect the generated SQL
   eval { $rs->search({}, { prefetch => 'artist' })->delete };
   is_same_sql_bind (
     $sql,
@@ -277,10 +285,40 @@ $schema->storage->debug (1);
   );
 
   $rs->result_source->name('cd');
+
+  # check that as_subselect_rs works ok
+  # inner query is untouched, then a selector
+  # and an IN condition
+  $schema->resultset('CD')->search({
+    'me.cdid' => 1,
+    'artist.name' => 'partytimecity',
+  }, {
+    join => 'artist',
+  })->as_subselect_rs->delete;
+
+  is_same_sql_bind (
+    $sql,
+    \@bind,
+    '
+      DELETE FROM cd
+      WHERE (
+        cdid IN (
+          SELECT me.cdid
+            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
+              WHERE artist.name = ? AND me.cdid = ?
+            ) me
+        )
+      )
+    ',
+    ["'partytimecity'", "'1'"],
+    'Delete from as_subselect_rs works correctly'
+  );
 }
 
 $schema->storage->debugobj ($orig_debugobj);
 $schema->storage->debug ($orig_debug);
 
-
 done_testing;