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

Emanuele Zeppieri ema_zep at libero.it
Wed Feb 6 19:23:00 GMT 2008

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');
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');
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:


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 =

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.

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 }


# 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.

# 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.

# 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');
$hash_ref =3D $cd_rs->search_related('artist', {
    name =3D> 'Mr. Crappy Crap',

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' }

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
--- 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=
+__PACKAGE__->mk_group_accessors('simple' =3D> qw/_source_handle/);

 =3Dhead1 NAME

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


+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=

     my $new_cache;

More information about the DBIx-Class mailing list