Now collapse is a flag, not a list
Peter Rabbitson [Wed, 17 Feb 2010 07:43:28 +0000 (07:43 +0000)]
lib/DBIx/Class/ResultSet.pm
lib/DBIx/Class/ResultSetColumn.pm
lib/DBIx/Class/ResultSource.pm
lib/DBIx/Class/Storage/DBI.pm
t/90join_torture.t

index cbf8be4..0cc7cd1 100644 (file)
@@ -536,7 +536,7 @@ sub find {
 
   # Run the query
   my $rs = $self->search ($query, {result_class => $self->result_class, %$attrs});
-  if (keys %{$rs->_resolved_attrs->{collapse}}) {
+  if ($rs->_resolved_attrs->{collapse}) {
     my $row = $rs->next;
     carp "Query returned more than one row" if $rs->next;
     return $row;
@@ -723,7 +723,7 @@ sub single {
 
   my $attrs = $self->_resolved_attrs_copy;
 
-  if (keys %{$attrs->{collapse}}) {
+  if ($attrs->{collapse}) {
     $self->throw_exception(
       'single() can not be used on resultsets prefetching has_many. Use find( \%cond ) or next() instead'
     );
@@ -1001,7 +1001,7 @@ sub _collapse_result {
     }
     return undef unless $has_def;
 
-    my $collapse = keys %{ $self->{_attrs}{collapse} || {} };
+    my $collapse = $self->_resolved_attrs->{collapse};
     my $rows     = [];
     my @row      = @$row_ref;
     do {
@@ -1236,7 +1236,7 @@ sub _count_subq_rs {
 
   # if we multi-prefetch we group_by primary keys only as this is what we would
   # get out of the rs via ->next/->all. We *DO WANT* to clobber old group_by regardless
-  if ( keys %{$attrs->{collapse}}  ) {
+  if ($attrs->{collapse}) {
     $sub_attrs->{group_by} = [ map { "$attrs->{alias}.$_" } ($rsrc->primary_columns) ]
   }
 
@@ -1306,7 +1306,7 @@ sub all {
 
   my @obj;
 
-  if (keys %{$self->_resolved_attrs->{collapse}}) {
+  if ($self->_resolved_attrs->{collapse}) {
     # Using $self->cursor->all is really just an optimisation.
     # If we're collapsing has_many prefetches it probably makes
     # very little difference, and this is cleaner than hacking
@@ -2841,7 +2841,6 @@ sub _resolved_attrs {
     }
   }
   else {
-
     # otherwise we intialise select & as to empty
     $attrs->{select} = [];
     $attrs->{as}     = [];
@@ -2931,9 +2930,8 @@ sub _resolved_attrs {
     }
   }
 
-  $attrs->{collapse} ||= {};
   if ( my $prefetch = delete $attrs->{prefetch} ) {
-    $prefetch = $self->_merge_attr( {}, $prefetch );
+    $attrs->{collapse} = 1;
 
     my $prefetch_ordering = [];
 
@@ -2958,8 +2956,7 @@ sub _resolved_attrs {
       }
     }
 
-    my @prefetch =
-      $source->_resolve_prefetch( $prefetch, $alias, $join_map, $prefetch_ordering, $attrs->{collapse} );
+    my @prefetch = $source->_resolve_prefetch( $prefetch, $alias, $join_map, $prefetch_ordering );
 
     # we need to somehow mark which columns came from prefetch
     $attrs->{_prefetch_select} = [ map { $_->[0] } @prefetch ];
@@ -2971,6 +2968,30 @@ sub _resolved_attrs {
     $attrs->{_collapse_order_by} = \@$prefetch_ordering;
   }
 
+  # run through the resulting joinstructure (starting from our current slot)
+  # and unset collapse if proven unnesessary
+  if ($attrs->{collapse} && ref $attrs->{from} eq 'ARRAY') {
+
+    if (@{$attrs->{from}} > 1) {
+
+      # find where our table-spec starts and consider only things after us
+      my @fromlist = @{$attrs->{from}};
+      while (@fromlist) {
+        my $t = shift @fromlist;
+        $t = $t->[0] if ref $t eq 'ARRAY';  #me vs join from-spec mismatch
+        last if ($t->{-alias} && $t->{-alias} eq $alias);
+      }
+
+      if (@fromlist) {
+        $attrs->{collapse} = scalar grep { ! $_->[0]{-is_single} } (@fromlist);
+      }
+    }
+    else {
+      # no joins - no collapse
+      $attrs->{collapse} = 0;
+    }
+  }
+
   # if both page and offset are specified, produce a combined offset
   # even though it doesn't make much sense, this is what pre 081xx has
   # been doing
index e11f868..78ac9ba 100644 (file)
@@ -44,7 +44,7 @@ sub new {
 
   $rs->throw_exception('column must be supplied') unless $column;
 
-  my $orig_attrs = $rs->_resolved_attrs;
+  my $orig_attrs = $rs->_resolved_attrs_copy;
 
   # If $column can be found in the 'as' list of the parent resultset, use the
   # corresponding element of its 'select' list (to keep any custom column
@@ -91,7 +91,7 @@ sub new {
 
   # {collapse} would mean a has_many join was injected, which in turn means
   # we need to group *IF WE CAN* (only if the column in question is unique)
-  if (!$new_attrs->{group_by} && keys %{$orig_attrs->{collapse}}) {
+  if (!$new_attrs->{group_by} && $orig_attrs->{collapse}) {
 
     # scan for a constraint that would contain our column only - that'd be proof
     # enough it is unique
index 9265d32..c1d9c3c 100644 (file)
@@ -1378,7 +1378,7 @@ sub _resolve_condition {
 # in the supplied relationships.
 
 sub _resolve_prefetch {
-  my ($self, $pre, $alias, $alias_map, $order, $collapse, $pref_path) = @_;
+  my ($self, $pre, $alias, $alias_map, $order, $pref_path) = @_;
   $pref_path ||= [];
 
   if (not defined $pre) {
@@ -1386,15 +1386,15 @@ sub _resolve_prefetch {
   }
   elsif( ref $pre eq 'ARRAY' ) {
     return
-      map { $self->_resolve_prefetch( $_, $alias, $alias_map, $order, $collapse, [ @$pref_path ] ) }
+      map { $self->_resolve_prefetch( $_, $alias, $alias_map, $order, [ @$pref_path ] ) }
         @$pre;
   }
   elsif( ref $pre eq 'HASH' ) {
     my @ret =
     map {
-      $self->_resolve_prefetch($_, $alias, $alias_map, $order, $collapse, [ @$pref_path ] ),
+      $self->_resolve_prefetch($_, $alias, $alias_map, $order, [ @$pref_path ] ),
       $self->related_source($_)->_resolve_prefetch(
-               $pre->{$_}, "${alias}.$_", $alias_map, $order, $collapse, [ @$pref_path, $_] )
+               $pre->{$_}, "${alias}.$_", $alias_map, $order, [ @$pref_path, $_] )
     } keys %$pre;
     return @ret;
   }
@@ -1427,14 +1427,11 @@ sub _resolve_prefetch {
 
       #my @col = map { (/^self\.(.+)$/ ? ("${as_prefix}.$1") : ()); }
       #              values %{$rel_info->{cond}};
-      $collapse->{".${as_prefix}${pre}"} = [ $rel_source->primary_columns ];
-        # action at a distance. prepending the '.' allows simpler code
-        # in ResultSet->_collapse_result
       my @key = map { (/^foreign\.(.+)$/ ? ($1) : ()); }
                     keys %{$rel_info->{cond}};
       my @ord = (ref($rel_info->{attrs}{order_by}) eq 'ARRAY'
                    ? @{$rel_info->{attrs}{order_by}}
-   
+
                 : (defined $rel_info->{attrs}{order_by}
                        ? ($rel_info->{attrs}{order_by})
                        : ()));
index 0030431..b8dafcf 100644 (file)
@@ -1793,8 +1793,8 @@ sub _select_args {
   # see if we need to tear the prefetch apart otherwise delegate the limiting to the
   # storage, unless software limit was requested
   if (
-    #limited has_many
-    ( $attrs->{rows} && keys %{$attrs->{collapse}} )
+    # limited collapsing has_many
+    ( $attrs->{rows} && $attrs->{collapse} )
        ||
     # limited prefetch with RNO subqueries
     (
index 6eeda5a..90a78b2 100644 (file)
@@ -1,37 +1,64 @@
 use strict;
-use warnings;  
+use warnings;
 
 use Test::More;
+use Test::Exception;
+
 use lib qw(t/lib);
 use DBICTest;
+use DBIC::SqlMakerTest;
 my $schema = DBICTest->init_schema();
 
-plan tests => 22;
-
- {
-   my $rs = $schema->resultset( 'CD' )->search(
-     {
-       'producer.name'   => 'blah',
-       'producer_2.name' => 'foo',
-     },
-     {
-       'join' => [
-         { cd_to_producer => 'producer' },
-         { cd_to_producer => 'producer' },
-       ],
-       'prefetch' => [
-         'artist',
-         { cd_to_producer => 'producer' },
-       ],
-     }
-   );
-  
-   eval {
-     my @rows = $rs->all();
-   };
-   is( $@, '' );
- }
-
+lives_ok (sub {
+  my $rs = $schema->resultset( 'CD' )->search(
+    {
+      'producer.name'   => 'blah',
+      'producer_2.name' => 'foo',
+    },
+    {
+      'join' => [
+        { cd_to_producer => 'producer' },
+        { cd_to_producer => 'producer' },
+      ],
+      'prefetch' => [
+        'artist',
+        { cd_to_producer => { producer => 'producer_to_cd' } },
+      ],
+    }
+  );
+
+  my @executed = $rs->all();
+
+  is_same_sql_bind (
+    $rs->as_query,
+    '(
+      SELECT  me.cdid, me.artist, me.title, me.year, me.genreid, me.single_track,
+              artist.artistid, artist.name, artist.rank, artist.charfield,
+              cd_to_producer.cd, cd_to_producer.producer, cd_to_producer.attribute,
+              producer.producerid, producer.name,
+              producer_to_cd.cd, producer_to_cd.producer, producer_to_cd.attribute
+        FROM cd me
+        LEFT JOIN cd_to_producer cd_to_producer
+          ON cd_to_producer.cd = me.cdid
+        LEFT JOIN producer producer
+          ON producer.producerid = cd_to_producer.producer
+        LEFT JOIN cd_to_producer producer_to_cd
+          ON producer_to_cd.producer = producer.producerid
+        LEFT JOIN cd_to_producer cd_to_producer_2
+          ON cd_to_producer_2.cd = me.cdid
+        LEFT JOIN producer producer_2
+          ON producer_2.producerid = cd_to_producer_2.producer
+        JOIN artist artist ON artist.artistid = me.artist
+      WHERE ( ( producer.name = ? AND producer_2.name = ? ) )
+      ORDER BY cd_to_producer.cd, producer_to_cd.producer
+    )',
+    [
+      [ 'producer.name' => 'blah' ],
+      [ 'producer_2.name' => 'foo' ],
+    ],
+  );
+
+}, 'Complex join parsed/executed properly');
 
 my @rs1a_results = $schema->resultset("Artist")->search_related('cds', {title => 'Forkful of bees'}, {order_by => 'title'});
 is($rs1a_results[0]->title, 'Forkful of bees', "bare field conditions okay after search related");
@@ -125,4 +152,4 @@ my $second_search_rs = $rs->search({ 'cds_2.cdid' => '2' }, { join =>
 is(scalar(@{$second_search_rs->{attrs}->{join}}), 3, 'both joins kept');
 ok($second_search_rs->next, 'query on double joined rel runs okay');
 
-1;
+done_testing;