Skip Menu |
 

This queue is for tickets about the Ima-DBI CPAN distribution.

Report information
The Basics
Id: 25073
Status: open
Priority: 0/
Queue: Ima-DBI

People
Owner: Nobody in particular
Requestors: mschwern [...] cpan.org
Cc: perl-cpan [...] bereft.net
Dan [...] DWright.Org
AdminCc:

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



Subject: [PATCH] Don't return an active statement handle
Download (untitled) / with headers
text/plain 900b
Ima::DBI uses DBI->prepare_cached(). By default the cache is stupid, it will return an active statement handle. This is bad if you do something like... my $sth = $obj->sql_foo; $sth->execute("foo"); my $another_sth = $different_obj_same_class->sql_foo; $another_sth->execute("bar"); Currently both calls to sql_foo() will return the same statement handle. DBI will warn but its very dangerous. Recently (in DBI 1.40) DBI introduced an extra parameter to prepare_cached(). If you set it to 3 (yeah, intuitive) it will Do The Right Thing which is rather than returning an active statement handle from the cache it will make a new one. This makes prepare_cached() much safer. This patch changes Ima::DBI to use that new feature. It requires raising the minimum DBI to 1.40. If that's undesirable I can make it optional. This is required for the CDBI iterator patch in 24959.
Subject: no_cache_if_active.patch
Auto-merging (0, 27456) /local/Ima-DBI to /vendor/Ima-DBI (base /vendor/Ima-DBI:27454). A t/active_handles.t U MANIFEST U lib/Ima/DBI.pm U Makefile.PL ==== Patch <-> level 1 Source: 9c88509d-e914-0410-b01c-b9530614cbfe:/local/Ima-DBI:27456 Target: 9c88509d-e914-0410-b01c-b9530614cbfe:/vendor/Ima-DBI:27454 Log: r27455@windhund: schwern | 2007-02-21 16:57:36 -0800 Local copy r27456@windhund: schwern | 2007-02-21 17:11:00 -0800 Make Ima::DBI use the new "$if_active" parameter to prepare_cached() to avoid returning an active statement handle from the cache. === t/active_handles.t ================================================================== --- t/active_handles.t (revision 27454) +++ t/active_handles.t (patch - level 1) @@ -0,0 +1,30 @@ +#!/usr/bin/perl -w + +package My::DBI; + +use strict; +use base 'Ima::DBI'; + +use Cwd; +use Test::More tests => 4; +use Test::NoWarnings; + +sub new { return bless {}; } + +# Test set_db +__PACKAGE__->set_db('test1', 'dbi:ExampleP:', '', '', + { AutoCommit => 1, Taint => 0 }); + +__PACKAGE__->set_sql('test', 'select mode,size,name from ?', 'test1'); + +my $obj = My::DBI->new; + +my $sth1 = $obj->sql_test; +$sth1->execute(cwd); +ok $sth1->{Active}; + +my $sth2 = $obj->sql_test; +$sth2->execute(cwd); +ok $sth2->{Active}; + +isnt $sth1, $sth2; === MANIFEST ================================================================== --- MANIFEST (revision 27454) +++ MANIFEST (patch - level 1) @@ -6,5 +6,6 @@ META.yml README t/DBI.t +t/active_handles.t t/pod-coverage.t t/pod.t === lib/Ima/DBI.pm ================================================================== --- lib/Ima/DBI.pm (revision 27454) +++ lib/Ima/DBI.pm (patch - level 1) @@ -379,7 +379,7 @@ # This is to do proper '%%' translation. my $sql = $class->transform_sql($statement => @_); return $cache - ? $dbh->prepare_cached($sql) + ? $dbh->prepare_cached($sql, {}, 3) : $dbh->prepare($sql); }; } === Makefile.PL ================================================================== --- Makefile.PL (revision 27454) +++ Makefile.PL (patch - level 1) @@ -17,10 +17,11 @@ NAME => 'Ima::DBI', VERSION_FROM => 'lib/Ima/DBI.pm', PREREQ_PM => { - 'DBI' => 1.20, + 'DBI' => 1.40, 'Class::Data::Inheritable' => 0.02, 'DBIx::ContextualFetch' => 1.00, 'Test::More' => 0.18, + 'Test::NoWarnings' => 0.08, }, ( $] > 5.005 ==== BEGIN SVK PATCH BLOCK ==== Version: svk v2.0.0 (darwin) eJyNlc1u20YQx3VoEYRA0QcoUGxjBnYC09pdLj+NqHZiKXUTu0bjNkAvzHJ3ZLGmSIWk/BHTB8qO g/TQQ4Dcihx6LHrpA/Rd8ihdUlEaJ86HIECr3Znfzuz+Z7aXbS+vkLLTwaVOcHnv5zu+v8ULMbhK WKnbJcioSDPdKmPYg1g3yzjd0VmZ8CGo1TwdZ6IeFDzbgaIeRGIXik6HKJw7xXUbxAzbUENepEmu ew0+KDIAnZTuileumPU30IlX5qBWGmyQwV6UR2miwqAOs2xlouyJ8k9HkARZmhavl2jtjUsRpzkE NX7FKtVX2VNdpdQ4yCgDoWI6VLNFQ5p5N4ZsBu5HMehuubG6ud7r3tt+19K6AKmOKArfNXXPQQkp N/gu1OOlrbvnrJv0rVcJvBkpUbmt2DXLVEFxKV+h3LJoc1FEexAMeCJjyJfqpGig09Ig7+fRKa9Z 5aNRfBgUcFBIiAte7xEEOmsUsdlqtU4v7U70f7pXn3a/rm7PnfTw87s//N3dft6rFiZrD56tVeu9 6pff1+BJt3p49mO1dNKrbqrJP5qFtbXJ50+/++rx7cnmWXdy72ztwaQ7GT7rTeLfvp/sVPcnWXX/ RKs2Tq6cqrmqd8Inc9+0x3nWDqOkPYIsRsa+po242OU7gDYOfX/t5vqypo1zQHmRRaJYDrkaz68P +fytfbkNeeH7G2kGqFDDHN3oILaZ3udZEiU7ufLMxyFKYB8doQyKcZagUB1bjo6Ol9Gxps2hWnwy 1IJga/XWndXb3SAwOgvzZH4Rzcsw8rsHfDiKYcvXLh+h1XGR3kqHw6ggi2ibRwlGx9eWtfxhnEOs ThsNUwmLefQIFuuyQf0sHaJvh4dIT8Nf0Q2jowLJiwGph8opWNaMDhyAGBewIPalQqW7Rudotbni Y0pplCfF4rK2wt6U+lQLShomLSXzwDOpCE3eN2nomba0XdMKbelJLgirL95638WzmmCHfbCZUI7c 9fpm3wPMbJcCZYQ4NlNqtafKeFR98dn1lmj99bYCtaI9SqUh0j3I1K3NJtTvhXE3u1JG3RCwFJYA DJQSh5pEOLjfN0F4XEzjVtb2BXXn1HXXVgpo6vKj1WS9hjnnK5PNOG0lsqXRsNnuozj7g2fqNNnZ pE8YFq6koeU5DiYmDh0REiwZUE5181W1VS8G1YtUu9R6edB60vp39PJgUSlzEZkXnlzDxgAm8aRl UtwnYArLtZjrgGsBc6QVuh+Mzq0JnDIBwDmXHCzLNRntYwaYmh4HZodEt8k0urPrZ1tfzrVOjVbV 2jtd+rNVaY8Ruzw/Lbr/q2wezT6q+vASvjD4ZmvMmWtyrORGQuxZQB1Hqr0ZCRmmQGTTHqlllSir b8Fa2Y8SORgn0kcoF4N9UPVbIoqxY2BqUIKI7VuOb9rIwC7GGrqbCh4jkY4OtSnC/jjC8QnxMZ4h 6m6NlCia1oPqxlMMoGkhV/SoH0zVfwWNeKZKvIAMFSkaZaD+QyC4GIBcuKbmNMT30ki+6jrqmBBP 0NRZdTJewBCSAk2LaNoo6m0awJKmzsHodGipUzp9Wbeb59H3f0qUf5bz+Kp6Hlg54sVAPUPq1VR/ xuNI1qJux/Uh1Lo2VAYzQeumXXrCdS3sSQM8wgysmr4RYiKM0LNMbBMmVC/oXKPle+FW2d6DRKbZ W3T2qXTdcj7Jzj+fhN+k8B9l28Pa ==== END SVK PATCH BLOCK ====
Download (untitled) / with headers
text/plain 423b
This revised patch replaces the first one. It also adds the ability to pass in parameters to the sql_*() methods. It will be handy in the future but right now this is needed to selectively turn off statement handle caching. This is needed for the new CDBI iterators, since they execute on demand rather than on creation if they use prepare_cached() they might both be given the same statement. So they need to override.
Auto-merging (0, 27458) /local/Ima-DBI to /vendor/Ima-DBI (base /vendor/Ima-DBI:27454). A t/caching.t U MANIFEST U lib/Ima/DBI.pm U Makefile.PL ==== Patch <-> level 1 Source: 9c88509d-e914-0410-b01c-b9530614cbfe:/local/Ima-DBI:27458 Target: 9c88509d-e914-0410-b01c-b9530614cbfe:/vendor/Ima-DBI:27454 Log: r27455@windhund: schwern | 2007-02-21 16:57:36 -0800 Local copy r27456@windhund: schwern | 2007-02-21 17:11:00 -0800 Make Ima::DBI use the new "$if_active" parameter to prepare_cached() to avoid returning an active statement handle from the cache. r27457@windhund: schwern | 2007-02-21 17:53:47 -0800 Rename the "active_handles" test to be "caching" because that's really what its about. r27458@windhund: schwern | 2007-02-21 17:57:09 -0800 Add a parameter argument to sql_* so we can override its behaviors. Right now its the caching behavior. === t/caching.t ================================================================== --- t/caching.t (revision 27454) +++ t/caching.t (patch - level 1) @@ -0,0 +1,52 @@ +#!/usr/bin/perl -w + +package My::DBI; + +use strict; +use base 'Ima::DBI'; + +use Cwd; +use Test::More tests => 10; +use Test::NoWarnings; + +sub new { return bless {}; } + +# Test set_db +__PACKAGE__->set_db('test1', 'dbi:ExampleP:', '', '', + { AutoCommit => 1, Taint => 0 }); + +__PACKAGE__->set_sql('test', 'select mode,size,name from ?', 'test1'); + +my $obj = My::DBI->new; + +{ + my $sth1 = $obj->sql_test; + $sth1->execute(cwd); + ok $sth1->{Active}; + + my $sth2 = $obj->sql_test; + $sth2->execute(cwd); + ok $sth2->{Active}; + + isnt $sth1, $sth2, "Active statement handles are not cached"; +} + +{ + my $sth1 = $obj->sql_test; + ok !$sth1->{Active}; + + my $sth2 = $obj->sql_test; + ok !$sth2->{Active}; + + is $sth1, $sth2, "Inactive statement handles are cached"; +} + +{ + my $sth1 = $obj->sql_test({ cache => 0 }); + ok !$sth1->{Active}; + + my $sth2 = $obj->sql_test({ cache => 0 }); + ok !$sth2->{Active}; + + isnt $sth1, $sth2, "Override default caching behavior"; +} \ No newline at end of file === MANIFEST ================================================================== --- MANIFEST (revision 27454) +++ MANIFEST (patch - level 1) @@ -6,5 +6,6 @@ META.yml README t/DBI.t +t/caching.t t/pod-coverage.t t/pod.t === lib/Ima/DBI.pm ================================================================== --- lib/Ima/DBI.pm (revision 27454) +++ lib/Ima/DBI.pm (patch - level 1) @@ -1,6 +1,6 @@ package Ima::DBI; -$VERSION = '0.34'; +$VERSION = '0.34_02'; use strict; use base 'Class::Data::Inheritable'; @@ -369,17 +369,21 @@ } sub _mk_sql_closure { - my ($class, $sql_name, $statement, $db_meth, $cache) = @_; + my ($class, $sql_name, $statement, $db_meth, $cache_default) = @_; return sub { my $class = shift; my $dbh = $class->$db_meth(); + my $params = pop @_ if ref $_[-1] eq 'HASH'; + my $cache = $cache_default; + $cache = $params->{cache} if defined $params->{cache}; + # Everything must pass through sprintf, even if @_ is empty. # This is to do proper '%%' translation. my $sql = $class->transform_sql($statement => @_); return $cache - ? $dbh->prepare_cached($sql) + ? $dbh->prepare_cached($sql, {}, 3) : $dbh->prepare($sql); }; } @@ -469,6 +473,7 @@ $sth = $obj->sql_*; $sth = $obj->sql_*(@sql_pieces); + $sth = $obj->sql_*(@sql_pieces, \%params) sql_*() is a catch-all name for the methods you set up with set_sql(). For instance, if you did: @@ -492,6 +497,8 @@ # statement 'Select * From Bar' $sth = $obj->sql_Search('Bar'); +The optional %params contains parameters to alter the behavior of the method. Currently the only option is C<cache> which will override the default statement handle caching behavior for this method set when it was created with C<set_sql>. + Be B<very careful> with what you feed this function. It cannot do any quoting or escaping for you, so it is totally up to you to take care of that. Fortunately if you have tainting on you === Makefile.PL ================================================================== --- Makefile.PL (revision 27454) +++ Makefile.PL (patch - level 1) @@ -17,10 +17,11 @@ NAME => 'Ima::DBI', VERSION_FROM => 'lib/Ima/DBI.pm', PREREQ_PM => { - 'DBI' => 1.20, + 'DBI' => 1.40, 'Class::Data::Inheritable' => 0.02, 'DBIx::ContextualFetch' => 1.00, 'Test::More' => 0.18, + 'Test::NoWarnings' => 0.08, }, ( $] > 5.005 ==== BEGIN SVK PATCH BLOCK ==== Version: svk v2.0.0 (darwin) eJyNVs9v48YVVoEggHnInoIUKFDM2lzIDiRrhj9FOVbl2lLsZNfreN20gNMyQ87QYkyRCklZ65hG Q0nrIEWxBQK0p6KHFOmpOfQfavs/9NRb35DSerf2ZlcQwOHMm++9N/y+96YXH210SNZu40wmOHv0 8Yet1gFN3f49omWykXHmp1Es61nAz3ggq1kQnchaFtIBh9UkGsWuGKQ0PuGpGPjuKU/bbQJwzRKu W0AsYAtUh6ZRmMhWAW+nMecyyZodK+uo4m/LxMoSDisFrB3zMz/xoxDCUExNb4IJ2BPYHw15aMdR lM6XrI4iduPMDaKE2wK+o2fwB3tFhpSKDcyPuQsxncNsWiAtdheG2gLY8wMuN7MHW/t7ve6jo5uW +i2QcES+c9O0+QIoIdkDesrFeP3g/gvWRfr6PIHnIyWQW8cQWCoERRl7BpU2XOr2/fBkXWSj2LKS 1cnLgZQSqFilw2Fwbqf8ccp4kFIBbtuybmjw7fYrlcpXn+XK5Owf3Xtfd3+av78y7eE/3X/49+7R H3v52mTn02928g96+fHTLr/q5vHVYd6Y9vJtmPxzsdDdmbz59e5Pnu5MDq+6k6OrnU/z7iToTtKn vak02Z3e+d39SZAfT87z4+nb+UdTMutN7+S9aTbtTbXvTib+7KNpbWcyYNPN3dn5g9mP9p68NTmY vTP9ZLZ6Otud9K7e+CD/7cVs9Jsr/Ksn7cPZ23/ozXq/35ntr9xtjJK44fhhY8jjANXHkjSk7ik9 4ejBeau18/O9DUkaJRwlaey76YZDYVzdG9Dq9pgd8SRttR5EMUcpDBO02UYE70e/pHEIx5zA1mTk oJCP0QWKeTqKQ+QEPEnQxeUGupSkFSToyxzJtg+2tj/cer9r2/X2apVUa6jKHL/VfUwHw4AftKSl C7Q1SqPtaDDw0xo6on6I0eXahpR8HiQ8gK+GBhHjtcT/gteE7pAXRwP0s8E5kiPnM7RZb0McENGF hOCXpH0i5mC3vVFv88fcHaV81R2ztej0YstN/TN+uSEpiuInYVqroWXYQlM+4GGK+jRkkAaikHgY pUjQirPlDekyOkV3/WQvpKsXD894HPuMI8Y9OgrgPJDD+/TMj+KO9rz6SnoCW1Ul05pNx9N0z3E9 z1UoYQSbim5g7HpMt7DQLejkJZTUBILheNzQXNVgtGl5qmdxrBlNhSsaIaahEVmzSs5+kb/5xkrF rfz1mSiktDGMWN2NIHIgwGICnrcGXLjDTPGYoZiepWiGpXmWpXpM45ZnYc4syykDBmvjlhpgihrQ ADIVNeKVytafgZkvVgltgdMAvq4PB4W7V8IZP3iYpshOMYhHNOw2meLolmliomLHdB2CIUeFKrJu 6eVp5n/p59+iuzP5b5V/Witf5seV/OBfq19Vvnnn34+/r1z9+D9vfZlHle92/isv5AXhLvQlf9w9 fLT3cB9toipeVzUbK3bJmnQN5jo22CzN9SMEdSEtLQleuwEFLW2ipO976cZ8kjl94PfmfLXehgl7 wNP+6ppAKUyGNKYDsXEYDQEd+R6o00OyfVwnv0b8c1Td3Xq0W10gFvQuEMVgEZlYvV4qIevti2Lm UkCCnR9ydmOtBuqvIVWoUAYZit0g0FKK7652xGPoc5cnNfTJvXLv2hF4iYYpNDcaoPkkcqMwhTKQ oOKdpzxOUBohGsAIpbBjoTcUecW7OAbgM0LbozgGIQfnxXQUwqBER36Ctt8r4myjcd93+2jsBwGK FmIW9vMDuFEQ0FxJ1369SAQCmKVnUe0AlYMbeFLIIOYAwcAHnMP2e6IYQvrt9Vv1VjCSUl31MIib uVrT9DAhqqFpWGWGZpkO8X6Q080CQdFczimljHJdb6qa4mGNY0W1KNcMh8gGKSl99e7VwZ2Vyqxe yStns/VvK7n0BGlL1bLqX1f5Klr8oPzjdXxr8IVrTLWmSjFUJ+JgS+eKaTLwrRFHwwonrGzw4B/F Qrt6Z+yHrD8KWQsKttsfc+B/hhSMzTpW6gpBxGjpZks1UB03MZbQ/cgFfrjR8FwqIYxXQ5gtQloY LyDEfeOZNpHofOKTixa2LPueTYvOsHxNOcG4YczhndtlH1hdgzkJ0bPIZ/OuJ0hBQ1RuvsmbolMJ NwXA+jx283Vi19WWZi5iP+RF4xNIy6Uve96qlosGLWJ1YG3O02V4cWmZIk2rCQRLA5DCGN4koCg0 OCcapYt4mq8Vj9nC1iKeLcYQfe6o4Jo6KvKGOAq1oyRCY5F2eK0w4XchoAS0euif9CGcMBoXS4tj el5n6xIwp95uK5msKOVt+qi4ErdavwjhFOKEBvfgSqhlQ5r24eoJN2V4GY18JppHIxC0Ef2jDt98 cXuWVSOz3GZTxxarc4todawRXHcwceuOpavYIJoLzba9pmQvBdezxhkPWRT/H7r2uuiybr6WXevF JFpFCv8DaK0SLQ== ==== END SVK PATCH BLOCK ====
From: Dan [...] DWright.Org
Download (untitled) / with headers
text/plain 501b
I am also seeing problems with the way that Ima::DBI is using prepare_cached. The fatal error that I'm seeing is: prepare_cached( SQL query here... ) statement handle DBIx::ContextualFetch::st=HASH(0x99d3418) still Active at /usr/local/lib/perl5/site_perl/5.8.3/Ima/DBI.pm line 398 I've found that this simple patch corrects the problem (borrowed from Schwern's patch): 399c392 < ? $dbh->prepare_cached($sql) --- Show quoted text
> ? $dbh->prepare_cached($sql, {}, 3)


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.