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

[Emanuele Zeppieri]

Could you please verify this bug report ( and review the attached =

patch(es) )?

The message could seem lengthy (well, it really is), but it will =

hopefully help the reader in quickly visualize the problem and its =

implications, and in promptly identifying the relevant code portions.

1. The problem
~~~~~~~~~~~~~~

To illustrate this bug, let's first see its effects rather than its =

cause: currently its main effect is that it prevents HashRefInflator =

from working (with the exception of the /simplest/ cases).

That is, though something like this works as advertised:

my $rs =3D $schema->resultset('Artist');
$rs->result_class('DBIx::Class::ResultClass::HashRefInflator');
my $hash_ref =3D $rs->find(1);

as soon as we pass *any* \%attr to find, the above code stops working:

my $hash_ref =3D $rs->find( 1, { key =3D> 'primary' } ); # For example.
print ref $hash_ref; # Prints 'DBICTest::Artist'!
                      # (assuming we're using that.)
print hash_ref->{name}; # Uninitialized value!

So despite having specified DBIx::Class::ResultClass::HashRefInflator as =

our result_class, we just get the usual DBIC row object and not the =

expected hash ref.

The same happens in a number of other cases, such as when we chain =

another resultset off of an HashRefInflator-ed resultset, like this:

my $rs =3D $schema->resultset('Artist');
$rs->result_class('DBIx::Class::ResultClass::HashRefInflator');
my $hash_ref =3D $rs->search({ artistid =3D> 1 })->first;
# Again $hash_ref is not an hash ref!

or when we use search_related() or $rs->page() (please check the =

attached test script for even more failing cases).

(To be clear: it's not HashRefInflator fault, which works perfectly of =

its own ;-)

2. The cause
~~~~~~~~~~~~

The cause is that when we say something like:

$rs->result_class('DBIx::Class::ResultClass::HashRefInflator');

the custom result_class is written *only* into the $rs->{result_class} =

slot (by the result_class() accessor/mutator generated by =

Class::Accessor::Grouped), whereas when ResultSet->new() is (secretly) =

invoked to build the resulting resultset (yes, new() is called even on a =

simple find() with some \%attrs), the custom result_class is searched =

*only* in the $rs->{attrs}{result_class} slot (please see ResultSet.pm =

line 101), which however is never assigned by the result_class() mutator.

Consider also that ResultSet->new() is invoked this way ( from a =

ResultSet instance - for example by search_rs() ):

my $rs =3D (ref $self)->new($self->result_source, $new_attrs);

so once in new() we've lost $self and the opportunity to read its =

result_class slot, which is why it should have somehow been copied into =

$new_attrs->{result_class} before calling new().

Basically, any time ResultSet->new() is invoked that way (which happens =

in several places inside ResultSet.pm), any custom result_class is lost =

and its value is reset by new() (usually to to the /table class/).

3. The proposed remedy
~~~~~~~~~~~~~~~~~~~~~~

Assigning $new_attrs->{result_class} before calling new() is probably =

the most straightforward workaround, but it's too fragile, sparse and =

inefficient too, since it wastes an assignment for every new() call.

So I opted for a tighter solution: a custom result_class() method which =

assigns both $self->{result_class} and $self->{attrs}{result_class} when =

used as a mutator, so that the two slots are always in sync ( =

$self->{attrs} is copied into $new_attrs before calling new() ).

I see it as sufficiently clean, centralized and efficient, and I didn't =

  find any side effect.

Your opinions?

4. Is that all...?
~~~~~~~~~~~~~~~~~~

... Unfortunately not! The poor HashRefInflator still has to face =

another (unrelated) menace along its perillous path.

If search_related() is used, HashRefInflator gets obliterated again by =

the following temp fix (ResultSet.pm line 1834):

#XXX - temp fix for result_class bug. There likely is a more elegant fix =

-groditi
my %attrs =3D %{$self->{attrs}||{}};
delete @attrs{qw(result_class alias)};

However, according to my understanding, the above fix proved to be a =

little bit too ecumenical, in that it's (fortunately) not necessary when =

the result_class is HashRefInflator, so my fix here is to simply skip =

the $attrs{result_class} deletion (only) in this particular case.

Could you please verify that also?

5. The tests
~~~~~~~~~~~~

Oddly enough, the existing HashRefInflator tests, though quite =

extensive, don't cover anything of find() with \%attrs, chained =

resultsets etc.
An additional test script is then provided =

(68inflate_resultclass_hashrefinflator_deep.t) which incorporates the =

above described cases and also stresses the new result_class accessor.

(Of course the proposed patches work flawlessly in all these new cases =

as well as with the rest of the DBIC test suite.)

6. That's all
~~~~~~~~~~~~~

For real (for now ;-)

Thank you for your patience.

Cheers,
Emanuele Zeppieri.
-------------- next part --------------
use strict;
use warnings;  =


use Test::More qw(no_plan);
use lib qw(t/lib);
use DBICTest;
use DBIx::Class::ResultClass::HashRefInflator;
my $schema =3D DBICTest->init_schema();

my $artist_rs =3D $schema->resultset('Artist');
my @artist_columns =3D sort $artist_rs->result_source->columns;

