From: Peter Rabbitson Date: Sat, 24 Sep 2016 13:08:53 +0000 (+0200) Subject: Switch infer_values_based_on to require_join_free_values in cond resolver X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=1bd54f3d4bc8428d602d2e28cb410b303bb242b7;p=dbsrgits%2FDBIx-Class.git Switch infer_values_based_on to require_join_free_values in cond resolver This further simplifies the cognitive surface of the condition resolver API just like 786c1cdd and a3ae79ed. During the sprint to add at least *some* sanity to the codepath infer_values_based_on was introduced as a stopgap to allow 83a6b244 to somehow proceed forward. Since then the amount of spots where this logic is necessary steadily went down, bringing us to the current place: there is just a single spot in the entire codebase that passes a non-empty inferrence structure. Given the entire codepath is rather baroque, the entire idea of inferrence is pushed to new_related instead, leaving the API of the resolver itself even simpler. There are no known issues as a result, verified by re-running the entire test plan for downstreams as described in 12e7015a. --- diff --git a/lib/DBIx/Class/Relationship/Base.pm b/lib/DBIx/Class/Relationship/Base.pm index c4d4df5..cfd23f2 100644 --- a/lib/DBIx/Class/Relationship/Base.pm +++ b/lib/DBIx/Class/Relationship/Base.pm @@ -8,8 +8,9 @@ use base qw/DBIx::Class/; use Scalar::Util qw/weaken blessed/; use DBIx::Class::_Util qw( UNRESOLVABLE_CONDITION DUMMY_ALIASPAIR - fail_on_internal_call + dbic_internal_try fail_on_internal_call ); +use DBIx::Class::SQLMaker::Util 'extract_equality_conditions'; use DBIx::Class::Carp; use namespace::clean; @@ -530,7 +531,11 @@ sub related_resultset { self_result_object => $self, # an extra sanity check guard - require_join_free_condition => ! $relcond_is_freeform, + require_join_free_condition => !!( + ! $relcond_is_freeform + and + $self->in_storage + ), # an API where these are optional would be too cumbersome, # instead always pass in some dummy values @@ -585,6 +590,7 @@ sub related_resultset { # FIXME - this loop doesn't seem correct - got to figure out # at some point what exactly it does. + # See also the FIXME at the end of new_related() ( ( $reverse->{$_}{attrs}{accessor}||'') eq 'multi' ) ? weaken( $attrs->{related_objects}{$_}[0] = $self ) : weaken( $attrs->{related_objects}{$_} = $self ) @@ -674,16 +680,94 @@ your storage until you call L on it. sub new_related { my ($self, $rel, $data) = @_; - $self->related_resultset($rel)->new_result( $self->result_source->_resolve_relationship_condition ( - infer_values_based_on => $data, + $self->throw_exception( + "Result object instantiation requires a hashref as argument" + ) unless ref $data eq 'HASH'; + + my $rsrc = $self->result_source; + my $rel_rsrc = $rsrc->related_source($rel); + +### +### This section deliberately does not rely on require_join_free_values, +### as quite often the resulting related object is useless without the +### contents of $data mixed in. Originally this code was part of +### resolve_relationship_condition() but given it has a single, very +### context-specific call-site it made no sense to expose it to end users. +### + + my $rel_resolution = $rsrc->_resolve_relationship_condition ( rel_name => $rel, self_result_object => $self, - # an API where these are optional would be too cumbersome, - # instead always pass in some dummy values - DUMMY_ALIASPAIR, + # In case we are *not* in_storage it is ok to treat failed resolution as an empty hash + # This happens e.g. as a result of various in-memory related graph of objects + require_join_free_condition => !! $self->in_storage, + + # dummy aliases with deliberately known lengths, so that we can + # quickly strip them below if needed + foreign_alias => 'F', + self_alias => 'S', + ); + + my $rel_values = + $rel_resolution->{join_free_values} + || + { map { substr( $_, 2 ) => $rel_resolution->{join_free_condition}{$_} } keys %{ $rel_resolution->{join_free_condition} } } + ; + + # mix everything together + my $amalgamated_values = { + %{ + # in case we got back join_free_values - they already have passed the extractor + $rel_resolution->{join_free_values} + ? $rel_values + : extract_equality_conditions( + $rel_values, + 'consider_nulls' + ) + }, + %$data, + }; + + # cleanup possible rogue { somecolumn => [ -and => 1,2 ] } + ($amalgamated_values->{$_}||'') eq UNRESOLVABLE_CONDITION + and + delete $amalgamated_values->{$_} + for keys %$amalgamated_values; + + if( my @nonvalues = grep { ! exists $amalgamated_values->{$_} } keys %$rel_values ) { + + $self->throw_exception( + "Unable to complete value inferrence - relationship '$rel' " + . "on source '@{[ $rsrc->source_name ]}' results " + . 'in expression(s) instead of definitive values: ' + . do { + # FIXME - used for diag only, but still icky + my $sqlm = + dbic_internal_try { $rsrc->schema->storage->sql_maker } + || + ( + require DBIx::Class::SQLMaker + and + DBIx::Class::SQLMaker->new + ) + ; + local $sqlm->{quote_char}; + local $sqlm->{_dequalify_idents} = 1; + ($sqlm->_recurse_where({ map { $_ => $rel_values->{$_} } @nonvalues }))[0] + } + ); + } - )->{inferred_values} ); + # And more complications - in case the relationship did not resolve + # we *have* to loop things through search_related ( essentially re-resolving + # everything we did so far, but with different type of handholding ) + # FIXME - this is still a mess, just a *little* better than it was + # See also the FIXME at the end of related_resultset() + exists $rel_resolution->{join_free_values} + ? $rel_rsrc->result_class->new({ -result_source => $rel_rsrc, %$amalgamated_values }) + : $self->related_resultset($rel)->new_result( $amalgamated_values ) + ; } =head2 create_related @@ -830,7 +914,7 @@ sub set_from_related { my ($self, $rel, $f_obj) = @_; $self->set_columns( $self->result_source->_resolve_relationship_condition ( - infer_values_based_on => {}, + require_join_free_values => 1, rel_name => $rel, foreign_values => ( # maintain crazy set_from_related interface @@ -865,7 +949,7 @@ sub set_from_related { # instead always pass in some dummy values DUMMY_ALIASPAIR, - )->{inferred_values} ); + )->{join_free_values} ); return 1; } diff --git a/lib/DBIx/Class/ResultSet.pm b/lib/DBIx/Class/ResultSet.pm index 128b554..39af76b 100644 --- a/lib/DBIx/Class/ResultSet.pm +++ b/lib/DBIx/Class/ResultSet.pm @@ -836,6 +836,7 @@ sub find { %$call_cond, %{ $rsrc->_resolve_relationship_condition( + require_join_free_values => 1, rel_name => $key, foreign_values => ( (! defined blessed $foreign_val) ? $foreign_val : do { @@ -861,12 +862,11 @@ sub find { +{ $foreign_val->get_columns }; } ), - infer_values_based_on => {}, # an API where these are optional would be too cumbersome, # instead always pass in some dummy values DUMMY_ALIASPAIR, - )->{inferred_values} }, + )->{join_free_values} }, }; } } diff --git a/lib/DBIx/Class/ResultSource.pm b/lib/DBIx/Class/ResultSource.pm index 2dec416..eba794f 100644 --- a/lib/DBIx/Class/ResultSource.pm +++ b/lib/DBIx/Class/ResultSource.pm @@ -2278,17 +2278,16 @@ Internals::SvREADONLY($UNRESOLVABLE_CONDITION => 1); # 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) -# infer_values_based_on => (either not supplied or a hashref, implies require_join_free_condition) +# 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) -# inferred_values => (in case of an available join_free condition, this is a hashref of -# *unqualified* column/value *EQUALITY* pairs, representing an amalgamation -# of the JF-cond parse and infer_values_based_on -# always either complete or 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 { my $self = shift; @@ -2318,10 +2317,7 @@ sub _resolve_relationship_condition { $self->throw_exception("No practical way to resolve $exception_rel_id between two data structures") 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'; - - $args->{require_join_free_condition} ||= !!$args->{infer_values_based_on}; + $args->{require_join_free_condition} ||= !!$args->{require_join_free_values}; $self->throw_exception( "Argument 'self_result_object' must be an object inheriting from '$__expected_result_class_isa'" ) if ( @@ -2514,32 +2510,45 @@ sub _resolve_relationship_condition { }; if ($args->{foreign_values}) { - $ret->{join_free_condition}{"$args->{self_alias}.$l_cols[$_]"} = $args->{foreign_values}{$f_cols[$_]} + $ret->{join_free_condition}{"$args->{self_alias}.$l_cols[$_]"} + = $ret->{join_free_values}{$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}, - $args->{self_result_object}, - $l_cols[$i], - ) if $args->{self_result_object}->in_storage; - - # FIXME - temporarly force-override - delete $args->{require_join_free_condition}; - delete $ret->{join_free_condition}; - last; - } - } + # FIXME - compat block due to inconsistency of get_columns() vs has_column_loaded() + # The former returns cached-in related single rels, while the latter is doing what + # it says on the tin. Thus the more logical "get all columns and barf if something + # is missing" is a non-starter, and we move through each column one by one :/ + + $args->{self_result_object}->has_column_loaded( $l_cols[$_] ) + + ? $ret->{join_free_condition}{"$args->{foreign_alias}.$f_cols[$_]"} + = $ret->{join_free_values}{$f_cols[$_]} + = $args->{self_result_object}->get_column( $l_cols[$_] ) + + : $args->{self_result_object}->in_storage + + ? $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}, + $args->{self_result_object}, + $l_cols[$_], + ) + + # non-resolvable yet not in storage - give it a pass + # FIXME - while this is what the code has done for ages, it doesn't seem right :( + : ( + delete $ret->{join_free_condition}, + delete $ret->{join_free_values}, + last + ) + + for 0 .. $#l_cols; } } elsif (ref $rel_info->{cond} eq 'ARRAY') { @@ -2562,7 +2571,7 @@ sub _resolve_relationship_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} ) ); - # we are discarding inferred_values from individual 'OR' branches here + # we are discarding join_free_values from individual 'OR' branches here # see @nonvalues checks below $subcond->{$_} and push @{$ret->{$_}}, $subcond->{$_} for (qw(condition join_free_condition)); } @@ -2599,9 +2608,14 @@ sub _resolve_relationship_condition { ); } - # we got something back - sanity check and infer values if we can + # we got something back (not from a static cond) - sanity check and infer values if we can + # ( in case of a static cond join_free_values is already pre-populated for us ) my @nonvalues; - if( $ret->{join_free_condition} ) { + if( + $ret->{join_free_condition} + and + ! $ret->{join_free_values} + ) { my $jfc_eqs = extract_equality_conditions( $ret->{join_free_condition}, @@ -2621,45 +2635,43 @@ sub _resolve_relationship_condition { ); if (exists $jfc_eqs->{$_} and ($jfc_eqs->{$_}||'') ne UNRESOLVABLE_CONDITION) { - $ret->{inferred_values}{$col} = $jfc_eqs->{$_}; + $ret->{join_free_values}{$col} = $jfc_eqs->{$_}; } - elsif ( !$args->{infer_values_based_on} or ! exists $args->{infer_values_based_on}{$col} ) { + else { push @nonvalues, { $_ => $ret->{join_free_condition}{$_} }; } } } # all or nothing - delete $ret->{inferred_values} if @nonvalues; + delete $ret->{join_free_values} if @nonvalues; } - # did the user explicitly ask - if ($args->{infer_values_based_on}) { - $self->throw_exception(sprintf ( - "Unable to complete value inferrence - $exception_rel_id results in expression(s) instead of definitive values: %s", - do { - # FIXME - used for diag only, but still icky - my $sqlm = - dbic_internal_try { $self->schema->storage->sql_maker } - || - ( - require DBIx::Class::SQLMaker - and - DBIx::Class::SQLMaker->new - ) - ; - local $sqlm->{quote_char}; - local $sqlm->{_dequalify_idents} = 1; - ($sqlm->_recurse_where({ -and => \@nonvalues }))[0] - } - )) if @nonvalues; - - $ret->{inferred_values} ||= {}; + # throw only if the user explicitly asked + $args->{require_join_free_values} + and + @nonvalues + and + $self->throw_exception( + "Unable to complete value inferrence - $exception_rel_id results in expression(s) instead of definitive values: " + . do { + # FIXME - used for diag only, but still icky + my $sqlm = + dbic_internal_try { $self->schema->storage->sql_maker } + || + ( + require DBIx::Class::SQLMaker + and + DBIx::Class::SQLMaker->new + ) + ; + local $sqlm->{quote_char}; + local $sqlm->{_dequalify_idents} = 1; + ($sqlm->_recurse_where({ -and => \@nonvalues }))[0] + } + ); - $ret->{inferred_values}{$_} = $args->{infer_values_based_on}{$_} - for keys %{$args->{infer_values_based_on}}; - } my $identity_map_incomplete; @@ -2746,6 +2758,11 @@ sub _resolve_relationship_condition { if $ret->{identity_map}; + # cleanup before final return, easier to eyeball + ! defined $ret->{$_} and delete $ret->{$_} + for keys %$ret; + + # FIXME - temporary, to fool the idiotic check in SQLMaker::_join_condition $ret->{condition} = { -and => [ $ret->{condition} ] } unless ( $ret->{condition} eq UNRESOLVABLE_CONDITION @@ -2772,10 +2789,14 @@ sub _resolve_relationship_condition { local $sqlm->{_dequalify_idents} = 1; - my ($cond_as_sql, $identmap_as_sql) = map - { join ' : ', map { defined $_ ? $_ : '{UNDEF}' } $sqlm->_recurse_where($_) } + my ( $cond_as_sql, $jf_cond_as_sql, $jf_vals_as_sql, $identmap_as_sql ) = map + { join ' : ', map { + ref $_ eq 'ARRAY' ? $_->[1] + : defined $_ ? $_ + : '{UNDEF}' + } $sqlm->_recurse_where($_) } ( - $ret->{condition}, + ( map { $ret->{$_} } qw( condition join_free_condition join_free_values ) ), { map { # inverse because of how the idmap is declared @@ -2784,6 +2805,7 @@ sub _resolve_relationship_condition { ) ; + emit_loud_diag( confess => 1, msg => sprintf ( @@ -2798,7 +2820,34 @@ sub _resolve_relationship_condition { $identmap_as_sql, dump_value( $rel_info->{cond} ), ), - ) if ( $ret->{identity_map_matches_condition} xor ( $cond_as_sql eq $identmap_as_sql ) ); + ) if ( + $ret->{identity_map_matches_condition} + xor + ( $cond_as_sql eq $identmap_as_sql ) + ); + + + emit_loud_diag( + confess => 1, + msg => sprintf ( + "Resolution of %s produced inconsistent metadata:\n\n" + . "returned 'join_free_condition' rendered as de-qualified SQL: %s\n" + . "returned 'join_free_values' rendered as de-qualified SQL: %s\n\n" + . "The condition declared on the misclassified relationship is: %s ", + $exception_rel_id, + $jf_cond_as_sql, + $jf_vals_as_sql, + dump_value( $rel_info->{cond} ), + ), + ) if ( + exists $ret->{join_free_condition} + and + ( + exists $ret->{join_free_values} + xor + ( $jf_cond_as_sql eq $jf_vals_as_sql ) + ) + ); } $ret; diff --git a/lib/DBIx/Class/Row.pm b/lib/DBIx/Class/Row.pm index 5adf4ea..ae89b78 100644 --- a/lib/DBIx/Class/Row.pm +++ b/lib/DBIx/Class/Row.pm @@ -1194,14 +1194,14 @@ sub copy { $copied->{$_->ID}++ or $_->copy( $foreign_vals ||= $rsrc->_resolve_relationship_condition( - infer_values_based_on => {}, + require_join_free_values => 1, rel_name => $rel_name, self_result_object => $new, # an API where these are optional would be too cumbersome, # instead always pass in some dummy values DUMMY_ALIASPAIR, - )->{inferred_values} + )->{join_free_values} ) for $self->related_resultset($rel_name)->all; }