[Dbix-class] ResultSet->new() ignores custom result_class [patch + tests]

[Emanuele Zeppieri]

Well, I mostly see your point, but I need some further clarifications, 
and I have some objections too (albeit slight ;-)

Matt S Trout wrote:

> On Wed, Feb 06, 2008 at 08:23:00PM +0100, Emanuele Zeppieri wrote:

>> as soon as we pass *any* \%attr to find, the above code stops working:
>>
>> my $hash_ref = $rs->find( 1, { key => 'primary' } ); # For example.
>> print ref $hash_ref; # Prints 'DBICTest::Artist'!
>>                      # (assuming we're using that.)
>> print hash_ref->{name}; # Uninitialized value!
> 
> That's a bug in find(). Actually, probably two bugs in find.
> 
> find uses search if it's got attributes. It shouldn't need to do that for
> just 'key'.

I don't understand exactly what you prefer here: do you want find() to 
*never* use search, or do you want find to not use search *only* when it 
gets the (only) 'key' attr?

Keep in mind that if you opt for the latter choice, we'll still have the 
HashRefInflator bug (for example when find gets the 'columns' attr - 
which is also covered in my tests).

> it also -should- pass $self->result_class to the search attrs, since
> $self->result_class doesn't persist across searches (only result_class
> supplied as an attr).
> 
>> my $rs = $schema->resultset('Artist');
>> $rs->result_class('DBIx::Class::ResultClass::HashRefInflator');
>> my $hash_ref = $rs->search({ artistid => 1 })->first;
>> # Again $hash_ref is not an hash ref!
> 
> That's because you passed it to the accessor.
> 
> If you want it to survive across the chain it needs to be passed an an
> attribute (turns out having the option to have either behaviour is really
> handy occasionally :).

Keep in mind that, with my patch, you can always reset the result_class 
in any moment ($rs->result_class(undef) - it's even covered in my tests) 
so it offers either behaviour anyway ;-)

> A doc patch making this clear would be much appreciated.

Sure, I'm willing to provide that, please see below...

>> or when we use search_related() or $rs->page() (please check the 
>> attached test script for even more failing cases).
> 
> search_related doesn't preserve the result_class and it isn't logical for
> it to do so. A search_related traverses the relationship on the source.
> 
> Though it might be possible for a result class to provide an __related_class
> API or something so that we can traverse the result class at the same time
> (and that way HashRefInflator could just return $self for that). I'd like
> to discuss that separately though, since it's fairly major work (so start
> another thread if you still want to pursue it :)

OK, let's put this in stand-by.

> page() doesn't work because it uses search(), which behaves as described
> above.

Indeed my patch solves also this, since it makes search() work in any 
case ;-)

> If you'd like to have a go at reworking your tests/patch along with doc
> tweaks as described, I'd love to have the find() behaviour fixed.

OK, I'm willing to do this (the way you prefer), but let's first review 
the two possible roadmaps (also to verify that I understood your points.)

You propose the following roadmap (according to my understanding):

1. $rs->result_class must not persist across searches: tests + doc 
patches for this, since currently the docs say just the opposite:
<<Specify $rs->result_class on a specific resultset to affect only that 
resultset (and any chained off of it);>>
http://search.cpan.org/~jrobinson/DBIx-Class-0.08009/lib/DBIx/Class/ResultClass/HashRefInflator.pm

2. result_class passed as an attr persists: tests + docs for this, since 
it's currently not specified anywhere in the docs (the user has to see 
the code).

3. Rewrite find() to make it not use search (but then it should *never* 
use search, not only for 'key', otherwise the bug still emerges).

4. Don't touch search_related() for now.

Did I understand correctly?

Assuming so, we however end up with two different result_class 
behaviours: one which persists (via attributes) and one which don't.
Do we really need this? (It could mess the API...)

Adopting my patch we instead would have:

1. $rs->result_class persists (as currently documented).
Also consider that we can reset it in any moment, as said (so it 
persists only if we really want it ;-)

2. result_class should never be passed as an attr (indeed it's not 
documented).

3. HashRefInflator-ed find() works perfectly in any condition (though I 
  agree that find() could be refactored in the future - and I'm willing 
to do it, regardless of the adoption of my patch).

4. search_related() can be put in stand-by for now (I accept to remove 
that fix from my patch, if you prefer so).

Could you please confirm that my understanding of your proposals is 
correct, that you prefer your roadmap over mine, and, in this case, 
clarify my doubts about the find() refactoring?

Cheers,
Emanuele.