my $crappycrap_id =3D $artist_rs->create(
    { name =3D> 'Mr. Crappy Crap', cds =3D> [ =

        { title =3D> 'My First Shit'         , year =3D> 2006 },
        { title =3D> 'Yet More Shit'         , year =3D> 2007 },
        { title =3D> 'The Biggest Shit Evah!', year =3D> 2008 }
    ]}
)->artistid;

$artist_rs->result_class('DBIx::Class::ResultClass::HashRefInflator');

# A find with the 'key' attr.
my $hash_ref =3D $artist_rs->find( $crappycrap_id, { key =3D> 'primary' } );

is( ref($hash_ref), 'HASH', 'returned data is a hash ref' );
is( $hash_ref->{name}, 'Mr. Crappy Crap', 'access a field value through the=
 hash' );
is_deeply( \@artist_columns, [ sort keys %$hash_ref ], 'returned columns' );

my @some_artist_columns =3D qw(name);

# A find with more attrs.
$hash_ref =3D $artist_rs->find( $crappycrap_id, {
    key     =3D> 'primary',
    columns =3D> \@some_artist_columns
});

is( ref($hash_ref), 'HASH', 'returned data from find is a hash ref' );
is( $hash_ref->{name}, 'Mr. Crappy Crap', 'access a field value through the=
 hash' );
is_deeply( \@some_artist_columns, [ keys %$hash_ref ], 'selected returned c=
olumns' );

# Let's reset the result_class.
$artist_rs->result_class(undef);

# Again a find with the 'key' attr.
my $row_obj =3D $artist_rs->find( $crappycrap_id, { key =3D> 'primary' } );

like( ref($row_obj), qr/Artist$/, 'returned data from find is a row object'=
 );
is( $row_obj->name, 'Mr. Crappy Crap', 'access a field value through the fi=
eld accessor' );

# Let's switch back to HashRefInflator.
$artist_rs->result_class('DBIx::Class::ResultClass::HashRefInflator');

# And try a chained off resultset.
$hash_ref =3D $artist_rs->search({ name =3D> 'Mr. Crappy Crap' })->first;

is( ref($hash_ref), 'HASH', 'returned data from chained resultset is a hash=
 ref' );
is( $hash_ref->{name}, 'Mr. Crappy Crap', 'access a field value through the=
 hash' );

# Try search_related.
my $cd_rs =3D $schema->resultset('CD');
$cd_rs->result_class('DBIx::Class::ResultClass::HashRefInflator');
$hash_ref =3D $cd_rs->search_related('artist', {
    name =3D> 'Mr. Crappy Crap',
})->first;

is( ref($hash_ref), 'HASH', 'returned data from search_related is a hash re=
f' );
is( $hash_ref->{name}, 'Mr. Crappy Crap', 'access a field value through the=
 hash' );

# Get all.
my @array_of_hash_refs =3D $cd_rs->search(
    { artist =3D> $crappycrap_id },
    { order_by =3D> 'year ASC'}
);

is( ref( $array_of_hash_refs[0] ), 'HASH', 'elements of the returned array =
are hash refs' );
is( $array_of_hash_refs[0]->{title}, 'My First Shit', 'access a field value=
 through the array of hash refs' );

# Test page().
@array_of_hash_refs =3D $cd_rs->search(
    { artist =3D> $crappycrap_id },
    { rows =3D> 3, order_by =3D> 'year ASC' }
)->page(1)->all;

is( ref( $array_of_hash_refs[0] ), 'HASH', 'page(): elements of the returne=
d array are hash refs' );
is( $array_of_hash_refs[2]->{title}, 'The Biggest Shit Evah!', 'page(): acc=
ess a field value through the array of hash refs' );
-------------- next part --------------
Index: lib/DBIx/Class/ResultSet.pm
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- lib/DBIx/Class/ResultSet.pm	(revision 4038)
+++ lib/DBIx/Class/ResultSet.pm	(working copy)
@@ -14,7 +14,7 @@
 use List::Util ();
 use base qw/DBIx::Class/;
 =

-__PACKAGE__->mk_group_accessors('simple' =3D> qw/result_class _source_hand=
le/);
+__PACKAGE__->mk_group_accessors('simple' =3D> qw/_source_handle/);
 =

 =3Dhead1 NAME
 =

@@ -923,6 +923,13 @@
 =

 =3Dcut
 =

+sub result_class {
+    my $self =3D shift;
+    return
+        @_
+        ? $self->{attrs}{result_class} =3D $self->{result_class} =3D $_[0]
+        : $self->{result_class};
+}
 =

 =3Dhead2 count
 =

@@ -1833,7 +1840,9 @@
 =

     #XXX - temp fix for result_class bug. There likely is a more elegant f=
ix -groditi
     my %attrs =3D %{$self->{attrs}||{}};
-    delete @attrs{qw(result_class alias)};
+    delete $attrs{alias};
+    delete $attrs{result_class}
+        unless $attrs{result_class} eq 'DBIx::Class::ResultClass::HashRefI=
nflator';
 =

     my $new_cache;
=20