Got rid of effectively dead dbh_do code in the txn_{begin|end|rollback} funcs
Brandon L. Black [Thu, 10 May 2007 23:46:23 +0000 (23:46 +0000)]
Reworked the AutoCommit/transaction_depth stuff so that transaction_depth is always 1 or higher with AutoCommit off
Doc updates to recommend AutoCommit => 1 / txn_do
Warn if the user doesn't explicitly set AutoCommit
Added AutoCommit => 1 to some tests that were triggering the above warning

lib/DBIx/Class/Storage/DBI.pm
t/19quotes_newstyle.t
t/94versioning.t
t/bindtype_columns.t
t/lib/DBICTest.pm
t/lib/DBICTest/Plain.pm

index 5cc795f..067a47a 100644 (file)
@@ -331,7 +331,12 @@ C<connect_info> here.
 
 The arrayref can either contain the same set of arguments one would
 normally pass to L<DBI/connect>, or a lone code reference which returns
-a connected database handle.
+a connected database handle.  Please note that the L<DBI> docs
+recommend that you always explicitly set C<AutoCommit> to either
+C<0> or C<1>.   L<DBIx::Class> further recommends that it be set
+to C<1>, and that you perform transactions via our L</txn_do>
+method.  L<DBIx::Class> will emit a warning if you fail to explicitly
+set C<AutoCommit> one way or the other.  See below for more details.
 
 In either case, if the final argument in your connect_info happens
 to be a hashref, C<connect_info> will look there for several
@@ -390,6 +395,21 @@ might not work very well, YMMV.  If you don't use a subref, DBIC will
 force this setting for you anyways.  Setting HandleError to anything
 other than simple exception object wrapper might cause problems too.
 
+Another Important Note:
+
+DBIC can do some wonderful magic with handling exceptions,
+disconnections, and transactions when you use C<AutoCommit =&gt; 1>
+combined with C<txn_do> for transaction support.
+
+If you set C<AutoCommit =&gt; 0> in your connect info, then you are always
+in an assumed transaction between commits, and you're telling us you'd
+like to manage that manually.  A lot of DBIC's magic protections
+go away.  We can't protect you from exceptions due to database
+disconnects because we don't know anything about how to restart your
+transactions.  You're on your own for handling all sorts of exceptional
+cases if you choose the C<AutoCommit =&gt 0> path, just as you would
+be with raw DBI.
+
 Examples:
 
   # Simple SQLite connection
@@ -404,7 +424,7 @@ Examples:
       'dbi:Pg:dbname=foo',
       'postgres',
       'my_pg_password',
-      { AutoCommit => 0 },
+      { AutoCommit => 1 },
       { quote_char => q{"}, name_sep => q{.} },
     ]
   );
@@ -415,7 +435,7 @@ Examples:
       'dbi:Pg:dbname=foo',
       'postgres',
       'my_pg_password',
