From: Peter Rabbitson Date: Fri, 15 May 2009 21:35:06 +0000 (+0000) Subject: A workable fix for the resultset multicol update/delete debacle - by default fallback... X-Git-Tag: v0.08103~64^2~9 X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=b2f73c3063de927725954bdf864f8e1291a5dedd;p=dbsrgits%2FDBIx-Class.git A workable fix for the resultset multicol update/delete debacle - by default fallback to per-row deletions, with the ability to overide this behavior for various storage drivers --- diff --git a/lib/DBIx/Class/ResultSet.pm b/lib/DBIx/Class/ResultSet.pm index 22c7237..079e0e4 100644 --- a/lib/DBIx/Class/ResultSet.pm +++ b/lib/DBIx/Class/ResultSet.pm @@ -1326,6 +1326,18 @@ sub first { return $_[0]->reset->next; } + +# _update_delete_via_subq +# +# Presence of some rs attributes requires a subquery to reliably +# update/deletre +# + +sub _update_delete_via_subq { + return $_[0]->_has_attr (qw/join seen_join group_by row offset page/); +} + + # _cond_for_update_delete # # update/delete require the condition to be modified to handle @@ -1340,18 +1352,6 @@ sub _cond_for_update_delete { # No-op. No condition, we're updating/deleting everything return $cond unless ref $full_cond; - # Some attributes when present require a subquery - # This might not work on some database (mysql), but... - # it won't work without the subquery either so who cares - if ($self->_has_attr (qw/join seen_join group_by row offset page/) ) { - - foreach my $pk ($self->result_source->primary_columns) { - $cond->{$pk} = { -in => $self->get_column($pk)->as_query }; - } - - return $cond; - } - if (ref $full_cond eq 'ARRAY') { $cond = [ map { @@ -1391,7 +1391,7 @@ sub _cond_for_update_delete { else { $self->throw_exception("Can't update/delete on resultset with condition unless hash or array"); } - + return $cond; } @@ -1414,9 +1414,14 @@ if no records were updated; exact type of success value is storage-dependent. sub update { my ($self, $values) = @_; - $self->throw_exception("Values for update must be a hash") + $self->throw_exception('Values for update must be a hash') unless ref $values eq 'HASH'; + # rs operations with subqueries are Storage dependent - delegate + if ($self->_update_delete_via_subq) { + return $self->result_source->storage->subq_update_delete($self, 'update', $values); + } + my $cond = $self->_cond_for_update_delete; return $self->result_source->storage->update( @@ -1441,7 +1446,7 @@ will run DBIC cascade triggers, while L will not. sub update_all { my ($self, $values) = @_; - $self->throw_exception("Values for update must be a hash") + $self->throw_exception('Values for update_all must be a hash') unless ref $values eq 'HASH'; foreach my $obj ($self->all) { $obj->set_columns($values)->update; @@ -1472,9 +1477,14 @@ need to respecify your query in a way that can be expressed without a join. =cut sub delete { - my ($self) = @_; - $self->throw_exception("Delete should not be passed any arguments") - if $_[1]; + my $self = shift; + $self->throw_exception('delete does not accept any arguments') + if @_; + + # rs operations with subqueries are Storage dependent - delegate + if ($self->_update_delete_via_subq) { + return $self->result_source->storage->subq_update_delete($self, 'delete'); + } my $cond = $self->_cond_for_update_delete; @@ -1498,7 +1508,10 @@ will run DBIC cascade triggers, while L will not. =cut sub delete_all { - my ($self) = @_; + my $self = shift; + $self->throw_exception('delete_all does not accept any arguments') + if @_; + $_->delete for $self->all; return 1; } diff --git a/lib/DBIx/Class/Storage/DBI.pm b/lib/DBIx/Class/Storage/DBI.pm index 8df1894..8eb67fa 100644 --- a/lib/DBIx/Class/Storage/DBI.pm +++ b/lib/DBIx/Class/Storage/DBI.pm @@ -1051,6 +1051,85 @@ sub delete { return $self->_execute('delete' => [], $source, $bind_attrs, @_); } +# We were sent here because the $rs contains a complex search +# which will require a subquery to select the correct rows +# (i.e. joined or limited resultsets) +# +# Genarating a single PK column subquery is trivial and supported +# by all RDBMS. However if we have a multicolumn PK, things get ugly. +# Look at multipk_update_delete() +sub subq_update_delete { + my $self = shift; + my ($rs, $op, $values) = @_; + + if ($rs->result_source->primary_columns == 1) { + return $self->_onepk_update_delete (@_); + } + else { + return $self->_multipk_update_delete (@_); + } +} + +# Generally a single PK resultset operation is trivially expressed +# with PK IN (subquery). However some databases (mysql) do not support +# modification of a table mentioned in the subselect. This method +# should be overriden in the appropriate storage class to be smarter +# in such situations +sub _onepk_update_delete { + + my $self = shift; + my ($rs, $op, $values) = @_; + + my $rsrc = $rs->result_source; + my @pcols = $rsrc->primary_columns; + + return $self->$op ( + $rsrc, + $op eq 'update' ? $values : (), + { $pcols[0] => { -in => $rs->get_column ($pcols[0])->as_query } }, + ); +} + +# ANSI SQL does not provide a reliable way to perform a multicol-PK +# resultset update/delete involving subqueries. So resort to simple +# (and inefficient) delete_all style per-row opearations, while allowing +# specific storages to override this with a faster implementation. +# +# We do not use $row->$op style queries, because resultset update/delete +# is not expected to cascade (this is what delete_all/update_all is for). +# +# There should be no race conditions as the entire operation is rolled +# in a transaction. +sub _multipk_update_delete { + my $self = shift; + my ($rs, $op, $values) = @_; + + my $rsrc = $rs->result_source; + my @pcols = $rsrc->primary_columns; + + my $guard = $self->txn_scope_guard; + + my $subrs_cur = $rs->search ({}, { columns => \@pcols })->cursor; + while (my @pks = $subrs_cur->next) { + + my $cond; + for my $i (0.. $#pcols) { + $cond->{$pcols[$i]} = $pks[$i]; + } + + $self->$op ( + $rsrc, + $op eq 'update' ? $values : (), + $cond, + ); + } + + $guard->commit; + + return 1; +} + + sub _select { my $self = shift; my $sql_maker = $self->sql_maker;