From: Peter Rabbitson Date: Mon, 19 Nov 2012 07:49:45 +0000 (+0100) Subject: Stop stripping table name qualifier on complex $rs->update/delete X-Git-Tag: v0.08205~101 X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?p=dbsrgits%2FDBIx-Class.git;a=commitdiff_plain;h=fd8076c89806c4e6222b032e5cc8f5b45e02c6e8 Stop stripping table name qualifier on complex $rs->update/delete RT#80015, RT#78844 --- diff --git a/Changes b/Changes index a5c8fe7..cf52651 100644 --- 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 index 1c757d9..f231f4e 100644 --- a/lib/DBIx/Class/ResultSet.pm +++ b/lib/DBIx/Class/ResultSet.pm @@ -1760,18 +1760,15 @@ 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 ( @@ -1779,32 +1776,33 @@ sub _rs_update_delete { $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 index 3de8bdb..d7fbb63 100644 --- 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)'); @@ -64,13 +69,38 @@ is_same_sql_bind ( \@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;