From: Peter Rabbitson <ribasushi@cpan.org>
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?p=dbsrgits%2FDBIx-Class.git;a=commitdiff_plain;h=40471d469bc450ab29789724d94f4c3c825c158f;hp=8fc4291ef4f19b6f4c4f25cd695cb613da613fe1

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] }) ),