Skip Menu |
 

This queue is for tickets about the DBIx-Class-Cursor-Cached CPAN distribution.

Report information
The Basics
Id: 62211
Status: resolved
Priority: 0/
Queue: DBIx-Class-Cursor-Cached

People
Owner: Nobody in particular
Requestors: TIMB [...] cpan.org
Cc:
AdminCc:

Bug Information
Severity: Critical
Broken in: (no value)
Fixed in: (no value)

Attachments
DBIx-Class-Cursor-Cached-as_query1.patch



Subject: ::Cached doesn't work with DBIx::Class::ResultSet::WithMetaData (and is too sensitive to irrelevant attrs)
Download (untitled) / with headers
text/plain 595b
DBIx-Class-Cursor-Cached doesn't work with DBIx::Class::ResultSet::WithMetaData, or any other module that adds code refs into the attributes, because Storable croaks. It's will also generate different keys for searches that differ only by attribute values that are irrelevant to the database query. It's also affected by the pseudo-random ordering of hash keys (because it doesn't set $Storable::canonical - which would be costly for hashes anyway). The attached patch (outlined by ribasushi++) avoids all these problems by basing the cache key on the rendered SQL query and bind values.
Subject: DBIx-Class-Cursor-Cached-as_query1.patch
--- perl/cpan/lib/perl5/DBIx/Class/Cursor/Cached.pm.orig 2010-10-17 22:35:43.000000000 +0100 +++ perl/cpan/lib/perl5/DBIx/Class/Cursor/Cached.pm 2010-10-17 22:40:58.000000000 +0100 @@ -48,7 +48,15 @@ sub _build_cache_key { my ($class, $storage, $args, $attrs) = @_; - return Digest::SHA1::sha1_hex(Storable::nfreeze([ $args, $attrs ])); + # compose the query and bind values, like as_query(), + # so the cache key is only affected by what the database sees + # and not any other cruft in $attrs + my $ref = $storage->_select_args_to_query(@{$args}[0..2], $attrs); + # nfreeze is used because bind args may include refs + # $Storable::canonical is used just in case the refs incude + # hash refs (unlikely, but this is cheap insurance) + local $Storable::canonical = 1; + return Digest::SHA1::sha1_hex(Storable::nfreeze( $ref )); } sub _fill_data {
Download (untitled) / with headers
text/plain 254b
One edge case to consider: if the user has multiple databases, or multiple connections to the same db but for different users, then the same query on each would get the same cache key. So the storage connect_info needs to be included in the cache key.
Download (untitled) / with headers
text/plain 596b
On Wed Oct 27 10:05:26 2010, TIMB wrote: Show quoted text
> One edge case to consider: if the user has multiple databases, or > multiple connections to the > same db but for different users, then the same query on each would get > the same cache key. > > So the storage connect_info needs to be included in the cache key.
arcanez implemented the necessary changes, module should ship to CPAN on today (Jan 24th) or soon. If time permits please audit the changes for sanity: lines 50-75 at http://dev.catalystframework.org/svnweb/bast/blame/DBIx-Class-Cursor-Cached/1.000/trunk/lib/DBIx/Class/Cursor/Cached.pm
CC: TIMB [...] cpan.org
Subject: Re: [rt.cpan.org #62211] ::Cached doesn't work with DBIx::Class::ResultSet::WithMetaData (and is too sensitive to irrelevant attrs)
Date: Tue, 25 Jan 2011 10:20:23 +0000
To: Peter Rabbitson via RT <bug-DBIx-Class-Cursor-Cached [...] rt.cpan.org>
From: Tim Bunce <Tim.Bunce [...] pobox.com>
Download (untitled) / with headers
text/plain 1.4k
On Mon, Jan 24, 2011 at 07:37:12AM -0500, Peter Rabbitson via RT wrote: Show quoted text
> <URL: https://rt.cpan.org/Ticket/Display.html?id=62211 > > > On Wed Oct 27 10:05:26 2010, TIMB wrote:
> > One edge case to consider: if the user has multiple databases, or > > multiple connections to the > > same db but for different users, then the same query on each would get > > the same cache key. > > > > So the storage connect_info needs to be included in the cache key.
> > arcanez implemented the necessary changes, module should ship to CPAN on > today (Jan 24th) or soon. If time permits please audit the changes for > sanity: lines 50-75 at > http://dev.catalystframework.org/svnweb/bast/blame/DBIx-Class-Cursor-Cached/1.000/trunk/lib/DBIx/Class/Cursor/Cached.pm
Logic looks sane. Thanks. It's a pity about the extra cost to disambiguate the db connection. I came up with this slightly smaller and cheaper version: my $conn; if (not ($conn = $storage->_dbh)) { my $connect_info = $storage->_dbi_connect_info; if (! ref($connect_info->[0]) ) { $conn = { Name => $connect_info->[0], Username => $connect_info->[1] }; } else { carp "Invoking connector coderef $connect_info->[0] in order to obtain cache-lookup information"; $conn = $connect_info->[0]->(); } } local $Storable::canonical = 1; return Digest::SHA1::sha1_hex(Storable::nfreeze( [ $ref, $conn->{Name}, $conn->{Username}||'' ] )); Tim.
fixed in 1.001000


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.