Permalink
Browse files

Fixes #1645: Added support for nested DB transactions

  • Loading branch information...
qiangxue committed Feb 16, 2014
1 parent f6c0b4c commit bea9e3fc062be891b5254482fb54f3b2941f2592
@@ -210,7 +210,7 @@ $command->execute();
Transactions
------------
If the underlying DBMS supports transactions, you can perform transactional SQL queries like the following:
You can perform transactional SQL queries like the following:
```php
$transaction = $connection->beginTransaction();
@@ -220,10 +220,34 @@ try {
// ... executing other SQL statements ...
$transaction->commit();
} catch(Exception $e) {
$transaction->rollback();
$transaction->rollBack();
}
```
You can also nest multiple transactions, if needed:
```php
// outer transaction
$transaction1 = $connection->beginTransaction();
try {
$connection->createCommand($sql1)->execute();
// inner transaction
$transaction2 = $connection->beginTransaction();
try {
$connection->createCommand($sql2)->execute();
$transaction2->commit();
} catch (Exception $e) {
$transaction2->rollBack();
}
$transaction1->commit();
} catch (Exception $e) {
$transaction1->rollBack();
}
```
Working with database schema
----------------------------
@@ -372,12 +372,12 @@ public function insert($runValidation = true, $attributes = null)
try {
$result = $this->insertInternal($attributes);
if ($result === false) {
$transaction->rollback();
$transaction->rollBack();
} else {
$transaction->commit();
}
} catch (\Exception $e) {
$transaction->rollback();
$transaction->rollBack();
throw $e;
}
} else {
@@ -473,12 +473,12 @@ public function update($runValidation = true, $attributes = null)
try {
$result = $this->updateInternal($attributes);
if ($result === false) {
$transaction->rollback();
$transaction->rollBack();
} else {
$transaction->commit();
}
} catch (\Exception $e) {
$transaction->rollback();
$transaction->rollBack();
throw $e;
}
} else {
@@ -589,14 +589,14 @@ public function delete()
}
if ($transaction !== null) {
if ($result === false) {
$transaction->rollback();
$transaction->rollBack();
} else {
$transaction->commit();
}
}
} catch (\Exception $e) {
if ($transaction !== null) {
$transaction->rollback();
$transaction->rollBack();
}
throw $e;
}
View
@@ -79,6 +79,7 @@ Yii Framework 2 Change Log
- Enh #1641: Added `BaseActiveRecord::updateAttributes()` (qiangxue)
- Enh #1646: Added postgresql `QueryBuilder::checkIntegrity` and `QueryBuilder::resetSequence` (Ragazzo)
- Enh #1645: Added `Connection::$pdoClass` property (Ragazzo)
- Enh #1645: Added support for nested DB transactions (qiangxue)
- Enh #1681: Added support for automatically adjusting the "for" attribute of label generated by `ActiveField::label()` (qiangxue)
- Enh #1706: Added support for registering a single JS/CSS file with dependency (qiangxue)
- Enh #1773: keyPrefix property of Cache is not restricted to alnum characters anymore, however it is still recommended (cebe)
@@ -337,12 +337,12 @@ public function insert($runValidation = true, $attributes = null)
try {
$result = $this->insertInternal($attributes);
if ($result === false) {
$transaction->rollback();
$transaction->rollBack();
} else {
$transaction->commit();
}
} catch (\Exception $e) {
$transaction->rollback();
$transaction->rollBack();
throw $e;
}
} else {
@@ -449,12 +449,12 @@ public function update($runValidation = true, $attributes = null)
try {
$result = $this->updateInternal($attributes);
if ($result === false) {
$transaction->rollback();
$transaction->rollBack();
} else {
$transaction->commit();
}
} catch (\Exception $e) {
$transaction->rollback();
$transaction->rollBack();
throw $e;
}
} else {
@@ -505,14 +505,14 @@ public function delete()
}
if ($transaction !== null) {
if ($result === false) {
$transaction->rollback();
$transaction->rollBack();
} else {
$transaction->commit();
}
}
} catch (\Exception $e) {
if ($transaction !== null) {
$transaction->rollback();
$transaction->rollBack();
}
throw $e;
}
@@ -68,7 +68,7 @@
* // ... executing other SQL statements ...
* $transaction->commit();
* } catch(Exception $e) {
* $transaction->rollback();
* $transaction->rollBack();
* }
* ~~~
*
@@ -396,7 +396,7 @@ public function createCommand($sql = null, $params = [])
*/
public function getTransaction()
{
return $this->_transaction && $this->_transaction->isActive ? $this->_transaction : null;
return $this->_transaction && $this->_transaction->getIsActive() ? $this->_transaction : null;
}
/**
@@ -406,9 +406,12 @@ public function getTransaction()
public function beginTransaction()
{
$this->open();
$this->_transaction = new Transaction(['db' => $this]);
$this->_transaction->begin();
return $this->_transaction;
if (($transaction = $this->getTransaction()) === null) {
$transaction = $this->_transaction = new Transaction(['db' => $this]);
}
$transaction->begin();
return $transaction;
}
/**
@@ -64,14 +64,14 @@ public function up()
$transaction = $this->db->beginTransaction();
try {
if ($this->safeUp() === false) {
$transaction->rollback();
$transaction->rollBack();
return false;
}
$transaction->commit();
} catch (\Exception $e) {
echo "Exception: " . $e->getMessage() . ' (' . $e->getFile() . ':' . $e->getLine() . ")\n";
echo $e->getTraceAsString() . "\n";
$transaction->rollback();
$transaction->rollBack();
return false;
}
return null;
@@ -89,14 +89,14 @@ public function down()
$transaction = $this->db->beginTransaction();
try {
if ($this->safeDown() === false) {
$transaction->rollback();
$transaction->rollBack();
return false;
}
$transaction->commit();
} catch (\Exception $e) {
echo "Exception: " . $e->getMessage() . ' (' . $e->getFile() . ':' . $e->getLine() . ")\n";
echo $e->getTraceAsString() . "\n";
$transaction->rollback();
$transaction->rollBack();
return false;
}
return null;
View
@@ -289,6 +289,41 @@ public function getLastInsertID($sequenceName = '')
}
}
/**
* @return boolean whether this DBMS supports [savepoint](http://en.wikipedia.org/wiki/Savepoint).
*/
public function supportsSavepoint()
{
return true;
}
/**
* Creates a new savepoint.
* @param string $name the savepoint name
*/
public function createSavepoint($name)
{
$this->db->createCommand("SAVEPOINT $name")->execute();
}
/**
* Releases an existing savepoint.
* @param string $name the savepoint name
*/
public function releaseSavepoint($name)
{
$this->db->createCommand("RELEASE SAVEPOINT $name")->execute();
}
/**
* Rolls back to a previously created savepoint.
* @param string $name the savepoint name
*/
public function rollBackSavepoint($name)
{
$this->db->createCommand("ROLLBACK TO SAVEPOINT $name")->execute();
}
/**
* Quotes a string value for use in a query.
* Note that if the parameter is not a string, it will be returned without change.
Oops, something went wrong.

20 comments on commit bea9e3f

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Feb 16, 2014

Contributor

I dont think this is good to force only savepoints as solution. I think we should not modify Transaction class, but add TransactionPdoType class that can be set in Connection::pdoClass.

Contributor

Ragazzo replied Feb 16, 2014

I dont think this is good to force only savepoints as solution. I think we should not modify Transaction class, but add TransactionPdoType class that can be set in Connection::pdoClass.

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Feb 16, 2014

Contributor

Related with issue - #1645

Contributor

Ragazzo replied Feb 16, 2014

Related with issue - #1645

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Feb 16, 2014

Member

Why do you need TransactionPdo? Based on our latest testing framework, we don't really need to use transactions for DB testing.

Member

qiangxue replied Feb 16, 2014

Why do you need TransactionPdo? Based on our latest testing framework, we don't really need to use transactions for DB testing.

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Feb 16, 2014

Contributor

Why do you need TransactionPdo?

i dont need it as subclass of transaction as you maybe thought, i am for design, you simple hardcoded savepoints, while some other developers may want to avoid using them (had some bad exp.) and simply use 1 transaciton for all, you can do it with this. So my suggestion is that we simply add 2 classes for pdo that can be configured via Connection::$pdoClass as needed.
Examples here in this issue.

Contributor

Ragazzo replied Feb 16, 2014

Why do you need TransactionPdo?

i dont need it as subclass of transaction as you maybe thought, i am for design, you simple hardcoded savepoints, while some other developers may want to avoid using them (had some bad exp.) and simply use 1 transaciton for all, you can do it with this. So my suggestion is that we simply add 2 classes for pdo that can be configured via Connection::$pdoClass as needed.
Examples here in this issue.

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Feb 16, 2014

Member

It's not just two classes. Potentially you need one for each DBMS, and you will also have trouble if you already have a customized PDO class.

If you don't want to use savepoint, simply override Schema::supportsSavepoint(). This achieves the same effect as you want with TransactionPdo.

Member

qiangxue replied Feb 16, 2014

It's not just two classes. Potentially you need one for each DBMS, and you will also have trouble if you already have a customized PDO class.

If you don't want to use savepoint, simply override Schema::supportsSavepoint(). This achieves the same effect as you want with TransactionPdo.

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Feb 16, 2014

Contributor

Also i dont think that this is a good design since it affects schemas that is not correct, because it only about PDO class as you can see in examples above.

Contributor

Ragazzo replied Feb 16, 2014

Also i dont think that this is a good design since it affects schemas that is not correct, because it only about PDO class as you can see in examples above.

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Feb 16, 2014

Contributor

Potentially you need one for each DBMS

solution with NestedPdo class file works well for common rdbms. Anyway that is better rather than having unneded and not supported by RDBMS features code in core i think) And we give user ability to choose while here he just need to face that he has this methods in schemas and thats all.

If you don't want to use savepoint, simply override Schema::supportsSavepoint()

so i need to create extra files per each RDBMS? Same drawback as you mentioned.

Contributor

Ragazzo replied Feb 16, 2014

Potentially you need one for each DBMS

solution with NestedPdo class file works well for common rdbms. Anyway that is better rather than having unneded and not supported by RDBMS features code in core i think) And we give user ability to choose while here he just need to face that he has this methods in schemas and thats all.

If you don't want to use savepoint, simply override Schema::supportsSavepoint()

so i need to create extra files per each RDBMS? Same drawback as you mentioned.

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Feb 16, 2014

Member

solution with NestedPdo class file works well for common rdbms.

Nope, that's not enough. Different DBMS have different syntaxes. You will end up dealing with different syntaxes in the same class, an ugly design.

so i need to create extra files per each RDBMS?

We can make supportsSavepoint a property of Schema, but I don't know if we really need it. I would like to see a convincing argument for it first.

...because it only about PDO class as you can see in examples above.

Schema is the class that serves for DB driver/adapter purpose (perhaps the name is a bit misleading). Putting these code in PDO would require a customized PDO for every DBMS, which ultimately makes no difference except more unnecessary classes.

Member

qiangxue replied Feb 16, 2014

solution with NestedPdo class file works well for common rdbms.

Nope, that's not enough. Different DBMS have different syntaxes. You will end up dealing with different syntaxes in the same class, an ugly design.

so i need to create extra files per each RDBMS?

We can make supportsSavepoint a property of Schema, but I don't know if we really need it. I would like to see a convincing argument for it first.

...because it only about PDO class as you can see in examples above.

Schema is the class that serves for DB driver/adapter purpose (perhaps the name is a bit misleading). Putting these code in PDO would require a customized PDO for every DBMS, which ultimately makes no difference except more unnecessary classes.

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Feb 16, 2014

Contributor

You will end up dealing with different syntaxes in the same class, an ugly design.

i will create class per RDBMS if needed, simply extracting methods from schemas in your solution to files. And overall i will have less methods since more RDBMS support this feature in common way, than you have now in all schemas.

Putting these code in PDO would require a customized PDO for every DBMS, which ultimately makes no difference except more unnecessary classes.

we will just catch everything in one point.

As for testing than maybe we should adjust fixtures to rollback? But some code as truncate/drop/create can lead to transaction autocommit.

Contributor

Ragazzo replied Feb 16, 2014

You will end up dealing with different syntaxes in the same class, an ugly design.

i will create class per RDBMS if needed, simply extracting methods from schemas in your solution to files. And overall i will have less methods since more RDBMS support this feature in common way, than you have now in all schemas.

Putting these code in PDO would require a customized PDO for every DBMS, which ultimately makes no difference except more unnecessary classes.

we will just catch everything in one point.

As for testing than maybe we should adjust fixtures to rollback? But some code as truncate/drop/create can lead to transaction autocommit.

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Feb 16, 2014

Member

i will create class per RDBMS if needed...than you have now in all schemas

That's not true. mysql/pgsql/sqlite doesn't have the code in their Schema class since they share the same base class implementation.

And as I said, your suggested approach will meet trouble if a DBMS has a customized PDO (e.g. mssql). You will not be able to swap it with the TransactionPdo unless you create a similar subclass specifically for mssql.

we will just catch everything in one point.

Why do we want to do this?

As for testing than maybe we should adjust fixtures to rollback? But some code as truncate/drop/create can lead to transaction autocommit.

Why? The only reason I can think of is improving testing speed, but it is associated with side effects. I think it's better to let the user to do that (how to do that, btw? how to catch exception that is thrown in a different method?)

Member

qiangxue replied Feb 16, 2014

i will create class per RDBMS if needed...than you have now in all schemas

That's not true. mysql/pgsql/sqlite doesn't have the code in their Schema class since they share the same base class implementation.

And as I said, your suggested approach will meet trouble if a DBMS has a customized PDO (e.g. mssql). You will not be able to swap it with the TransactionPdo unless you create a similar subclass specifically for mssql.

we will just catch everything in one point.

Why do we want to do this?

As for testing than maybe we should adjust fixtures to rollback? But some code as truncate/drop/create can lead to transaction autocommit.

Why? The only reason I can think of is improving testing speed, but it is associated with side effects. I think it's better to let the user to do that (how to do that, btw? how to catch exception that is thrown in a different method?)

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Feb 16, 2014

Contributor

The only reason I can think of is improving testing speed

yes, thats the main question, unitl all will have i7 ))

how to catch exception that is thrown in a different method?

?

Contributor

Ragazzo replied Feb 16, 2014

The only reason I can think of is improving testing speed

yes, thats the main question, unitl all will have i7 ))

how to catch exception that is thrown in a different method?

?

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Feb 16, 2014

Member

I added Connection::enableSavepoint, which should allow turning on/off savepoint easily.

As for testing than maybe we should adjust fixtures to rollback?

Any idea how to do this?

Member

qiangxue replied Feb 16, 2014

I added Connection::enableSavepoint, which should allow turning on/off savepoint easily.

As for testing than maybe we should adjust fixtures to rollback?

Any idea how to do this?

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Feb 16, 2014

Contributor

Any idea how to do this?

as i see it - simple begin/rollback transaction in setUp and tearDown of DbTestCase, but once again some RDBMS operations (TRUNCATE/CREATE/DROP etc) can lead to autocommit and we cant do anything with that. Maybe we can add DbTestCase::$transactional (if so, then need to add note in php-doc that some situations can trigger autocommit) property to let the user decide should everything be in transaction or not, but anyway not big addition, can be done by the user himself. What do you think?

Contributor

Ragazzo replied Feb 16, 2014

Any idea how to do this?

as i see it - simple begin/rollback transaction in setUp and tearDown of DbTestCase, but once again some RDBMS operations (TRUNCATE/CREATE/DROP etc) can lead to autocommit and we cant do anything with that. Maybe we can add DbTestCase::$transactional (if so, then need to add note in php-doc that some situations can trigger autocommit) property to let the user decide should everything be in transaction or not, but anyway not big addition, can be done by the user himself. What do you think?

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Feb 16, 2014

Member

Should the transaction be started before or after fixtures are loaded?

Member

qiangxue replied Feb 16, 2014

Should the transaction be started before or after fixtures are loaded?

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Feb 16, 2014

Contributor

before any data will be commited to database, or what is the sence to use it if not rollback data? or i missed something? Not sure also how unload should be done in active fixtures and other fixtures since we can not simply bypass it, but unneeded DELETE also is not good, but it will not take too much time since there is no data in table, because it was rollbacked.

Contributor

Ragazzo replied Feb 16, 2014

before any data will be commited to database, or what is the sence to use it if not rollback data? or i missed something? Not sure also how unload should be done in active fixtures and other fixtures since we can not simply bypass it, but unneeded DELETE also is not good, but it will not take too much time since there is no data in table, because it was rollbacked.

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Feb 16, 2014

Member

I think we should let the user to explicitly start transactions because it's possible the user is working with multiple DB connections whose name is not db. We do need to provide a way so that the user can insert code right after mockApplication() is called though.

Member

qiangxue replied Feb 16, 2014

I think we should let the user to explicitly start transactions because it's possible the user is working with multiple DB connections whose name is not db. We do need to provide a way so that the user can insert code right after mockApplication() is called though.

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Feb 16, 2014

Contributor

it's possible the user is working with multiple DB connections

yes, but trait can have properties and include it in class. For example breif implementation is:


namespace yii\test;

trait DbTransaction
{
     public $db = ['db', 'otherConnection1', 'otherConnection2'];

     public function beginTransaction()
     {
           //start transactions for all connections
     }

     public function rollbackTransaction()
     {
           //rollback transactions for all connections
     }

}

this trait also will be useful in codeception cest files they are like phpunit-classes with _before/_after methods, there user can use this trait to start and rollback, to avoid saving to database, for example when testing signup in yii2-advanced.

Contributor

Ragazzo replied Feb 16, 2014

it's possible the user is working with multiple DB connections

yes, but trait can have properties and include it in class. For example breif implementation is:


namespace yii\test;

trait DbTransaction
{
     public $db = ['db', 'otherConnection1', 'otherConnection2'];

     public function beginTransaction()
     {
           //start transactions for all connections
     }

     public function rollbackTransaction()
     {
           //rollback transactions for all connections
     }

}

this trait also will be useful in codeception cest files they are like phpunit-classes with _before/_after methods, there user can use this trait to start and rollback, to avoid saving to database, for example when testing signup in yii2-advanced.

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Feb 16, 2014

Member

With this trait, the user still needs to explicitly call these two methods at appropriate places, right?

Member

qiangxue replied Feb 16, 2014

With this trait, the user still needs to explicitly call these two methods at appropriate places, right?

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Feb 16, 2014

Contributor

yes, but it is like FixtureTrait. I know this trait is not too big, but it is handy shortcut and user dont need each time to copy-paste foreach. Maybe something more will be added to it later, so it is fine as trait. Examples of usage:

#yii2-codeception class file
class DbTestCase
{

    use DbTransaction;

    public $transactional = true;

    /**
     * @inheritdoc
     */
    protected function setUp()
    {
        if ($this->transactional) {
            $this->beginTransactions();
        }
        parent::setUp();
    }

    /**
     * @inheritdoc
     */
    protected function tearDown()
    {
        if ($this->transactional) {
            $this->rollback();
        }
        parent::tearDown();
    }
}

