Altered PdoConnection to allow nested transaction. #5

JReissmueller · 2017-01-17T00:28:35Z

Created/altered tests to account for this.

Fixes #4

Created/altered tests to account for this. Fixes phillipsdata#4

clphillips

I don't like this for a number of reasons:

It doesn't allow nested transactions. It simply silently ignores when begin() is called multiple times, then does nothing when commit() is called, unless you call it an equal number of times.
As mentioned above, calling commit() does nothing in some cases. That's unexpected behavior, and a sinkhole for bugs.
This does nothing to support real nested transactions on systems that support nested transactions, like innoDB. Nor can is solve the issue of implicit transactions (i.e. DROP TABLE) within a nested call set.
Nested transaction calls can easily be avoided using a service layer. Controller calls Service, Service calls begin/commit/rollback around the models to perform a particular action.

clphillips · 2017-01-17T04:37:49Z

src/PdoConnection.php

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


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.

$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.

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?

clphillips · 2017-01-17T04:42:31Z

src/PdoConnection.php

+            return $this->connect()->beginTransaction();
+        }
+
+        return ($this->transactionCount >= 0);


superfluous parens.

clphillips · 2017-01-17T04:46:33Z

src/PdoConnection.php

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


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.

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.

clphillips · 2017-01-17T04:48:11Z

src/PdoConnection.php

+            }
+
+            $this->rollbackNeeded = false;
+            $this->connect()->rollback();


??? Rolling back within a commit has got to be some kind of crossing-the-streams kind of bad.

Yea, it feels a bit weird, but if one of the inner transactions fails, then we must not commit the outer transaction. We return false to indicate that this has happened.

clphillips · 2017-01-17T04:49:10Z

src/PdoConnection.php

@@ -248,7 +268,17 @@ public function rollBack()
     */
    public function commit()
    {
-        return $this->connect()->commit();
+        if (--$this->transactionCount === 0) {
+            if (!$this->rollbackNeeded) {


(╯°□°)╯︵ ┻━┻

clphillips · 2017-01-17T05:12:32Z

src/PdoConnection.php

@@ -248,7 +268,17 @@ public function rollBack()
     */
    public function commit()
    {
-        return $this->connect()->commit();
+        if (--$this->transactionCount === 0) {


Again, suppose this is called before begin()

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

clphillips · 2017-01-17T05:14:55Z

src/PdoConnection.php

+            return false;
+        }
+
+        return ($this->transactionCount >= 0 && !$this->rollbackNeeded);


return return return 😭

imo, the only time it makes sense to return something other than false is if you return $this->connect()->commit().

The return is meant to signal whether an error occurred. In the case were we have not reached the outer transaction, and none of the inner transactions have tried to rollback, we return true for success.

clphillips · 2017-01-17T05:18:30Z

tests/Unit/PdoConnectionTest.php

+        $this->connection->setConnection($this->mockConnection('beginTransaction', true));
+        foreach ($actions as $action) {
+            $this->assertTrue($this->connection->begin());
+        }


probably ought to add 'begin' to the list of actions in the data provider.

clphillips · 2017-01-17T05:19:55Z

tests/Unit/PdoConnectionTest.php

+
+        end($actions);
+        $end_index = key($actions);
+        reset($actions);


(╯°□°)╯︵ ┻━┻

$end_index = count($actions) - 1;

does PSR2 allow underscore in variable names? I thought they had to be camelCase...

clphillips · 2017-01-17T05:29:13Z

tests/Unit/PdoConnectionTest.php

+            } else {
+                $this->connection->{$action}();
+            }
+        }


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); }

JReissmueller · 2017-01-17T16:57:54Z

So I could fix a lot of the things you commented on, but it looks like you disagree with this change as a concept. It did seem that you were not quite understanding the intention for some of the code though so I'll try and clear that up.

You were totally correct in saying that this does not actually implement nested transactions, but is purely meant to ignore them so that one function that is currently using a transaction does not need to worry about calling another one that also uses a transaction. To that end it is designed to, as you said, only begin on the first begin, and commit on the last commit.

