[Dbix-class] patch for ResultSet::find_or_new

[Jason Kohles]

On Jan 14, 2008, at 2:19 PM, Zbigniew Lukasiak wrote:

> On Jan 13, 2008 7:20 PM, Matt S Trout <dbix-class at trout.me.uk> wrote:
>> On Sun, Jan 13, 2008 at 10:41:43AM +0100, Patrick Weemeeuw wrote:
>>> There is a small bug in find_or_new: when the find part
>>> fails, it calls new_result with the hash containing the
>>> values that were used for the search. It should use no
>>> values at all instead.
>>
>> This isn't a bug. If that's the behaviour you want, do
>>
>> my $o = $rs->find({ id => $id }) || $rs->new({});
>>
>>> Example buggy case: $o = $...->find_or_new( { id => $id } )
>>> with id a not null primary key. When $id is undefined, there
>>> is obviously no row in the DB, and a new result object is
>>> returned. However, the object returned contains the column
>>> id => NULL, which (1) is invalid for this kind of object,
>>> and (2) prevents in some backends (e.g. Pg) that the
>>> sequence is used to generate a unique id.
>>
>> So don't pass id if it isn't a valid value. Passing undef there is  
>> a bug
>> in your code, not in DBIx::Class.
>>
>> The usual use of find_or_new is to pass a unique key plus additional
>> attributes to be used for object creation (which are ignored in the  
>> find()
>> by specifying the key attr as well). Consider for example
>>
>> my $stats = $schema->resultset('PageViews')->find_or_new(
>>              { page => $page, views => 0 },
>>              { key => 'page' }
>>            );
>>
>
> How can we excercise the _or_new part of find_or_new?
>

The results you are seeing are *exactly* the way it is expected to  
work.  In this example you are doing:

$rs->find_or_new( { id => undef } )

and getting back an object where id is undefined.  How is that  
mysterious or incorrect behaviour?  You told it 'find me a row in the  
database where id is undef, and if you can't find one, create a new  
object where id is undef' and that's exactly what it did.

The big difference between ->find_or_new and ->find_or_create, is that  
->find_or_new doesn't attempt to do the insert yet, and so is not  
guaranteed to return to you an object that even *can* be inserted.   
What I would do in this case is either not use ->find_or_new when you  
don't have a valid primary key, or do something like this:

my $obj = $rs->find_or_new( $id ? { id => $id } : {} );

or even...

my $obj = $rs->find_or_new( { id => $id } );
if ( ! defined $obj->id ) { $obj->id( 'foo' ) }


>> From the above I conclude that we should omit 'page' in the request
> (instead of setting it to 'undef'):
>

> my $stats = $schema->resultset('PageViews')->find_or_new(
>               { views => 0 },
>               { key => 'page' }
>             );
> but then if we have another record with views == 0 it will be found
> and no new row would be created.
>
You've just created an even more bizarre situation, what you have now  
is essentially

my $rs = $schema->resultset( 'PageViews' );
my $stats = $rs->find( {} ) || $rs->new( { views => 0 } );


The documentation for find_or_create says "Tries to find a record  
based on its primary key or unique constraint; if none is found,  
creates one and returns that instead."  What you are trying to do is  
use it without providing a primary key or a unique constraint, and the  
behavior you are seeing is exactly what I would expect to happen in  
that situation, since ->find can't match anything without a primary  
key or a unique constraint, you get back a new object instead.

> This is because find falls back on finding by all columns in the query
> (ie by 'views' in this case)
> if the key is not represented in the query - this is not documented,
> but it is commented in the source:
>
> # @unique_queries = () if there is no 'key' in $input_query
>
> # Build the final query: Default to the disjunction of the unique  
> queries,
> # but allow the input query in case the ResultSet defines the query  
> or the
> # user is abusing find
> my $alias = exists $attrs->{alias} ? $attrs->{alias} : $self->{attrs} 
> {alias};
> my $query = @unique_queries
>    ? [ map { $self->_add_alias($_, $alias) } @unique_queries ]
>    : $self->_add_alias($input_query, $alias);
>
> I would say - we should get rid of that 'special feature'.
>

I would say it's working exactly as intended, you are abusing find and  
it's doing the best it can to accomodate you, by creating new objects  
when your ->find fails to find a matching row.  The only alternative I  
can see would be for it to throw an exception when you try and call  
find_or_create without the

-- 
Jason Kohles, RHCA RHCDS RHCE
email at jasonkohles.com - http://www.jasonkohles.com/
"A witty saying proves nothing."  -- Voltaire