From: Peter Rabbitson Date: Fri, 25 Jul 2014 12:35:19 +0000 (+0200) Subject: This was an embarrassing close call - entirely redo custom set_from_related X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=e884e5d9de1fcb734ab4598e6b2923293b0674f7;p=dbsrgits%2FDBIx-Class-Historic.git This was an embarrassing close call - entirely redo custom set_from_related 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... --- diff --git a/Changes b/Changes index a4f3870..44b873e 100644 --- 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) diff --git a/lib/DBIx/Class/Relationship/Base.pm b/lib/DBIx/Class/Relationship/Base.pm index e2efc99..882d47b 100644 --- a/lib/DBIx/Class/Relationship/Base.pm +++ b/lib/DBIx/Class/Relationship/Base.pm @@ -182,13 +182,31 @@ clause of the C statement associated with this relationship. While every coderef-based condition must return a valid C clause, it may elect to additionally return a simplified B 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. 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 in the form of a hashref. If a foreign result object is supplied +(which is valid usage of L), its values will be extracted +into hashref form by calling L. + +=back + +Note that the above scenarios are mutually exclusive, that is you will be supplied +none or only one of C and C. 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. For -instance the C 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 (and by extension +L<< $result->create_related|DBIx::Class::Relationship::Base/create_related >>), +the returned join free condition B contain only plain values/deflatable +objects. For instance the C 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 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} ); diff --git a/lib/DBIx/Class/ResultSource.pm b/lib/DBIx/Class/ResultSource.pm index 7e0ae81..33a0ca4 100644 --- a/lib/DBIx/Class/ResultSource.pm +++ b/lib/DBIx/Class/ResultSource.pm @@ -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 { diff --git a/t/lib/DBICTest/Schema/Track.pm b/t/lib/DBICTest/Schema/Track.pm index 3cfbc31..5b0811e 100644 --- a/t/lib/DBICTest/Schema/Track.pm +++ b/t/lib/DBICTest/Schema/Track.pm @@ -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} }, ); } diff --git a/t/lib/DBICTest/Util.pm b/t/lib/DBICTest/Util.pm index 0588a9d..3760df8 100644 --- a/t/lib/DBICTest/Util.pm +++ b/t/lib/DBICTest/Util.pm @@ -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; } diff --git a/t/relationship/custom.t b/t/relationship/custom.t index 899b244..131ebe1 100644 --- a/t/relationship/custom.t +++ b/t/relationship/custom.t @@ -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;