Modify the null-branch pruning introduced in ce55688, restore compat

In the case of "direct to HRI" we want to throw stuff away as before. In other cases however we no longer take an educated guess when to prune. Instead we mark each null-branch by simply blessing its containing arrayref. This allows us to keep the arguments to inflate_result 100% backwards compatible, while at the same time allowing "enlightened" inflate_result implementations to skip over the dead branches with minimal effort. Both ::Row::inflate_result and ::HRI::inflate_result were adjusted to react correctly to these marks. As a result the HRI folder gained another 5% speedup (unless sidestepped by the direct-to-HRI code, which is naturally much much more efficient). The current name of the "null class" is stored in the global $DBIx::Class::ResultSource::RowParser::Util::null_branch_class. Given that the entire codebase begs for standalone CPAN extraction this should do for the time being. While implementing the test changes it became clear that Test::More::is_deeply does not cut it (it ignores the blessed-ness of structures by design: Test-More/test-more#347). As a result a dep on Test::Deep was added, and several of the tests looking at inflate_result RVs were converted.
Perl5 · Feb 20, 2013 · 52864fb · 52864fb
1 parent b8ced1f
commit 52864fb
Show file tree

Hide file tree

Showing 15 changed files with 322 additions and 536 deletions.
diff --git a/Changes b/Changes
@@ -1,5 +1,10 @@
 Revision history for DBIx::Class
 
+    * New Features / Changes
+        - Revert to passing the original (pre-0.08240) arguments to
+          inflate_result() and remove the warning about ResultClass
+          inheritance.
+
 0.08240-TRIAL (EXPERIMENTAL BETA RELEASE) 2013-02-14 05:56 (UTC)
     * New Features / Changes
         - Rewrite from scratch the result constructor codepath - many bugfixes

diff --git a/Makefile.PL b/Makefile.PL
@@ -94,6 +94,7 @@ my $build_requires = {
 
 my $test_requires = {
   'File::Temp'               => '0.22',
+  'Test::Deep'               => '0.101',
   'Test::Exception'          => '0.31',
   'Test::Warn'               => '0.21',
   'Test::More'               => '0.94',

diff --git a/lib/DBIx/Class/ResultClass/HashRefInflator.pm b/lib/DBIx/Class/ResultClass/HashRefInflator.pm
@@ -66,37 +66,26 @@ my $mk_hash;
 $mk_hash = sub {
 
   my $hash = {
+
     # the main hash could be an undef if we are processing a skipped-over join
     $_[0] ? %{$_[0]} : (),
 
     # the second arg is a hash of arrays for each prefetched relation
-    map {
-      ref $_[1]->{$_}[0] eq 'ARRAY' # multi rel or not?
-        ? ( $_ => [ map
-            { $mk_hash->(@$_) || () }
-            @{$_[1]->{$_}}
-        ] )
-        : ( $_ => $mk_hash->( @{$_[1]->{$_}} ) )
-
-    } ( $_[1] ? ( keys %{$_[1]} ) : () )
+    map { $_ => (
+
+      # null-branch or not
+      ref $_[1]->{$_} eq $DBIx::Class::ResultSource::RowParser::Util::null_branch_class
+
+        ? ref $_[1]->{$_}[0] eq 'ARRAY' ? [] : undef
+
+        : ref $_[1]->{$_}[0] eq 'ARRAY'
+          ? [ map { $mk_hash->( @$_ ) || () } @{$_[1]->{$_}} ]
+          : $mk_hash->( @{$_[1]->{$_}} )
+
+    ) } ($_[1] ? keys %{$_[1]} : ())
   };
 
-  # if there is at least one defined column *OR* we are at the root of
-  # the resultset - consider the result real (and not an emtpy has_many
-  # rel containing one empty hashref)
-  # an empty arrayref is an empty multi-sub-prefetch - don't consider
-  # those either
-  return $hash if $_[2];
-
-  for (values %$hash) {
-    return $hash if (
-      defined $_
-        and
-      (ref $_ ne 'ARRAY' or scalar @$_)
-    );
-  }
-
-  return undef;
+  ($_[2] || keys %$hash) ? $hash : undef;
 };
 
 =head1 METHODS

diff --git a/lib/DBIx/Class/ResultSet.pm b/lib/DBIx/Class/ResultSet.pm
@@ -1406,39 +1406,17 @@ sub _construct_objects {
       collapse => $attrs->{collapse},
       premultiplied => $attrs->{_main_source_premultiplied},
       hri_style => 1,
-      prune_null_branches => 1,
     }) )->($rows, @extra_collapser_args);
   }
   # Regular multi-object
   else {
 
-    # The rationale is - if this is the ::Row inflator itself, or an around()
-    # we do prune, because we expect it.
-    # If not the case - let the user deal with the full output themselves
-    # Warn them while we are at it so we get a better idea what is out there
-    # on the DarkPan
-    $self->{_result_inflator}{prune_null_branches} = do {
-      $res_class->isa('DBIx::Class::Row')
-    } ? 1 : 0 unless defined $self->{_result_inflator}{prune_null_branches};
-
-    unless ($self->{_result_inflator}{prune_null_branches}) {
-      carp_once (
-        "ResultClass $res_class does not inherit from DBIx::Class::Row and "
-      . 'therefore its inflate_result() will receive the full prefetched data '
-      . 'tree, without any branch definedness checks. This is a compatibility '
-      . 'measure which will eventually disappear entirely. Please refer to '
-      . 't/resultset/inflate_result_api.t for an exhaustive description of the '
-      . 'upcoming changes'
-      );
-    }
-
-    ( $self->{_row_parser}{classic}{$self->{_result_inflator}{prune_null_branches}} ||= $rsrc->_mk_row_parser({
+    ( $self->{_row_parser}{classic} ||= $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}{prune_null_branches},
     }) )->($rows, @extra_collapser_args);
 
     $_ = $inflator_cref->($res_class, $rsrc, @$_) for @$rows;

diff --git a/lib/DBIx/Class/ResultSource/RowParser.pm b/lib/DBIx/Class/ResultSource/RowParser.pm
@@ -106,7 +106,6 @@ sub _mk_row_parser {
     $src = assemble_simple_parser({
       val_index => $val_index,
       hri_style => $args->{hri_style},
-      prune_null_branches => $args->{prune_null_branches},
     });
   }
   else {
@@ -130,13 +129,12 @@ sub _mk_row_parser {
       val_index => $val_index,
       collapse_map => $collapse_map,
       hri_style => $args->{hri_style},
-      prune_null_branches => $args->{prune_null_branches},
     });
   }
 