-      { AutoCommit => 0, quote_char => q{"}, name_sep => q{.} },
+      { AutoCommit => 1, quote_char => q{"}, name_sep => q{.} },
     ]
   );
 
@@ -462,6 +482,18 @@ sub connect_info {
     pop(@$info) if !keys %$last_info;
   }
 
+  # Now check the (possibly new) final argument for AutoCommit,
+  #  but not in the coderef case, obviously.
+  if(ref $info->[0] ne 'CODE') {
+      $last_info = $info->[3];
+
+      warn "You *really* should explicitly set AutoCommit "
+         . "(preferably to 1) in your db connect info"
+           if !$last_info
+              || ref $last_info ne 'HASH'
+              || !defined $last_info->{AutoCommit};
+  }
+
   $self->_connect_info($info);
 }
 
@@ -688,6 +720,10 @@ sub _populate_dbh {
   my @info = @{$self->_connect_info || []};
   $self->_dbh($self->_connect(@info));
 
+  # Always set the transaction depth on connect, since
+  #  there is no transaction in progress by definition
+  $self->{transaction_depth} = $self->_dbh->{AutoCommit} ? 0 : 1;
+
   if(ref $self eq 'DBIx::Class::Storage::DBI') {
     my $driver = $self->_dbh->{Driver}->{Name};
     if ($self->load_optional_class("DBIx::Class::Storage::DBI::${driver}")) {
@@ -741,75 +777,61 @@ sub _connect {
   $dbh;
 }
 
-sub _dbh_txn_begin {
-  my ($self, $dbh) = @_;
-  if ($dbh->{AutoCommit}) {
-    $self->debugobj->txn_begin()
-      if ($self->debug);
-    $dbh->begin_work;
-  }
-}
 
 sub txn_begin {
   my $self = shift;
-  $self->dbh_do($self->can('_dbh_txn_begin'))
-    if $self->{transaction_depth}++ == 0;
-}
-
-sub _dbh_txn_commit {
-  my ($self, $dbh) = @_;
-  if ($self->{transaction_depth} == 0) {
-    unless ($dbh->{AutoCommit}) {
-      $self->debugobj->txn_commit()
-        if ($self->debug);
-      $dbh->commit;
-    }
-  }
-  else {
-    if (--$self->{transaction_depth} == 0) {
-      $self->debugobj->txn_commit()
-        if ($self->debug);
-      $dbh->commit;
-    }
+  if($self->{transaction_depth}++ == 0) {
+    $self->debugobj->txn_begin()
+      if $self->debug;
+    # this isn't ->_dbh-> because
+    #  we should reconnect on begin_work
+    #  for AutoCommit users
+    $self->dbh->begin_work;
   }
 }
 
 sub txn_commit {
   my $self = shift;
-  $self->dbh_do($self->can('_dbh_txn_commit'));
+  if ($self->{transaction_depth} == 1) {
+    my $dbh = $self->_dbh;
+    $self->debugobj->txn_commit()
+      if ($self->debug);
+    $dbh->commit;
+    $self->{transaction_depth} = 0
+      if $dbh->{AutoCommit};
+  }
+  elsif($self->{transaction_depth} > 1) {
+    $self->{transaction_depth}--
+  }
 }
 
-sub _dbh_txn_rollback {
-  my ($self, $dbh) = @_;
-  if ($self->{transaction_depth} == 0) {
-    unless ($dbh->{AutoCommit}) {
+sub txn_rollback {
+  my $self = shift;
+  my $dbh = $self->_dbh;
+  my $autocommit;
+  eval {
+    $autocommit = $dbh->{AutoCommit};
+    if ($self->{transaction_depth} == 1) {
       $self->debugobj->txn_rollback()
         if ($self->debug);
       $dbh->rollback;
+      $self->{transaction_depth} = 0
+        if $autocommit;
     }
-  }
-  else {
-    if (--$self->{transaction_depth} == 0) {
-      $self->debugobj->txn_rollback()
-        if ($self->debug);
-      $dbh->rollback;
+    elsif($self->{transaction_depth} > 1) {
+      $self->{transaction_depth}--;
     }
     else {
       die DBIx::Class::Storage::NESTED_ROLLBACK_EXCEPTION->new;
     }
-  }
-}
-
-sub txn_rollback {
-  my $self = shift;
-
-  eval { $self->dbh_do($self->can('_dbh_txn_rollback')) };
+  };
   if ($@) {
     my $error = $@;
     my $exception_class = "DBIx::Class::Storage::NESTED_ROLLBACK_EXCEPTION";
     $error =~ /$exception_class/ and $self->throw_exception($error);
-    $self->{transaction_depth} = 0;          # ensure that a failed rollback
-    $self->throw_exception($error);          # resets the transaction depth
+    # ensure that a failed rollback resets the transaction depth
+    $self->{transaction_depth} = $autocommit ? 0 : 1;
+    $self->throw_exception($error);
   }
 }
 
index b9d7411..02c1450 100644 (file)
@@ -22,7 +22,13 @@ my $orig_debug = $schema->storage->debug;
 diag('Testing against ' . join(' ', map { $schema->storage->dbh->get_info($_) } qw/17 18/));
 
 my $dsn = $schema->storage->connect_info->[0];
-$schema->connection($dsn, { quote_char => '`', name_sep => '.' });
+$schema->connection(
+  $dsn,
+  undef,
+  undef,
+  { AutoCommit => 1 },
+  { quote_char => '`', name_sep => '.' },
+);
 
 my $sql = '';
 $schema->storage->debugcb(sub { $sql = $_[1] });
@@ -47,7 +53,12 @@ $rs = $schema->resultset('CD')->search({},
 eval { $rs->first };
 like($sql, qr/ORDER BY \Q${order}\E/, 'did not quote ORDER BY with scalarref');
 
-$schema->connection($dsn, { quote_char => [qw/[ ]/], name_sep => '.' });
+$schema->connection(
+  $dsn,
+  undef,
+  undef,
+  { AutoCommit => 1, quote_char => [qw/[ ]/], name_sep => '.' }
+);
 $schema->storage->debugcb(sub { $sql = $_[1] });
 $schema->storage->debug(1);
 
@@ -62,7 +73,12 @@ my %data = (
        order => '12'
 );
 
-$schema->connection($dsn, { quote_char => '`', name_sep => '.' });
+$schema->connection(
+  $dsn,
+  undef,
+  undef,
+  { AutoCommit => 1, quote_char => '`', name_sep => '.' }
+);
 
 is($schema->storage->sql_maker->update('group', \%data), 'UPDATE `group` SET `name` = ?, `order` = ?', 'quoted table names for UPDATE');
 
index 66ea346..52bf415 100644 (file)
@@ -21,7 +21,12 @@ unlink($db_file . "-journal") if -e $db_file . "-journal";
 mkdir("t/var") unless -d "t/var";
 unlink('t/var/DBICVersion-Schema-1.0-SQLite.sql');
 
-my $schema_orig = DBICVersion::Schema->connect("dbi:SQLite:$db_file");
+my $schema_orig = DBICVersion::Schema->connect(
+  "dbi:SQLite:$db_file",
+  undef,
+  undef,
+  { AutoCommit => 1 },
+);
 # $schema->storage->ensure_connected();
 
 is($schema_orig->ddl_filename('SQLite', 't/var', '1.0'), File::Spec->catfile('t', 'var', 'DBICVersion-Schema-1.0-SQLite.sql'), 'Filename creation working');
@@ -35,7 +40,12 @@ my $tvrs = $schema_orig->resultset('Table');
 is($schema_orig->exists($tvrs), 1, 'Created schema from DDL file');
 
 eval "use DBICVersionNew";
-my $schema_new = DBICVersion::Schema->connect("dbi:SQLite:$db_file");
+my $schema_new = DBICVersion::Schema->connect(
+  "dbi:SQLite:$db_file",
+  undef,
+  undef,
+  { AutoCommit => 1 },
+);
 
 unlink('t/var/DBICVersion-Schema-2.0-SQLite.sql');
 unlink('t/var/DBICVersion-Schema-1.0-2.0-SQLite.sql');
@@ -43,7 +53,12 @@ $schema_new->create_ddl_dir('SQLite', undef, 't/var', '1.0');
 ok(-f 't/var/DBICVersion-Schema-1.0-2.0-SQLite.sql', 'Created DDL upgrade file');
 
 ## create new to pick up filedata for upgrade files we just made (on_connect)
-my $schema_upgrade = DBICVersion::Schema->connect("dbi:SQLite:$db_file");
+my $schema_upgrade = DBICVersion::Schema->connect(
+  "dbi:SQLite:$db_file",
+  undef,
+  undef,
+  { AutoCommit => 1 },
+);
 
 ## do this here or let Versioned.pm do it?
 $schema_upgrade->upgrade();
index 9d0ad92..5b83255 100644 (file)
@@ -12,7 +12,7 @@ plan skip_all => 'Set $ENV{DBICTEST_PG_DSN}, _USER and _PASS to run this test'
   
 plan tests => 3;
 
-my $schema = DBICTest::Schema->connection($dsn, $dbuser, $dbpass);
+my $schema = DBICTest::Schema->connection($dsn, $dbuser, $dbpass, { AutoCommit => 1 });
 
 my $dbh = $schema->storage->dbh;
 
index 27c8549..8f1521c 100755 (executable)
@@ -60,7 +60,7 @@ sub init_schema {
                            : 'compose_namespace');
 
     my $schema = DBICTest::Schema->$compose_method('DBICTest')
-                                 ->connect($dsn, $dbuser, $dbpass);
+                     ->connect($dsn, $dbuser, $dbpass, { AutoCommit => 1 });
     $schema->storage->on_connect_do(['PRAGMA synchronous = OFF']);
     if ( !$args{no_deploy} ) {
         __PACKAGE__->deploy_schema( $schema );
index 03a1976..209cc3e 100644 (file)
@@ -15,7 +15,13 @@ mkdir("t/var") unless -d "t/var";
 my $dsn = "dbi:SQLite:${db_file}";
 
 __PACKAGE__->load_classes("Test");
-my $schema = __PACKAGE__->compose_connection(__PACKAGE__, $dsn);
+my $schema = __PACKAGE__->compose_connection(
+  __PACKAGE__,
+  $dsn,
+  undef,
+  undef,
+  { AutoCommit => 1 }
+);
 
 my $dbh = DBI->connect($dsn);