DBIC now warns on explicit false AutoCommit, and when altering external $dbh's
Peter Rabbitson [Wed, 19 Jan 2011 11:17:47 +0000 (12:17 +0100)]
Changes
lib/DBIx/Class/Storage/DBI.pm
t/cdbi/DeepAbstractSearch/01_search.t
t/storage/base.t
t/storage/on_connect_call.t
t/storage/on_connect_do.t
t/storage/txn.t

diff --git a/Changes b/Changes
index 0449717..2340424 100644 (file)
--- 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
index 70dcb24..baa9ed6 100644 (file)
@@ -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 {
index 599cec1..cc08010 100644 (file)
@@ -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;
index 6b25576..2aac70c 100644 (file)
@@ -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 => [ {
index bea5085..265835c 100644 (file)
@@ -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;
index 31d39c6..2ce77b2 100644 (file)
@@ -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 ] ],
index c4ecefd..f84fa9a 100644 (file)
@@ -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');