Warn in case of iterative collapse being upgraded to an eager cursor slurp

[dbsrgits/DBIx-Class.git] / lib / DBIx / Class / ResultSet.pm

diff --git a/lib/DBIx/Class/ResultSet.pm b/lib/DBIx/Class/ResultSet.pm
index cedb982..2582fe2 100644 (file)
--- a/lib/DBIx/Class/ResultSet.pm
+++ b/lib/DBIx/Class/ResultSet.pm
@@ -1277,9 +1277,17 @@ sub _construct_results {
   my $rsrc = $self->result_source;
   my $attrs = $self->_resolved_attrs;
 
-  if (!$fetch_all and ! $attrs->{order_by} and $attrs->{collapse}) {
+  if (
+    ! $fetch_all
+      and
+    ! $attrs->{order_by}
+      and
+    $attrs->{collapse}
+      and
+    my @pcols = $rsrc->primary_columns
+  ) {
     # default order for collapsing unless the user asked for something
-    $attrs->{order_by} = [ map { join '.', $attrs->{alias}, $_} $rsrc->primary_columns ];
+    $attrs->{order_by} = [ map { join '.', $attrs->{alias}, $_} @pcols ];
     $attrs->{_ordered_for_collapse} = 1;
     $attrs->{_order_is_artificial} = 1;
   }
@@ -1291,6 +1299,8 @@ sub _construct_results {
   # a surprising amount actually
   my $rows = delete $self->{_stashed_rows};
 
+  my $did_fetch_all = $fetch_all;
+
   if ($fetch_all) {
     # FIXME SUBOPTIMAL - we can do better, cursor->next/all (well diff. methods) should return a ref
     $rows = [ ($rows ? @$rows : ()), $cursor->all ];
@@ -1327,7 +1337,7 @@ sub _construct_results {
     } unless defined $attrs->{_ordered_for_collapse};
 
     if (! $attrs->{_ordered_for_collapse}) {
-      $fetch_all = 1;
+      $did_fetch_all = 1;
 
       # instead of looping over ->next, use ->all in stealth mode
       # *without* calling a ->reset afterwards
@@ -1339,7 +1349,7 @@ sub _construct_results {
     }
   }
 
-  if (! $fetch_all and ! @{$rows||[]} ) {
+  if (! $did_fetch_all and ! @{$rows||[]} ) {
     # FIXME SUBOPTIMAL - we can do better, cursor->next/all (well diff. methods) should return a ref
     if (scalar (my @r = $cursor->next) ) {
       $rows = [ \@r ];
@@ -1349,7 +1359,7 @@ sub _construct_results {
   return undef unless @{$rows||[]};
 
   my @extra_collapser_args;
-  if ($attrs->{collapse} and ! $fetch_all ) {
+  if ($attrs->{collapse} and ! $did_fetch_all ) {
 
     @extra_collapser_args = (
       # FIXME SUBOPTIMAL - we can do better, cursor->next/all (well diff. methods) should return a ref
@@ -1368,12 +1378,23 @@ sub _construct_results {
 
   my $infmap = $attrs->{as};
 
-  $self->{_result_inflator}{is_hri} = do { ( $inflator_cref == (
-    require DBIx::Class::ResultClass::HashRefInflator
-      &&
-    DBIx::Class::ResultClass::HashRefInflator->can('inflate_result')
-  ) ) ? 1 : 0
-  } unless defined $self->{_result_inflator}{is_hri};
+
+  $self->{_result_inflator}{is_core_row} = ( (
+    $inflator_cref
+      ==
+    ( \&DBIx::Class::Row::inflate_result || die "No ::Row::inflate_result() - can't happen" )
+  ) ? 1 : 0 ) unless defined $self->{_result_inflator}{is_core_row};
+
+  $self->{_result_inflator}{is_hri} = ( (
+    ! $self->{_result_inflator}{is_core_row}
+      and
+    $inflator_cref == (
+      require DBIx::Class::ResultClass::HashRefInflator
+        &&
+      DBIx::Class::ResultClass::HashRefInflator->can('inflate_result')
+    )
+  ) ? 1 : 0 ) unless defined $self->{_result_inflator}{is_hri};
+
 
   if (! $attrs->{_related_results_construction}) {
     # construct a much simpler array->hash folder for the one-table cases right here
@@ -1401,7 +1422,7 @@ sub _construct_results {
       );
     }
   }
-  # Special-case multi-object HRI (we always prune)
+  # Special-case multi-object HRI (we always prune, and there is no $inflator_cref pass)
   elsif ($self->{_result_inflator}{is_hri}) {
     ( $self->{_row_parser}{hri} ||= $rsrc->_mk_row_parser({
       eval => 1,
@@ -1410,26 +1431,34 @@ sub _construct_results {
       collapse => $attrs->{collapse},
       premultiplied => $attrs->{_main_source_premultiplied},
       hri_style => 1,
+      prune_null_branches => 1,
     }) )->($rows, @extra_collapser_args);
   }
   # Regular multi-object
   else {
+    my $parser_type = $self->{_result_inflator}{is_core_row} ? 'classic_pruning' : 'classic_nonpruning';
 
-    ( $self->{_row_parser}{classic} ||= $rsrc->_mk_row_parser({
+    ( $self->{_row_parser}{$parser_type} ||= $rsrc->_mk_row_parser({
       eval => 1,
       inflate_map => $infmap,
       selection => $attrs->{select},
       collapse => $attrs->{collapse},
       premultiplied => $attrs->{_main_source_premultiplied},
+      prune_null_branches => $self->{_result_inflator}{is_core_row},
     }) )->($rows, @extra_collapser_args);
 
     $_ = $inflator_cref->($res_class, $rsrc, @$_) for @$rows;
   }
 
-  # CDBI compat stuff
-  if ($attrs->{record_filter}) {
-    $_ = $attrs->{record_filter}->($_) for @$rows;
-  }
+  # The @$rows check seems odd at first - why wouldn't we want to warn
+  # regardless? The issue is things like find() etc, where the user
+  # *knows* only one result will come back. In these cases the ->all
+  # is not a pessimization, but rather something we actually want
+  carp_unique(
+    'Unable to properly collapse has_many results in iterator mode due '
+  . 'to order criteria - performed an eager cursor slurp underneath. '
+  . 'Consider using ->all() instead'
+  ) if ( ! $fetch_all and @$rows > 1 );
 
   return $rows;
 }