Add (slowish) sanity check to detect incorrect non-nullable metadata
Peter Rabbitson [Fri, 19 Apr 2013 14:22:46 +0000 (16:22 +0200)]
We can't get away with this - the alternative is a cryptic 'Use of uninitialized value'
exception - good luck debugging that

Changes
lib/DBIx/Class/ResultSet.pm
lib/DBIx/Class/ResultSource/RowParser.pm
t/resultset/rowparser_internals.t

diff --git a/Changes b/Changes
index 7edc5f8..54c259a 100644 (file)
--- a/Changes
+++ b/Changes
@@ -6,6 +6,10 @@ Revision history for DBIx::Class
         - Invoking get_inflated_columns() no longer fires get_columns() but
           instead retrieves data from individual non-inflatable columns via
           get_column()
+        - Limited checks are performed on whether columns without declared
+          is_nullable => 1 metadata do in fact sometimes fetch NULLs from
+          the database (the check is currently very limited and is performed
+          only on resultset collapse when the alternative is rather worse)
 
     * Fixes
         - Fix _dbi_attrs_for_bind() being called befor DBI has been loaded
index df7b701..d02d6ff 100644 (file)
@@ -1376,16 +1376,6 @@ sub _construct_results {
     }
   }
 
-  my @extra_collapser_args;
-  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
-      sub { my @r = $cursor->next or return; \@r }, # how the collapser gets more rows
-      ($self->{_stashed_rows} = []),                # where does it stuff excess
-    );
-  }
-
   # hotspot - skip the setter
   my $res_class = $self->_result_class;
 
@@ -1439,35 +1429,79 @@ sub _construct_results {
       );
     }
   }
