From: Peter Rabbitson Date: Sun, 24 Feb 2013 17:45:23 +0000 (+0100) Subject: Scale back validation of the 'as' attribute (revert 95e41036) X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=25a942fa2ac003838d9c0f19ea3f6aed54ac8378;p=dbsrgits%2FDBIx-Class-Historic.git 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 --- 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 --- 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 --- 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 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;