From: Peter Rabbitson Date: Fri, 19 Apr 2013 14:22:46 +0000 (+0200) Subject: Add (slowish) sanity check to detect incorrect non-nullable metadata X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=02a73c96af68914a8a53a4772d214e34333b9e57;p=dbsrgits%2FDBIx-Class-Historic.git Add (slowish) sanity check to detect incorrect non-nullable metadata We can't get away with this - the alternative is a cryptic 'Use of uninitialized value' exception - good luck debugging that --- diff --git a/Changes b/Changes index 7edc5f8..54c259a 100644 --- 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 diff --git a/lib/DBIx/Class/ResultSet.pm b/lib/DBIx/Class/ResultSet.pm index df7b701..d02d6ff 100644 --- a/lib/DBIx/Class/ResultSet.pm +++ b/lib/DBIx/Class/ResultSet.pm @@ -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 diff --git a/lib/DBIx/Class/ResultSource/RowParser.pm b/lib/DBIx/Class/ResultSource/RowParser.pm index fa69299..704ebf8 100644 --- a/lib/DBIx/Class/ResultSource/RowParser.pm +++ b/lib/DBIx/Class/ResultSource/RowParser.pm @@ -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, + ); } diff --git a/t/resultset/rowparser_internals.t b/t/resultset/rowparser_internals.t index c53b3eb..b089ecc 100644 --- a/t/resultset/rowparser_internals.t +++ b/t/resultset/rowparser_internals.t @@ -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);