From: Peter Rabbitson <ribasushi@cpan.org>
Date: Tue, 27 May 2014 11:15:45 +0000 (+0200)
Subject: Fix multiple savepointing transactions on DBD::SQLite
X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=398215b1;p=dbsrgits%2FDBIx-Class-Historic.git

Fix multiple savepointing transactions on DBD::SQLite

Problem was missed during review of 86a51471ce (how the fuck did I let this
abomination through anyway), and not encountered due to insufficient testing.
The naive statement parser in DBD::SQLite when running against older libsqlite
mistakenly treats ROLLBACK TRANSACTION TO... as an actual TXN rollback, and
as a result desyncs the internal AutoCommit flag state [1]
Fix by simply using the shorter (still valid) syntax [2], and by removing the
sloppy workaround hiding the actual problem.

[1] https://github.com/DBD-SQLite/DBD-SQLite/blob/1.42/dbdimp.c#L824:L852
[2] http://www.sqlite.org/lang_savepoint.html
---

diff --git a/Changes b/Changes
index e363637..bbd5815 100644
--- a/Changes
+++ b/Changes
@@ -11,6 +11,8 @@ Revision history for DBIx::Class
           up by create() and populate()
         - Ensure definitive condition extractor handles bizarre corner cases
           without bombing out (RT#93244)
+        - Fix inability to handle multiple consecutive transactions with
+          savepoints on DBD::SQLite < 1.39
 
 0.08270 2014-01-30 21:54 (PST)
     * Fixes
diff --git a/lib/DBIx/Class/Storage.pm b/lib/DBIx/Class/Storage.pm
index 1addeaf..ad1770e 100644
--- a/lib/DBIx/Class/Storage.pm
+++ b/lib/DBIx/Class/Storage.pm
@@ -227,6 +227,7 @@ sub txn_commit {
     $self->debugobj->txn_commit() if $self->debug;
     $self->_exec_txn_commit;
     $self->{transaction_depth}--;
+    $self->savepoints([]);
   }
   elsif($self->transaction_depth > 1) {
     $self->{transaction_depth}--;
@@ -252,6 +253,7 @@ sub txn_rollback {
     $self->debugobj->txn_rollback() if $self->debug;
     $self->_exec_txn_rollback;
     $self->{transaction_depth}--;
+    $self->savepoints([]);
   }
   elsif ($self->transaction_depth > 1) {
     $self->{transaction_depth}--;
diff --git a/lib/DBIx/Class/Storage/DBI/SQLite.pm b/lib/DBIx/Class/Storage/DBI/SQLite.pm
index 2e4e312..2778dbd 100644
--- a/lib/DBIx/Class/Storage/DBI/SQLite.pm
+++ b/lib/DBIx/Class/Storage/DBI/SQLite.pm
@@ -126,11 +126,23 @@ sub _exec_svp_release {
 sub _exec_svp_rollback {
   my ($self, $name) = @_;
 
-  # For some reason this statement changes the value of $dbh->{AutoCommit}, so
-  # we localize it here to preserve the original value.
-  local $self->_dbh->{AutoCommit} = $self->_dbh->{AutoCommit};
+  $self->_dbh->do("ROLLBACK TO SAVEPOINT $name");
+}
+
+# older SQLite has issues here too - both of these are in fact
+# completely benign warnings (or at least so say the tests)
+sub _exec_txn_rollback {
+  local $SIG{__WARN__} = sigwarn_silencer( qr/rollback ineffective/ )
+    unless $DBD::SQLite::__DBIC_TXN_SYNC_SANE__;
 
-  $self->_dbh->do("ROLLBACK TRANSACTION TO SAVEPOINT $name");
+  shift->next::method(@_);
+}
+
+sub _exec_txn_commit {
+  local $SIG{__WARN__} = sigwarn_silencer( qr/commit ineffective/ )
+    unless $DBD::SQLite::__DBIC_TXN_SYNC_SANE__;
+
+  shift->next::method(@_);
 }
 
 sub _ping {
diff --git a/t/752sqlite.t b/t/752sqlite.t
index 70bd612..273a1ed 100644
--- a/t/752sqlite.t
+++ b/t/752sqlite.t
@@ -125,6 +125,46 @@ DDL
   }
 }
 
+# test blank begin/svp/commit/begin cycle
+warnings_are {
+  my $schema = DBICTest->init_schema( no_populate => 1 );
+  my $rs = $schema->resultset('Artist');
+  is ($rs->count, 0, 'Start with empty table');
+
+  for my $do_commit (1, 0) {
+    $schema->txn_begin;
+    $schema->svp_begin;
+    $schema->svp_rollback;
+
+    $schema->svp_begin;
+    $schema->svp_rollback;
+
+    $schema->svp_release;
+
+    $schema->svp_begin;
+
+    $schema->txn_rollback;
+
+    $schema->txn_begin;
+    $schema->svp_begin;
+    $schema->svp_rollback;
+
+    $schema->svp_begin;
+    $schema->svp_rollback;
+
+    $schema->svp_release;
+
+    $schema->svp_begin;
+
+    $do_commit ? $schema->txn_commit : $schema->txn_rollback;
+
+    is_deeply $schema->storage->savepoints, [], 'Savepoint names cleared away'
+  }
+
+  $schema->txn_do(sub {
+    ok (1, 'all seems fine');
+  });
+} [], 'No warnings emitted';
 
 my $schema = DBICTest->init_schema();