From: Peter Rabbitson Date: Sat, 30 Jul 2016 14:03:12 +0000 (+0200) Subject: Promote resolve_relationship_condition to a 1st-class API method X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?p=dbsrgits%2FDBIx-Class.git;a=commitdiff_plain;h=7293955e14a24ad5abecc41e0ec485ccdfb3d2f0 Promote resolve_relationship_condition to a 1st-class API method The encapsulated logic is just too complex to try to replicate externally, especially now that everything within DBIC itself uses this method underneath. Patches to the only widely known user (::Resultset::RecursiveUpdate) will follow shortly --- diff --git a/lib/DBIx/Class/Relationship/Accessor.pm b/lib/DBIx/Class/Relationship/Accessor.pm index 42d7e38..e6d4fb4 100644 --- a/lib/DBIx/Class/Relationship/Accessor.pm +++ b/lib/DBIx/Class/Relationship/Accessor.pm @@ -54,7 +54,7 @@ sub add_relationship_accessor { and - $jfc = ( $rsrc->_resolve_relationship_condition( + $jfc = ( $rsrc->resolve_relationship_condition( rel_name => %1$s, foreign_alias => %1$s, self_alias => 'me', diff --git a/lib/DBIx/Class/Relationship/Base.pm b/lib/DBIx/Class/Relationship/Base.pm index ab18bed..5924db0 100644 --- a/lib/DBIx/Class/Relationship/Base.pm +++ b/lib/DBIx/Class/Relationship/Base.pm @@ -562,9 +562,9 @@ sub related_resultset { # ->in_storage() is orders of magnitude faster than the Try::Tiny-like # construct below ( perl's low level tooling is truly shit :/ ) ( $self->in_storage or DBIx::Class::_Util::in_internal_try ) - ? $rsrc->_resolve_relationship_condition($rrc_args)->{join_free_condition} + ? $rsrc->resolve_relationship_condition($rrc_args)->{join_free_condition} : dbic_internal_try { - $rsrc->_resolve_relationship_condition($rrc_args)->{join_free_condition} + $rsrc->resolve_relationship_condition($rrc_args)->{join_free_condition} } dbic_internal_catch { $unique_carper->( @@ -729,7 +729,7 @@ sub new_related { ### context-specific call-site it made no sense to expose it to end users. ### - my $rel_resolution = $rsrc->_resolve_relationship_condition ( + my $rel_resolution = $rsrc->resolve_relationship_condition ( rel_name => $rel, self_result_object => $self, @@ -947,7 +947,7 @@ L to update them in the storage. sub set_from_related { my ($self, $rel, $f_obj) = @_; - $self->set_columns( $self->result_source->_resolve_relationship_condition ( + $self->set_columns( $self->result_source->resolve_relationship_condition ( require_join_free_values => 1, rel_name => $rel, foreign_values => ( diff --git a/lib/DBIx/Class/ResultSet.pm b/lib/DBIx/Class/ResultSet.pm index 39af76b..1d6d177 100644 --- a/lib/DBIx/Class/ResultSet.pm +++ b/lib/DBIx/Class/ResultSet.pm @@ -835,7 +835,7 @@ sub find { $call_cond = { %$call_cond, - %{ $rsrc->_resolve_relationship_condition( + %{ $rsrc->resolve_relationship_condition( require_join_free_values => 1, rel_name => $key, foreign_values => ( @@ -2531,7 +2531,7 @@ sub populate { $colinfo->{$rel}{rs} = $rsrc->related_source($rel)->resultset; - $colinfo->{$rel}{fk_map} = { reverse %{ $rsrc->_resolve_relationship_condition( + $colinfo->{$rel}{fk_map} = { reverse %{ $rsrc->resolve_relationship_condition( rel_name => $rel, # an API where these are optional would be too cumbersome, diff --git a/lib/DBIx/Class/ResultSource.pm b/lib/DBIx/Class/ResultSource.pm index eba794f..ddef544 100644 --- a/lib/DBIx/Class/ResultSource.pm +++ b/lib/DBIx/Class/ResultSource.pm @@ -1832,7 +1832,7 @@ sub reverse_relationship_info { # Some custom rels may not resolve without a $schema # my $our_resolved_relcond = dbic_internal_try { - $self->_resolve_relationship_condition( + $self->resolve_relationship_condition( rel_name => $rel, # an API where these are optional would be too cumbersome, @@ -1898,7 +1898,7 @@ sub reverse_relationship_info { and my $their_resolved_relcond = dbic_internal_try { - $other_rsrc->_resolve_relationship_condition( + $other_rsrc->resolve_relationship_condition( rel_name => $other_rel, # an API where these are optional would be too cumbersome, @@ -2096,7 +2096,7 @@ sub _resolve_join { -alias => $as, -relation_chain_depth => ( $seen->{-relation_chain_depth} || 0 ) + 1, }, - $self->_resolve_relationship_condition( + $self->resolve_relationship_condition( rel_name => $join, self_alias => $alias, foreign_alias => $as, @@ -2169,18 +2169,29 @@ sub _compare_relationship_keys :DBIC_method_is_indirect_sugar { bag_eq( $_[1], $_[2] ); } -sub resolve_condition { - carp 'resolve_condition is a private method, stop calling it'; +sub _resolve_relationship_condition :DBIC_method_is_indirect_sugar { + DBIx::Class::_ENV_::ASSERT_NO_INTERNAL_INDIRECT_CALLS and fail_on_internal_call; + + # carp() - has been on CPAN for less than 2 years + carp '_resolve_relationship_condition() is deprecated - see resolve_relationship_condition() instead'; + + shift->resolve_relationship_condition(@_); +} + +sub resolve_condition :DBIC_method_is_indirect_sugar { + DBIx::Class::_ENV_::ASSERT_NO_INTERNAL_INDIRECT_CALLS and fail_on_internal_call; + + # carp() - has been discouraged forever + carp 'resolve_condition() is deprecated - see resolve_relationship_condition() instead'; + shift->_resolve_condition (@_); } -sub _resolve_condition { -# carp_unique sprintf -# '_resolve_condition is a private method, and moreover is about to go ' -# . 'away. Please contact the development team at %s if you believe you ' -# . 'have a genuine use for this method, in order to discuss alternatives.', -# DBIx::Class::_ENV_::HELP_URL, -# ; +sub _resolve_condition :DBIC_method_is_indirect_sugar { + DBIx::Class::_ENV_::ASSERT_NO_INTERNAL_INDIRECT_CALLS and fail_on_internal_call; + + # carp_unique() - the interface replacing it only became reality in Sep 2016 + carp_unique '_resolve_condition() is deprecated - see resolve_relationship_condition() instead'; ####################### ### API Design? What's that...? (a backwards compatible shim, kill me now) @@ -2232,21 +2243,21 @@ sub _resolve_condition { }; # Allowing passing relconds different than the relationshup itself is cute, - # but likely dangerous. Remove that from the (still unofficial) API of - # _resolve_relationship_condition, and instead make it "hard on purpose" + # but likely dangerous. Remove that from the API of resolve_relationship_condition, + # and instead make it "hard on purpose" local $self->relationship_info( $args->{rel_name} )->{cond} = $cond if defined $cond; ####################### # now it's fucking easy isn't it?! - my $rc = $self->_resolve_relationship_condition( $args ); + my $rc = $self->resolve_relationship_condition( $args ); my @res = ( ( $rc->{join_free_condition} || $rc->{condition} ), ! $rc->{join_free_condition}, ); - # _resolve_relationship_condition always returns qualified cols even in the + # resolve_relationship_condition always returns qualified cols even in the # case of join_free_condition, but nothing downstream expects this if ($rc->{join_free_condition} and ref $res[0] eq 'HASH') { $res[0] = { map @@ -2268,34 +2279,73 @@ our $UNRESOLVABLE_CONDITION = UNRESOLVABLE_CONDITION; # we are moving to a constant Internals::SvREADONLY($UNRESOLVABLE_CONDITION => 1); -# Resolves the passed condition to a concrete query fragment and extra -# metadata -# -## self-explanatory API, modeled on the custom cond coderef: -# rel_name => (scalar) -# foreign_alias => (scalar) -# foreign_values => (either not supplied or a hashref ) -# self_alias => (scalar) -# self_result_object => (either not supplied or a result object) -# require_join_free_condition => (boolean, throws on failure to construct a JF-cond) -# require_join_free_values => (boolean, throws on failure to return an equality-only JF-cond, implies require_join_free_condition) -# -## returns a hash -# condition => (a valid *likely fully qualified* sqla cond structure) -# identity_map => (a hashref of foreign-to-self *unqualified* column equality names) -# identity_map_matches_condition => (boolean, indicates whether the entire condition is expressed in the identity-map) -# join_free_condition => (a valid *fully qualified* sqla cond structure, maybe unset) -# join_free_values => (IFF the returned join_free_condition contains only exact values (no expressions) -# this would be a hashref of identical join_free_condition, except with all column -# names *unqualified* ) -# -sub _resolve_relationship_condition { +=head2 resolve_relationship_condition + +NOTE: You generally B need to use this functionality... until you +do. The API description is terse on purpose. If the text below doesn't make +sense right away (based on the context which prompted you to look here) it is +almost certain you are reaching for the wrong tool. Please consider asking for +advice in any of the support channels before proceeding. + +=over 4 + +=item Arguments: C<\%args> as shown below (C> denotes mandatory args): + + * rel_name => $string + + * foreign_alias => $string + + * self_alias => $string + + foreign_values => \%column_value_pairs + + self_result_object => $ResultObject + + require_join_free_condition => $bool ( results in exception on failure to construct a JF-cond ) + + require_join_free_values => $bool ( results in exception on failure to return an equality-only JF-cond ) + +=item Return Value: C<\%resolution_result> as shown below (C> denotes always-resent parts of the result): + + * condition => $sqla_condition ( always present, valid, *likely* fully qualified, SQL::Abstract-compatible structure ) + + identity_map => \%foreign_to_self_equailty_map ( list of declared-equal foreign/self *unqualified* column names ) + + identity_map_matches_condition => $bool ( indicates whether the entire condition is expressed within the identity_map ) + + join_free_condition => \%sqla_condition_fully_resolvable_via_foreign_table + ( always a hash, all keys guaranteed to be valid *fully qualified* columns ) + + join_free_values => \%unqalified_version_of_join_free_condition + ( IFF the returned join_free_condition contains only exact values (no expressions), this would be + a hashref identical to join_free_condition, except with all column names *unqualified* ) + +=back + +This is the low-level method used to convert a declared relationship into +various parameters consumed by higher level functions. It is provided as a +stable official API, as the logic it encapsulates grew incredibly complex with +time. While calling this method directly B, you +absolutely B in codepaths containing the moral equivalent +of: + + ... + if( ref $some_rsrc->relationship_info($somerel)->{cond} eq 'HASH' ) { + ... + } + ... + +=cut + +# TODO - expand the documentation above, too terse + +sub resolve_relationship_condition { my $self = shift; my $args = { ref $_[0] eq 'HASH' ? %{ $_[0] } : @_ }; for ( qw( rel_name self_alias foreign_alias ) ) { - $self->throw_exception("Mandatory argument '$_' to _resolve_relationship_condition() is not a plain string") + $self->throw_exception("Mandatory argument '$_' to resolve_relationship_condition() is not a plain string") if !defined $args->{$_} or length ref $args->{$_}; } @@ -2560,7 +2610,7 @@ sub _resolve_relationship_condition { else { my @subconds = map { local $rel_info->{cond} = $_; - $self->_resolve_relationship_condition( $args ); + $self->resolve_relationship_condition( $args ); } @{ $rel_info->{cond} }; if( @{ $rel_info->{cond} } == 1 ) { diff --git a/lib/DBIx/Class/ResultSource/RowParser.pm b/lib/DBIx/Class/ResultSource/RowParser.pm index 6540dc7..df3627a 100644 --- a/lib/DBIx/Class/ResultSource/RowParser.pm +++ b/lib/DBIx/Class/ResultSource/RowParser.pm @@ -194,7 +194,7 @@ sub _resolve_collapse { rsrc => $self->related_source($rel), fk_map => ( dbic_internal_try { - $self->_resolve_relationship_condition( + $self->resolve_relationship_condition( rel_name => $rel, # an API where these are optional would be too cumbersome, diff --git a/lib/DBIx/Class/Row.pm b/lib/DBIx/Class/Row.pm index ae89b78..cc66d74 100644 --- a/lib/DBIx/Class/Row.pm +++ b/lib/DBIx/Class/Row.pm @@ -1193,7 +1193,7 @@ sub copy { $copied->{$_->ID}++ or $_->copy( - $foreign_vals ||= $rsrc->_resolve_relationship_condition( + $foreign_vals ||= $rsrc->resolve_relationship_condition( require_join_free_values => 1, rel_name => $rel_name, self_result_object => $new, diff --git a/t/relationship/resolve_relationship_condition.t b/xt/extra/diagnostics/invalid_resolve_relationship_condition_arguments.t similarity index 86% rename from t/relationship/resolve_relationship_condition.t rename to xt/extra/diagnostics/invalid_resolve_relationship_condition_arguments.t index 801b1ea..050b5ca 100644 --- a/t/relationship/resolve_relationship_condition.t +++ b/xt/extra/diagnostics/invalid_resolve_relationship_condition_arguments.t @@ -6,10 +6,9 @@ use warnings; use Test::More; use Test::Exception; - use DBICTest; -my $schema = DBICTest->init_schema(); +my $schema = DBICTest->init_schema( no_deploy => 1 ); for ( { year => [1,2] }, @@ -18,7 +17,7 @@ for ( { -and => [ year => 1, year => 2 ] }, ) { throws_ok { - $schema->source('Track')->_resolve_relationship_condition( + $schema->source('Track')->resolve_relationship_condition( rel_name => 'cd_cref_cond', self_alias => 'me', foreign_alias => 'cd',