[Dbix-class] find redux

[Matt S Trout]

DWC, PING. If this looks good for you I want it for 0.08100

On Tue, Feb 19, 2008 at 01:58:29AM +0100, Zbigniew Lukasiak wrote:
> On Feb 18, 2008 12:57 PM, Zbigniew Lukasiak <zzbbyy at gmail.com> wrote:
> > On Feb 17, 2008 5:03 PM, Matt S Trout <dbix-class at trout.me.uk> wrote:
> > > > >
> > > > >  Looks good. Fancy doing the warn-if-more-than-one-result bit while you're
> > > > >  in there as well?
> > > >
> > > > Attached.  I think it could even croak in that case.
> > >
> > > Your patch seems to bypass using $rs->single, which is a useful performance
> > > optimisation.
> > >
> > > Please fix that.
> >
> > Sorry - I tried to compress it a bit and did not notice that I changed
> > the logic a bit (I am not sure if it really bypasses - since I still
> > call $rs->single - but anyway, now I have made another patch without
> > that change).
> >
> > >
> > > Also, shouldn't you ignore @unique_queries altogether if you've got a key
> > > attr or am I misunderstanding the impact?
> > >
> >
> > _unique_queries seems to work for that case - so this was the minimal
> > change. But I am all too happy to get rid of it - so now I attach a
> > patch bypassing that call.
> >
> > There is still one problem - it does not pass the last of t/83cache.t tests:
> >
> > $queries = 0;
> >
> > $rs = $schema->resultset('Artist')->search(undef, { prefetch => [qw/cds/] });
> > $artist = $rs->find(1);
> >
> > is( $queries, 1, 'only one select statement on find with has_many
> > prefetch on resultset' );
> >
> > It looks that in that case the second ->next call will generate
> > another query.  I've tried to dump it:
> >
> > SELECT me.artistid, me.name, cds.cdid, cds.artist, cds.title, cds.year
> > FROM artist me LEFT JOIN cd cds ON ( cds.artist = me.artistid ) WHERE
> > ( ( me.artistid = ? ) ) ORDER BY cds.artist, cds.year: '1'
> > SELECT me.artistid, me.name, cds.cdid, cds.artist, cds.title, cds.year
> > FROM artist me LEFT JOIN cd cds ON ( cds.artist = me.artistid ) ORDER
> > BY cds.artist, cds.year:
> > DBIx::Class::ResultSet::find(): Cursor not exhausted in find at
> > 83cache.t line 13
> >
> > So it seems that the second query runs the query from the ->search call above.
> 
> A silly mistake - fixed.  Another patch attached.
> 
> -- 
> Zbigniew Lukasiak
> http://brudnopis.blogspot.com/

> Index: t/61findnot.t
> ===================================================================
> --- t/61findnot.t	(revision 4085)
> +++ t/61findnot.t	(working copy)
> @@ -7,7 +7,7 @@
>  
>  my $schema = DBICTest->init_schema();
>  
> -plan tests => 17;
> +plan tests => 18;
>  
>  my $art = $schema->resultset("Artist")->find(4);
>  ok(!defined($art), 'Find on primary id: artist not found');
> @@ -44,3 +44,9 @@
>  @cd = $cd->single;
>  cmp_ok(@cd, '==', 1, 'Return something even in array context');
>  ok(@cd && !defined($cd[0]), 'Array contains an undef as only element');
> +
> +$cd = $schema->resultset("CD")->first;
> +my $artist_rs = $schema->resultset("Artist")->search( { artistid => $cd->artist->artistid } );
> +$art = $artist_rs->find( { name => 'some other name' }, { key => 'primary' } );
> +ok( $art, 'Artist found by key in the resultset' );
> +
> Index: lib/DBIx/Class/Storage/DBI.pm
> ===================================================================
> --- lib/DBIx/Class/Storage/DBI.pm	(revision 4085)
> +++ lib/DBIx/Class/Storage/DBI.pm	(working copy)
> @@ -1164,6 +1164,7 @@
>    my $self = shift;
>    my ($rv, $sth, @bind) = $self->_select(@_);
>    my @row = $sth->fetchrow_array;
> +  carp( "Cursor not exhausted in select_single" ) if $sth->fetchrow_array; 
>    # Need to call finish() to work round broken DBDs
>    $sth->finish();
>    return @row;
> Index: lib/DBIx/Class/ResultSet.pm
> ===================================================================
> --- lib/DBIx/Class/ResultSet.pm	(revision 4085)
> +++ lib/DBIx/Class/ResultSet.pm	(working copy)
> @@ -384,25 +384,46 @@
>      @{$input_query}{@keys} = values %related;
>    }
>  
> -  my @unique_queries = $self->_unique_queries($input_query, $attrs);
>  
>    # 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
> +  my $query;
> +  if ( exists $attrs->{key} ){
> +    my @unique_cols = $self->result_source->unique_constraint_columns($attrs->{key});
> +    my $unique_query = $self->_build_unique_query($input_query, \@unique_cols);
> +    $query = $self->_add_alias( $unique_query, $alias );
> +  }
> +  else {
> +    my @unique_queries = $self->_unique_queries($input_query, $attrs);
> +    $query = @unique_queries
>      ? [ map { $self->_add_alias($_, $alias) } @unique_queries ]
>      : $self->_add_alias($input_query, $alias);
> +  }
>  
>    # Run the query
>    if (keys %$attrs) {
>      my $rs = $self->search($query, $attrs);
> -    return keys %{$rs->_resolved_attrs->{collapse}} ? $rs->next : $rs->single;
> +    if ( keys %{$rs->_resolved_attrs->{collapse}} ){
> +      my $row = $rs->next;
> +      carp "Cursor not exhausted in find" if $rs->next;
> +      return $row;
> +    }
> +    else {
> +      return $rs->single;
> +    }
>    }
>    else {
> -    return keys %{$self->_resolved_attrs->{collapse}}
> -      ? $self->search($query)->next
> -      : $self->single($query);
> +    if ( keys %{$self->_resolved_attrs->{collapse}} ){
> +      my $rs = $self->search($query);
> +      my $row = $rs->next;
> +      carp "Cursor not exhausted in find" if $rs->next;
> +      return $row;
> +    }
> +    else {
> +      return $self->single($query);
> +    }
>    }
>  }
>  

> _______________________________________________
> List: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/dbix-class
> IRC: irc.perl.org#dbix-class
> SVN: http://dev.catalyst.perl.org/repos/bast/DBIx-Class/
> Searchable Archive: http://www.grokbase.com/group/dbix-class@lists.rawmode.org

-- 
      Matt S Trout       Need help with your Catalyst or DBIx::Class project?
   Technical Director                    http://www.shadowcat.co.uk/catalyst/
 Shadowcat Systems Ltd.  Want a managed development or deployment platform?
http://chainsawblues.vox.com/            http://www.shadowcat.co.uk/servers/