Skip to content

Commit

Permalink
This was an embarrassing close call - entirely redo custom set_from_r…
Browse files Browse the repository at this point in the history 
…elated

I am flailing at this point - unacceptable. TL;DR: 03f6d1f is wrong, 1adbd3f
is wrong, so is 5592d63 and most importantly 350e8d5 is unacceptably wrong.

The entire "I will inflate a bag of data into a synthetic object" was wrong
from the very beginning (03f6d1f) 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...
  • Loading branch information
@ribasushi
ribasushi committed Jul 25, 2014
1 parent e56f801 commit e884e5d
Show file tree
Hide file tree
Showing 6 changed files with 142 additions and 106 deletions.
4 changes: 2 additions & 2 deletions Changes
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
74 changes: 48 additions & 26 deletions lib/DBIx/Class/Relationship/Base.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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},
}
);
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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} );
Expand Down
103 changes: 50 additions & 53 deletions lib/DBIx/Class/ResultSource.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 ) : () ),
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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';
Expand All @@ -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" );
}
}

Expand All @@ -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 )
),
};

Expand All @@ -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;
}
Expand Down Expand Up @@ -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]);
}
}
}
}
Expand All @@ -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));
}
Expand All @@ -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) {

Expand Down Expand Up @@ -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) {
Expand All @@ -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 {
Expand Down
4 changes: 2 additions & 2 deletions t/lib/DBICTest/Schema/Track.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}
},
);
}
Expand Down
16 changes: 8 additions & 8 deletions t/lib/DBICTest/Util.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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;
}
Expand Down
Loading

0 comments on commit e884e5d

Please sign in to comment.