Skip to content

Altered PdoConnection to allow nested transaction. #5

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
JReissmueller wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Altered PdoConnection to allow nested transaction. #5

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c30a4ee
Altered PdoConnection to allow nested transaction.
Jan 17, 2017
6ef6c0a
Added/removed/updated comments.
Jan 17, 2017
0e622e4
Removed comments.
Jan 17, 2017
e70746e
Updated PdoConnectionTest::testNestedConnections to include 'begin' a…
Jan 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 42 additions & 7 deletions src/PdoConnection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,16 @@ class PdoConnection
*/
private $reuseConnection = true;

/**
* @var int Number of open transactions
*/
private $transactionCount = 0;

/**
* @var bool Flag that tells whether a rollback is needed
*/
private $rollbackNeeded = false;
Copy link
Contributor

@clphillips clphillips Jan 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have a flag that does that same thing as $transactionCount === 0? Also the naming is misleading. Just because a transaction has been processed doesn't mean it needs to be rolled back.

Copy link
Member Author

@JReissmueller JReissmueller Jan 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$transactionCount represents how many transactions are currently open. $rollbackNeeded is only true when a rollback is called on a transaction within another transaction, which is very different from $transactionCount === 0. I means that if the outer transaction tries to commit, we need to rollback instead.

Copy link
Contributor

@clphillips clphillips Jan 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performing a rollback within a commit is not a good design choice, imo. If you can't commit, or you can't rollback you probably need to throw an exception because that's definitely programmer error.

Also, if this is just a queue system, why not use a queue?


/**
* Creates a new Model object that establishes a new PDO connection using
* the given database info, or the default configured info set in the database
Expand Down Expand Up @@ -228,27 +238,52 @@ public function prepare($sql, $fetchMode = null)
*/
public function begin()
{
return $this->connect()->beginTransaction();
if (!$this->transactionCount++) {
return $this->connect()->beginTransaction();
}

return $this->transactionCount >= 0;
}

/**
* Rolls back and closes the transaction
* Rolls back and closes the transaction or mark this transaction set for rollback
*
* @return boolean True if the transaction was successfully rolled back and closed, false otherwise
* @return boolean True if this is the outermost transaction and it was successfully rolled
* back and closed, false otherwise
*/
public function rollBack()
{
return $this->connect()->rollBack();
// Rollback if this is the outermost transaction, otherwise mark the set as erroring
if (--$this->transactionCount === 0) {
Copy link
Contributor

@clphillips clphillips Jan 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have you considered what happens if you call rollBack() before calling begin()? 😃

Copy link
Member Author

@JReissmueller JReissmueller Jan 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, relatively bad ju-ju-magumbo. Counter will go negative and rollback needed will be set to false. If a corresponding number of begins are called afterward, that the counter will return to 0 and the only consequence will be that rollbackNeeded will be set to true. Which is actually pretty bad. Like I mentioned in my other comment, this seems like a pretty obvious dev error.

$this->rollbackNeeded = false;
return $this->connect()->rollBack();
}

$this->rollbackNeeded = true;
Copy link
Contributor

@clphillips clphillips Jan 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, flag not needed, but since it's here, kinda weird to set something to true when it must already be true to reach this line.

Copy link
Member Author

@JReissmueller JReissmueller Jan 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The flag is false by default and is only set to true when a rollback is called on an inner transaction. It doesn't depend on the current value of rollbackNeeded at all.

return false;
}

/**
* Commits a transaction
* Commits a transaction or rolls back an erroring transaction set
*
* @return boolean True if the transaction was successfully commited and closed, false otherwise
* @return boolean True if the transaction was successfully commited, closed, and this is the outermost
* transaction or this is a non-erroring inner transaction, false otherwise
*/
public function commit()
{
return $this->connect()->commit();
$return = null;
if (--$this->transactionCount === 0) {
Copy link
Contributor

@clphillips clphillips Jan 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, suppose this is called before begin()

Copy link
Member Author

@JReissmueller JReissmueller Jan 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could try to make it more robust for handling cases like these, but that seems like a pretty clear dev error.

// Rollback if this transaction set is erroring, commit otherwise
if ($this->rollbackNeeded) {
$this->rollbackNeeded = false;
$this->connect()->rollback();
$return = false;
} else {
$return = $this->connect()->commit();
}
}

return (isset($return) ? $return : $this->transactionCount >= 0 && !$this->rollbackNeeded);
}

