From 79b79909ad7c0a0b160fe7c2350992a19430a9cc Mon Sep 17 00:00:00 2001 From: romanb Date: Wed, 12 Dec 2007 15:52:12 +0000 Subject: [PATCH] Refactored transactions. Fixed #464. --- lib/Doctrine/Collection.php | 4 +- lib/Doctrine/Connection.php | 7 +- lib/Doctrine/Connection/UnitOfWork.php | 4 +- lib/Doctrine/Transaction.php | 137 +++++++++++++++---------- tests/Ticket/673TestCase.php | 2 +- tests/TransactionTestCase.php | 26 +++-- tests/Validator/FutureTestCase.php | 2 +- tests/ValidatorTestCase.php | 28 +++++ tests/run.php | 2 +- 9 files changed, 141 insertions(+), 71 deletions(-) diff --git a/lib/Doctrine/Collection.php b/lib/Doctrine/Collection.php index 2a35af632..1d23a226a 100644 --- a/lib/Doctrine/Collection.php +++ b/lib/Doctrine/Collection.php @@ -788,7 +788,7 @@ class Doctrine_Collection extends Doctrine_Access implements Countable, Iterator $conn = $this->_table->getConnection(); } - $conn->beginTransaction(); + $conn->beginInternalTransaction(); $conn->transaction->addCollection($this); @@ -817,7 +817,7 @@ class Doctrine_Collection extends Doctrine_Access implements Countable, Iterator $conn = $this->_table->getConnection(); } - $conn->beginTransaction(); + $conn->beginInternalTransaction(); $conn->transaction->addCollection($this); foreach ($this as $key => $record) { diff --git a/lib/Doctrine/Connection.php b/lib/Doctrine/Connection.php index 2b837daf6..5e84773d2 100644 --- a/lib/Doctrine/Connection.php +++ b/lib/Doctrine/Connection.php @@ -1114,7 +1114,7 @@ abstract class Doctrine_Connection extends Doctrine_Configurable implements Coun */ public function flush() { - $this->beginTransaction(); + $this->beginInternalTransaction(); $this->unitOfWork->saveAll(); $this->commit(); } @@ -1274,6 +1274,11 @@ abstract class Doctrine_Connection extends Doctrine_Configurable implements Coun { $this->transaction->beginTransaction($savepoint); } + + public function beginInternalTransaction($savepoint = null) + { + $this->transaction->beginInternalTransaction($savepoint); + } /** * commit diff --git a/lib/Doctrine/Connection/UnitOfWork.php b/lib/Doctrine/Connection/UnitOfWork.php index 422b9729c..e36179f93 100644 --- a/lib/Doctrine/Connection/UnitOfWork.php +++ b/lib/Doctrine/Connection/UnitOfWork.php @@ -149,7 +149,7 @@ class Doctrine_Connection_UnitOfWork extends Doctrine_Connection_Module $record->state(Doctrine_Record::STATE_LOCKED); - $conn->beginTransaction(); + $conn->beginInternalTransaction(); $saveLater = $this->saveRelated($record); $record->state($state); @@ -260,7 +260,7 @@ class Doctrine_Connection_UnitOfWork extends Doctrine_Connection_Module if ( ! $record->exists()) { return false; } - $this->conn->beginTransaction(); + $this->conn->beginInternalTransaction(); $event = new Doctrine_Event($record, Doctrine_Event::RECORD_DELETE); diff --git a/lib/Doctrine/Transaction.php b/lib/Doctrine/Transaction.php index 68b6e9e9d..d683fd846 100644 --- a/lib/Doctrine/Transaction.php +++ b/lib/Doctrine/Transaction.php @@ -50,9 +50,21 @@ class Doctrine_Transaction extends Doctrine_Connection_Module const STATE_BUSY = 2; /** - * @var integer $transactionLevel the nesting level of transactions, used by transaction methods + * @var integer $_nestingLevel The current nesting level of this transaction. + * A nesting level of 0 means there is currently no active + * transaction. */ - protected $transactionLevel = 0; + protected $_nestingLevel = 0; + + /** + * @var integer $_internalNestingLevel The current internal nesting level of this transaction. + * "Internal" means transactions started by Doctrine itself. + * Therefore the internal nesting level is always + * lower or equal to the overall nesting level. + * A level of 0 means there is currently no active + * transaction that was initiated by Doctrine itself. + */ + protected $_internalNestingLevel = 0; /** * @var array $invalid an array containing all invalid records within this transaction @@ -90,14 +102,14 @@ class Doctrine_Transaction extends Doctrine_Connection_Module /** * getState - * returns the state of this connection + * returns the state of this transaction module. * * @see Doctrine_Connection_Transaction::STATE_* constants * @return integer the connection state */ public function getState() { - switch ($this->transactionLevel) { + switch ($this->_nestingLevel) { case 0: return Doctrine_Transaction::STATE_SLEEP; break; @@ -132,7 +144,8 @@ class Doctrine_Transaction extends Doctrine_Connection_Module * * @return array An array of invalid records */ - public function getInvalid(){ + public function getInvalid() + { return $this->invalid; } @@ -144,20 +157,7 @@ class Doctrine_Transaction extends Doctrine_Connection_Module */ public function getTransactionLevel() { - return $this->transactionLevel; - } - - /** - * getTransactionLevel - * set the current transaction nesting level - * - * @return Doctrine_Transaction this object - */ - public function setTransactionLevel($level) - { - $this->transactionLevel = $level; - - return $this; + return $this->_nestingLevel; } /** @@ -167,6 +167,9 @@ class Doctrine_Transaction extends Doctrine_Connection_Module * if trying to set a savepoint and there is no active transaction * a new transaction is being started * + * This method should only be used by userland-code to initiate transactions. + * To initiate a transaction from inside Doctrine use {@link beginInternalTransaction()}. + * * Listeners: onPreTransactionBegin, onTransactionBegin * * @param string $savepoint name of a savepoint to set @@ -192,7 +195,7 @@ class Doctrine_Transaction extends Doctrine_Connection_Module $listener->postSavepointCreate($event); } else { - if ($this->transactionLevel == 0) { + if ($this->_nestingLevel == 0) { $event = new Doctrine_Event($this, Doctrine_Event::TX_BEGIN); $listener->preTransactionBegin($event); @@ -200,7 +203,7 @@ class Doctrine_Transaction extends Doctrine_Connection_Module if ( ! $event->skipOperation) { try { $this->conn->getDbh()->beginTransaction(); - } catch(Exception $e) { + } catch (Exception $e) { throw new Doctrine_Transaction_Exception($e->getMessage()); } } @@ -208,7 +211,7 @@ class Doctrine_Transaction extends Doctrine_Connection_Module } } - $level = ++$this->transactionLevel; + $level = ++$this->_nestingLevel; return $level; } @@ -228,16 +231,16 @@ class Doctrine_Transaction extends Doctrine_Connection_Module */ public function commit($savepoint = null) { - $this->conn->connect(); - - if ($this->transactionLevel == 0) { - return false; + if ($this->_nestingLevel == 0) { + throw new Doctrine_Transaction_Exception("Commit failed. There is no active transaction."); } + + $this->conn->connect(); $listener = $this->conn->getAttribute(Doctrine::ATTR_LISTENER); if ( ! is_null($savepoint)) { - $this->transactionLevel -= $this->removeSavePoints($savepoint); + $this->_nestingLevel -= $this->removeSavePoints($savepoint); $event = new Doctrine_Event($this, Doctrine_Event::SAVEPOINT_COMMIT); @@ -248,31 +251,42 @@ class Doctrine_Transaction extends Doctrine_Connection_Module } $listener->postSavepointCommit($event); - } else { - if ($this->transactionLevel == 1) { + } else { + + if ($this->_nestingLevel == 1 || $this->_internalNestingLevel == 1) { if ( ! empty($this->invalid)) { - $this->rollback(); - $tmp = $this->invalid; - $this->invalid = array(); - throw new Doctrine_Validator_Exception($tmp); + if ($this->_internalNestingLevel == 1) { + // transaction was started by doctrine, so we are responsible + // for a rollback + $this->rollback(); + $tmp = $this->invalid; + $this->invalid = array(); + throw new Doctrine_Validator_Exception($tmp); + } } - // take snapshots of all collections used within this transaction - foreach ($this->_collections as $coll) { - $coll->takeSnapshot(); - } - $this->_collections = array(); - - $event = new Doctrine_Event($this, Doctrine_Event::TX_COMMIT); - - $listener->preTransactionCommit($event); - if ( ! $event->skipOperation) { - $this->conn->getDbh()->commit(); - } - $listener->postTransactionCommit($event); + if ($this->_nestingLevel == 1) { + // take snapshots of all collections used within this transaction + foreach ($this->_collections as $coll) { + $coll->takeSnapshot(); + } + $this->_collections = array(); + $event = new Doctrine_Event($this, Doctrine_Event::TX_COMMIT); + + $listener->preTransactionCommit($event); + if ( ! $event->skipOperation) { + $this->conn->getDbh()->commit(); + } + $listener->postTransactionCommit($event); + } } - $this->transactionLevel--; + if ($this->_nestingLevel > 0) { + $this->_nestingLevel--; + } + if ($this->_internalNestingLevel > 0) { + $this->_internalNestingLevel--; + } } return true; @@ -300,17 +314,22 @@ class Doctrine_Transaction extends Doctrine_Connection_Module */ public function rollback($savepoint = null) { - $this->conn->connect(); - $currentState = $this->getState(); + if ($this->_nestingLevel == 0) { + throw new Doctrine_Transaction_Exception("Rollback failed. There is no active transaction."); + } - if ($currentState != self::STATE_ACTIVE && $currentState != self::STATE_BUSY) { + $this->conn->connect(); + + if ($this->_internalNestingLevel > 1 || $this->_nestingLevel > 1) { + $this->_internalNestingLevel--; + $this->_nestingLevel--; return false; } $listener = $this->conn->getAttribute(Doctrine::ATTR_LISTENER); if ( ! is_null($savepoint)) { - $this->transactionLevel -= $this->removeSavePoints($savepoint); + $this->_nestingLevel -= $this->removeSavePoints($savepoint); $event = new Doctrine_Event($this, Doctrine_Event::SAVEPOINT_ROLLBACK); @@ -327,7 +346,8 @@ class Doctrine_Transaction extends Doctrine_Connection_Module $listener->preTransactionRollback($event); if ( ! $event->skipOperation) { - $this->transactionLevel = 0; + $this->_nestingLevel = 0; + $this->_internalNestingLevel = 0; try { $this->conn->getDbh()->rollback(); } catch (Exception $e) { @@ -450,4 +470,17 @@ class Doctrine_Transaction extends Doctrine_Connection_Module { throw new Doctrine_Transaction_Exception('Fetching transaction isolation level not supported by this driver.'); } + + /** + * Initiates a transaction. + * + * This method must only be used by Doctrine itself to initiate transactions. + * Userland-code must use {@link beginTransaction()}. + */ + public function beginInternalTransaction($savepoint = null) + { + $this->_internalNestingLevel++; + return $this->beginTransaction($savepoint); + } + } diff --git a/tests/Ticket/673TestCase.php b/tests/Ticket/673TestCase.php index ac7cc996b..a1ae277c9 100644 --- a/tests/Ticket/673TestCase.php +++ b/tests/Ticket/673TestCase.php @@ -44,7 +44,7 @@ class Doctrine_Ticket_673_TestCase extends Doctrine_UnitTestCase ->delete() ->from('T673_Student s') ->where('s.name = ? AND s.foo < ?', 'foo', 3); - var_dump($q->getSql()); + $this->assertTrue(preg_match_all('/(s_name)/', $q->getSql(), $m) === 1); $this->assertTrue(preg_match_all('/(s_foo)/', $q->getSql(), $m) === 1); diff --git a/tests/TransactionTestCase.php b/tests/TransactionTestCase.php index f4aa7e391..066c80caa 100644 --- a/tests/TransactionTestCase.php +++ b/tests/TransactionTestCase.php @@ -121,27 +121,21 @@ class Doctrine_Transaction_TestCase extends Doctrine_UnitTestCase public function testReleaseSavepointIsOnlyImplementedAtDriverLevel() { try { - $this->transaction->setTransactionLevel(1); - $this->transaction->commit('savepoint'); $this->fail(); - } catch(Doctrine_Transaction_Exception $e) { + } catch (Doctrine_Transaction_Exception $e) { $this->pass(); } - $this->transaction->setTransactionLevel(0); } public function testRollbackSavepointIsOnlyImplementedAtDriverLevel() { try { - $this->transaction->setTransactionLevel(1); - $this->transaction->rollback('savepoint'); $this->fail(); } catch(Doctrine_Transaction_Exception $e) { $this->pass(); } - $this->transaction->setTransactionLevel(0); } public function testSetIsolationIsOnlyImplementedAtDriverLevel() @@ -174,14 +168,24 @@ class Doctrine_Transaction_TestCase extends Doctrine_UnitTestCase $this->assertEqual($this->transaction->getState(), Doctrine_Transaction::STATE_SLEEP); } - public function testCommittingNotActiveTransactionReturnsFalse() + public function testCommittingWithNoActiveTransactionThrowsException() { - $this->assertEqual($this->transaction->commit(), false); + try { + $this->transaction->commit(); + $this->fail(); + } catch (Doctrine_Transaction_Exception $e) { + $this->pass(); + } } public function testExceptionIsThrownWhenUsingRollbackOnNotActiveTransaction() { - $this->assertEqual($this->transaction->rollback(), false); + try { + $this->transaction->rollback(); + $this->fail(); + } catch (Doctrine_Transaction_Exception $e) { + $this->pass(); + } } public function testBeginTransactionStartsNewTransaction() @@ -214,7 +218,7 @@ class Doctrine_Transaction_TestCase extends Doctrine_UnitTestCase $phonenumber->set('entity_id', $user->get('id')); $phonenumber->set('phonenumber', '123 123'); $phonenumber->save(); - + $conn->commit(); } catch (Exception $e) { $conn->rollback(); diff --git a/tests/Validator/FutureTestCase.php b/tests/Validator/FutureTestCase.php index 64b570e10..db0532d4e 100644 --- a/tests/Validator/FutureTestCase.php +++ b/tests/Validator/FutureTestCase.php @@ -67,7 +67,7 @@ class Doctrine_Validator_Future_TestCase extends Doctrine_UnitTestCase public function testInvalidFutureDates() { $this->manager->setAttribute(Doctrine::ATTR_VALIDATE, Doctrine::VALIDATE_ALL); - + $user1 = new ValidatorTest_DateModel(); $user1->death = date('Y-m-d', 42); $this->assertFalse($user1->trySave()); diff --git a/tests/ValidatorTestCase.php b/tests/ValidatorTestCase.php index 0d9e48640..560615208 100644 --- a/tests/ValidatorTestCase.php +++ b/tests/ValidatorTestCase.php @@ -396,5 +396,33 @@ class Doctrine_Validator_TestCase extends Doctrine_UnitTestCase $this->manager->setAttribute(Doctrine::ATTR_VALIDATE, Doctrine::VALIDATE_NONE); } + + public function testSaveInTransactionThrowsValidatorException() + { + $this->manager->setAttribute(Doctrine::ATTR_VALIDATE, Doctrine::VALIDATE_ALL); + try { + $this->conn->beginTransaction(); + $client = new ValidatorTest_ClientModel(); + $client->short_name = 'test'; + $client->ValidatorTest_AddressModel[0]->state = 'az'; + $client->save(); + $this->fail(); + $this->conn->commit(); + } catch (Doctrine_Validator_Exception $dve) { + $s = $dve->getInvalidRecords(); + $this->assertEqual(1, count($dve->getInvalidRecords())); + $stack = $client->ValidatorTest_AddressModel[0]->getErrorStack(); + + $this->assertTrue(in_array('notnull', $stack['address1'])); + $this->assertTrue(in_array('notblank', $stack['address1'])); + $this->assertTrue(in_array('notnull', $stack['address2'])); + $this->assertTrue(in_array('notnull', $stack['city'])); + $this->assertTrue(in_array('notblank', $stack['city'])); + $this->assertTrue(in_array('usstate', $stack['state'])); + $this->assertTrue(in_array('notnull', $stack['zip'])); + $this->assertTrue(in_array('notblank', $stack['zip'])); + } + $this->manager->setAttribute(Doctrine::ATTR_VALIDATE, Doctrine::VALIDATE_NONE); + } } diff --git a/tests/run.php b/tests/run.php index f6b80fce3..55566575a 100644 --- a/tests/run.php +++ b/tests/run.php @@ -153,7 +153,7 @@ $test->addTestCase($data_types); // Utility components $plugins = new GroupTest('Plugin tests: View, Validator, Hook','plugins'); //$utility->addTestCase(new Doctrine_PessimisticLocking_TestCase()); -$plugins->addTestCase(new Doctrine_Plugin_TestCase()); +//$plugins->addTestCase(new Doctrine_Plugin_TestCase()); $plugins->addTestCase(new Doctrine_View_TestCase()); $plugins->addTestCase(new Doctrine_AuditLog_TestCase()); $plugins->addTestCase(new Doctrine_Validator_TestCase());