Skip to content

Commit 786c1cd

Browse files
ribasushiribasushi
committed
Stop accepting foreign_values => undef/rowobj in the resolver
There are just a few spots that need this, things are complex enough as it is Introduces a subtle change in behavior - now results of $foreign->get_columns are scrutinized just as a plain hashref, and as a result the sanity checks are somewhat relaxed. There should not be any fallout due to this - tested on a wide range of downstreams Adjust some tested-for exceptions added in 7e5a0e7 as a result of the above Read under -w
1 parent 3aac91f commit 786c1cd

File tree

6 files changed

+131
-62
lines changed

6 files changed

+131
-62
lines changed

‎lib/DBIx/Class/Relationship/Base.pm

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ use base qw/DBIx::Class/;
77

88
use Scalar::Util qw/weaken blessed/;
99
use DBIx::Class::_Util qw( UNRESOLVABLE_CONDITION fail_on_internal_call );
10+
use DBIx::Class::Carp;
1011
use namespace::clean;
1112

1213
=head1 NAME
@@ -822,7 +823,34 @@ sub set_from_related {
822823
$self->set_columns( $self->result_source->_resolve_relationship_condition (
823824
infer_values_based_on => {},
824825
rel_name => $rel,
825-
foreign_values => $f_obj,
826+
foreign_values => (
827+
# maintain crazy set_from_related interface
828+
#
829+
( ! defined $f_obj ) ? +{}
830+
: ( ! defined blessed $f_obj ) ? $f_obj
831+
: do {
832+
833+
my $f_result_class = $self->result_source->related_source($rel)->result_class;
834+
835+
unless( $f_obj->isa($f_result_class) ) {
836+
837+
$self->throw_exception(
838+
'Object supplied to set_from_related() must inherit from '
839+
. "'$DBIx::Class::ResultSource::__expected_result_class_isa'"
840+
) unless $f_obj->isa(
841+
$DBIx::Class::ResultSource::__expected_result_class_isa
842+
);
843+
844+
carp_unique(
845+
'Object supplied to set_from_related() usually should inherit from '
846+
. "the related ResultClass ('$f_result_class'), perhaps you've made "
847+
. 'a mistake?'
848+
);
849+
}
850+
851+
+{ $f_obj->get_columns };
852+
}
853+
),
826854
foreign_alias => $rel,
827855
self_alias => 'me',
828856
)->{inferred_values} );

‎lib/DBIx/Class/ResultSet.pm

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -818,6 +818,7 @@ sub find {
818818

819819
for my $key (keys %$call_cond) {
820820
if (
821+
# either a structure or a result-ish object
821822
length ref($call_cond->{$key})
822823
and
823824
( $rel_list ||= { map { $_ => 1 } $rsrc->relationships } )
@@ -826,7 +827,7 @@ sub find {
826827
! is_literal_value( $call_cond->{$key} )
827828
and
828829
# implicitly skip has_many's (likely MC), via the delete()
829-
( ref( my $val = delete $call_cond->{$key} ) ne 'ARRAY' )
830+
( ref( my $foreign_val = delete $call_cond->{$key} ) ne 'ARRAY' )
830831
) {
831832

832833
# FIXME: it seems wrong that relationship conditions take precedence...?
@@ -835,7 +836,30 @@ sub find {
835836

836837
%{ $rsrc->_resolve_relationship_condition(
837838
rel_name => $key,
838-
foreign_values => $val,
839+
foreign_values => (
840+
(! defined blessed $foreign_val) ? $foreign_val : do {
841+
842+
my $f_result_class = $rsrc->related_source($key)->result_class;
843+
844+
unless( $foreign_val->isa($f_result_class) ) {
845+
846+
$self->throw_exception(
847+
'Objects supplied to find() must inherit from '
848+
. "'$DBIx::Class::ResultSource::__expected_result_class_isa'"
849+
) unless $foreign_val->isa(
850+
$DBIx::Class::ResultSource::__expected_result_class_isa
851+
);
852+
853+
carp_unique(
854+
"Objects supplied to find() via '$key' usually should inherit from "
855+
. "the related ResultClass ('$f_result_class'), perhaps you've made "
856+
. 'a mistake?'
857+
);
858+
}
859+
860+
+{ $foreign_val->get_columns };
861+
}
862+
),
839863
infer_values_based_on => {},
840864

841865
self_alias => "\xFE", # irrelevant

‎lib/DBIx/Class/ResultSource.pm

Lines changed: 71 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -2160,6 +2160,10 @@ sub _resolve_condition {
21602160
$is_objlike[$_] = 0;
21612161
$res_args[$_] = '__gremlins__';
21622162
}
2163+
# more compat
2164+
elsif( $_ == 0 and $res_args[0]->isa( $__expected_result_class_isa ) ) {
2165+
$res_args[0] = { $res_args[0]->get_columns };
2166+
}
21632167
}
21642168
else {
21652169
$res_args[$_] ||= {};
@@ -2225,7 +2229,7 @@ Internals::SvREADONLY($UNRESOLVABLE_CONDITION => 1);
22252229
## self-explanatory API, modeled on the custom cond coderef:
22262230
# rel_name => (scalar)
22272231
# foreign_alias => (scalar)
2228-
# foreign_values => (either not supplied, or a hashref, or a foreign ResultObject (to be ->get_columns()ed), or plain undef )
2232+
# foreign_values => (either not supplied or a hashref )
22292233
# self_alias => (scalar)
22302234
# self_result_object => (either not supplied or a result object)
22312235
# require_join_free_condition => (boolean, throws on failure to construct a JF-cond)
@@ -2287,67 +2291,78 @@ sub _resolve_relationship_condition {
22872291

22882292
my $rel_rsrc = $self->related_source($args->{rel_name});
22892293

2290-
if (exists $args->{foreign_values}) {
2291-
2292-
if (! defined $args->{foreign_values} ) {
2293-
# fallback: undef => {}
2294-
$args->{foreign_values} = {};
2295-
}
2296-
elsif (defined blessed $args->{foreign_values}) {
2297-
2298-
$self->throw_exception( "Objects supplied as 'foreign_values' ($args->{foreign_values}) must inherit from '$__expected_result_class_isa'" )
2299-
unless $args->{foreign_values}->isa( $__expected_result_class_isa );
2300-
2301-
carp_unique(
2302-
"Objects supplied as 'foreign_values' ($args->{foreign_values}) "
2303-
. "usually should inherit from the related ResultClass ('@{[ $rel_rsrc->result_class ]}'), "
2304-
. "perhaps you've made a mistake invoking the condition resolver?"
2305-
) unless $args->{foreign_values}->isa($rel_rsrc->result_class);
2306-
2307-
$args->{foreign_values} = { $args->{foreign_values}->get_columns };
2308-
}
2309-
elsif ( ref $args->{foreign_values} eq 'HASH' ) {
2310-
2311-
# re-build {foreign_values} excluding identically named rels
2312-
if( keys %{$args->{foreign_values}} ) {
2294+
if (
2295+
exists $args->{foreign_values}
2296+
and
2297+
(
2298+
ref $args->{foreign_values} eq 'HASH'
2299+
or
2300+
$self->throw_exception(
2301+
"Argument 'foreign_values' must be a hash reference"
2302+
)
2303+
)
2304+
and
2305+
keys %{$args->{foreign_values}}
2306+
) {
23132307

2314-
my ($col_idx, $rel_idx) = map
2315-
{ { map { $_ => 1 } $rel_rsrc->$_ } }
2316-
qw( columns relationships )
2317-
;
2308+
my ($col_idx, $rel_idx) = map
2309+
{ { map { $_ => 1 } $rel_rsrc->$_ } }
2310+
qw( columns relationships )
2311+
;
23182312

2319-
my $equivalencies = extract_equality_conditions(
2320-
$args->{foreign_values},
2321-
'consider nulls',
2322-
);
2313+
my $equivalencies;
23232314

2324-
$args->{foreign_values} = { map {
2325-
# skip if relationship *and* a non-literal ref
2326-
# this means a multicreate stub was passed in
2315+
# re-build {foreign_values} excluding refs as follows
2316+
# ( hot codepath: intentionally convoluted )
2317+
#
2318+
$args->{foreign_values} = { map {
2319+
(
2320+
$_ !~ /^-/
2321+
or
2322+
$self->throw_exception(
2323+
"The key '$_' supplied as part of 'foreign_values' during "
2324+
. 'relationship resolution must be a column name, not a function'
2325+
)
2326+
)
2327+
and
2328+
(
2329+
# skip if relationship ( means a multicreate stub was passed in )
2330+
# skip if literal ( can't infer anything about it )
2331+
# or plain throw if nonequiv yet not literal
2332+
(
2333+
length ref $args->{foreign_values}{$_}
2334+
and
23272335
(
23282336
$rel_idx->{$_}
2329-
and
2330-
length ref $args->{foreign_values}{$_}
2331-
and
2332-
! is_literal_value($args->{foreign_values}{$_})
2337+
or
2338+
is_literal_value($args->{foreign_values}{$_})
2339+
or
2340+
(
2341+
(
2342+
! exists(
2343+
( $equivalencies ||= extract_equality_conditions( $args->{foreign_values}, 'consider nulls' ) )
2344+
->{$_}
2345+
)
2346+
or
2347+
($equivalencies->{$_}||'') eq UNRESOLVABLE_CONDITION
2348+
)
2349+
and
2350+
$self->throw_exception(
2351+
"Resolution of relationship '$args->{rel_name}' failed: "
2352+
. "supplied value for foreign column '$_' is not a direct "
2353+
. 'equivalence expression'
2354+
)
2355+
)
23332356
)
2334-
? ()
2335-
: ( $_ => (
2336-
! $col_idx->{$_}
2337-
? $self->throw_exception( "Key '$_' supplied as 'foreign_values' is not a column on related source '@{[ $rel_rsrc->source_name ]}'" )
2338-
: ( !exists $equivalencies->{$_} or ($equivalencies->{$_}||'') eq UNRESOLVABLE_CONDITION )
2339-
? $self->throw_exception( "Value supplied for '...{foreign_values}{$_}' is not a direct equivalence expression" )
2340-
: $args->{foreign_values}{$_}
2341-
))
2342-
} keys %{$args->{foreign_values}} };
2343-
}
2344-
}
2345-
else {
2346-
$self->throw_exception(
2347-
"Argument 'foreign_values' must be either an object inheriting from '@{[ $rel_rsrc->result_class ]}', "
2348-
. "or a hash reference, or undef"
2349-
);
2350-
}
2357+
) ? ()
2358+
: $col_idx->{$_} ? ( $_ => $args->{foreign_values}{$_} )
2359+
: $self->throw_exception(
2360+
"The key '$_' supplied as part of 'foreign_values' during "
2361+
. 'relationship resolution is not a column on related source '
2362+
. "'@{[ $rel_rsrc->source_name ]}'"
2363+
)
2364+
)
2365+
} keys %{$args->{foreign_values}} };
23512366
}
23522367

23532368
my $ret;

‎t/cdbi/06-hasa.t

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ sub fail_with_bad_object {
118118
NumExplodingSheep => 23
119119
}
120120
);
121-
} qr/isn't a Director/;
121+
} qr/is not a column on related source 'Director'/;
122122
}
123123

124124
package Foo;

‎t/cdbi/18-has_a.t

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,7 @@ is(
110110
Rating => 'R',
111111
NumExplodingSheep => 23
112112
});
113-
} qr/isn't a Director/, "Can't have film as codirector";
113+
} qr/is not a column on related source 'Director'/, "Can't have film as codirector";
114114
is $fail, undef, "We didn't get anything";
115115

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

‎t/relationship/resolve_relationship_condition.t

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,9 @@ for (
2727
} qr/
2828
\Qis not a column on related source 'CD'\E
2929
|
30-
\QValue supplied for '...{foreign_values}{year}' is not a direct equivalence expression\E
30+
\Qsupplied value for foreign column 'year' is not a direct equivalence expression\E
31+
|
32+
\QThe key '-\E \w+ \Q' supplied as part of 'foreign_values' during relationship resolution must be a column name, not a function\E
3133
/x;
3234
}
3335

0 commit comments

Comments
 (0)