From: Peter Rabbitson Date: Sun, 5 Jul 2015 14:38:58 +0000 (+0200) Subject: Multiple common sense fixes for m2m accessors X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=5294fdc61249a48e2e330796f82e0f2035d0faaa;p=dbsrgits%2FDBIx-Class.git Multiple common sense fixes for m2m accessors Notably ensure set_X is executed in a transaction, and remove the erroneous call to _resolve_condition introduced back in 303cf522 and never questioned since --- diff --git a/Changes b/Changes index 5d0bd69..79647e8 100644 --- a/Changes +++ b/Changes @@ -17,6 +17,7 @@ Revision history for DBIx::Class notably on_connect* failures now properly abort the entire connect - Fix corner case of stringify-only overloaded objects being used in create()/populate() + - Fix several corner cases with Many2Many over custom relationships - Fix t/52leaks.t failures on compilerless systems (RT#104429) - Fix t/storage/quote_names.t failures on systems with specified Oracle test credentials while missing the optional Math::Base36 diff --git a/lib/DBIx/Class/Relationship/ManyToMany.pm b/lib/DBIx/Class/Relationship/ManyToMany.pm index 37c8461..6dfa22e 100644 --- a/lib/DBIx/Class/Relationship/ManyToMany.pm +++ b/lib/DBIx/Class/Relationship/ManyToMany.pm @@ -84,7 +84,7 @@ EOW "${add_meth} needs an object or hashref" ); - my $link = $self->search_related_rs($rel)->new_result( + my $link = $self->new_related( $rel, ( @_ > 1 && ref $_[-1] eq 'HASH' ) ? pop : {} @@ -100,6 +100,7 @@ EOW ; $link->set_from_related($f_rel, $far_obj); + $link->insert(); return $far_obj; @@ -111,37 +112,36 @@ EOW @_ > 0 or $self->throw_exception( "{$set_meth} needs a list of objects or hashrefs" ); - my @to_set = (ref($_[0]) eq 'ARRAY' ? @{ $_[0] } : @_); + + my $guard = $self->result_source->schema->storage->txn_scope_guard; + # if there is a where clause in the attributes, ensure we only delete # rows that are within the where restriction + if ($rel_attrs && $rel_attrs->{where}) { $self->search_related( $rel, $rel_attrs->{where},{join => $f_rel})->delete; } else { $self->search_related( $rel, {} )->delete; } # add in the set rel objects - $self->$add_meth($_, ref($_[1]) ? $_[1] : {}) for (@to_set); + $self->$add_meth($_, ref($_[1]) ? $_[1] : {}) + for ( ref($_[0]) eq 'ARRAY' ? @{ $_[0] } : @_ ); + + $guard->commit; }; my $remove_meth_name = join '::', $class, $remove_meth; *$remove_meth_name = subname $remove_meth_name, sub { my ($self, $obj) = @_; + $self->throw_exception("${remove_meth} needs an object") unless blessed ($obj); - my $rel_source = $self->search_related($rel)->result_source; - my $cond = $rel_source->relationship_info($f_rel)->{cond}; - my ($link_cond, $crosstable) = $rel_source->_resolve_condition( - $cond, $obj, $f_rel, $f_rel - ); - $self->throw_exception( - "Relationship '$rel' does not resolve to a join-free condition, " - ."unable to use with the ManyToMany helper '$f_rel'" - ) if $crosstable; - - $self->search_related($rel, $link_cond)->delete; + $self->search_related_rs($rel)->search_rs( + $obj->ident_condition( $f_rel ), + { join => $f_rel }, + )->delete; }; - } } diff --git a/t/lib/DBICTest/Schema/Artwork_to_Artist.pm b/t/lib/DBICTest/Schema/Artwork_to_Artist.pm index 052da73..c84d74f 100644 --- a/t/lib/DBICTest/Schema/Artwork_to_Artist.pm +++ b/t/lib/DBICTest/Schema/Artwork_to_Artist.pm @@ -33,10 +33,13 @@ __PACKAGE__->belongs_to('artist_limited_rank', 'DBICTest::Schema::Artist', { "$args->{foreign_alias}.artistid" => { -ident => "$args->{self_alias}.artist_id" }, "$args->{foreign_alias}.rank" => { '<' => 10 }, }, - $args->{self_result_object} && { + !$args->{self_result_object} ? () : { "$args->{foreign_alias}.artistid" => $args->{self_result_object}->artist_id, "$args->{foreign_alias}.rank" => { '<' => 10 }, - } + }, + !$args->{foreign_values} ? () : { + "$args->{self_alias}.artist_id" => $args->{foreign_values}{artistid}, + }, ); } ); diff --git a/t/relationship/custom.t b/t/relationship/custom.t index 4d3052b..058636b 100644 --- a/t/relationship/custom.t +++ b/t/relationship/custom.t @@ -219,7 +219,7 @@ is_deeply ( my $artwork = $schema->resultset('Artwork')->search({},{ order_by => 'cd_id' })->first; -my @artists = $artwork->artists->all; +my @artists = $artwork->artists->search({}, { order_by => 'artistid' } ); is(scalar @artists, 2, 'the two artists are associated'); my @artwork_artists = $artwork->artwork_to_artist->all; @@ -265,6 +265,62 @@ for (qw( artist_limited_rank artist_limited_rank_opaque )) { 1, 'Expected one m2m associated artist object via opaque custom cond + conditional far cond' ); + + cmp_ok( + $artwork->${\"remove_from_$_"} ( $artists[1] ), + '==', + 0, + 'deletion action reports 0' + ); + + is ( + scalar $artwork->all_artists_via_opaque_customcond->all, + 2, + 'Indeed nothing was removed' + ); + + cmp_ok( + $artwork->${\"remove_from_$_"} ( $artists[0] ), + '==', + 1, + 'Removal reports correct count' + ); + + is ( + scalar $artwork->all_artists_via_opaque_customcond->all, + 1, + 'Indeed removed the matching artist' + ); + + $artwork->${\"set_$_"}([]); + + is ( + scalar $artwork->all_artists_via_opaque_customcond->all, + 0, + 'Everything removed via limited far cond' + ); + + # can't use the opaque one - need set_from_related to work + $artwork->set_artist_limited_rank( @artists ); + + { + local $TODO = 'Taking into account the relationship bridge condition is not likely to ever work... unless we get DQ hooked somehow'; + + is ( + scalar $artwork->all_artists_via_opaque_customcond->all, + 1, + 'Re-Addition passed through only one of the artists' + ); + } + + throws_ok { $artwork->set_all_artists_via_opaque_customcond( \@artists ) } + qr/\QRelationship 'artwork_to_artist_via_opaque_customcond' on source 'Artwork' does not resolve to a join-free condition fragment/; + + is ( + scalar $artwork->all_artists_via_opaque_customcond->all, + 2, + 'Everything still there as expected' + ); }