[Dbix-class] Unexpected behavior with possible NULL foreign key relationship

[Matt S Trout]

On Thu, Aug 07, 2008 at 02:08:54PM -0500, mike wrote:
> i'm bringing this issue back up, as i've had time to look at it again.
>  first, some context:
> 
> On Fri, Mar 21, 2008 at 03:14:40PM -0700, Damon Snyder wrote:
> > Hi Everyone,
> > I'm new to the list so bear with me if I'm missing something. I have a
> > foreign key relationship where it's possible that the belongs_to
> > relationship is NULL. See the classes below. When I create a new
> > Smsmessage object e.g. like so:
> 
> [snip]
> 
> >> >  On Wed, Apr 2, 2008 at 3:52 PM, mike <pulsation at gmail.com> wrote:
> >> however, this current problem is not mysql.  i have confirmed that it
> >> only occurs
> >> on newly created objects by adding a call to discard_changes after the creation.
> >> following is the modified patch including the test that illustrates
> >> this behavior.
> >
> > (1) your mail client mangled the patch
> > (2) after unmangling it, it makes 60core and 91debug fail.
> >
> > Can I have another patch that doesn't break anything else please? :)
> 
> attached you will find my modified patch, produced against 0.08 trunk
> (r4736).  if only by virtue of being an attachment, it should not have
> been mangled by gmail or this unholy fvwm->rdesktop->win32->firefox
> setup i use.
> 
> i've been perusing around the innards, but i'm not sure where the issue
> is.  the field is not specified to create(), so Row's constructor is not
> calling store_column for it.  if the field is specified, the accessor
> returns undef, as expected.  however, if the field is not specified, the
> accessor returns an object that describes a row from that table.  if the
> table is empty, the accessor returns undef.
> 
> if the table contains multiple rows, we see the warning:
> 
> 	DBICTest::Schema::CD::genre(): Query returned more than one row.
> 	SQL that returns multiple rows is DEPRECATED for ->find and ->single
> 
> it appears that the search_related that is called performs a simple
> SELECT * FROM table.  on lib/DBIx/Class/Relationship/Accessor.pm:21-33:

I think the correct approach is to say: "if the column isn't loaded, die().
If the object hasn't been inserted yet, return a WHERE 0=1 resultset for
the relation."

Which I've done, and seems to not be overly fucked up. It's not yet
committed though because now multi_create is completely screwed, since it
relied on the bug. I need to figure out how to get DBIC to correctly
infer whether it needs to do a $self->${foo}_related call versus
$self->related_source($rel)->resultset->$foo and then fixup the info
post-insert. It's probably not amazingly hard but the multi_create code
has always been a mess of heuristics and I'm trying not to make it worse
in the process.

Watch this space. I've attached my diff so far for anybody who's interested
in following along.

-- 
      Matt S Trout       Need help with your Catalyst or DBIx::Class project?
   Technical Director                    http://www.shadowcat.co.uk/catalyst/
 Shadowcat Systems Ltd.  Want a managed development or deployment platform?
http://chainsawblues.vox.com/            http://www.shadowcat.co.uk/servers/
-------------- next part --------------
=== lib/DBIx/Class/Relationship/Base.pm
==================================================================
--- lib/DBIx/Class/Relationship/Base.pm	(revision 23206)
+++ lib/DBIx/Class/Relationship/Base.pm	(local)
@@ -190,11 +190,18 @@
       $rel_obj->{cond}, $rel, $self
     );
     if (ref $cond eq 'ARRAY') {
-      $cond = [ map { my $hash;
-        foreach my $key (keys %$_) {
-          my $newkey = $key =~ /\./ ? "me.$key" : $key;
-          $hash->{$newkey} = $_->{$key};
-        }; $hash } @$cond ];
+      $cond = [ map {
+        if (ref $_ eq 'HASH') {
+          my $hash;
+          foreach my $key (keys %$_) {
+            my $newkey = $key =~ /\./ ? "me.$key" : $key;
+            $hash->{$newkey} = $_->{$key};
+          }
+          $hash;
+        } else {
+          $_;
+        }
+      } @$cond ];
     } else {
       foreach my $key (grep { ! /\./ } keys %$cond) {
         $cond->{"me.$key"} = delete $cond->{$key};
=== lib/DBIx/Class/ResultSource.pm
==================================================================
--- lib/DBIx/Class/ResultSource.pm	(revision 23206)
+++ lib/DBIx/Class/ResultSource.pm	(local)
@@ -796,7 +796,14 @@
         $self->throw_exception("Invalid rel cond val ${v}");
       if (ref $for) { # Object
         #warn "$self $k $for $v";
-        $ret{$k} = $for->get_column($v) if $for->has_column_loaded($v);
+        unless ($for->has_column_loaded($v)) {
+          if ($for->in_storage) {
+            $self->throw_exception("Column ${v} not loaded on ${for} trying to reolve relationship");
+          }
+          return [ \'1 = 0' ];
+        }
+        $ret{$k} = $for->get_column($v);
+        #$ret{$k} = $for->get_column($v) if $for->has_column_loaded($v);
         #warn %ret;
       } elsif (!defined $for) { # undef, i.e. "no object"
         $ret{$k} = undef;
=== t/66relationship.t
==================================================================
--- t/66relationship.t	(revision 23206)
+++ t/66relationship.t	(local)
@@ -212,7 +212,7 @@
 
 my $undef_artist_cd = $schema->resultset("CD")->new_result({ 'title' => 'badgers', 'year' => 2007 });
 is($undef_artist_cd->has_column_loaded('artist'), '', 'FK not loaded');
-is($undef_artist_cd->search_related('artist')->count, 3, 'open search on undef FK');
+is($undef_artist_cd->search_related('artist')->count, 0, '0=1 search when FK does not exist and object not yet in db');
 
 my $def_artist_cd = $schema->resultset("CD")->new_result({ 'title' => 'badgers', 'year' => 2007, artist => undef });
 is($def_artist_cd->has_column_loaded('artist'), 1, 'FK loaded');