From: Peter Rabbitson Date: Sun, 16 Jan 2011 00:14:03 +0000 (+0100) Subject: Fix $object->search_related aliasing, change semantics of _resolve_condition X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=aa56106b252283cef5338312d66fdf62cc92df20;p=dbsrgits%2FDBIx-Class-Historic.git Fix $object->search_related aliasing, change semantics of _resolve_condition Change the RV of _resolve_condition one last time. Now it checks the RV of the resolved coderef (if any), and chooses (preferrably) the join-free condition or the cross-table one. In list context returns a flag signifying if the resulting condition is available for standalone use (false) or requires the joins to remain (true) --- diff --git a/lib/DBIx/Class/Relationship/Base.pm b/lib/DBIx/Class/Relationship/Base.pm index a6af9db..7100ea1 100644 --- a/lib/DBIx/Class/Relationship/Base.pm +++ b/lib/DBIx/Class/Relationship/Base.pm @@ -417,13 +417,7 @@ sub related_resultset { # condition resolution may fail if an incomplete master-object prefetch # is encountered - that is ok during prefetch construction (not yet in_storage) - - # if $rel_info->{cond} is a CODE, we might need to join from the - # current resultsource instead of just querying the target - # resultsource, in that case, the condition might provide an - # additional condition in order to avoid an unecessary join if - # that is at all possible. - my ($cond, $extended_cond) = try { + my ($cond, $is_crosstable) = try { $source->_resolve_condition( $rel_info->{cond}, $rel, $self, $rel ) } catch { @@ -434,49 +428,48 @@ sub related_resultset { $DBIx::Class::ResultSource::UNRESOLVABLE_CONDITION; # RV }; - if ($cond eq $DBIx::Class::ResultSource::UNRESOLVABLE_CONDITION) { - my $reverse = $source->reverse_relationship_info($rel); - foreach my $rev_rel (keys %$reverse) { - if ($reverse->{$rev_rel}{attrs}{accessor} && $reverse->{$rev_rel}{attrs}{accessor} eq 'multi') { - $attrs->{related_objects}{$rev_rel} = [ $self ]; - weaken $attrs->{related_object}{$rev_rel}[0]; - } else { - $attrs->{related_objects}{$rev_rel} = $self; - weaken $attrs->{related_object}{$rev_rel}; - } - } + # keep in mind that the following if() block is part of a do{} - no return()s!!! + if ($is_crosstable) { + $self->throw_exception ( + "A cross-table relationship condition returned for statically declared '$rel'") + unless ref $rel_info->{cond} eq 'CODE'; + + # A WHOREIFFIC hack to reinvoke the entire condition resolution + # with the correct alias. Another way of doing this involves a + # lot of state passing around, and the @_ positions are already + # mapped out, making this crap a less icky option. + # + # The point of this exercise is to retain the spirit of the original + # $obj->search_related($rel) where the resulting rset will have the + # root alias as 'me', instead of $rel (as opposed to invoking + # $rs->search_related) + + + local $source->{_relationships}{me} = $source->{_relationships}{$rel}; # make the fake 'me' rel + my $obj_table_alias = lc($source->source_name) . '__row'; + + $source->resultset->search( + $self->ident_condition($obj_table_alias), + { alias => $obj_table_alias }, + )->search_related('me', $query, $attrs) } - - if (ref $rel_info->{cond} eq 'CODE' && !$extended_cond) { - # since we don't have the extended condition, we need to step - # back, get a resultset for the current row and do a - # search_related there. - - my $row_srcname = $source->source_name; - my $base_rs_class = $source->resultset_class; - my $base_rs_attr = $source->resultset_attributes; - my $base_rs = $base_rs_class->new - ($source, - { - %$base_rs_attr, - alias => $source->storage->relname_to_table_alias(lc($row_srcname).'__row',1) - }); - my $alias = $base_rs->current_source_alias; - my %identity = map { ( "${alias}.${_}" => $self->get_column($_) ) } $source->primary_columns; - my $row_rs = $base_rs->search(\%identity); - my $related = $row_rs->related_resultset($rel, { %$attrs, alias => 'me' }); - $related->search($query); - - } else { - # when we have the extended condition or we have a simple - # relationship declaration, it can optimize the JOIN away by - # simply adding the identity in WHERE. - - if (ref $rel_info->{cond} eq 'CODE' && $extended_cond) { - $cond = $extended_cond; + else { + # FIXME - this conditional doesn't seem correct - got to figure out + # at some point what it does. Also the entire UNRESOLVABLE_CONDITION + # business seems shady - we could simply not query *at all* + if ($cond eq $DBIx::Class::ResultSource::UNRESOLVABLE_CONDITION) { + my $reverse = $source->reverse_relationship_info($rel); + foreach my $rev_rel (keys %$reverse) { + if ($reverse->{$rev_rel}{attrs}{accessor} && $reverse->{$rev_rel}{attrs}{accessor} eq 'multi') { + $attrs->{related_objects}{$rev_rel} = [ $self ]; + weaken $attrs->{related_object}{$rev_rel}[0]; + } else { + $attrs->{related_objects}{$rev_rel} = $self; + weaken $attrs->{related_object}{$rev_rel}; + } + } } - - if (ref $cond eq 'ARRAY') { + elsif (ref $cond eq 'ARRAY') { $cond = [ map { if (ref $_ eq 'HASH') { my $hash; @@ -489,15 +482,17 @@ sub related_resultset { $_; } } @$cond ]; - } elsif (ref $cond eq 'HASH') { - foreach my $key (grep { ! /\./ } keys %$cond) { + } + elsif (ref $cond eq 'HASH') { + foreach my $key (grep { ! /\./ } keys %$cond) { $cond->{"me.$key"} = delete $cond->{$key}; } } $query = ($query ? { '-and' => [ $cond, $query ] } : $cond); $self->result_source->related_source($rel)->resultset->search( - $query, $attrs); + $query, $attrs + ); } }; } @@ -697,28 +692,24 @@ set them in the storage. sub set_from_related { my ($self, $rel, $f_obj) = @_; - my $rel_info = $self->relationship_info($rel); - $self->throw_exception( "No such relationship ${rel}" ) unless $rel_info; - my $cond = $rel_info->{cond}; - $self->throw_exception( - "set_from_related can only handle a hash condition; the ". - "condition for $rel is of type ". - (ref $cond ? ref $cond : 'plain scalar') - ) unless ref $cond eq 'HASH'; + + my $rel_info = $self->relationship_info($rel) + or $self->throw_exception( "No such relationship ${rel}" ); + if (defined $f_obj) { my $f_class = $rel_info->{class}; $self->throw_exception( "Object $f_obj isn't a ".$f_class ) unless blessed $f_obj and $f_obj->isa($f_class); } - # _resolve_condition might return two hashrefs, specially in the - # current case, since we know $f_object is an object. - my ($condref1, $condref2) = $self->result_source->_resolve_condition + my ($cond, $crosstable) = $self->result_source->_resolve_condition ($rel_info->{cond}, $f_obj, $rel, $rel); - # if we get two condrefs, we need to use the second, otherwise we - # use the first. - $self->set_columns($condref2 ? $condref2 : $condref1); + $self->throw_exception( + "Custom relationship '$rel' does not resolve to a join-free condition fragment" + ) if $crosstable; + + $self->set_columns($cond); return 1; } diff --git a/lib/DBIx/Class/Relationship/ManyToMany.pm b/lib/DBIx/Class/Relationship/ManyToMany.pm index 675f206..b82995d 100644 --- a/lib/DBIx/Class/Relationship/ManyToMany.pm +++ b/lib/DBIx/Class/Relationship/ManyToMany.pm @@ -133,20 +133,15 @@ EOW unless blessed ($obj); my $rel_source = $self->search_related($rel)->result_source; my $cond = $rel_source->relationship_info($f_rel)->{cond}; - my $link_cond; - if (ref $cond eq 'CODE') { - my ($cond_should_join, $cond_optimized) = $rel_source->_resolve_condition - ($cond, $obj, $f_rel, $f_rel); - if ($cond_optimized) { - $link_cond = $cond_optimized; - } else { - $self->throw_exception('Extended relationship '.$rel. - ' requires optimized version for ManyToMany.'); - } - } else { - $link_cond = $rel_source->_resolve_condition - ($cond, $obj, $f_rel); - } + my ($link_cond, $crosstable) = $rel_source->_resolve_condition( + $cond, $obj, $f_rel, $f_rel + ); + + $self->throw_exception( + "Custom relationship '$rel' does not resolve to a join-free condition, " + ."unable to use with the ManyToMany helper '$f_rel'" + ) if $crosstable; + $self->search_related($rel, $link_cond)->delete; }; diff --git a/lib/DBIx/Class/ResultSource.pm b/lib/DBIx/Class/ResultSource.pm index d596d2a..63a8ddf 100644 --- a/lib/DBIx/Class/ResultSource.pm +++ b/lib/DBIx/Class/ResultSource.pm @@ -10,7 +10,7 @@ use DBIx::Class::Exception; use Carp::Clan qw/^DBIx::Class/; use Try::Tiny; use List::Util 'first'; -use Scalar::Util qw/weaken isweak/; +use Scalar::Util qw/blessed weaken isweak/; use Storable qw/nfreeze thaw/; use namespace::clean; @@ -1546,26 +1546,63 @@ sub resolve_condition { $self->_resolve_condition (@_); } -# Resolves the passed condition to a concrete query fragment. If given an alias, -# returns a join condition; if given an object, inverts that object to produce -# a related conditional from that object. our $UNRESOLVABLE_CONDITION = \ '1 = 0'; +# Resolves the passed condition to a concrete query fragment and a flag +# indicating whether this is a cross-table condition. sub _resolve_condition { my ($self, $cond, $as, $for, $relname) = @_; - if (ref $cond eq 'CODE') { - my $obj_rel = !!ref $for; + my $obj_rel = !!blessed $for; + + if (ref $cond eq 'CODE') { + my $relalias = $obj_rel ? 'me' : $as; - return $cond->({ + my ($crosstable_cond, $joinfree_cond) = $cond->({ self_alias => $obj_rel ? $as : $for, - foreign_alias => $obj_rel ? 'me' : $as, + foreign_alias => $relalias, self_resultsource => $self, foreign_relname => $relname || ($obj_rel ? $as : $for), self_rowobj => $obj_rel ? $for : undef }); - } elsif (ref $cond eq 'HASH') { + if ($joinfree_cond) { + + # FIXME sanity check until things stabilize, remove at some point + $self->throw_exception ( + "A join-free condition returned for relationship '$relname' whithout a row-object to chain from" + ) unless $obj_rel; + + # FIXME another sanity check + if ( + ref $joinfree_cond ne 'HASH' + or + first { $_ !~ /^\Q$relalias.\E.+/ } keys %$joinfree_cond + ) { + $self->throw_exception ( + "The join-free condition returned for relationship '$relname' must be a hash " + .'reference with all keys being valid columns on the related result source' + ); + } + + # normalize + for (values %$joinfree_cond) { + $_ = $_->{'='} if ( + ref $_ eq 'HASH' + and + keys %$_ == 1 + and + exists $_->{'='} + ); + } + + return wantarray ? ($joinfree_cond, 0) : $joinfree_cond; + } + else { + return wantarray ? ($crosstable_cond, 1) : $crosstable_cond; + } + } + elsif (ref $cond eq 'HASH') { my %ret; foreach my $k (keys %{$cond}) { my $v = $cond->{$k}; @@ -1605,11 +1642,23 @@ sub _resolve_condition { $ret{"${as}.${k}"} = { -ident => "${for}.${v}" }; } } - return \%ret; - } elsif (ref $cond eq 'ARRAY') { - return [ map { $self->_resolve_condition($_, $as, $for, $relname) } @$cond ]; - } else { - $self->throw_exception ("Can't handle condition $cond yet :("); + + return wantarray + ? ( \%ret, ($obj_rel || !defined $as || ref $as) ? 0 : 1 ) + : \%ret + ; + } + elsif (ref $cond eq 'ARRAY') { + my (@ret, $crosstable); + for (@$cond) { + my ($cond, $crosstab) = $self->_resolve_condition($_, $as, $for, $relname); + push @ret, $cond; + $crosstable ||= $crosstab; + } + return wantarray ? (\@ret, $crosstable) : \@ret; + } + else { + $self->throw_exception ("Can't handle condition $cond for relationship '$relname' yet :("); } }