This was an embarrassing close call - entirely redo custom set_from_related
Peter Rabbitson [Fri, 25 Jul 2014 12:35:19 +0000 (14:35 +0200)]
I am flailing at this point - unacceptable. TL;DR: 03f6d1f7 is wrong, 1adbd3fc
is wrong, so is 5592d63 and most importantly 350e8d57 is unacceptably wrong.

The entire "I will inflate a bag of data into a synthetic object" was wrong
from the very beginning (03f6d1f7) but I was too busy keeping the tets passing
to realize what crack-mountain I was standing on.

The crux of the issue is: it didn't occur to me that forward-resolution
(related_resultset) *always* starts from an object by definition, while
reverse-resolution (set_from_related) takes either an object or a bag of
values or undef. Moreover: in the custom coderef the user does not care at
all about the type - all they want is to get their values. In light of this
self_result_object is actually a mistake - we should have simply provided
self_values from the start, but that ship has already sailed. On the other
hand the reverse-resolution bag is a new feature, and we can simply (and
correctly) settle on passing in a hashref of values: foreign_values

This fully avoids the "how do I get an object out of plain data" problem
and nicely settles all the outsanding (untested until now) problems.

I just wish I would have seen this a week earlier... sigh. At least not
shipping this live is a consolation...

Changes
lib/DBIx/Class/Relationship/Base.pm
lib/DBIx/Class/ResultSource.pm
t/lib/DBICTest/Schema/Track.pm
t/lib/DBICTest/Util.pm
t/relationship/custom.t

diff --git a/Changes b/Changes
index a4f3870..44b873e 100644 (file)
--- a/Changes
+++ b/Changes
@@ -7,8 +7,8 @@ Revision history for DBIx::Class
           returned from storage
         - Custom condition relationships are now invoked with a slightly
           different signature (existing coderefs will continue to work)
