From: Peter Rabbitson Date: Sat, 26 Mar 2016 22:14:09 +0000 (+0100) Subject: Fix incorrect data returned in a corner case of partial-select HRI X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=40471d469bc450ab29789724d94f4c3c825c158f;p=dbsrgits%2FDBIx-Class-Historic.git Fix incorrect data returned in a corner case of partial-select HRI While investigating better reporting on disagreeing metadata and actual results returned by the data source, I stumbled across an incorrect optimization applied to the HRI fast-path in aa1d8a87. The reason this has never been a problem in the wild is that the failure case is very convoluted: In order for the problem to present itself one needs to have a subchain of ... something-single ... with the "something" not fetching anything AND the entire chain being hit exactly once (no multiplication of the branch neither by a result or a parallel 1:M) AND the HRI fast-path code needs to be in effect. Then and only then everything from "something"onwards will present as "nonexisting left join" and a sole undef will be returned in place of the entire substructure. --- diff --git a/Changes b/Changes index ba1c4aa..2714c0f 100644 --- a/Changes +++ b/Changes @@ -34,6 +34,9 @@ Revision history for DBIx::Class similar produces a large warning - Make sure exception objects stringifying to '' are properly handled and warned about (GH#15) + - Fix incorrect data returned in a corner case of partial-select HRI + invocation (no known manifestations of this bug in the field, see + commit message for description of exact failure scenario) - Fix corner case of stringify-only overloaded objects being used in create()/populate() - Fix spurious ROLLBACK statements when a TxnScopeGuard fails a commit diff --git a/lib/DBIx/Class/ResultSource/RowParser/Util.pm b/lib/DBIx/Class/ResultSource/RowParser/Util.pm index a20d07c..d82f7b4 100644 --- a/lib/DBIx/Class/ResultSource/RowParser/Util.pm +++ b/lib/DBIx/Class/ResultSource/RowParser/Util.pm @@ -285,19 +285,19 @@ sub __visit_infmap_collapse { ); if ($args->{collapse_map}->{-is_single}) { - push @src, sprintf ( '( %s %s %s%s ),', + push @src, sprintf ( '( %s %s %s = %s ),', $parent_attach_slot, (HAS_DOR ? '//=' : '||='), $node_idx_slot, - $me_struct ? " = $me_struct" : '', + $me_struct || '{}', ); } else { - push @src, sprintf('( (! %s) and push @{%s}, %s%s ),', + push @src, sprintf('( (! %s) and push @{%s}, %s = %s ),', $node_idx_slot, $parent_attach_slot, $node_idx_slot, - $me_struct ? " = $me_struct" : '', + $me_struct || '{}', ); } } diff --git a/t/inflate/hri_torture.t b/t/inflate/hri_torture.t index c0b763e..610a47b 100644 --- a/t/inflate/hri_torture.t +++ b/t/inflate/hri_torture.t @@ -40,6 +40,7 @@ $schema->resultset('CD')->create({ title => 'Oxygene', year => 1976, artist => { name => 'JMJ' }, + artwork => {}, tracks => [ { title => 'o2', position => 2}, # the position should not be needed here, bug in MC ], @@ -330,6 +331,43 @@ cmp_deeply 'collapsing 1:1:1:M:M chain ' . $rs->result_class, ; +cmp_deeply + [ $rs->search_rs ( + { + 'tracks.title' => 'e2', + 'cds.title' => 'Oxygene', + }, + { + collapse => 1, + join => [ + 'tracks', + { single_track => { cd => 'mandatory_artwork' } }, + { artist => { cds => 'mandatory_artwork'} }, + ], + columns => { + cdid => 'cdid', + 'single_track.cd.mandatory_artwork.cd_id' => 'mandatory_artwork.cd_id', + 'artist.cds.mandatory_artwork.cd_id' => 'mandatory_artwork_2.cd_id', + }, + }, + )->all ], + [ + { + cdid => 3, + single_track => { + cd => { + mandatory_artwork => { cd_id => 2 }, + }, + }, + artist => { + cds => [ + { mandatory_artwork => { cd_id => 2 } } + ] + }, + }, + ], +; + } done_testing; diff --git a/t/resultset/rowparser_internals.t b/t/resultset/rowparser_internals.t index 3f6c38a..0ad3107 100644 --- a/t/resultset/rowparser_internals.t +++ b/t/resultset/rowparser_internals.t @@ -343,10 +343,10 @@ is_same_src ( # prefetch data of single_track (placed in root) ( (! defined($cur_row_data->[1]) ) ? $collapse_idx[0]{$cur_row_ids{4}}{$cur_row_ids{5}}{single_track} = undef : do { - ( $collapse_idx[0]{$cur_row_ids{4}}{$cur_row_ids{5}}{single_track} //= $collapse_idx[1]{$cur_row_ids{1}}{$cur_row_ids{4}}{$cur_row_ids{5}} ), + ( $collapse_idx[0]{$cur_row_ids{4}}{$cur_row_ids{5}}{single_track} //= $collapse_idx[1]{$cur_row_ids{1}}{$cur_row_ids{4}}{$cur_row_ids{5}} = {} ), # prefetch data of cd (placed in single_track) - ( $collapse_idx[1]{$cur_row_ids{1}}{$cur_row_ids{4}}{$cur_row_ids{5}}{cd} //= $collapse_idx[2]{$cur_row_ids{1}}{$cur_row_ids{4}}{$cur_row_ids{5}} ), + ( $collapse_idx[1]{$cur_row_ids{1}}{$cur_row_ids{4}}{$cur_row_ids{5}}{cd} //= $collapse_idx[2]{$cur_row_ids{1}}{$cur_row_ids{4}}{$cur_row_ids{5}} = {} ), # prefetch data of artist ( placed in single_track->cd) ( $collapse_idx[2]{$cur_row_ids{1}}{$cur_row_ids{4}}{$cur_row_ids{5}}{artist} //= $collapse_idx[3]{$cur_row_ids{1}}{$cur_row_ids{4}}{$cur_row_ids{5}} = { artistid => $cur_row_data->[1] } ), @@ -787,7 +787,7 @@ is_same_src ( ( $collapse_idx[0]{$cur_row_ids{10}}{single_track} //= ($collapse_idx[1]{$cur_row_ids{0}} = { trackid => $$cur_row_data[0] }) ), - ( $collapse_idx[1]{$cur_row_ids{0}}{cd} //= $collapse_idx[2]{$cur_row_ids{0}} ), + ( $collapse_idx[1]{$cur_row_ids{0}}{cd} //= $collapse_idx[2]{$cur_row_ids{0}} = {} ), ( $collapse_idx[2]{$cur_row_ids{0}}{artist} //= ($collapse_idx[3]{$cur_row_ids{0}} = { artistid => $$cur_row_data[6] }) ),