The rollbackNeeded function will only be true when one of the "nested" transactions tries to rollback its changes, In this case, we do not want to rollback immediately, because we would then end our transaction before reaching the end of the encompassing transaction. Still, we do need to acknowledge that because the inner transaction failed, so should the outer. So in the event that the outer succeeds and tries to commit, I decided to rollback and return false based on "rollbackNeeded".

In response to your conceptual objections:

You are totally correct.
You can tell when a commit failed by the return value. Not sleek, but it is predictable.
As you stated in (1), this is not meant to truly implement nested transactions, only remove the error of calling a transaction within another. As far as implicit commits, I'm not sure what that has to do with nested transactions in particular (unless you meant something different by "implicit transactions").
Not really sure what you mean here. Are you saying to implement this behavior, just in a separate layer?

Changed some formatting. Updated variable name. Restructured PdoConnection::commit and PdoConnectionTest::testNestedTransactions.

…s one of the actions given by the data provider and added test cases.

clphillips · 2017-01-19T05:07:36Z

I presume the reason for this PR is to silently ignore when begin() is called after already having been called, and commit() if more than 1 begin() has queued. I'm guessing the logic is something like:

class Model {
    public function saveThisThing() {
        $db->begin();
        // stuff
        $this->saveThatThing();
        $db->commit();
    }

    public function saveThatThing() {
        $db->begin();
        // stuff
        $db->commit();
    }

To solve this on a service layer you'd simply execute a single transaction per service call. It would look something like:

class ThisThingService {
    public function createThisThing(Model $model) {
        $db->begin();
        $model->saveThisThing();
        $model->saveThatThing();
        $db->commit();
    }
}

class Model {
    public function saveThisThing() {
        // stuff
    }

    public function saveThatThing() {
        // stuff
    }
}

If you needed to saveThisThing() without saveThatThing() somewhere else, you'd just add another method to your ThisThingService. Much more flexible.

Basically what I'm getting at is that nested transactions are a lot like buffered queries. If you need them, you're doing something wrong.

Altered PdoConnection to allow nested transaction.

c30a4ee

Created/altered tests to account for this. Fixes phillipsdata#4

clphillips requested changes Jan 17, 2017

View reviewed changes

Temporary added 3 commits January 17, 2017 10:06

Added/removed/updated comments.

6ef6c0a

Changed some formatting. Updated variable name. Restructured PdoConnection::commit and PdoConnectionTest::testNestedTransactions.

Removed comments.

0e622e4

Updated PdoConnectionTest::testNestedConnections to include 'begin' a…

e70746e

…s one of the actions given by the data provider and added test cases.

clphillips requested a review from tysonphillips January 19, 2017 05:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Altered PdoConnection to allow nested transaction. #5

Altered PdoConnection to allow nested transaction. #5

JReissmueller commented Jan 17, 2017

clphillips left a comment

clphillips Jan 17, 2017

JReissmueller Jan 17, 2017

clphillips Jan 19, 2017 •

edited

Loading

clphillips Jan 17, 2017

clphillips Jan 17, 2017

JReissmueller Jan 17, 2017

clphillips Jan 17, 2017

JReissmueller Jan 17, 2017

clphillips Jan 17, 2017

clphillips Jan 17, 2017

JReissmueller Jan 17, 2017

clphillips Jan 17, 2017

JReissmueller Jan 17, 2017

clphillips Jan 17, 2017

clphillips Jan 17, 2017

clphillips Jan 17, 2017

clphillips Jan 17, 2017

JReissmueller commented Jan 17, 2017 •

edited

Loading

clphillips commented Jan 19, 2017

Altered PdoConnection to allow nested transaction. #5

Are you sure you want to change the base?

Altered PdoConnection to allow nested transaction. #5

Conversation

JReissmueller commented Jan 17, 2017

clphillips left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clphillips Jan 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JReissmueller commented Jan 17, 2017 • edited Loading

clphillips commented Jan 19, 2017

clphillips Jan 19, 2017 •

edited

Loading

JReissmueller commented Jan 17, 2017 •

edited

Loading