Stop stripping table name qualifier on complex $rs->update/delete

RT#80015, RT#78844
Perl5 · Nov 19, 2012 · fd8076c · fd8076c
1 parent dfd722e
commit fd8076c
Show file tree

Hide file tree

Showing 3 changed files with 137 additions and 52 deletions.
diff --git a/Changes b/Changes
@@ -1,6 +1,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)
         - 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
@@ -1760,51 +1760,49 @@ sub _rs_update_delete {
 
   my $attrs = { %{$self->_resolved_attrs} };
 
-  # "needs" is a strong word here - if the subquery is part of an IN clause - no point of
-  # even adding the group_by. It will really be used only when composing a poor-man's
-  # multicolumn-IN equivalent OR set
-  my $needs_group_by_subq = defined $attrs->{group_by};
+  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 = {};
 
-  # simplify the joinmap and maybe decide if a grouping (and thus subquery) is necessary
-  my $relation_classifications;
   if (ref($attrs->{from}) eq 'ARRAY') {
-    if (@{$attrs->{from}} == 1) {
-      # not a fucking JOIN at all, quit with the dickery
-      $relation_classifications = {};
-    } else {
+    # 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);
 
       $relation_classifications = $storage->_resolve_aliastypes_from_select_args (
         [ @{$attrs->{from}}[1 .. $#{$attrs->{from}}] ],
         $attrs->{select},
         $cond,
         $attrs
-      ) unless $needs_group_by_subq;  # we already know we need a group, no point of resolving them
+      );
     }
   }
   else {
-    $needs_group_by_subq ||= 1; # if {from} is unparseable assume the worst
+    $needs_subq ||= 1; # if {from} is unparseable assume the worst
   }
 
-  $needs_group_by_subq ||= exists $relation_classifications->{multiplying};
-
-  # if no subquery - life is easy-ish
-  unless (
-    $needs_group_by_subq
-      or
-    keys %$relation_classifications # if any joins at all - need to wrap a subq
-      or
-    $self->_has_resolved_attr(qw/rows offset/) # limits call for a subq
+  # 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
   ) {
     # 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.
-    my $sqla = $rsrc->storage->sql_maker;
-    local $sqla->{_dequalify_idents} = 1;
+    my $cond = do {
+      my $sqla = $rsrc->storage->sql_maker;
+      local $sqla->{_dequalify_idents} = 1;
+      \[ $sqla->_recurse_where($self->{cond}) ];
+    };
     return $rsrc->storage->$op(
       $rsrc,
       $op eq 'update' ? $values : (),
-      $self->{cond},
+      $cond,
     );
   }
 
@@ -1816,7 +1814,6 @@ sub _rs_update_delete {
       $rsrc->source_name,
     )
   );
-  my $existing_group_by = delete $attrs->{group_by};
 
   # make a new $rs selecting only the PKs (that's all we really need for the subq)
   delete $attrs->{$_} for qw/collapse _collapse_order_by select _prefetch_selector_range as/;
@@ -1847,13 +1844,15 @@ sub _rs_update_delete {
     );
   }
   else {
+
     # 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
-    my $subq_group_by;
-    if ($needs_group_by_subq) {
-      $subq_group_by = $attrs->{columns};
-
+    if (
+      $existing_group_by
+        or
+      keys %{ $relation_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
       # right then and there
@@ -1866,7 +1865,7 @@ sub _rs_update_delete {
         if (
           join ("\x00", sort @current_group_by)
             ne
-          join ("\x00", sort @$subq_group_by )
+          join ("\x00", sort @{$attrs->{columns}} )
         ) {
           $self->throw_exception (
             "You have just attempted a $op operation on a resultset which does group_by"
@@ -1877,12 +1876,14 @@ sub _rs_update_delete {
           );
         }
       }
+
+      $subrs = $subrs->search({}, { group_by => $attrs->{columns} });
     }
 
     my $guard = $storage->txn_scope_guard;
 
     my @op_condition;
-    for my $row ($subrs->search({}, { group_by => $subq_group_by })->cursor->all) {
+    for my $row ($subrs->cursor->all) {
       push @op_condition, { map
         { $idcols->[$_] => $row->[$_] }
         (0 .. $#$idcols)

diff --git a/t/resultset/update_delete.t b/t/resultset/update_delete.t
@@ -17,10 +17,11 @@ my $orig_debug = $schema->storage->debug;
 
 my $tkfks = $schema->resultset('FourKeys_to_TwoKeys');
 
-my ($fa, $fb) = $tkfks->related_resultset ('fourkeys')->populate ([
+my ($fa, $fb, $fc) = $tkfks->related_resultset ('fourkeys')->populate ([
   [qw/foo bar hello goodbye sensors read_count/],
   [qw/1   1   1     1       a       10         /],
   [qw/2   2   2     2       b       20         /],
+  [qw/1   1   1     2       c       30         /],
 ]);
 
 # This is already provided by DBICTest
@@ -48,8 +49,12 @@ is ($tkfks->count, $tkfk_cnt += 4, 'FourKeys_to_TwoKeys populated succesfully');
 #
 
 # create a resultset matching $fa and $fb only
-my $fks = $schema->resultset ('FourKeys')
-                  ->search ({ map { $_ => [1, 2] } qw/foo bar hello goodbye/}, { join => 'fourkeys_to_twokeys' });
+my $fks = $schema->resultset ('FourKeys')->search (
+  {
+    sensors => { '!=', 'c' },
+    ( map { $_ => [1, 2] } qw/foo bar hello goodbye/ ),
+  }, { join => 'fourkeys_to_twokeys'}
+);
 
 is ($fks->count, 4, 'Joined FourKey count correct (2x2)');
 
@@ -59,18 +64,43 @@ $fks->update ({ read_count => \ 'read_count + 1' });
 $schema->storage->debugobj ($orig_debugobj);
 $schema->storage->debug ($orig_debug);
 
+is_same_sql_bind (
+  $sql,
+  \@bind,
+  'UPDATE fourkeys
+   SET read_count = read_count + 1
+   WHERE ( ( ( bar = ? OR bar = ? ) AND ( foo = ? OR foo = ? ) AND ( goodbye = ? OR goodbye = ? ) AND ( hello = ? OR hello = ? ) AND sensors != ? ) )
+  ',
+  [ ("'1'", "'2'") x 4, "'c'" ],
+  'Correct update-SQL with multijoin with pruning',
+);
+
+is ($fa->discard_changes->read_count, 11, 'Update ran only once on discard-join resultset');
+is ($fb->discard_changes->read_count, 21, 'Update ran only once on discard-join resultset');
+is ($fc->discard_changes->read_count, 30, 'Update did not touch outlier');
+
+# make the multi-join stick
+$fks = $fks->search({ 'fourkeys_to_twokeys.pilot_sequence' => { '!=' => 666 } });
+
+$schema->storage->debugobj ($debugobj);
+$schema->storage->debug (1);
+$fks->update ({ read_count => \ 'read_count + 1' });
+$schema->storage->debugobj ($orig_debugobj);
+$schema->storage->debug ($orig_debug);
+
 is_same_sql_bind (
   $sql,
   \@bind,
   'UPDATE fourkeys
    SET read_count = read_count + 1
    WHERE ( bar = ? AND foo = ? AND goodbye = ? AND hello = ? ) OR ( bar = ? AND foo = ? AND goodbye = ? AND hello = ? )',
   [ map { "'$_'" } ( (1) x 4, (2) x 4 ) ],
-  'Correct update-SQL without multicolumn in support',
+  'Correct update-SQL with multijoin without pruning',
 );
 
-is ($fa->discard_changes->read_count, 11, 'Update ran only once on joined resultset');
-is ($fb->discard_changes->read_count, 21, 'Update ran only once on joined resultset');
+is ($fa->discard_changes->read_count, 12, 'Update ran only once on joined resultset');
+is ($fb->discard_changes->read_count, 22, 'Update ran only once on joined resultset');
+is ($fc->discard_changes->read_count, 30, 'Update did not touch outlier');
 
 # try the same sql with forced multicolumn in
 $schema->storage->_use_multicolumn_in (1);
@@ -90,11 +120,20 @@ is_same_sql_bind (
       (foo, bar, hello, goodbye) IN (
         SELECT me.foo, me.bar, me.hello, me.goodbye
           FROM fourkeys me
-        WHERE ( bar = ? OR bar = ? ) AND ( foo = ? OR foo = ? ) AND ( goodbye = ? OR goodbye = ? ) AND ( hello = ? OR hello = ? )
+          LEFT JOIN fourkeys_to_twokeys fourkeys_to_twokeys ON
+                fourkeys_to_twokeys.f_bar = me.bar
+            AND fourkeys_to_twokeys.f_foo = me.foo
+            AND fourkeys_to_twokeys.f_goodbye = me.goodbye
+            AND fourkeys_to_twokeys.f_hello = me.hello
+        WHERE fourkeys_to_twokeys.pilot_sequence != ? AND ( bar = ? OR bar = ? ) AND ( foo = ? OR foo = ? ) AND ( goodbye = ? OR goodbye = ? ) AND ( hello = ? OR hello = ? ) AND sensors != ?
       )
     )
   ',
-  [ map { "'$_'" } ( (1, 2) x 4 ) ],
+  [
+    "'666'",
+    ("'1'", "'2'") x 4,
+    "'c'",
+  ],
   'Correct update-SQL with multicolumn in support',
 );
 
@@ -180,24 +219,67 @@ $tkfks->search ({}, { rows => 1 })->delete;
 is ($tkfks->count, $tkfk_cnt -= 1, 'Only one row deleted');
 
 
-# Make sure prefetch is properly stripped too
-# check with sql-equality, as sqlite will accept bad sql just fine
+# check with sql-equality, as sqlite will accept most bad sql just fine
 $schema->storage->debugobj ($debugobj);
 $schema->storage->debug (1);
-$schema->resultset('CD')->search(
-  { year => { '!=' => 2010 } },
-  { prefetch => 'liner_notes' },
-)->delete;
+
+{
+  my $rs = $schema->resultset('CD')->search(
+    { 'me.year' => { '!=' => 2010 } },
+  );
+
+  $rs->search({}, { join => 'liner_notes' })->delete;
+  is_same_sql_bind (
+    $sql,
+    \@bind,
+    'DELETE FROM cd WHERE ( year != ? )',
+    ["'2010'"],
+    'Non-restricting multijoins properly thrown out'
+  );
+
+  $rs->search({}, { prefetch => 'liner_notes' })->delete;
+  is_same_sql_bind (
+    $sql,
+    \@bind,
+    'DELETE FROM cd WHERE ( year != ? )',
+    ["'2010'"],
+    'Non-restricting multiprefetch thrown out'
+  );
+
+  $rs->search({}, { prefetch => 'artist' })->delete;
+  is_same_sql_bind (
+    $sql,
+    \@bind,
+    'DELETE FROM cd WHERE ( cdid IN ( SELECT me.cdid FROM cd me JOIN artist artist ON artist.artistid = me.artist WHERE ( me.year != ? ) ) )',
+    ["'2010'"],
+    'Restricting prefetch left in, selector thrown out'
+  );
+
+  $rs->result_source->name('schema_qualified.cd');
+  # this is expected to fail - we only want to collect the generated SQL
+  eval { $rs->delete };
+  is_same_sql_bind (
+    $sql,
+    \@bind,
+    'DELETE FROM schema_qualified.cd WHERE ( year != ? )',
+    ["'2010'"],
+    'delete with fully qualified table name and subquery correct'
+  );
+
+  eval { $rs->search({}, { prefetch => 'artist' })->delete };
+  is_same_sql_bind (
+    $sql,
+    \@bind,
+    'DELETE FROM schema_qualified.cd WHERE ( cdid IN ( SELECT me.cdid FROM schema_qualified.cd me JOIN artist artist ON artist.artistid = me.artist WHERE ( me.year != ? ) ) )',
+    ["'2010'"],
+    'delete with fully qualified table name and subquery correct'
+  );
+
+  $rs->result_source->name('cd');
+}
 
 $schema->storage->debugobj ($orig_debugobj);
 $schema->storage->debug ($orig_debug);
 
-is_same_sql_bind (
-  $sql,
-  \@bind,
-  'DELETE FROM cd WHERE ( cdid IN ( SELECT me.cdid FROM cd me WHERE ( year != ? ) ) )',
-  ["'2010'"],
-  'Update on prefetching resultset strips prefetch correctly'
-);
 
 done_testing;