Fix $object->search_related aliasing, change semantics of _resolve_condition
Peter Rabbitson [Sun, 16 Jan 2011 00:14:03 +0000 (01:14 +0100)]
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)

lib/DBIx/Class/Relationship/Base.pm
lib/DBIx/Class/Relationship/ManyToMany.pm
lib/DBIx/Class/ResultSource.pm

index a6af9db..7100ea1 100644 (file)
@@ -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;
 }
index 675f206..b82995d 100644 (file)
@@ -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;
     };
 
index d596d2a..63a8ddf 100644 (file)
@@ -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 :(");
   }
 }