From: Peter Rabbitson <ribasushi@cpan.org>
Date: Thu, 17 Sep 2009 11:54:44 +0000 (+0000)
Subject: Fix left-join chaining
X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=8a3fa4ae894b55795bcea24a643b42d779cc9d13;p=dbsrgits%2FDBIx-Class-Historic.git

Fix left-join chaining
---

diff --git a/Changes b/Changes
index a0d6efb..ce6d3e5 100644
--- a/Changes
+++ b/Changes
@@ -13,6 +13,7 @@ Revision history for DBIx::Class
           tables that needed it
         - More informative exception on failing _resolve_relationship
         - Fix unreported rollback exceptions in TxnScopeGuard
+        - Fix overly-eager left-join chain enforcing code
 
 0.08111 2009-09-06 21:58:00 (UTC)
         - The hashref to connection_info now accepts a 'dbh_maker'
diff --git a/lib/DBIx/Class/ResultSource.pm b/lib/DBIx/Class/ResultSource.pm
index 24018f3..3982965 100644
--- a/lib/DBIx/Class/ResultSource.pm
+++ b/lib/DBIx/Class/ResultSource.pm
@@ -1194,7 +1194,7 @@ sub resolve_join {
 
 # Returns the {from} structure used to express JOIN conditions
 sub _resolve_join {
-  my ($self, $join, $alias, $seen, $jpath, $force_left) = @_;
+  my ($self, $join, $alias, $seen, $jpath, $parent_force_left) = @_;
 
   # we need a supplied one, because we do in-place modifications, no returns
   $self->throw_exception ('You must supply a seen hashref as the 3rd argument to _resolve_join')
@@ -1205,47 +1205,56 @@ sub _resolve_join {
 
   $jpath = [@$jpath];
 
-  if (ref $join eq 'ARRAY') {
+  if (not defined $join) {
+    return ();
+  }
+  elsif (ref $join eq 'ARRAY') {
     return
       map {
-        $self->_resolve_join($_, $alias, $seen, $jpath, $force_left);
+        $self->_resolve_join($_, $alias, $seen, $jpath, $parent_force_left);
       } @$join;
-  } elsif (ref $join eq 'HASH') {
-    return
-      map {
-        my $as = ($seen->{$_} ? join ('_', $_, $seen->{$_} + 1) : $_);  # the actual seen value will be incremented below
-        local $force_left->{force} = $force_left->{force};
-        (
-          $self->_resolve_join($_, $alias, $seen, [@$jpath], $force_left),
-          $self->related_source($_)->_resolve_join(
-            $join->{$_}, $as, $seen, [@$jpath, $_], $force_left
-          )
-        );
-      } keys %$join;
-  } elsif (ref $join) {
-    $self->throw_exception("No idea how to resolve join reftype ".ref $join);
-  } else {
+  }
+  elsif (ref $join eq 'HASH') {
+
+    my @ret;
+    for my $rel (keys %$join) {
+
+      my $rel_info = $self->relationship_info($rel)
+        or $self->throw_exception("No such relationship ${rel}");
+
+      my $force_left = $parent_force_left;
+      $force_left ||= lc($rel_info->{attrs}{join_type}||'') eq 'left';
+
+      # the actual seen value will be incremented by the recursion
+      my $as = ($seen->{$rel} ? join ('_', $rel, $seen->{$rel} + 1) : $rel);
 
-    return() unless defined $join;
+      push @ret, (
+        $self->_resolve_join($rel, $alias, $seen, [@$jpath], $force_left),
+        $self->related_source($rel)->_resolve_join(
+          $join->{$rel}, $as, $seen, [@$jpath, $rel], $force_left
+        )
+      );
+    }
+    return @ret;
 
+  }
+  elsif (ref $join) {
+    $self->throw_exception("No idea how to resolve join reftype ".ref $join);
+  }
+  else {
     my $count = ++$seen->{$join};
     my $as = ($count > 1 ? "${join}_${count}" : $join);
 
-    my $rel_info = $self->relationship_info($join);
-    $self->throw_exception("No such relationship ${join}") unless $rel_info;
-    my $type;
-    if ($force_left) {
-      $type = 'left';
-    }
-    else {
-      $type = $rel_info->{attrs}{join_type};
-      $force_left = 1 if lc($type||'') eq 'left';
-    }
+    my $rel_info = $self->relationship_info($join)
+      or $self->throw_exception("No such relationship ${join}");
 
     my $rel_src = $self->related_source($join);
     return [ { $as => $rel_src->from,
                -source_handle => $rel_src->handle,
-               -join_type => $type,
+               -join_type => $parent_force_left
+                  ? 'left'
+                  : $rel_info->{attrs}{join_type}
+                ,
                -join_path => [@$jpath, $join],
                -alias => $as,
                -relation_chain_depth => $seen->{-relation_chain_depth} || 0,
@@ -1439,7 +1448,10 @@ sub _resolve_prefetch {
   my ($self, $pre, $alias, $alias_map, $order, $collapse, $pref_path) = @_;
   $pref_path ||= [];
 
-  if( ref $pre eq 'ARRAY' ) {
+  if (not defined $pre) {
+    return ();
+  }
+  elsif( ref $pre eq 'ARRAY' ) {
     return
       map { $self->_resolve_prefetch( $_, $alias, $alias_map, $order, $collapse, [ @$pref_path ] ) }
         @$pre;
diff --git a/t/prefetch/join_type.t b/t/prefetch/join_type.t
new file mode 100644
index 0000000..6a21f22
--- /dev/null
+++ b/t/prefetch/join_type.t
@@ -0,0 +1,47 @@
+use warnings;
+
+use Test::More;
+use Test::Exception;
+use lib qw(t/lib);
+use DBIC::SqlMakerTest;
+use DBICTest;
+
+my $schema = DBICTest->init_schema();
+
+
+# a regular belongs_to prefetch
+my $cds = $schema->resultset('CD')->search ({}, { prefetch => 'artist' } );
+
+my $nulls = {
+  hashref => {},
+  arrayref => [],
+  undef => undef,
+};
+
+# make sure null-prefetches do not screw with the final sql:
+for my $type (keys %$nulls) {
+#  is_same_sql_bind (
+#    $cds->search({}, { prefetch => { artist => $nulls->{$type} } })->as_query,
+#    $cds->as_query,
+#    "same sql with null $type prefetch"
+#  );
+}
+
+# make sure left join is carried only starting from the first has_many
+is_same_sql_bind (
+  $cds->search({}, { prefetch => { artist => { cds => 'artist' } } })->as_query,
+  '(
+    SELECT  me.cdid, me.artist, me.title, me.year, me.genreid, me.single_track,
+            artist.artistid, artist.name, artist.rank, artist.charfield,
+            cds.cdid, cds.artist, cds.title, cds.year, cds.genreid, cds.single_track,
+            artist_2.artistid, artist_2.name, artist_2.rank, artist_2.charfield
+      FROM cd me
+      JOIN artist artist ON artist.artistid = me.artist
+      LEFT JOIN cd cds ON cds.artist = artist.artistid
+      LEFT JOIN artist artist_2 ON artist_2.artistid = cds.artist
+    ORDER BY cds.artist, cds.year
+  )',
+  [],
+);
+
+done_testing;