[Dbix-class] A couple of bugs in DBIx::Class::Storage::DBI

[Matt S Trout]

Matt Ittigson wrote:
> I've been having a heck of a time getting the following 
> DBIx::Class::ResultSet working and have tracked a couple of bugs through 
> DBIx::Class::Storage::DBI (or SQL::Abstract, really).  Here's the ResulSet:
> 
> sub most_for_campaign {
>   my ($self, $limit) = @_;
> 
>   return $self->search(undef,
>                        { select => [ 'Id', 'first_name', 'last_name',
>                                      { count => 'Id' }
>                                    ],
>                          as => [ qw/ Id first_name last_name total / ],
>                          rows => $limit,
>                          group_by => [ qw/ Id first_name last_name / ],
>                        });
> }
> 
> First of all, I'm using a MS SQL database with the DBD::Sybase driver.
> 
> Because of the way SQL::Abstract::Limit rewrites MS SQL queries, that 
> turns into:
> 
> SELECT * FROM
> (
>     SELECT TOP 10 * FROM
>     (
>         SELECT TOP 10  Id, first_name, last_name, COUNT( Id ) FROM 
> contacts me
>     ) AS foo
>    
> ) AS bar
> 
> which breaks MS SQL because there is no implicit name for the COUNT( Id 
> ) "column."  I fixed this with the following patch to 
> DBIx::Class::Storage:: DBI.pm in _recurse_fields.
> 
> -        .'( '.$self->_recurse_fields($fields->{$func}).' )';
> +        .'( '.$self->_recurse_fields($fields->{$func}).' ) as _' . $func;

We really need a nice way to specify the SQL AS as well as the DBIC-level as. 
This isn't really a fix, it merely defers the problem until you have two 
COUNTs in the same statement (which, given we support GROUP BY and HAVING, is 
far from impossible). In the meantime, I'd go for \"COUNT(id) AS id_count"

> The second issue I don't have an easy fix for.
> 
> DBIC::SQL::Abstract (inside DBIx::Class::Storage::DBI), which subclasses 
> SQL::Abstract::Limit, overrides the select method.  The $order parameter 
> becomes a HASH ref instead of a string, and the group_by ARRAY ref gets 
> stuck inside $order.  DBIC::SQL::Abstract also overrides the 
> SQL::Abstract::_order_by method to process the HASH ref and appends the 
> contents of group_by to the query (assuming everything works well).  
> There are a couple of problems with the SQL::Abstract::Limit::select 
> method (before it calls the SQL::Abstract::select method).  First, and 
> the correct line is commented out, if $rows is not defined, the 
> SQL::Abstract::Limit::select calls SQL::Abstract->new->select instead of 
> $self->SUPER::select which would do cause the carefully overridden 
> SQL::Abstract methods in DBIC::SQL::Abstract to be ignored.  If $rows is 
> defined (in my case), the call to $self->SUPER::select doesn't pass the 
> $order parameter because, according the the comment, "with LIMIT 
> parameters, get the basic SQL without the ORDER BY clause."
> 
> Is the answer to always pass $order to the SQL::Abstract::select 
> method?  Should the SQL::Abstract->new->select line be changed to 
> $self->SUPER::select above?

In theory, but that code path should never get hit under DBIC - if there's no 
row limit the DBIC::SQL::Abstract code passes -1, then yanks that out further 
down. Plus I'm pretty sure the ORDER BY should get reconnected somewhere.

Any chance of a test case that illustrates what you're on about here?

-- 
      Matt S Trout       Offering custom development, consultancy and support
   Technical Director    contracts for Catalyst, DBIx::Class and BAST. Contact
Shadowcat Systems Ltd.  mst (at) shadowcatsystems.co.uk for more information

+ Help us build a better perl ORM: http://dbix-class.shadowcatsystems.co.uk/ +