From: Brandon L. Black Date: Thu, 10 May 2007 23:46:23 +0000 (+0000) Subject: Got rid of effectively dead dbh_do code in the txn_{begin|end|rollback} funcs X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=77d76d0f2fe3f253eea0fb2b4b1f0f604d130d9e;p=dbsrgits%2FDBIx-Class-Historic.git Got rid of effectively dead dbh_do code in the txn_{begin|end|rollback} funcs 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 --- diff --git a/lib/DBIx/Class/Storage/DBI.pm b/lib/DBIx/Class/Storage/DBI.pm index 5cc795f..067a47a 100644 --- a/lib/DBIx/Class/Storage/DBI.pm +++ b/lib/DBIx/Class/Storage/DBI.pm @@ -331,7 +331,12 @@ C here. The arrayref can either contain the same set of arguments one would normally pass to L, or a lone code reference which returns -a connected database handle. +a connected database handle. Please note that the L docs +recommend that you always explicitly set C to either +C<0> or C<1>. L further recommends that it be set +to C<1>, and that you perform transactions via our L +method. L will emit a warning if you fail to explicitly +set C 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 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 +combined with C for transaction support. + +If you set C 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 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); } } diff --git a/t/19quotes_newstyle.t b/t/19quotes_newstyle.t index b9d7411..02c1450 100644 --- a/t/19quotes_newstyle.t +++ b/t/19quotes_newstyle.t @@ -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'); diff --git a/t/94versioning.t b/t/94versioning.t index 66ea346..52bf415 100644 --- a/t/94versioning.t +++ b/t/94versioning.t @@ -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(); diff --git a/t/bindtype_columns.t b/t/bindtype_columns.t index 9d0ad92..5b83255 100644 --- a/t/bindtype_columns.t +++ b/t/bindtype_columns.t @@ -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; diff --git a/t/lib/DBICTest.pm b/t/lib/DBICTest.pm index 27c8549..8f1521c 100755 --- a/t/lib/DBICTest.pm +++ b/t/lib/DBICTest.pm @@ -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 ); diff --git a/t/lib/DBICTest/Plain.pm b/t/lib/DBICTest/Plain.pm index 03a1976..209cc3e 100644 --- a/t/lib/DBICTest/Plain.pm +++ b/t/lib/DBICTest/Plain.pm @@ -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);