From: Anders Nor Berle <berle@cpan.org>
Date: Tue, 25 Mar 2008 02:19:26 +0000 (+0000)
Subject: Various fun things.
X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=ddf66ced51e4cb9b888001bc926bc0b3c3af0f8b;p=dbsrgits%2FDBIx-Class-Historic.git

Various fun things.

DONE:
* We now maintain a stack for savepoints.
* Most savepoint tests are now in a separate test file.
* Arguments to svp_* methods are now optional.
* We throw exceptions instead of warn on many places.
* gphat++

TODO:
* Document all this.
---

diff --git a/lib/DBIx/Class/Storage.pm b/lib/DBIx/Class/Storage.pm
index 79064a4..1895ab4 100644
--- a/lib/DBIx/Class/Storage.pm
+++ b/lib/DBIx/Class/Storage.pm
@@ -294,6 +294,15 @@ Rollback to the savepoint of the specified name.
 
 sub svp_rollback { die "Virtual method!" }
 
+=head2 svp_generate_name
+
+Generates a name for the next savepoint.  Defaults to 'savepoint_$count',
+where count is the number of current savepoints + 1.
+
+=cut
+
+sub svp_generate_name { die "Virtual method!" }
+
 =for comment
 
 =head2 txn_scope_guard
diff --git a/lib/DBIx/Class/Storage/DBI.pm b/lib/DBIx/Class/Storage/DBI.pm
index cb55d20..bfdaece 100644
--- a/lib/DBIx/Class/Storage/DBI.pm
+++ b/lib/DBIx/Class/Storage/DBI.pm
@@ -15,7 +15,7 @@ __PACKAGE__->mk_group_accessors('simple' =>
     qw/_connect_info _dbi_connect_info _dbh _sql_maker _sql_maker_opts
        _conn_pid _conn_tid disable_sth_caching on_connect_do
        on_disconnect_do transaction_depth unsafe _dbh_autocommit
-       auto_savepoint/
+       auto_savepoint savepoints/
 );
 
 __PACKAGE__->cursor_class('DBIx::Class::Storage::DBI::Cursor');
