Scale back validation of the 'as' attribute (revert 95e41036)

It turned out that users employ invalid-but-sensical inflation maps in
production. Remove the early sanity check of the infmap and move the code
throwing an exception all the way back to ::Row

Follow through this thread for more details:
http://lists.scsys.co.uk/pipermail/dbix-class/2013-February/011115.html

diff --git a/Changes b/Changes
index 04dbd47..fb6c0e6 100644 (file)
--- a/Changes
+++ b/Changes
@@ -5,6 +5,10 @@ Revision history for DBIx::Class
           explicitly forbidden. The behavior was undefined before, and
           would result in wildly differing outcomes depending on $rs
           attributes.
+        - Scale back validation of the 'as' attribute - in the field
+          there are legitimate-ish uses of a inflating into an apparently
+          invalid relationship graph
+
 
 0.08209 2013-03-01 12:56 (UTC)
     * New Features / Changes

diff --git a/lib/DBIx/Class/ResultSet.pm b/lib/DBIx/Class/ResultSet.pm
index 6c94721..bfd6064 100644 (file)
--- a/lib/DBIx/Class/ResultSet.pm
+++ b/lib/DBIx/Class/ResultSet.pm
@@ -3338,40 +3338,6 @@ sub _chain_relationship {
   return {%$attrs, from => $from, seen_join => $seen};
 }
 
-# FIXME - this needs to go live in Schema with the tree walker... or
-# something
-my $inflatemap_checker;
-$inflatemap_checker = sub {
-  my ($rsrc, $relpaths) = @_;
-
-  my $rels;
-
-  for (@$relpaths) {
-    $_ =~ /^ ( [^\.]+ ) \. (.+) $/x
-      or next;
-
-    push @{$rels->{$1}}, $2;
-  }
-
-  for my $rel (keys %$rels) {
-    my $rel_rsrc = try {
-      $rsrc->related_source ($rel)
-    } catch {
-    $rsrc->throw_exception(sprintf(
-      "Inflation into non-existent relationship '%s' of '%s' requested, "
-    . "check the inflation specification (columns/as) ending in '...%s.%s'",
-      $rel,
-      $rsrc->source_name,
-      $rel,
-      ( sort { length($a) <=> length ($b) } @{$rels->{$rel}} )[0],
-    ))};
-
-    $inflatemap_checker->($rel_rsrc, $rels->{$rel});
-  }
-
-  return;
-};
-
 sub _resolved_attrs {
   my $self = shift;
   return $self->{_attrs} if $self->{_attrs};
@@ -3443,14 +3409,6 @@ sub _resolved_attrs {
     }
   }
 
-  # validate the user-supplied 'as' chain
-  # folks get too confused by the (logical) exception message, need to
-  # go to some lengths to clarify the text
-  #
-  # FIXME - this needs to go live in Schema with the tree walker... or
-  # something
-  $inflatemap_checker->($source, \@as);
-
   $attrs->{select} = \@sel;
   $attrs->{as} = \@as;
 

diff --git a/lib/DBIx/Class/Row.pm b/lib/DBIx/Class/Row.pm
index 39cd754..bdc7aee 100644 (file)
--- a/lib/DBIx/Class/Row.pm
+++ b/lib/DBIx/Class/Row.pm
@@ -1191,7 +1191,22 @@ sub inflate_result {
           and
         ref($prefetch->{$pre}) ne $DBIx::Class::ResultSource::RowParser::Util::null_branch_class
       ) {
-        my $pre_source = $source->related_source($pre);
+        my $pre_source = try {
+          $source->related_source($pre)
+        } catch {
+          my $err = sprintf
+            "Inflation into non-existent relationship '%s' of '%s' requested",
+            $pre,
+            $source->source_name,
+          ;
+          if (my ($colname) = sort { length($a) <=> length ($b) } keys %{$prefetch->{$pre}[0] || {}} ) {
+            $err .= sprintf ", check the inflation specification (columns/as) ending in '...%s.%s'",
+            $pre,
+            $colname,
+          }
+
+          $source->throw_exception($err);
+        };
 
         @pre_objects = map {
           $pre_source->result_class->inflate_result( $pre_source, @$_ )

diff --git a/t/resultset/inflatemap_abuse.t b/t/resultset/inflatemap_abuse.t
new file mode 100644 (file)
index 0000000..1be2f69
--- /dev/null
+++ b/t/resultset/inflatemap_abuse.t
@@ -0,0 +1,73 @@
+use strict;
+use warnings;
+
+use Test::More;
+use Test::Exception;
+use lib qw(t/lib);
+use DBICTest;
+
+# From http://lists.scsys.co.uk/pipermail/dbix-class/2013-February/011119.html
+#
+# > Right, at this point we have an "undefined situation turned into an
+# > unplanned feature", therefore 0.08242 will downgrade the exception to a
+# > single-warning-per-process. This seems like a sane middle ground for
+# > "you gave me an 'as' that worked by accident before - fix it at your
+# > convenience".
+#
+# When the things were reshuffled it became apparent implementing a warning
+# for the HRI case *only* is going to complicate the code a lot, without
+# adding much benefit at this point. So just make sure everything works the
+# way it used to and move on
+
+
+my $s = DBICTest->init_schema;
+
+my $rs_2nd_track = $s->resultset('Track')->search(
+  { 'me.position' => 2 },
+  {
+    join => { cd => 'artist' },
+    'columns' => [ 'me.title', { 'artist.cdtitle' => 'cd.title' }, 'artist.name' ],
+    order_by => 'artist.name',
+  }
+);
+
+is_deeply (
+  [ map { $_->[-1] } $rs_2nd_track->cursor->all ],
+  [ ('Caterwauler McCrae') x 3, 'Random Boy Band', 'We Are Goth' ],
+  'Artist name cartesian product correct off cursor',
+);
+
+is_deeply (
+  $rs_2nd_track->all_hri,
+  [
+    {
+      artist => { cdtitle => "Caterwaulin' Blues", name => "Caterwauler McCrae" },
+      title => "Howlin"
+    },
+    {
+      artist => { cdtitle => "Forkful of bees", name => "Caterwauler McCrae" },
+      title => "Stripy"
+    },
+    {
+      artist => { cdtitle => "Spoonful of bees", name => "Caterwauler McCrae" },
+      title => "Apiary"
+    },
+    {
+      artist => { cdtitle => "Generic Manufactured Singles", name => "Random Boy Band" },
+      title => "Boring Song"
+    },
+    {
+      artist => { cdtitle => "Come Be Depressed With Us", name => "We Are Goth" },
+      title => "Under The Weather"
+    }
+  ],
+  'HRI with invalid inflate map works'
+);
+
+throws_ok
+  { $rs_2nd_track->next }
+  qr!\QInflation into non-existent relationship 'artist' of 'Track' requested, check the inflation specification (columns/as) ending in '...artist.name'!,
+  'Correct exception on illegal ::Row inflation attempt'
+;
+
+done_testing;