[Dbix-class] Relations on non in_storage rows (my todays gotcha)

[Zbigniew Lukasiak]

On Sun, May 11, 2008 at 9:01 PM, luke saunders <luke.saunders at gmail.com> wrote:
> On Sun, May 11, 2008 at 5:48 PM, Zbigniew Lukasiak <zzbbyy at gmail.com> wrote:
>> On Sun, May 11, 2008 at 4:34 PM, Matt S Trout <dbix-class at trout.me.uk> wrote:
>>> On Sun, May 11, 2008 at 12:06:54PM +0200, Zbigniew Lukasiak wrote:
>>>> If you have a new object, not yet in the database, like:
>>>>
>>>> $new_cd = $schema->resultset("CD")->new( {} );
>>>>
>>>> Then if you call $new_cd->artist - this will return a first artist in
>>>> the 'artist' table.
>>>
>>> That's a bug, but the test we were given for it didn't work. Better test
>>> case welcome and we can fix it.
>>
>> There is a strange test in 66relationship.t:
>>
>> 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');
>>
>> So it tests that a new row has 3 related artists - I cannot understand
>> what that is supposed to mean.  I my opinion that count there should
>> be 0.
>>
>
> It was me that made that change after a discussion with Matt.
> Essentially the reasoning was that if the FK has not been set then an
> open search is okay as anything could, in theory, be related.
>
>> If we need another test then for example:
>>
>> my $new_cd = $schema->resultset("CD")->new( {} );
>> is( $new_cd->search_related( 'artist' )->first, undef, 'No artist
>> found for new cd' );
>>
>> Maybe I don't understand what is oing on here - but if my thinking is
>> right and the test 'open search on undef FK' is wrong than the fix
>> could be to change line 796 in lib/DBIx/Class/ResultSource.pm:
>>
>> -        $ret{$k} = $for->get_column($v) if $for->has_column_loaded($v);
>> +        $ret{$k} = $for->get_column($v);
>
> Yes, this was how it was before. When this was the case this code
>
>  my $rel_object = $object->search_related($belongs_to_rel, { $fk =>
> $value })->first;
>
> would result in $rel_object being undef even if there was a related
> row corresponding to $value - the SQL would look something like
>
>  ... WHERE (PK IS NULL) AND (PK = $value);
>
> This was causing problems with nested inserts as this test shows
>
>  my $new_artist = $schema->resultset("Artist")->create({ artistid =>
> 18, name => 'larry' });
>
>  eval {
>    $schema->resultset("CD")->create({
>              cdid => 28,
>               title => 'Boogie Wiggle',
>              year => '2007',
>              artist => { artistid => 18, name => 'larry' }
>             });
>  };
>  is($@, '', 'new cd created without clash on related artist');
>
> With the old way the artist would be inserted twice as the
> find_or_new_related for the second insert would not find the existing
> artist with id 18, and this would of course cause a PK clash.
>
> I have just added this test to t/96multi_create.t so that this bug
> doesn't arise again if we revise the behaviour.
>
> So that's the background to this problem. If the open search seems
> unintuitive to people then probably we should revise/revert the
> behaviour but we need to think about search_related and
> find_or_new_related.


How about setting the new object's columns before calling
find_or_new_related in Row->new ?

Fortunately resolve_condition works on hash_refs instead of objects so:

Index: lib/DBIx/Class/Row.pm
===================================================================
--- lib/DBIx/Class/Row.pm       (revision 4379)
+++ lib/DBIx/Class/Row.pm       (working copy)
@@ -107,6 +107,9 @@
           ## 'filter' should disappear and get merged in with 'single' above!
           my $rel_obj = delete $attrs->{$key};
           if(!Scalar::Util::blessed($rel_obj)) {
+            $new->set_columns(
+              $new->result_source->resolve_condition(
+                $info->{cond}, $rel_obj, $key));
             $rel_obj = $new->find_or_new_related($key, $rel_obj);
             $new->{_rel_in_storage} = 0 unless ($rel_obj->in_storage);
           }


(and of course for other relationship types - 'multi' and 'single' the same)

Now find_or_new_related finds the related row as advertised and your
new test in 96multi_create.t is satisfied without allowing every row
in Artist to be relate to the new CD record.  It seems to not break
any more tests.

By the way podcoverage.t and broken_single_accessor.t don't pass.

-- 
Zbigniew Lukasiak
http://brudnopis.blogspot.com/