@@ -328,6 +328,7 @@ sub new {
 
   $new->transaction_depth(0);
   $new->_sql_maker_opts({});
+  $new->{savepoints} = [];
   $new->{_in_dbh_do} = 0;
   $new->{_dbh_gen} = 0;
 
@@ -877,56 +878,87 @@ sub _connect {
 
 sub svp_begin {
   my ($self, $name) = @_;
- 
-  $self->throw_exception("You failed to provide a savepoint name!") if !$name;
 
-  if($self->{transaction_depth} == 0) {
-    warn("Can't use savepoints without a transaction.");
-    return 0;
-  }
+  $name = $self->_svp_generate_name
+    unless defined $name;
+
+  $self->throw_exception ("You can't use savepoints outside a transaction")
+    if $self->{transaction_depth} == 0;
+
+  $self->throw_exception ("Your Storage implementation doesn't support savepoints")
+    unless $self->can('_svp_begin');
+  
+  push @{ $self->{savepoints} }, $name;
 
-  if(!$self->can('_svp_begin')) {
-    warn("Your Storage implementation doesn't support savepoints!");
-    return 0;
-  }
   $self->debugobj->svp_begin($name) if $self->debug;
-  $self->_svp_begin($name);
+  
+  return $self->_svp_begin($name);
 }
 
 sub svp_release {
   my ($self, $name) = @_;
 
-  $self->throw_exception("You failed to provide a savepoint name!") if !$name;
+  $self->throw_exception ("You can't use savepoints outside a transaction")
+    if $self->{transaction_depth} == 0;
 
-  if($self->{transaction_depth} == 0) {
-    warn("Can't use savepoints without a transaction.");
-    return 0;
-  }
+  $self->throw_exception ("Your Storage implementation doesn't support savepoints")
+    unless $self->can('_svp_release');
+
+  if (defined $name) {
+    $self->throw_exception ("Savepoint '$name' does not exist")
+      unless grep { $_ eq $name } @{ $self->{savepoints} };
 
-  if(!$self->can('_svp_release')) {
-      warn("Your Storage implementation doesn't support savepoint releasing!");
-      return 0;
+    # Dig through the stack until we find the one we are releasing.  This keeps
+    # the stack up to date.
+    my $svp;
+
+    do { $svp = pop @{ $self->{savepoints} } } while $svp ne $name;
+  } else {
+    $name = pop @{ $self->{savepoints} };
   }
+
   $self->debugobj->svp_release($name) if $self->debug;
-  $self->_svp_release($name);
+
+  return $self->_svp_release($name);
 }
 
 sub svp_rollback {
   my ($self, $name) = @_;
 
-  $self->throw_exception("You failed to provide a savepoint name!") if !$name;
+  $self->throw_exception ("You can't use savepoints outside a transaction")
+    if $self->{transaction_depth} == 0;
 
-  if($self->{transaction_depth} == 0) {
-    warn("Can't use savepoints without a transaction.");
-    return 0;
-  }
+  $self->throw_exception ("Your Storage implementation doesn't support savepoints")
+    unless $self->can('_svp_rollback');
 
-  if(!$self->can('_svp_rollback')) {
-      warn("Your Storage implementation doesn't support savepoints!");
-      return 0;
+  if (defined $name) {
+      # If they passed us a name, verify that it exists in the stack
+      unless(grep({ $_ eq $name } @{ $self->{savepoints} })) {
+          $self->throw_exception("Savepoint '$name' does not exist!");
+      }
+
+      # Dig through the stack until we find the one we are releasing.  This keeps
+      # the stack up to date.
+      while(my $s = pop(@{ $self->{savepoints} })) {
+          last if($s eq $name);
+      }
+      # Add the savepoint back to the stack, as a rollback doesn't remove the
+      # named savepoint, only everything after it.
+      push(@{ $self->{savepoints} }, $name);
+  } else {
+      # We'll assume they want to rollback to the last savepoint
+      $name = $self->{savepoints}->[-1];
   }
+
   $self->debugobj->svp_rollback($name) if $self->debug;
-  $self->_svp_rollback($name);
+  
+  return $self->_svp_rollback($name);
+}
+
+sub _svp_generate_name {
+    my ($self) = @_;
+
+    return 'savepoint_'.scalar(@{ $self->{'savepoints'} });
 }
 
 sub txn_begin {
@@ -940,7 +972,7 @@ sub txn_begin {
     #  for AutoCommit users
     $self->dbh->begin_work;
   } elsif ($self->auto_savepoint) {
-    $self->svp_begin ("savepoint_$self->{transaction_depth}");
+    $self->svp_begin;
   }
   $self->{transaction_depth}++;
 }
@@ -957,7 +989,7 @@ sub txn_commit {
   }
   elsif($self->{transaction_depth} > 1) {
     $self->{transaction_depth}--;
-    $self->svp_release ("savepoint_$self->{transaction_depth}")
+    $self->svp_release
       if $self->auto_savepoint;
   }
 }
@@ -976,8 +1008,8 @@ sub txn_rollback {
     elsif($self->{transaction_depth} > 1) {
       $self->{transaction_depth}--;
       if ($self->auto_savepoint) {
-        $self->svp_rollback ("savepoint_$self->{transaction_depth}");
-        $self->svp_release ("savepoint_$self->{transaction_depth}");
+        $self->svp_rollback;
+        $self->svp_release;
       }
     }
     else {
diff --git a/t/71mysql.t b/t/71mysql.t
index 3d53820..8f8d1db 100644
--- a/t/71mysql.t
+++ b/t/71mysql.t
@@ -1,5 +1,5 @@
 use strict;
-use warnings;  
+use warnings;
 
 use Test::More;
 use lib qw(t/lib);
@@ -14,7 +14,7 @@ my ($dsn, $user, $pass) = @ENV{map { "DBICTEST_MYSQL_${_}" } qw/DSN USER PASS/};
 plan skip_all => 'Set $ENV{DBICTEST_MYSQL_DSN}, _USER and _PASS to run this test'
   unless ($dsn && $user);
 
-plan tests => 9;
+plan tests => 7;
 
 my $schema = DBICTest::Schema->connect($dsn, $user, $pass);
 
@@ -72,31 +72,27 @@ my $test_type_info = {
     },
 };
 
-$schema->txn_begin();
+$schema->txn_begin;
 
 my $arty = $schema->resultset('Artist')->find(1);
 
-my $name = $arty->name();
+my $name = $arty->name;
 
-$schema->svp_begin('savepoint1');
-
-cmp_ok($stats->{'SVP_BEGIN'}, '==', 1, 'Statistics svp_begin tickled');
+$schema->storage->_svp_begin ("mysavepoint");
 
 $arty->update({ name => 'Jheephizzy' });
 
-$arty->discard_changes();
-
-cmp_ok($arty->name(), 'eq', 'Jheephizzy', 'Name changed');
+$arty->discard_changes;
 
-$schema->svp_rollback('savepoint1');
+cmp_ok($arty->name, 'eq', 'Jheephizzy', 'Name changed');
 
-cmp_ok($stats->{'SVP_ROLLBACK'}, '==', 1, 'Statistics svp_rollback tickled');
+$schema->storage->_svp_rollback ("mysavepoint");
 
-$arty->discard_changes();
+$arty->discard_changes;
 
-cmp_ok($arty->name(), 'eq', $name, 'Name rolled back');
+cmp_ok($arty->name, 'eq', $name, 'Name rolled back');
 
-$schema->txn_commit();
+$schema->txn_commit;
 
 SKIP: {
     my $mysql_version = $dbh->get_info( $GetInfoType{SQL_DBMS_VER} );
diff --git a/t/72pg.t b/t/72pg.t
index fcee899..512b7b9 100644
--- a/t/72pg.t
+++ b/t/72pg.t
@@ -28,7 +28,7 @@ my ($dsn, $user, $pass) = @ENV{map { "DBICTEST_PG_${_}" } qw/DSN USER PASS/};
 plan skip_all => 'Set $ENV{DBICTEST_PG_DSN}, _USER and _PASS to run this test'
  . ' (note: creates and drops tables named artist and casecheck!)' unless ($dsn && $user);
 
-plan tests => 43;
+plan tests => 34;
 
 DBICTest::Schema->load_classes( 'Casecheck' );
 my $schema = DBICTest::Schema->connect($dsn, $user, $pass, { auto_savepoint => 1});
@@ -200,73 +200,27 @@ SKIP: {
   is($st->pkid1, 55, "Oracle Auto-PK without trigger: First primary key set manually");
 }
 
-$schema->txn_begin();
+$schema->txn_begin;
 
 my $arty = $schema->resultset('Artist')->find(1);
 
-my $name = $arty->name();
+my $name = $arty->name;
 
-$schema->svp_begin('savepoint1');
-
-cmp_ok($stats->{'SVP_BEGIN'}, '==', 1, 'Statistics svp_begin tickled');
+$schema->storage->_svp_begin ("mysavepoint");
 
 $arty->update({ name => 'Jheephizzy' });
 
-$arty->discard_changes();
-
-cmp_ok($arty->name(), 'eq', 'Jheephizzy', 'Name changed');
-
-$schema->svp_rollback('savepoint1');
-
-cmp_ok($stats->{'SVP_ROLLBACK'}, '==', 1, 'Statistics svp_rollback tickled');
-
-$arty->discard_changes();
-
-cmp_ok($arty->name(), 'eq', $name, 'Name rolled back');
-
-$schema->txn_commit();
-
-$schema->txn_do (sub {
-    $schema->txn_do (sub {
-        $arty->name ('Muff');
-
-        $arty->update;
-      });
-
-    eval {
-      $schema->txn_do (sub {
-          $arty->name ('Moff');
-
-          $arty->update;
-
-          $arty->discard_changes;
-
-          is($arty->name,'Moff','Value updated in nested transaction');
-
-          $schema->storage->dbh->do ("GUARANTEED TO PHAIL");
-        });
-    };
-
-    ok ($@,'Nested transaction failed (good)');
-
-    $arty->discard_changes;
-
-    is($arty->name,'Muff','auto_savepoint rollback worked');
+$arty->discard_changes;
 
-    $arty->name ('Miff');
+cmp_ok($arty->name, 'eq', 'Jheephizzy', 'Name changed');
 
-    $arty->update;
-  });
+$schema->storage->_svp_rollback ("mysavepoint");
 
 $arty->discard_changes;
 
-is($arty->name,'Miff','auto_savepoint worked');
-
-cmp_ok($stats->{'SVP_BEGIN'},'==',3,'Correct number of savepoints created');
-
-cmp_ok($stats->{'SVP_RELEASE'},'==',2,'Correct number of savepoints released');
+cmp_ok($arty->name, 'eq', $name, 'Name rolled back');
 
-cmp_ok($stats->{'SVP_ROLLBACK'},'==',2,'Correct number of savepoint rollbacks');
+$schema->txn_commit;
 
 END {
     if($dbh) {
diff --git a/t/98savepoints.t b/t/98savepoints.t
new file mode 100644
index 0000000..8e85f20
--- /dev/null
+++ b/t/98savepoints.t
@@ -0,0 +1,155 @@
+use strict;
+use warnings;
+
+use Test::More;
+use lib qw(t/lib);
+use DBICTest;
+use DBICTest::Stats;
+
+my ($create_sql, $dsn, $user, $pass);
+
+if (exists $ENV{DBICTEST_PG_DSN}) {
+  ($dsn, $user, $pass) = @ENV{map { "DBICTEST_PG_${_}" } qw/DSN USER PASS/};
+
+  $create_sql = "CREATE TABLE artist (artistid serial PRIMARY KEY, name VARCHAR(100), charfield CHAR(10))";
+} elsif (exists $ENV{DBICTEST_MYSQL_DSN}) {
+  ($dsn, $user, $pass) = @ENV{map { "DBICTEST_MYSQL_${_}" } qw/DSN USER PASS/};
+
+  $create_sql = "CREATE TABLE artist (artistid INTEGER NOT NULL AUTO_INCREMENT PRIMARY KEY, name VARCHAR(255), charfield CHAR(10)) ENGINE=InnoDB";
+} else {
+  plan skip_all => 'Set DBICTEST_(PG|MYSQL)_DSN _USER and _PASS if you want to run savepoint tests';
+}
+
+plan tests => 16;
+
+my $schema = DBICTest::Schema->connect ($dsn,$user,$pass,{ auto_savepoint => 1 });
+
+my $stats = DBICTest::Stats->new;
+
+$schema->storage->debugobj($stats);
+
+$schema->storage->debug(1);
+
+$schema->storage->dbh->do ($create_sql);
+
+$schema->resultset('Artist')->create({ name => 'foo' });
+
+$schema->txn_begin;
+
+my $arty = $schema->resultset('Artist')->find(1);
+
+my $name = $arty->name;
+
+# First off, test a generated savepoint name
+$schema->svp_begin;
+
+cmp_ok($stats->{'SVP_BEGIN'}, '==', 1, 'Statistics svp_begin tickled');
+
+$arty->update({ name => 'Jheephizzy' });
+
+$arty->discard_changes;
+
+cmp_ok($arty->name, 'eq', 'Jheephizzy', 'Name changed');
+
+# Rollback the generated name
+# Active: 0
+$schema->svp_rollback;
+
+cmp_ok($stats->{'SVP_ROLLBACK'}, '==', 1, 'Statistics svp_rollback tickled');
+
+$arty->discard_changes;
+
+cmp_ok($arty->name, 'eq', $name, 'Name rolled back');
+
+$arty->update({ name => 'Jheephizzy'});
+
+# Active: 0 1
+$schema->svp_begin('testing1');
+
+$arty->update({ name => 'yourmom' });
+
+# Active: 0 1 2
+$schema->svp_begin('testing2');
+
+$arty->update({ name => 'gphat' });
+$arty->discard_changes;
+cmp_ok($arty->name, 'eq', 'gphat', 'name changed');
+# Active: 0 1 2
+# Rollback doesn't DESTROY the savepoint, it just rolls back to the value
+# at it's conception
+$schema->svp_rollback('testing2');
+$arty->discard_changes;
+cmp_ok($arty->name, 'eq', 'yourmom', 'testing2 reverted');
+
+# Active: 0 1 2 3
+$schema->svp_begin('testing3');
+$arty->update({ name => 'coryg' });
+# Active: 0 1 2 3 4
+$schema->svp_begin('testing4');
+$arty->update({ name => 'watson' });
+
+# Release 3, which implicitly releases 4
+# Active: 0 1 2
+$schema->svp_release('testing3');
+$arty->discard_changes;
+cmp_ok($arty->name, 'eq', 'watson', 'release left data');
+# This rolls back savepoint 2
+# Active: 0 1 2
+$schema->svp_rollback;
+$arty->discard_changes;
+cmp_ok($arty->name, 'eq', 'yourmom', 'rolled back to 2');
+
+# Rollback the original savepoint, taking us back to the beginning, implicitly
+# rolling back savepoint 1 and 2
+$schema->svp_rollback('savepoint_0');
+$arty->discard_changes;
+cmp_ok($arty->name, 'eq', 'foo', 'rolled back to start');
+
+$schema->txn_commit;
+
+# And now to see if txn_do will behave correctly
+
+$schema->txn_do (sub {
+    $schema->txn_do (sub {
+        $arty->name ('Muff');
+
+        $arty->update;
+      });
+
+    eval {
+      $schema->txn_do (sub {
+          $arty->name ('Moff');
+
+          $arty->update;
+
+          $arty->discard_changes;
+
+          is($arty->name,'Moff','Value updated in nested transaction');
+
+          $schema->storage->dbh->do ("GUARANTEED TO PHAIL");
+        });
+    };
+
+    ok ($@,'Nested transaction failed (good)');
+
+    $arty->discard_changes;
+
+    is($arty->name,'Muff','auto_savepoint rollback worked');
+
+    $arty->name ('Miff');
+
+    $arty->update;
+  });
+
+$arty->discard_changes;
+
+is($arty->name,'Miff','auto_savepoint worked');
+
+cmp_ok($stats->{'SVP_BEGIN'},'==',7,'Correct number of savepoints created');
+
+cmp_ok($stats->{'SVP_RELEASE'},'==',3,'Correct number of savepoints released');
+
+cmp_ok($stats->{'SVP_ROLLBACK'},'==',5,'Correct number of savepoint rollbacks');
+
+END { $schema->storage->dbh->do ("DROP TABLE artist") if defined $schema }
+