Skip to content

Commit

Permalink
Fix fallout from 786c1cd
Browse files Browse the repository at this point in the history
Before the interface of resolve_relationship_condition was simplified, it
contained a codepath exempting result objects from any checks of their
get_columns() results.

Keep the stringent check ( so that new code using this function will not
assume questionable defaults going forward ), and instead fix all callsites
where this condition might arise

--

This commit fixes the bug revealed by downstream RapidApp test case in
this commit:

 * vanstyn/RapidApp@c319a41904

Huge thanks to @ribasushi for identifing the needed code changes to
address this once I was able to provide the failing test case in
RapidApp
  • Loading branch information
vanstyn committed Oct 16, 2017
1 parent 1044aa1 commit 942c5be
Show file tree
Hide file tree
Showing 5 changed files with 54 additions and 7 deletions.
20 changes: 18 additions & 2 deletions lib/DBIx/Class/Relationship/Base.pm
Expand Up @@ -957,7 +957,8 @@ sub set_from_related {
: ( ! defined blessed $f_obj ) ? $f_obj
: do {

my $f_result_class = $self->result_source->related_source($rel)->result_class;
my $f_result_source = $self->result_source->related_source($rel);
my $f_result_class = $f_result_source->result_class;

unless( $f_obj->isa($f_result_class) ) {

Expand All @@ -975,7 +976,22 @@ sub set_from_related {
);
}

+{ $f_obj->get_columns };
my $fvals = { $f_obj->get_columns };

# The very low level resolve_relationship_condition() deliberately contains
# extra logic to ensure that it isn't passed garbage. Unfortunately we can
# get into a situation where an object *has* extra columns on it, which
# the interface of ->get_columns is obligated to return. In order not to
# compromise the sanity checks within r_r_c, simply do a cleanup pass here,
# and in 2 other spots within the codebase to keep things consistent
#
# FIXME - perhaps this should warn, but that's a battle for another day
#
+{ map {
exists $fvals->{$_}
? ( $_ => $fvals->{$_} )
: ()
} $f_result_source->columns };
}
),

Expand Down
20 changes: 18 additions & 2 deletions lib/DBIx/Class/ResultSet.pm
Expand Up @@ -841,7 +841,8 @@ sub find {
foreign_values => (
(! defined blessed $foreign_val) ? $foreign_val : do {

my $f_result_class = $rsrc->related_source($key)->result_class;
my $f_rsrc = $rsrc->related_source($key);
my $f_result_class = $f_rsrc->result_class;

unless( $foreign_val->isa($f_result_class) ) {

Expand All @@ -859,7 +860,22 @@ sub find {
);
}

+{ $foreign_val->get_columns };
my $fvals = { $foreign_val->get_columns };

# The very low level resolve_relationship_condition() deliberately contains
# extra logic to ensure that it isn't passed garbage. Unfortunately we can
# get into a situation where an object *has* extra columns on it, which
# the interface of ->get_columns is obligated to return. In order not to
# compromise the sanity checks within r_r_c, simply do a cleanup pass here,
# and in 2 other spots within the codebase to keep things consistent
#
# FIXME - perhaps this should warn, but that's a battle for another day
#
+{ map {
exists $fvals->{$_}
? ( $_ => $fvals->{$_} )
: ()
} $f_rsrc->columns };
}
),

Expand Down
17 changes: 16 additions & 1 deletion lib/DBIx/Class/ResultSource.pm
Expand Up @@ -2218,7 +2218,22 @@ sub _resolve_condition :DBIC_method_is_indirect_sugar {
}
# more compat
elsif( $_ == 0 and $res_args[0]->isa( $__expected_result_class_isa ) ) {
$res_args[0] = { $res_args[0]->get_columns };
my $fvals = { $res_args[0]->get_columns };

# The very low level resolve_relationship_condition() deliberately contains
# extra logic to ensure that it isn't passed garbage. Unfortunately we can
# get into a situation where an object *has* extra columns on it, which
# the interface of ->get_columns is obligated to return. In order not to
# compromise the sanity checks within r_r_c, simply do a cleanup pass here,
# and in 2 other spots within the codebase to keep things consistent
#
# FIXME - perhaps this should warn, but that's a battle for another day
#
$res_args[0] = { map {
exists $fvals->{$_}
? ( $_ => $fvals->{$_} )
: ()
} $res_args[0]->result_source->columns };
}
}
else {
Expand Down
2 changes: 1 addition & 1 deletion t/cdbi/06-hasa.t
Expand Up @@ -118,7 +118,7 @@ sub fail_with_bad_object {
NumExplodingSheep => 23
}
);
} qr/is not a column on related source 'Director'/;
} qr/isn't a Director/;
}

package Foo;
Expand Down
2 changes: 1 addition & 1 deletion t/cdbi/18-has_a.t
Expand Up @@ -110,7 +110,7 @@ is(
Rating => 'R',
NumExplodingSheep => 23
});
} qr/is not a column on related source 'Director'/, "Can't have film as codirector";
} qr/isn't a Director/, "Can't have film as codirector";
is $fail, undef, "We didn't get anything";

my $tastes_bad = YA::Film->create({
Expand Down

0 comments on commit 942c5be

Please sign in to comment.