From: Peter Rabbitson Date: Wed, 19 Jan 2011 11:17:47 +0000 (+0100) Subject: DBIC now warns on explicit false AutoCommit, and when altering external $dbh's X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=6c925c721b4fefc6f8e3b3c2ad82c7f3efa268a3;p=dbsrgits%2FDBIx-Class-Historic.git DBIC now warns on explicit false AutoCommit, and when altering external $dbh's --- diff --git a/Changes b/Changes index 0449717..2340424 100644 --- a/Changes +++ b/Changes @@ -1,5 +1,11 @@ Revision history for DBIx::Class + * New Features / Changes + - DBIx::Class now warns when the user erroneously supplies + AutoCommit => 0 to connect() + - A warning is also issued before forcing the RaiseError + setting of externally supplied DBI handles + * Fixes - Throw comprehensible exception on erroneous $schema->source() invocation diff --git a/lib/DBIx/Class/Storage/DBI.pm b/lib/DBIx/Class/Storage/DBI.pm index 70dcb24..baa9ed6 100644 --- a/lib/DBIx/Class/Storage/DBI.pm +++ b/lib/DBIx/Class/Storage/DBI.pm @@ -595,8 +595,18 @@ sub connect_info { my @args = @{ $info->{arguments} }; - $self->_dbi_connect_info([@args, - %attrs && !(ref $args[0] eq 'CODE') ? \%attrs : ()]); + if (keys %attrs and ref $args[0] ne 'CODE') { + carp + 'You provided explicit AutoCommit => 0 in your connection_info. ' + . 'This is almost universally a bad idea (see the footnotes of ' + . 'DBIx::Class::Storage::DBI for more info). If you still want to ' + . 'do this you can set $ENV{DBIC_UNSAFE_AUTOCOMMIT_OK} to disable ' + . 'this warning.' + if ! $attrs{AutoCommit} and ! $ENV{DBIC_UNSAFE_AUTOCOMMIT_OK}; + + push @args, \%attrs if keys %attrs; + } + $self->_dbi_connect_info(\@args); # FIXME - dirty: # save attributes them in a separate accessor so they are always @@ -666,11 +676,12 @@ sub _normalize_connect_info { return \%info; } -sub _default_dbi_connect_attributes { - return { +sub _default_dbi_connect_attributes () { + +{ AutoCommit => 1, - RaiseError => 1, PrintError => 0, + RaiseError => 1, + ShowErrorStatement => 1, }; } @@ -1257,6 +1268,27 @@ sub _connect { unless ($self->unsafe) { + $self->throw_exception( + 'Refusing clobbering of {HandleError} installed on externally supplied ' + ."DBI handle $dbh. Either remove the handler or use the 'unsafe' attribute." + ) if $dbh->{HandleError} and ref $dbh->{HandleError} ne '__DBIC__DBH__ERROR__HANDLER__'; + + # Default via _default_dbi_connect_attributes is 1, hence it was an explicit + # request, or an external handle. Complain and set anyway + unless ($dbh->{RaiseError}) { + carp( ref $info[0] eq 'CODE' + + ? "The 'RaiseError' of the externally supplied DBI handle is set to false. " + ."DBIx::Class will toggle it back to true, unless the 'unsafe' connect " + .'attribute has been supplied' + + : 'RaiseError => 0 supplied in your connection_info, without an explicit ' + .'unsafe => 1. Toggling RaiseError back to true' + ); + + $dbh->{RaiseError} = 1; + } + # this odd anonymous coderef dereference is in fact really # necessary to avoid the unwanted effect described in perl5 # RT#75792 @@ -1264,7 +1296,9 @@ sub _connect { my $weak_self = $_[0]; weaken $weak_self; - $_[1]->{HandleError} = sub { + # the coderef is blessed so we can distinguish it from externally + # supplied handles (which must be preserved) + $_[1]->{HandleError} = bless sub { if ($weak_self) { $weak_self->throw_exception("DBI Exception: $_[0]"); } @@ -1273,12 +1307,8 @@ sub _connect { # the scope of DBIC croak ("DBI Exception (unhandled by DBIC, ::Schema GCed): $_[0]"); } - }; + }, '__DBIC__DBH__ERROR__HANDLER__'; }->($self, $dbh); - - $dbh->{ShowErrorStatement} = 1; - $dbh->{RaiseError} = 1; - $dbh->{PrintError} = 0; } } catch { diff --git a/t/cdbi/DeepAbstractSearch/01_search.t b/t/cdbi/DeepAbstractSearch/01_search.t index 599cec1..cc08010 100644 --- a/t/cdbi/DeepAbstractSearch/01_search.t +++ b/t/cdbi/DeepAbstractSearch/01_search.t @@ -12,6 +12,10 @@ BEGIN { my $DB = "t/var/cdbi_testdb"; unlink $DB if -e $DB; +# not usre why this test needs an AutoCommit => 0 and a commit further +# down - EDONOTCARE +$ENV{DBIC_UNSAFE_AUTOCOMMIT_OK} = 1; + my @DSN = ("dbi:SQLite:dbname=$DB", '', '', { AutoCommit => 0 }); package Music::DBI; diff --git a/t/storage/base.t b/t/storage/base.t index 6b25576..2aac70c 100644 --- a/t/storage/base.t +++ b/t/storage/base.t @@ -140,6 +140,7 @@ my $invocations = { AutoCommit => 0, }, ], + warn => qr/\QYou provided explicit AutoCommit => 0 in your connection_info/, }, 'connect_info ([ \%attr_with_coderef ])' => { args => [ { diff --git a/t/storage/on_connect_call.t b/t/storage/on_connect_call.t index bea5085..265835c 100644 --- a/t/storage/on_connect_call.t +++ b/t/storage/on_connect_call.t @@ -10,8 +10,9 @@ use DBIx::Class::Storage::DBI; # !!! do not replace this with done_testing - tests reside in the callbacks # !!! number of calls is important -use Test::More tests => 16; +use Test::More tests => 17; # !!! +use Test::Warn; my $schema = DBICTest::Schema->clone; @@ -81,7 +82,7 @@ my $schema = DBICTest::Schema->clone; { ok $schema->connection( - sub { DBI->connect(DBICTest->_database) }, + sub { DBI->connect(DBICTest->_database, undef, undef, { AutoCommit => 0 } ) }, { # method list form on_connect_call => [ sub { ok 1, "on_connect_call after DT parser" }, ], @@ -91,7 +92,10 @@ my $schema = DBICTest::Schema->clone; ok (! $schema->storage->connected, 'start disconnected'); - $schema->storage->_determine_driver; # this should connect due to the coderef + # this should connect due to the coderef, and also warn due to the false autocommit above + warnings_exist { + $schema->storage->_determine_driver + } qr/The 'RaiseError' of the externally supplied DBI handle is set to false/, 'Warning on clobbered AutoCommit => 0 fired'; ok ($schema->storage->connected, 'determine driver connects'); $schema->storage->disconnect; diff --git a/t/storage/on_connect_do.t b/t/storage/on_connect_do.t index 31d39c6..2ce77b2 100644 --- a/t/storage/on_connect_do.t +++ b/t/storage/on_connect_do.t @@ -3,8 +3,9 @@ use warnings; # !!! do not replace this with done_testing - tests reside in the callbacks # !!! number of calls is important -use Test::More tests => 12; +use Test::More tests => 13; # !!! +use Test::Warn; use Test::Exception; use lib qw(t/lib); @@ -33,7 +34,7 @@ is_deeply ( $schema->storage->disconnect; ok $schema->connection( - sub { DBI->connect(DBICTest->_database) }, + sub { DBI->connect(DBICTest->_database, undef, undef, { AutoCommit => 0 }) }, { on_connect_do => [ 'CREATE TABLE TEST_empty (id INTEGER)', @@ -45,6 +46,11 @@ ok $schema->connection( }, ), 'connection()'; +warnings_exist { + $schema->storage->ensure_connected +} qr/The 'RaiseError' of the externally supplied DBI handle is set to false/, +'Warning on clobbered AutoCommit => 0 fired'; + is_deeply ( $schema->storage->dbh->selectall_arrayref('SELECT * FROM TEST_empty'), [ [ 2 ], [ 3 ], [ 7 ] ], diff --git a/t/storage/txn.t b/t/storage/txn.t index c4ecefd..f84fa9a 100644 --- a/t/storage/txn.t +++ b/t/storage/txn.t @@ -363,9 +363,10 @@ my $fail_code = sub { # make sure AutoCommit => 0 on external handles behaves correctly with scope_guard warnings_are { - my $factory = DBICTest->init_schema (AutoCommit => 0); + my $factory = DBICTest->init_schema; cmp_ok ($factory->resultset('CD')->count, '>', 0, 'Something to delete'); my $dbh = $factory->storage->dbh; + $dbh->{AutoCommit} = 0; ok (!$dbh->{AutoCommit}, 'AutoCommit is off on $dbh'); my $schema = DBICTest::Schema->connect (sub { $dbh }); @@ -385,14 +386,14 @@ warnings_are { # make sure AutoCommit => 0 on external handles behaves correctly with txn_do warnings_are { - my $factory = DBICTest->init_schema (AutoCommit => 0); + my $factory = DBICTest->init_schema; cmp_ok ($factory->resultset('CD')->count, '>', 0, 'Something to delete'); my $dbh = $factory->storage->dbh; + $dbh->{AutoCommit} = 0; ok (!$dbh->{AutoCommit}, 'AutoCommit is off on $dbh'); my $schema = DBICTest::Schema->connect (sub { $dbh }); - lives_ok ( sub { $schema->txn_do (sub { $schema->resultset ('CD')->delete }); }, 'No attempt to start a atransaction with txn_do');