-        - Add extra custom condition coderef attribute 'foreign_result_object'
-          to allow for proper reverse-relationship emulation
+        - Add extra custom condition coderef attribute 'foreign_values'
+          to allow for proper reverse-relationship-like behavior
           (i.e. $result->set_from_related($custom_rel, $foreign_result_object)
         - When in a transaction, DBIC::Ordered now seamlesly handles result
           objects that went out of sync with the storage (RT#96499)
index e2efc99..882d47b 100644 (file)
@@ -182,13 +182,31 @@ clause of the C<JOIN> statement associated with this relationship.
 
 While every coderef-based condition must return a valid C<ON> clause, it may
 elect to additionally return a simplified B<optional> join-free condition
-hashref when invoked as C<< $result->$relationship >>, as opposed to
-C<< $rs->related_resultset('relationship') >>. In this case C<$result> is
-passed to the coderef as C<< $args->{self_result_object} >>. Alternatively
-the user-space could be calling C<< $result->set_from_related( $rel =>
-$foreign_related_object ) >>, in which case C<$foreign_related_object> will
-be passed to the coderef as C<< $args->{foreign_result_object >>. In other
-words if you define your condition coderef as:
+consisting of a hashref with B<all keys being fully qualified names of columns
+declared on the corresponding result source>. This boils down to two scenarios:
+
+=over
+
+=item *
+
+When relationship resolution is invoked after C<< $result->$rel_name >>, as
+opposed to C<< $rs->related_resultset($rel_name) >>, the C<$result> object
+is passed to the coderef as C<< $args->{self_result_object} >>.
+
+=item *
+
+Alternatively when the user-space invokes resolution via
+C<< $result->set_from_related( $rel_name => $foreign_values_or_object ) >>, the
+corresponding data is passed to the coderef as C<< $args->{foreign_values} >>,
+B<always> in the form of a hashref. If a foreign result object is supplied
+(which is valid usage of L</set_from_related>), its values will be extracted
+into hashref form by calling L<get_columns|DBIx::Class::Row/get_columns>.
+
+=back
+
+Note that the above scenarios are mutually exclusive, that is you will be supplied
+none or only one of C<self_result_object> and C<foreign_values>. In other words if
+you define your condition coderef as:
 
   sub {
     my $args = shift;
@@ -202,8 +220,8 @@ words if you define your condition coderef as:
         "$args->{foreign_alias}.artist" => $args->{self_result_object}->artistid,
         "$args->{foreign_alias}.year"   => { '>', "1979", '<', "1990" },
       },
-      ! $args->{foreign_result_object} ? () : {
-        "$args->{self_alias}.artistid" => $args->{foreign_result_object}->artist,
+      ! $args->{foreign_values} ? () : {
+        "$args->{self_alias}.artistid" => $args->{foreign_values}{artist},
       }
     );
   }
@@ -233,34 +251,38 @@ While this code:
 
 Will properly set the C<< $artist->artistid >> field of this new object to C<1>
 
-Note that in order to be able to use
-L<< $result->create_related|DBIx::Class::Relationship::Base/create_related >>,
-the coderef must not only return as its second such a "simple" condition
-hashref which does not depend on joins being available, but the hashref must
-contain only plain values/deflatable objects, such that the result can be
-passed directly to L<DBIx::Class::Relationship::Base/set_from_related>. For
-instance the C<year> constraint in the above example prevents the relationship
-from being used to create related objects (an exception will be thrown).
+Note that in order to be able to use L</set_from_related> (and by extension
+L<< $result->create_related|DBIx::Class::Relationship::Base/create_related >>),
+the returned join free condition B<must> contain only plain values/deflatable
+objects. For instance the C<year> constraint in the above example prevents
+the relationship from being used to create related objects using
+C<< $artst->create_related( cds_80s => { title => 'blah' } ) >> (an
+exception will be thrown).
 
 In order to allow the user to go truly crazy when generating a custom C<ON>
 clause, the C<$args> hashref passed to the subroutine contains some extra
 metadata. Currently the supplied coderef is executed as:
 
   $relationship_info->{cond}->({
-    self_resultsource     => The resultsource instance on which rel_name is registered
-    rel_name              => The relationship name (does *NOT* always match foreign_alias)
+    self_resultsource   => The resultsource instance on which rel_name is registered
+    rel_name            => The relationship name (does *NOT* always match foreign_alias)
 
-    self_alias            => The alias of the invoking resultset
-    foreign_alias         => The alias of the to-be-joined resultset (does *NOT* always match rel_name)
+    self_alias          => The alias of the invoking resultset
+    foreign_alias       => The alias of the to-be-joined resultset (does *NOT* always match rel_name)
 
     # only one of these (or none at all) will ever be supplied to aid in the
     # construction of a join-free condition
-    self_result_object    => The invocant object itself in case of a $result_object->$rel_name( ... ) call
-    foreign_result_object => The related object in case of $result_object->set_from_related( $rel_name, $foreign_result_object )
+
+    self_result_object  => The invocant *object* itself in case of a call like
+                           $result_object->$rel_name( ... )
+
+    foreign_values      => A *hashref* of related data: may be passed in directly or
+                           derived via ->get_columns() from a related object in case of
+                           $result_object->set_from_related( $rel_name, $foreign_result_object )
 
     # deprecated inconsistent names, will be forever available for legacy code
-    self_rowobj           => Old deprecated slot for self_result_object
-    foreign_relname       => Old deprecated slot for rel_name
+    self_rowobj         => Old deprecated slot for self_result_object
+    foreign_relname     => Old deprecated slot for rel_name
   });
 
 =head3 attributes
@@ -798,7 +820,7 @@ sub set_from_related {
   $self->set_columns( $self->result_source->_resolve_relationship_condition (
     infer_values_based_on => {},
     rel_name => $rel,
-    foreign_result_object => $f_obj,
+    foreign_values => $f_obj,
     foreign_alias => $rel,
     self_alias => 'me',
   )->{inferred_values} );
index 7e0ae81..33a0ca4 100644 (file)
@@ -1722,9 +1722,9 @@ sub _resolve_condition {
 
     # where-is-waldo block guesses relname, then further down we override it if available
     (
-      $is_objlike[1] ? ( rel_name => $res_args[0], self_alias => $res_args[0], foreign_alias => 'me',         self_result_object    => $res_args[1] )
-    : $is_objlike[0] ? ( rel_name => $res_args[1], self_alias => 'me',         foreign_alias => $res_args[1], foreign_result_object => $res_args[0] )
-    :                  ( rel_name => $res_args[0], self_alias => $res_args[1], foreign_alias => $res_args[0]                                    )
+      $is_objlike[1] ? ( rel_name => $res_args[0], self_alias => $res_args[0], foreign_alias => 'me',         self_result_object  => $res_args[1] )
+    : $is_objlike[0] ? ( rel_name => $res_args[1], self_alias => 'me',         foreign_alias => $res_args[1], foreign_values      => $res_args[0] )
+    :                  ( rel_name => $res_args[0], self_alias => $res_args[1], foreign_alias => $res_args[0]                                      )
     ),
 
     ( $rel_name ? ( rel_name => $rel_name ) : () ),
@@ -1767,11 +1767,11 @@ Internals::SvREADONLY($UNRESOLVABLE_CONDITION => 1);
 ## self-explanatory API, modeled on the custom cond coderef:
 # rel_name
 # foreign_alias
-# foreign_result_object
+# foreign_values
 # self_alias
 # self_result_object
 # require_join_free_condition
-# infer_values_based_on (optional, mandatory hashref argument)
+# infer_values_based_on (either not supplied or a hashref, implies require_join_free_condition)
 # condition (optional, derived from $self->rel_info(rel_name))
 #
 ## returns a hash
@@ -1799,7 +1799,7 @@ sub _resolve_relationship_condition {
     or $self->throw_exception( "No such $exception_rel_id" );
 
   $self->throw_exception("No practical way to resolve $exception_rel_id between two data structures")
-    if defined $args->{self_result_object} and defined $args->{foreign_result_object};
+    if exists $args->{self_result_object} and exists $args->{foreign_values};
 
   $self->throw_exception( "Argument to infer_values_based_on must be a hash" )
     if exists $args->{infer_values_based_on} and ref $args->{infer_values_based_on} ne 'HASH';
@@ -1808,29 +1808,31 @@ sub _resolve_relationship_condition {
 
   $args->{condition} ||= $rel_info->{cond};
 
+  my $rel_rsrc = $self->related_source($args->{rel_name});
+
   if (exists $args->{self_result_object}) {
-    if (defined blessed $args->{self_result_object}) {
-      $self->throw_exception( "Object '$args->{self_result_object}' must be of class '@{[ $self->result_class ]}'" )
-        unless $args->{self_result_object}->isa($self->result_class);
-    }
-    else {
-      $args->{self_result_object} = DBIx::Class::Core->new({
-        -result_source => $self,
-        %{ $args->{self_result_object}||{} }
-      });
-    }
+    $self->throw_exception( "Argument 'self_result_object' must be an object of class '@{[ $self->result_class ]}'" )
+      unless defined blessed $args->{self_result_object};
+
+    $self->throw_exception( "Object '$args->{self_result_object}' must be of class '@{[ $self->result_class ]}'" )
+      unless $args->{self_result_object}->isa($self->result_class);
   }
 
-  if (exists $args->{foreign_result_object}) {
-    if (defined blessed $args->{foreign_result_object}) {
-      $self->throw_exception( "Object '$args->{foreign_result_object}' must be of class '$rel_info->{class}'" )
-        unless $args->{foreign_result_object}->isa($rel_info->{class});
+  if (exists $args->{foreign_values}) {
+    if (defined blessed $args->{foreign_values}) {
+      $self->throw_exception( "Object supplied as 'foreign_values' ($args->{foreign_values}) must be of class '$rel_info->{class}'" )
+        unless $args->{foreign_values}->isa($rel_info->{class});
+
+      $args->{foreign_values} = { $args->{foreign_values}->get_columns };
+    }
+    elsif (! defined $args->{foreign_values} or ref $args->{foreign_values} eq 'HASH') {
+      my $ci = $rel_rsrc->columns_info;
+      ! exists $ci->{$_} and $self->throw_exception(
+        "Key '$_' supplied as 'foreign_values' is not a column on related source '@{[ $rel_rsrc->source_name ]}'"
+      ) for keys %{ $args->{foreign_values} ||= {} };
     }
     else {
-      $args->{foreign_result_object} = DBIx::Class::Core->new({
-        -result_source => $self->related_source($args->{rel_name}),
-        %{ $args->{foreign_result_object}||{} }
-      });
+      $self->throw_exception( "Argument 'foreign_values' must be either an object inheriting from '$rel_info->{class}' or a hash reference or undef" );
     }
   }
 
@@ -1845,7 +1847,7 @@ sub _resolve_relationship_condition {
       foreign_alias => $args->{foreign_alias},
       ( map
         { (exists $args->{$_}) ? ( $_ => $args->{$_} ) : () }
-        qw( self_result_object foreign_result_object )
+        qw( self_result_object foreign_values )
       ),
     };
 
@@ -1870,9 +1872,9 @@ sub _resolve_relationship_condition {
       my ($joinfree_alias, $joinfree_source);
       if (defined $args->{self_result_object}) {
         $joinfree_alias = $args->{foreign_alias};
-        $joinfree_source = $self->related_source($args->{rel_name});
+        $joinfree_source = $rel_rsrc;
       }
-      elsif (defined $args->{foreign_result_object}) {
+      elsif (defined $args->{foreign_values}) {
         $joinfree_alias = $args->{self_alias};
         $joinfree_source = $self;
       }
@@ -1920,39 +1922,32 @@ sub _resolve_relationship_condition {
       $ret->{identity_map}{$l_cols[$_]} = $f_cols[$_];
     };
 
-    if (exists $args->{self_result_object} or exists $args->{foreign_result_object}) {
-
-      my ($obj, $obj_alias, $plain_alias, $obj_cols, $plain_cols) = defined $args->{self_result_object}
-        ? ( @{$args}{qw( self_result_object self_alias foreign_alias )}, \@l_cols, \@f_cols )
-        : ( @{$args}{qw( foreign_result_object foreign_alias self_alias )}, \@f_cols, \@l_cols )
-      ;
-
-      for my $i (0..$#$obj_cols) {
-
-        if (
-          defined $args->{self_result_object}
-            and
-          ! $obj->has_column_loaded($obj_cols->[$i])
-        ) {
+    if ($args->{foreign_values}) {
+      $ret->{join_free_condition}{"$args->{self_alias}.$l_cols[$_]"} = $args->{foreign_values}{$f_cols[$_]}
+        for 0..$#f_cols;
+    }
+    elsif (defined $args->{self_result_object}) {
 
+      for my $i (0..$#l_cols) {
+        if ( $args->{self_result_object}->has_column_loaded($l_cols[$i]) ) {
+          $ret->{join_free_condition}{"$args->{foreign_alias}.$f_cols[$i]"} = $args->{self_result_object}->get_column($l_cols[$i]);
+        }
+        else {
           $self->throw_exception(sprintf
             "Unable to resolve relationship '%s' from object '%s': column '%s' not "
           . 'loaded from storage (or not passed to new() prior to insert()). You '
           . 'probably need to call ->discard_changes to get the server-side defaults '
           . 'from the database.',
             $args->{rel_name},
-            $obj,
-            $obj_cols->[$i],
-          ) if $obj->in_storage;
+            $args->{self_result_object},
+            $l_cols[$i],
+          ) if $args->{self_result_object}->in_storage;
 
           # FIXME - temporarly force-override
           delete $args->{require_join_free_condition};
           $ret->{join_free_condition} = UNRESOLVABLE_CONDITION;
           last;
         }
-        else {
-          $ret->{join_free_condition}{"$plain_alias.$plain_cols->[$i]"} = $obj->get_column($obj_cols->[$i]);
-        }
       }
     }
   }
@@ -1976,8 +1971,8 @@ sub _resolve_relationship_condition {
         { $self->_resolve_relationship_condition({ %$args, condition => $_ }) }
         @{$args->{condition}}
       ) {
-        $self->throw_exception('Either all or none of the OR-condition members can resolve to a join-free condition')
-          if $ret->{join_free_condition} and ! $subcond->{join_free_condition};
+        $self->throw_exception('Either all or none of the OR-condition members must resolve to a join-free condition')
+          if ( $ret and ( $ret->{join_free_condition} xor $subcond->{join_free_condition} ) );
 
         $subcond->{$_} and push @{$ret->{$_}}, $subcond->{$_} for (qw(condition join_free_condition));
       }
@@ -1993,11 +1988,13 @@ sub _resolve_relationship_condition {
     ( ! $ret->{join_free_condition} or $ret->{join_free_condition} eq UNRESOLVABLE_CONDITION )
   );
 
+  my $storage = $self->schema->storage;
+
   # we got something back - sanity check and infer values if we can
   my @nonvalues;
   if ( my $jfc = $ret->{join_free_condition} and $ret->{join_free_condition} ne UNRESOLVABLE_CONDITION ) {
 
-    my $jfc_eqs = $self->schema->storage->_extract_fixed_condition_columns($jfc, 'consider_nulls');
+    my $jfc_eqs = $storage->_extract_fixed_condition_columns($jfc, 'consider_nulls');
 
     if (keys %$jfc_eqs) {
 
@@ -2037,7 +2034,7 @@ sub _resolve_relationship_condition {
   # (may already be there, since easy to calculate on the fly in the HASH case)
   if ( ! $ret->{identity_map} ) {
 
-    my $col_eqs = $self->schema->storage->_extract_fixed_condition_columns($ret->{condition});
+    my $col_eqs = $storage->_extract_fixed_condition_columns($ret->{condition});
 
     my $colinfos;
     for my $lhs (keys %$col_eqs) {
@@ -2047,9 +2044,9 @@ sub _resolve_relationship_condition {
 
       # there is no way to know who is right and who is left
       # therefore the ugly scan below
-      $colinfos ||= $self->schema->storage->_resolve_column_info([
+      $colinfos ||= $storage->_resolve_column_info([
         { -alias => $args->{self_alias}, -rsrc => $self },
-        { -alias => $args->{foreign_alias}, -rsrc => $self->related_source($args->{rel_name}) },
+        { -alias => $args->{foreign_alias}, -rsrc => $rel_rsrc },
       ]);
 
       my ($l_col, $l_alias, $r_col, $r_alias) = map {
index 3cfbc31..5b0811e 100644 (file)
@@ -70,8 +70,8 @@ sub {
      "$args->{foreign_alias}.cdid" => $args->{self_result_object}->cd
     },
 
-    ! $args->{foreign_result_object} ? () : {
-     "$args->{self_alias}.cd" => $args->{foreign_result_object}->cdid
+    ! $args->{foreign_values} ? () : {
+     "$args->{self_alias}.cd" => $args->{foreign_values}{cdid}
     },
   );
 }
index 0588a9d..3760df8 100644 (file)
@@ -99,10 +99,10 @@ sub check_customcond_args ($) {
   confess "Passed resultsource has no record of the supplied rel_name - likely wrong \$rsrc"
     unless ref $args->{self_resultsource}->relationship_info($args->{rel_name});
 
-  my $rowobj_cnt = 0;
+  my $struct_cnt = 0;
 
   if (defined $args->{self_result_object} or defined $args->{self_rowobj} ) {
-    $rowobj_cnt++;
+    $struct_cnt++;
     for (qw(self_result_object self_rowobj)) {
       confess "Custom condition argument '$_' must be a result instance"
         unless defined blessed $args->{$_} and $args->{$_}->isa('DBIx::Class::Row');
@@ -112,15 +112,15 @@ sub check_customcond_args ($) {
       if refaddr($args->{self_result_object}) != refaddr($args->{self_rowobj});
   }
 
-  if (defined $args->{foreign_result_object}) {
-    $rowobj_cnt++;
+  if (defined $args->{foreign_values}) {
+    $struct_cnt++;
 
-    confess "Custom condition argument 'foreign_result_object' must be a result instance"
-      unless defined blessed $args->{foreign_result_object} and $args->{foreign_result_object}->isa('DBIx::Class::Row');
+    confess "Custom condition argument 'foreign_values' must be a hash reference"
+      unless ref $args->{foreign_values} eq 'HASH';
   }
 
-  confess "Result objects supplied on both ends of a relationship"
-    if $rowobj_cnt == 2;
+  confess "Data structures supplied on both ends of a relationship"
+    if $struct_cnt == 2;
 
   $args;
 }
index 899b244..131ebe1 100644 (file)
@@ -273,21 +273,38 @@ is_deeply (
   'Prefetched singles in proper order'
 );
 
-# test set_from_related with a belongs_to custom condition
-my $cd = $schema->resultset("CD")->find(4);
-$artist = $cd->search_related('artist');
-my $track = $schema->resultset("Track")->create( {
-  trackid => 1,
-  cd => 3,
-  position => 99,
-  title => 'Some Track'
-} );
-$track->set_from_related( cd_cref_cond => $cd );
-is ($track->get_column('cd'), 4, 'set from related via coderef cond');
-is_deeply (
-  { $track->cd->get_columns },
-  { $cd->get_columns },
+# test set_from_related/find_related with a belongs_to custom condition
+my $preexisting_cd = $schema->resultset('CD')->find(1);
+
+my $cd_single_track = $schema->resultset('CD')->create({
+  artist => $artist,
+  title => 'one one one',
+  year => 2001,
+  tracks => [{ title => 'uno uno uno' }]
+});
+
+my $single_track = $cd_single_track->tracks->next;
+
+is_deeply
+  { $schema->resultset('Track')->find({ cd_cref_cond => { cdid => $cd_single_track->id } })->get_columns },
+  { $single_track->get_columns },
+  'Proper find with related via coderef cond',
+;
+
+$single_track->set_from_related( cd_cref_cond => undef );
+ok $single_track->is_column_changed('cd');
+is $single_track->get_column('cd'), undef, 'UNset from related via coderef cond';
+is $single_track->cd, undef, 'UNset related object via coderef cond';
+
+$single_track->discard_changes;
+
+$single_track->set_from_related( cd_cref_cond => $preexisting_cd );
+ok $single_track->is_column_changed('cd');
+is $single_track->get_column('cd'), 1, 'set from related via coderef cond';
+is_deeply
+  { $single_track->cd->get_columns },
+  { $preexisting_cd->get_columns },
   'set from related via coderef cond inflates properly',
-);
+;
 
 done_testing;