/**
Expand Down
68 changes: 68 additions & 0 deletions tests/Unit/PdoConnectionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,27 +134,95 @@ public function testBegin()
/**
* @covers ::rollBack
* @covers ::__construct
* @covers ::begin
* @covers ::getConnection
* @covers ::setConnection
* @covers ::connect
*/
public function testRollBack()
{
$this->connection->setConnection($this->mockConnection('rollBack', true));
$this->connection->begin();
$this->assertTrue($this->connection->rollBack());

$this->assertFalse($this->connection->rollBack());
Copy link
Contributor

@clphillips clphillips Jan 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment is unnecessary. The assert explains what's expected

}

/**
* @covers ::commit
* @covers ::__construct
* @covers ::begin
* @covers ::getConnection
* @covers ::setConnection
* @covers ::connect
*/
public function testCommit()
{
$this->connection->setConnection($this->mockConnection('commit', true));
$this->connection->begin();
$this->assertTrue($this->connection->commit());

$this->assertFalse($this->connection->commit());
}

/**
* Passes a list of data sets to pass to testNestedTransacitons
*
* @return array A list of data sets to pass to testNestedTransacitons
*/
public function nestedTransactionData()
Copy link
Contributor

@clphillips clphillips Jan 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing doc comments

{
return array(
array(array('begin', 'begin', 'begin', 'commit', 'commit', 'commit'), true),
array(array('begin', 'begin', 'begin', 'rollback', 'rollback', 'rollback'), true),
array(array('begin', 'begin', 'begin', 'commit', 'commit', 'rollback'), true),
array(array('begin', 'begin', 'begin', 'rollback', 'commit', 'rollback'), true),
array(array('commit', 'begin', 'begin', 'commit'), true),
array(array('rollback', 'begin', 'begin', 'rollback'), true),
array(array('rollback', 'rollback', 'begin', 'begin'), true),
array(array('commit', 'commit', 'begin', 'begin'), true),
array(array('begin', 'begin', 'rollback', 'commit'), false),
array(array('begin', 'begin', 'begin', 'commit', 'rollback', 'commit'), false),
array(array('rollback', 'begin', 'begin', 'commit'), false),
array(array('begin', 'commit', 'commit'), false),
array(array('begin', 'rollback', 'rollback'), false)
);
}

/**
* @covers ::__construct
* @covers ::begin
* @covers ::commit
* @covers ::rollback
* @covers ::connect
* @covers ::getConnection
* @covers ::setConnection
* @dataProvider nestedTransactionData
*
* @param array $actions
* @param bool $return
*/
public function testNestedTransactions(array $actions, $return)
{
$transactions = 0;
$end = count($actions) - 1;
foreach ($actions as $index => $action) {
if ($action == 'begin') {
if ($transactions++ === 0) {
$this->connection->setConnection($this->mockConnection('beginTransaction', true));
}
} else {
$transactions--;
if ($index === $end && $return) {
$this->connection->setConnection($this->mockConnection($action, true));
}
}

$actual = $this->connection->{$action}();
if ($index === $end) {
$this->assertEquals($return, $actual);
}
}
Copy link
Contributor

@clphillips clphillips Jan 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be written

$end = count($actions) -1;
foreach ($actions as $index => $action) {
    if ($index === $end) {
        $this->connection->setConnection($this->mockConnection($action, $expected);
    }
    $actual = $this->connection->{$action}();
    $index === $end && $this->assertEquals($expected, $actual);
}

}

/**
Expand Down