-  # Special-case multi-object HRI (we always prune, and there is no $inflator_cref pass)
-  elsif ($self->{_result_inflator}{is_hri}) {
+  else {
+    my $parser_type =
+        $self->{_result_inflator}{is_hri}       ? 'hri'
+      : $self->{_result_inflator}{is_core_row}  ? 'classic_pruning'
+      :                                           'classic_nonpruning'
+    ;
 
     # $args and $attrs to _mk_row_parser are seperated to delineate what is
     # core collapser stuff and what is dbic $rs specific
-    ( $self->{_row_parser}{hri} ||= $rsrc->_mk_row_parser({
+    @{$self->{_row_parser}{$parser_type}}{qw(cref nullcheck)} = $rsrc->_mk_row_parser({
       eval => 1,
       inflate_map => $infmap,
       collapse => $attrs->{collapse},
       premultiplied => $attrs->{_main_source_premultiplied},
-      hri_style => 1,
-      prune_null_branches => 1,
-    }, $attrs) )->($rows, @extra_collapser_args);
+      hri_style => $self->{_result_inflator}{is_hri},
+      prune_null_branches => $self->{_result_inflator}{is_hri} || $self->{_result_inflator}{is_core_row},
+    }, $attrs) unless $self->{_row_parser}{$parser_type}{cref};
+
+    # column_info metadata historically hasn't been too reliable.
+    # We need to start fixing this somehow (the collapse resolver
+    # can't work without it). Add an explicit check for the *main*
+    # result, hopefully this will gradually weed out such errors
+    #
+    # FIXME - this is a temporary kludge that reduces perfromance
+    # It is however necessary for the time being
+    my ($unrolled_non_null_cols_to_check, $err);
+
+    if (my $check_non_null_cols = $self->{_row_parser}{$parser_type}{nullcheck} ) {
+
+      $err =
+        'Collapse aborted due to invalid ResultSource metadata - the following '
+      . 'selections are declared non-nullable but NULLs were retrieved: '
+      ;
+
+      my @violating_idx;
+      COL: for my $i (@$check_non_null_cols) {
+        ! defined $_->[$i] and push @violating_idx, $i and next COL for @$rows;
+      }
+
+      $self->throw_exception( $err . join (', ', map { "'$infmap->[$_]'" } @violating_idx ) )
+        if @violating_idx;
+
+      $unrolled_non_null_cols_to_check = join (',', @$check_non_null_cols);
+    }
+
+    my $next_cref =
+      ($did_fetch_all or ! $attrs->{collapse})  ? undef
+    : defined $unrolled_non_null_cols_to_check  ? eval sprintf <<'EOS', $unrolled_non_null_cols_to_check
+sub {
+  # FIXME SUBOPTIMAL - we can do better, cursor->next/all (well diff. methods) should return a ref
+  my @r = $cursor->next or return;
+  if (my @violating_idx = grep { ! defined $r[$_] } (%s) ) {
+    $self->throw_exception( $err . join (', ', map { "'$infmap->[$_]'" } @violating_idx ) )
   }
-  # Regular multi-object
-  else {
-    my $parser_type = $self->{_result_inflator}{is_core_row} ? 'classic_pruning' : 'classic_nonpruning';
+  \@r
+}
+EOS
+    : sub {
+        # FIXME SUBOPTIMAL - we can do better, cursor->next/all (well diff. methods) should return a ref
+        my @r = $cursor->next or return;
+        \@r
+      }
+    ;
 
-    # $args and $attrs to _mk_row_parser are seperated to delineate what is
-    # core collapser stuff and what is dbic $rs specific
-    ( $self->{_row_parser}{$parser_type} ||= $rsrc->_mk_row_parser({
-      eval => 1,
-      inflate_map => $infmap,
-      collapse => $attrs->{collapse},
-      premultiplied => $attrs->{_main_source_premultiplied},
-      prune_null_branches => $self->{_result_inflator}{is_core_row},
-    }, $attrs) )->($rows, @extra_collapser_args);
+    $self->{_row_parser}{$parser_type}{cref}->(
+      $rows,
+      $next_cref ? ( $next_cref, $self->{_stashed_rows} = [] ) : (),
+    );
 
-    $_ = $inflator_cref->($res_class, $rsrc, @$_) for @$rows;
+    # Special-case multi-object HRI - there is no $inflator_cref pass
+    unless ($self->{_result_inflator}{is_hri}) {
+      $_ = $inflator_cref->($res_class, $rsrc, @$_) for @$rows
+    }
   }
 
   # The @$rows check seems odd at first - why wouldn't we want to warn
index fa69299..704ebf8 100644 (file)
@@ -109,34 +109,38 @@ sub _mk_row_parser {
     },
   );
 
-  my $src = $args->{collapse}
-    ? assemble_collapsing_parser({
+  my $check_null_columns;
+
+  my $src = (! $args->{collapse} ) ? assemble_simple_parser(\%common) : do {
+    my $collapse_map = $self->_resolve_collapse ({
+      # FIXME
+      # only consider real columns (not functions) during collapse resolution
+      # this check shouldn't really be here, as fucktards are not supposed to
+      # alias random crap to existing column names anyway, but still - just in
+      # case
+      # FIXME !!!! - this does not yet deal with unbalanced selectors correctly
+      # (it is now trivial as the attrs specify where things go out of sync
+      # needs MOAR tests)
+      as => { map
+        { ref $attrs->{select}[$common{val_index}{$_}] ? () : ( $_ => $common{val_index}{$_} ) }
+        keys %{$common{val_index}}
+      },
+      premultiplied => $args->{premultiplied},
+    });
+
+    $check_null_columns = $collapse_map->{-identifying_columns}
+      if @{$collapse_map->{-identifying_columns}};
+
+    assemble_collapsing_parser({
       %common,
-      collapse_map => $self->_resolve_collapse ({
-        # FIXME
-        # only consider real columns (not functions) during collapse resolution
-        # this check shouldn't really be here, as fucktards are not supposed to
-        # alias random crap to existing column names anyway, but still - just in
-        # case
-        # FIXME !!!! - this does not yet deal with unbalanced selectors correctly
-        # (it is now trivial as the attrs specify where things go out of sync
-        # needs MOAR tests)
-        as => { map
-          { ref $attrs->{select}[$common{val_index}{$_}] ? () : ( $_ => $common{val_index}{$_} ) }
-          keys %{$common{val_index}}
-        },
-        premultiplied => $args->{premultiplied},
-      })
-    })
-    : assemble_simple_parser(
-      \%common
-    )
-  ;
-
-  return $args->{eval}
-    ? ( eval "sub $src" || die $@ )
-    : $src
-  ;
+      collapse_map => $collapse_map,
+    });
+  };
+
+  return (
+    $args->{eval} ? ( eval "sub $src" || die $@ ) : $src,
+    $check_null_columns,
+  );
 }
 
 
