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 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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'
+  );
 }