From: Peter Rabbitson <ribasushi@cpan.org>
Date: Wed, 17 Apr 2013 13:21:08 +0000 (+0200)
Subject: Make 'filter' rels work half-way sanely with partial prefetch
X-Git-Tag: v0.08250~31^2~5
X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=6dd43920c45d7ae898f1bb902a086a9f99741976;p=dbsrgits%2FDBIx-Class.git

Make 'filter' rels work half-way sanely with partial prefetch
---

diff --git a/Changes b/Changes
index cef5fe3..7edc5f8 100644
--- a/Changes
+++ b/Changes
@@ -1,5 +1,12 @@
 Revision history for DBIx::Class
 
+    * New Features / Changes
+        - Deprecate returning of prefetched 'filter' rels as part of
+          get_columns() and get_inflated_columns() data
+        - Invoking get_inflated_columns() no longer fires get_columns() but
+          instead retrieves data from individual non-inflatable columns via
+          get_column()
+
     * Fixes
         - Fix _dbi_attrs_for_bind() being called befor DBI has been loaded
           (regression in 0.08210)
diff --git a/lib/DBIx/Class/Relationship/Accessor.pm b/lib/DBIx/Class/Relationship/Accessor.pm
index 1609122..fb95c35 100644
--- a/lib/DBIx/Class/Relationship/Accessor.pm
+++ b/lib/DBIx/Class/Relationship/Accessor.pm
@@ -3,7 +3,9 @@ package # hide from PAUSE
 
 use strict;
 use warnings;
-use Sub::Name ();
+use Sub::Name;
+use DBIx::Class::Carp;
+use namespace::clean;
 
 our %_pod_inherit_config =
   (
@@ -56,8 +58,24 @@ sub add_relationship_accessor {
         deflate => sub {
           my ($val, $self) = @_;
           $self->throw_exception("'$val' isn't a $f_class") unless $val->isa($f_class);
-          return ($val->_ident_values)[0];
-            # WARNING: probably breaks for multi-pri sometimes. FIXME
+
+          # MASSIVE FIXME - this code assumes we pointed at the PK, but the belongs_to
+          # helper does not check any of this
+          # fixup the code a bit to make things saner, but ideally 'filter' needs to
+          # be deprecated ASAP and removed shortly after
+          # Not doing so before 0.08250 however, too many things in motion already
+          my ($pk_col, @rest) = $val->_pri_cols;
+          $self->throw_exception(
+            "Relationship '$rel' of type 'filter' can not work with a multicolumn primary key on source '$f_class'"
+          ) if @rest;
+
+          my $v = $val->$pk_col;
+          carp_unique (
+            "Unable to deflate 'filter'-type relationship '$rel' (related object "
+          . "primary key not retrieved), assuming undef instead"
+          ) if ( ! defined $v and $val->in_storage );
+
+          return $v;
         }
       }
     );
@@ -73,7 +91,7 @@ sub add_relationship_accessor {
     no warnings 'redefine';
     foreach my $meth (keys %meth) {
       my $name = join '::', $class, $meth;
-      *$name = Sub::Name::subname($name, $meth{$meth});
+      *$name = subname($name, $meth{$meth});
     }
   }
 }
diff --git a/lib/DBIx/Class/Relationship/BelongsTo.pm b/lib/DBIx/Class/Relationship/BelongsTo.pm
index e55d1bd..df95541 100644
--- a/lib/DBIx/Class/Relationship/BelongsTo.pm
+++ b/lib/DBIx/Class/Relationship/BelongsTo.pm
@@ -73,6 +73,8 @@ sub belongs_to {
       and
     keys %$cond == 1
       and
+    (keys %$cond)[0] =~ /^foreign\./
+      and
     $class->has_column($rel)
   ) ? 'filter' : 'single';
 