index c53b3eb..b089ecc 100644 (file)
@@ -21,9 +21,9 @@ my $infmap = [qw/
 /];
 
 is_same_src (
-  $schema->source ('CD')->_mk_row_parser({
+  ($schema->source ('CD')->_mk_row_parser({
     inflate_map => $infmap,
-  }),
+  }))[0],
   '$_ = [
     { year => $_->[1] },
     { single_track => ( ! defined( $_->[0]) )
@@ -60,9 +60,9 @@ $infmap = [qw/
 /];
 
 is_same_src (
-  $schema->source ('CD')->_mk_row_parser({
+  ($schema->source ('CD')->_mk_row_parser({
     inflate_map => $infmap,
-  }),
+  }))[0],
   '$_ = [
     { artist => $_->[5], title => $_->[4], year => $_->[2] },
     {
@@ -137,10 +137,10 @@ is_same_src (
 );
 
 is_same_src (
-  $schema->source ('CD')->_mk_row_parser({
+  ($schema->source ('CD')->_mk_row_parser({
     prune_null_branches => 1,
     inflate_map => $infmap,
-  }),
+  }))[0],
   '$_ = [
     { artist => $_->[5], title => $_->[4], year => $_->[2] },
     {
@@ -173,11 +173,11 @@ is_same_src (
 );
 
 is_same_src (
-  $schema->source ('CD')->_mk_row_parser({
+  ($schema->source ('CD')->_mk_row_parser({
     hri_style => 1,
     prune_null_branches => 1,
     inflate_map => $infmap,
-  }),
+  }))[0],
   '$_ = {
       artist => $_->[5], title => $_->[4], year => $_->[2],
 
@@ -243,10 +243,10 @@ is_deeply (
 );
 
 is_same_src (
-  $schema->source ('CD')->_mk_row_parser({
+  ($schema->source ('CD')->_mk_row_parser({
     inflate_map => $infmap,
     collapse => 1,
-  }),
+  }))[0],
   ' my $rows_pos = 0;
     my ($result_pos, @collapse_idx, $cur_row_data, %cur_row_ids);
 
@@ -301,12 +301,12 @@ is_same_src (
 );
 
 is_same_src (
-  $schema->source ('CD')->_mk_row_parser({
+  ($schema->source ('CD')->_mk_row_parser({
     inflate_map => $infmap,
     collapse => 1,
     hri_style => 1,
     prune_null_branches => 1,
-  }),
+  }))[0],
   ' my $rows_pos = 0;
     my ($result_pos, @collapse_idx, $cur_row_data);
 
@@ -420,10 +420,10 @@ is_deeply (
 );
 
 is_same_src (
-  $schema->source ('CD')->_mk_row_parser({
+  ($schema->source ('CD')->_mk_row_parser({
     inflate_map => $infmap,
     collapse => 1,
-  }),
+  }))[0],
   ' my $rows_pos = 0;
     my ($result_pos, @collapse_idx, $cur_row_data, %cur_row_ids);
 
@@ -486,11 +486,11 @@ is_same_src (
 );
 
 is_same_src (
-  $schema->source ('CD')->_mk_row_parser({
+  ($schema->source ('CD')->_mk_row_parser({
     inflate_map => $infmap,
     collapse => 1,
     prune_null_branches => 1,
-  }),
+  }))[0],
   ' my $rows_pos = 0;
     my ($result_pos, @collapse_idx, $cur_row_data);
 
@@ -601,10 +601,10 @@ is_deeply (
 );
 
 is_same_src (
-  $schema->source ('CD')->_mk_row_parser({
+  ($schema->source ('CD')->_mk_row_parser({
     inflate_map => $infmap,
     collapse => 1,
-  }),
+  }))[0],
   ' my $rows_pos = 0;
     my ($result_pos, @collapse_idx, $cur_row_data, %cur_row_ids);
 
@@ -669,12 +669,12 @@ is_same_src (
 );
 
 is_same_src (
-  $schema->source ('CD')->_mk_row_parser({
+  ($schema->source ('CD')->_mk_row_parser({
     inflate_map => $infmap,
     collapse => 1,
     hri_style => 1,
     prune_null_branches => 1,
-  }),
+  }))[0],
   ' my $rows_pos = 0;
     my ($result_pos, @collapse_idx, $cur_row_data, %cur_row_ids);