From: Peter Rabbitson Date: Mon, 19 Nov 2012 09:11:11 +0000 (+0100) Subject: Fix proper handling of composite resultset heads (e.g. as_subselect_rs) X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=31073ac79e22ae0f977c7e0fc4e9857d250143c4;p=dbsrgits%2FDBIx-Class-Historic.git 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 --- 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 --- 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 --- 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;