diff --git a/lib/DBIx/Class/ResultSource.pm b/lib/DBIx/Class/ResultSource.pm
index 151d2c8..f5d2112 100644
--- a/lib/DBIx/Class/ResultSource.pm
+++ b/lib/DBIx/Class/ResultSource.pm
@@ -1669,7 +1669,7 @@ our $UNRESOLVABLE_CONDITION = \ '1 = 0';
 sub _resolve_condition {
   my ($self, $cond, $as, $for, $rel_name) = @_;
 
-  my $obj_rel = !!blessed $for;
+  my $obj_rel = defined blessed $for;
 
   if (ref $cond eq 'CODE') {
     my $relalias = $obj_rel ? 'me' : $as;
diff --git a/lib/DBIx/Class/Row.pm b/lib/DBIx/Class/Row.pm
index b36fe0e..bdc3b24 100644
--- a/lib/DBIx/Class/Row.pm
+++ b/lib/DBIx/Class/Row.pm
@@ -8,6 +8,7 @@ use base qw/DBIx::Class/;
 use Scalar::Util 'blessed';
 use List::Util 'first';
 use Try::Tiny;
+use DBIx::Class::Carp;
 
 ###
 ### Internal method
@@ -718,8 +719,22 @@ sub get_columns {
   my $self = shift;
   if (exists $self->{_inflated_column}) {
     foreach my $col (keys %{$self->{_inflated_column}}) {
-      $self->store_column($col, $self->_deflated_column($col, $self->{_inflated_column}{$col}))
-        unless exists $self->{_column_data}{$col};
+      unless (exists $self->{_column_data}{$col}) {
+
+        # if cached related_resultset is present assume this was a prefetch
+        carp_unique(
+          "Returning primary keys of prefetched 'filter' rels as part of get_columns() is deprecated and will "
+        . 'eventually be removed entirely (set DBIC_COLUMNS_INCLUDE_FILTER_RELS to disable this warning)'
+        ) if (
+          ! $ENV{DBIC_COLUMNS_INCLUDE_FILTER_RELS}
+            and
+          defined $self->{related_resultsets}{$col}
+            and
+          defined $self->{related_resultsets}{$col}->get_cache
+        );
+
+        $self->store_column($col, $self->_deflated_column($col, $self->{_inflated_column}{$col}));
+      }
     }
   }
   return %{$self->{_column_data}};
@@ -819,19 +834,43 @@ sub get_inflated_columns {
     grep { $self->has_column_loaded($_) } $self->columns
   ]);
 
-  my %inflated;
-  for my $col (keys %$loaded_colinfo) {
-    if (exists $loaded_colinfo->{$col}{accessor}) {
-      my $acc = $loaded_colinfo->{$col}{accessor};
-      $inflated{$col} = $self->$acc if defined $acc;
-    }
-    else {
-      $inflated{$col} = $self->$col;
+  my %cols_to_return = ( %{$self->{_column_data}}, %$loaded_colinfo );
+
+  unless ($ENV{DBIC_COLUMNS_INCLUDE_FILTER_RELS}) {
+    for (keys %$loaded_colinfo) {
+      # if cached related_resultset is present assume this was a prefetch
+      if (
+        $loaded_colinfo->{$_}{_inflate_info}
+          and
+        defined $self->{related_resultsets}{$_}
+          and
+        defined $self->{related_resultsets}{$_}->get_cache
+      ) {
+        carp_unique(
+          "Returning prefetched 'filter' rels as part of get_inflated_columns() is deprecated and will "
+        . 'eventually be removed entirely (set DBIC_COLUMNS_INCLUDE_FILTER_RELS to disable this warning)'
+        );
+        last;
+      }
     }
   }
 
-  # return all loaded columns with the inflations overlayed on top
-  return %{ { $self->get_columns, %inflated } };
+  map { $_ => (
+  (
+    ! exists $loaded_colinfo->{$_}
+      or
+    (
+      exists $loaded_colinfo->{$_}{accessor}
+        and
+      ! defined $loaded_colinfo->{$_}{accessor}
+    )
+  ) ? $self->get_column($_)
+    : $self->${ \(
+      defined $loaded_colinfo->{$_}{accessor}
+        ? $loaded_colinfo->{$_}{accessor}
+        : $_
+      )}
+  )} keys %cols_to_return;
 }
 
 sub _is_column_numeric {
diff --git a/t/prefetch/manual.t b/t/prefetch/manual.t
index 8135342..1ad2253 100644
--- a/t/prefetch/manual.t
+++ b/t/prefetch/manual.t
@@ -8,6 +8,8 @@ use Test::Exception;
 use lib qw(t/lib);
 use DBICTest;
 
+delete $ENV{DBIC_COLUMNS_INCLUDE_FILTER_RELS};
+
 my $schema = DBICTest->init_schema(no_populate => 1);
 
 $schema->resultset('Artist')->create({ name => 'JMJ', cds => [{
@@ -117,18 +119,106 @@ cmp_deeply (
   'W00T, manual prefetch with collapse works'
 );
 
-TODO: {
-  my ($row) = $rs->all;
-  local $TODO = 'Something is wrong with filter type rels, they throw on incomplete objects >.<';
-
-  lives_ok {
-    cmp_deeply (
-      { $row->single_track->get_columns },
-      {},
-      'empty intermediate object ok',
-    )
-  } 'no exception';
-}
+lives_ok { my $dummy = $rs;  warnings_exist {
+
+##############
+### This is a bunch of workarounds for deprecated behavior - delete entire block when fixed
+  my $cd_obj = ($rs->all)[0]->single_track->cd;
+  my $art_obj = $cd_obj->artist;
+
+  my $empty_single_columns = {
+    cd => undef
+  };
+  my $empty_single_inflated_columns = {
+    cd => $cd_obj
+  };
+  my $empty_cd_columns = {
+    artist => $art_obj->artistid
+  };
+  my $empty_cd_inflated_columns = {
+    artist => $art_obj
+  };
+
+  {
+    local $TODO = "Returning prefetched 'filter' rels as part of get_columns/get_inflated_columns is deprecated";
+    is_deeply($_, {}) for (
+      $empty_single_columns, $empty_single_inflated_columns, $empty_cd_columns, $empty_cd_inflated_columns
+    );
+  }
+##############
+
+
+### this tests the standard root -> single -> filter ->filter
+  my ($row) = $rs->all; # don't trigger order warnings
+
+  is_deeply(
+    { $row->single_track->get_columns },
+    $empty_single_columns,
+    "No unexpected columns available on intermediate 'single' rel with a chained 'filter' prefetch",
+  );
+
+  is_deeply(
+    { $row->single_track->get_inflated_columns },
+    $empty_single_inflated_columns,
+    "No unexpected inflated columns available on intermediate 'single' rel with a chained 'filter' prefetch",
+  );
+
+  is_deeply(
+    { $row->single_track->cd->get_columns },
+    $empty_cd_columns,
+    "No unexpected columns available on intermediate 'single' rel with 2x chained 'filter' prefetch",
+  );
+
+  is_deeply(
+    { $row->single_track->cd->get_inflated_columns },
+    $empty_cd_inflated_columns,
+    "No unexpected inflated columns available on intermediate 'single' rel with 2x chained 'filter' prefetch",
+  );
+
+### also try a different arangement root -> single -> single ->filter
+  ($row) = $rs->result_source->resultset->search({ 'artist.artistid' => 1 }, {
+    join => { single_track => { disc => { artist => 'cds' } } },
+    '+columns' => {
+      'single_track.disc.artist.artistid' => 'artist.artistid',
+      'single_track.disc.artist.cds.cdid' => 'cds.cdid',
+    },
+    collapse => 1,
+  })->all;
+
+  is_deeply(
+    { $row->single_track->get_columns },
+    {},
+    "No unexpected columns available on intermediate 'single' rel with a chained 'single' prefetch",
+  );
+
+  is_deeply(
+    { $row->single_track->get_inflated_columns },
+    {},
+    "No unexpected inflated columns available on intermediate 'single' rel with a chained 'single' prefetch",
+  );
+
+  is_deeply(
+    { $row->single_track->disc->get_columns },
+    $empty_cd_columns,
+    "No unexpected columns available on intermediate 'single' rel with chained 'single' and chained 'filter' prefetch",
+  );
+
+  is_deeply(
+    { $row->single_track->disc->get_inflated_columns },
+    $empty_cd_inflated_columns,
+    "No unexpected inflated columns available on intermediate 'single' rel with chained 'single' and chained 'filter' prefetch",
+  );
+
+} [
+  qr/\QReturning primary keys of prefetched 'filter' rels as part of get_columns()/,
+  qr/\QUnable to deflate 'filter'-type relationship 'cd' (related object primary key not retrieved)/,
+  qr/\QReturning prefetched 'filter' rels as part of get_inflated_columns()/,
+  qr/\QReturning primary keys of prefetched 'filter' rels as part of get_columns()/,
+  qr/\QReturning prefetched 'filter' rels as part of get_inflated_columns()/,
+  qr/\QReturning primary keys of prefetched 'filter' rels as part of get_columns()/,
+  qr/\QReturning prefetched 'filter' rels as part of get_inflated_columns()/,
+], 'expected_warnings'
+} 'traversing prefetch chain with empty intermediates works';
 
 TODO: {
 local $TODO = 'this does not work at all, need to promote rsattrs to an object on its own';