-  return (! $args->{eval})
-    ? $src
-    : eval "sub { $src }" || die $@
+  return $args->{eval}
+    ? ( eval "sub { $src }" || die $@ )
+    : $src
   ;
 }
 

diff --git a/lib/DBIx/Class/ResultSource/RowParser/Util.pm b/lib/DBIx/Class/ResultSource/RowParser/Util.pm
@@ -13,6 +13,9 @@ our @EXPORT_OK = qw(
   assemble_collapsing_parser
 );
 
+# working title - we are hoping to extract this eventually...
+our $null_branch_class = 'DBIx::ResultParser::RelatedNullBranch';
+
 sub assemble_simple_parser {
   #my ($args) = @_;
 
@@ -51,24 +54,38 @@ sub __visit_infmap_simple {
   my @relperl;
   for my $rel (sort keys %$rel_cols) {
 
-    push @relperl, join ' => ', perlstring($rel), __visit_infmap_simple({ %$args,
+    my $rel_struct = __visit_infmap_simple({ %$args,
       val_index => $rel_cols->{$rel},
     });
 
-    if ($args->{prune_null_branches} and keys %$my_cols) {
+    if (keys %$my_cols) {
 
-      my @branch_null_checks = map
+      my $branch_null_checks = join ' && ', map
         { "( ! defined '\xFF__VALPOS__${_}__\xFF' )" }
         sort { $a <=> $b } values %{$rel_cols->{$rel}}
       ;
 
-      $relperl[-1] = sprintf ( '(%s) ? ( %s => %s ) : ( %s )',
-        join (' && ', @branch_null_checks ),
-        perlstring($rel),
-        $args->{hri_style} ? 'undef' : '[]',
-        $relperl[-1],
-      );
+      if ($args->{hri_style}) {
+        $rel_struct = sprintf ( '( (%s) ? undef : %s )',
+          $branch_null_checks,
+          $rel_struct,
+        );
+      }
+      else {
+        $rel_struct = sprintf ( '( (%s) ? bless( (%s), %s ) : %s )',
+          $branch_null_checks,
+          $rel_struct,
+          perlstring($null_branch_class),
+          $rel_struct,
+        );
+      }
     }
+
+    push @relperl, sprintf '( %s => %s )',
+      perlstring($rel),
+      $rel_struct,
+    ;
+
   }
 
   my $me_struct;
@@ -159,7 +176,7 @@ sub assemble_collapsing_parser {
     );
 
     # the rel assemblers
-    %4$s
+%4$s
 
     $_[0][$result_pos++] = $collapse_idx[0]%2$s
       if $is_new_res;
@@ -195,24 +212,25 @@ sub __visit_infmap_collapse {
   }
 
 
-  my $node_key = $args->{collapse_map}->{-custom_node_key} || join ('', map
-    { "{'\xFF__IDVALPOS__${_}__\xFF'}" }
-    @{$args->{collapse_map}->{-identifying_columns}}
-  );
-
-  my $me_struct;
-
   if ($args->{hri_style}) {
     delete $my_cols->{$_} for grep { $rel_cols->{$_} } keys %$my_cols;
   }
 
-  if (keys %$my_cols) {
-    $me_struct = __visit_dump({ map { $_ => "\xFF__VALPOS__$my_cols->{$_}__\xFF" } (keys %$my_cols) });
-    $me_struct = "[ $me_struct ]" unless $args->{hri_style};
-  }
+  my $me_struct;
+  $me_struct = __visit_dump({ map { $_ => "\xFF__VALPOS__$my_cols->{$_}__\xFF" } (keys %$my_cols) })
+    if keys %$my_cols;
 
+  $me_struct = sprintf( '[ %s ]', $me_struct||'' )
+    unless $args->{hri_style};
+
+
+  my $node_key = $args->{collapse_map}->{-custom_node_key} || join ('', map
+    { "{'\xFF__IDVALPOS__${_}__\xFF'}" }
+    @{$args->{collapse_map}->{-identifying_columns}}
+  );
   my $node_idx_slot = sprintf '$collapse_idx[%d]%s', $cur_node_idx, $node_key;
 
+
   my @src;
 
   if ($cur_node_idx == 0) {
@@ -251,9 +269,6 @@ sub __visit_infmap_collapse {
   for my $rel (sort keys %$rel_cols) {
 
     my $relinfo = $args->{collapse_map}{$rel};
-    if ($args->{collapse_map}{-is_optional}) {
-      $relinfo = { %$relinfo, -is_optional => 1 };
-    }
 
     ($rel_src, $stats->{$rel}) = __visit_infmap_collapse({ %$args,
       val_index => $rel_cols->{$rel},
@@ -267,8 +282,6 @@ sub __visit_infmap_collapse {
     push @src, @$rel_src;
 
     if (
-      $args->{prune_null_branches}
-        and
       $relinfo->{-is_optional}
         and
       defined ( my $first_distinct_child_idcol = first
@@ -277,17 +290,28 @@ sub __visit_infmap_collapse {
       )
     ) {
 
-      $src[$rel_src_pos] = sprintf( '%s and %s',
-        "( defined '\xFF__VALPOS__${first_distinct_child_idcol}__\xFF' )",
-        $src[$rel_src_pos],
-      );
+      if ($args->{hri_style}) {
 
-      splice @src, $rel_src_pos + 1, 0, sprintf ( '%s%s{%s} ||= %s;',
-        $node_idx_slot,
-        $args->{hri_style} ? '' : '[1]',
-        perlstring($rel),
-        $args->{hri_style} && $relinfo->{-is_single} ? 'undef' : '[]',
-      );
+        $src[$rel_src_pos] = sprintf( '%s and %s',
+          "( defined '\xFF__VALPOS__${first_distinct_child_idcol}__\xFF' )",
+          $src[$rel_src_pos],
+        );
+
+        splice @src, $rel_src_pos + 1, 0, sprintf ( '%s{%s} ||= %s;',
+          $node_idx_slot,
+          perlstring($rel),
+          $relinfo->{-is_single} ? 'undef' : '[]',
+        );
+      }
+      else {
+
+        splice @src, $rel_src_pos + 1, 0, sprintf ( '(defined %s) or bless (%s[1]{%s}, %s);',
+          "'\xFF__VALPOS__${first_distinct_child_idcol}__\xFF'",
+          $node_idx_slot,
+          perlstring($rel),
+          perlstring($null_branch_class),
+        );
+      }
     }
   }
 
@@ -305,6 +329,7 @@ sub __visit_infmap_collapse {
 # keep our own DD object around so we don't have to fitz with quoting
 my $dumper_obj;
 sub __visit_dump {
+
   # we actually will be producing functional perl code here,
   # thus no second-guessing of what these globals might have
   # been set to. DO NOT CHANGE!

diff --git a/lib/DBIx/Class/Row.pm b/lib/DBIx/Class/Row.pm
@@ -1186,7 +1186,11 @@ sub inflate_result {
     for my $pre ( keys %$prefetch ) {
 
       my @pre_objects;
-      if (@{$prefetch->{$pre}||[]}) {
+      if (
+        @{$prefetch->{$pre}||[]}
+          and
+        ref($prefetch->{$pre}) ne $DBIx::Class::ResultSource::RowParser::Util::null_branch_class
+      ) {
         my $pre_source = $source->related_source($pre);
 
         @pre_objects = map {