class MyDbTest extends DbTestCase
{
        public $db = ['custom1', 'custom2', 'custom3']; // in most cases it will be simply "db"
}

in cest file:

class SignupCest
{

      use DbTransaction;

      public function _before($e)
      {
              $this->beginTransaction();
      }

      public function _after($e)
      {
              $this->rollbackTransaction();
      }

      public function test_user_can_signup($I, $scenario)
      {
           //test signup here, save AR and other data
      }

}

if needed we also can provide base cest class in yii2-codeception extension so user will have ability to configure it (transactional) as DbTestCase if needed.

Contributor

Ragazzo replied Feb 16, 2014

yes, but it is like FixtureTrait. I know this trait is not too big, but it is handy shortcut and user dont need each time to copy-paste foreach. Maybe something more will be added to it later, so it is fine as trait. Examples of usage:

#yii2-codeception class file
class DbTestCase
{

    use DbTransaction;

    public $transactional = true;

    /**
     * @inheritdoc
     */
    protected function setUp()
    {
        if ($this->transactional) {
            $this->beginTransactions();
        }
        parent::setUp();
    }

    /**
     * @inheritdoc
     */
    protected function tearDown()
    {
        if ($this->transactional) {
            $this->rollback();
        }
        parent::tearDown();
    }
}

class MyDbTest extends DbTestCase
{
        public $db = ['custom1', 'custom2', 'custom3']; // in most cases it will be simply "db"
}

in cest file:

class SignupCest
{

      use DbTransaction;

      public function _before($e)
      {
              $this->beginTransaction();
      }

      public function _after($e)
      {
              $this->rollbackTransaction();
      }

      public function test_user_can_signup($I, $scenario)
      {
           //test signup here, save AR and other data
      }

}

if needed we also can provide base cest class in yii2-codeception extension so user will have ability to configure it (transactional) as DbTestCase if needed.

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Feb 16, 2014

Contributor

Or as for DbTestCase it can be better to move transaction start to InitDbFixture::beforeLoad as for integrity check is done, and transaction rollback in beforeUnload. Not sure about it though.

Contributor

Ragazzo replied Feb 16, 2014

Or as for DbTestCase it can be better to move transaction start to InitDbFixture::beforeLoad as for integrity check is done, and transaction rollback in beforeUnload. Not sure about it though.

Please sign in to comment.