Transactions #3
Transactions #3
Conversation
…ployment replica sets or sharded clusters
transaction support create/insert,update,delete,etc operation
Supports infinite-level nested transactions, but outside transaction rollbacks do not affect the commit of inside transactions
```
DB::beginTransaction();
DB::collection('users')->where('name', 'klinson')->update(['age' => 18]);
DB::transaction(function () {
DB::collection('users')->where('name', 'mongodb')->update(['age' => 30]);
});
DB::rollBack();
```
Co-Authored-By: Stas <[email protected]>
# Conflicts: # .travis.yml # README.md
# Conflicts: # tests/TestCase.php
levon80999
requested review from
jmikola and
alcaeus
and removed request for
jmikola and
alcaeus
|
|
||
| echo setup.sh time now: `date +"%T" ` | ||
| mongo --host ${mongodb1}:${port} <<EOF | ||
| var cfg = { |
There was a problem hiding this comment.
It doesn't look like transactionLifetimeLimitSeconds was ever added to the configuration. I remember the three of us spoke about that in a meeting sometime after I left this comment, and @alcaeus agreed that it should be added.
Additionally, this entire mongosh command should be reduced to rs.initiate(), which is much more concise. If the replica set configuration is ever modified down the line, we can always change this later -- although it'd probably be easier to use rs.add() instead of writing a verbose configuration like we have here.
src/Concerns/TransactionManager.php
Outdated
| { | ||
| /** | ||
| * A list of transaction session. | ||
| * @var Session|null |
There was a problem hiding this comment.
This annotation seems redundant in light of the property type below.
src/Concerns/TransactionManager.php
Outdated
| */ | ||
| public function getSession(): ?Session | ||
| { | ||
| return $this->session ?? null; |
There was a problem hiding this comment.
Is ?? null necessary? The property declaration already agrees with this method's return type.
There was a problem hiding this comment.
In this case, it is, as the typed property is not initialised (see https://3v4l.org/ct02o). This can be avoided by initialising the property with a null value in the declaration (see https://3v4l.org/NkjmT).
src/Concerns/TransactionManager.php
Outdated
| * To use transactions on MongoDB 4.2 deployments(replica sets and sharded clusters), clients must use MongoDB drivers updated for MongoDB 4.2. | ||
| * | ||
| * @see https://docs.mongodb.com/manual/core/transactions/ | ||
| * @return void |
There was a problem hiding this comment.
The @param and @return annotations here are redundant. My original point was that if PHP syntax can communicate the necessary information there is no reason to preserve doc block comments.
In @alcaeus' recent PHPLIB PRs where he added type hints, he removed doc block annotations unless they communicated something that couldn't be expressed the allowed PHP syntax (e.g. some union types).
| $this->session = $session; | ||
| } | ||
|
|
||
| $callbackFunction = function (Session $session) use ($callback, &$attemptsLeft, &$callbackResult) { |
There was a problem hiding this comment.
This comment should be added to the source code, as it's helpful context for anyone that comes across this method definition in the future.
src/Concerns/TransactionManager.php
Outdated
| $this->throwExceptionIfTransactionDoesNotStart(); | ||
| $session = $this->getSession(); | ||
|
|
||
| $session?->abortTransaction(); |
There was a problem hiding this comment.
Note: I'm assuming the decision to not NOP in these methods when there is no active session is intentional. I recall we spoke about this during the last meeting but I forgot what the conclusion was. I think the original PR (or perhaps Eloquent itself) did NOP and maybe we talked about not wanting to preserve that behavior.
What is the point of using the null-safe operator if you've already verified that the session exists with throwExceptionIfTransactionDoesNotStart()? I could imagine this being necessary to satisfy code analysis tools since they might not be able to infer that throwExceptionIfTransactionDoesNotStart() guaranteed that getSession() would never return null; however, that's not relevant here.
In light of my later comments on throwExceptionIfTransactionDoesNotStart (suggesting it be renamed and reduced in complexity), I think what you really need here is a variant of getSession() that returns Session instead of ?Session. Perhaps a private method named getSessionOrThrow(). That could then reduce this method to a single statement:
$this->getSessionOrThrow()->abortTransaction();This suggestion also applies to other methods where you're using ?-> after throwExceptionIfTransactionDoesNotStart().
There was a problem hiding this comment.
What is the point of using the null-safe operator if you've already verified that the session exists with
throwExceptionIfTransactionDoesNotStart()? I could imagine this being necessary to satisfy code analysis tools since they might not be able to infer thatthrowExceptionIfTransactionDoesNotStart()guaranteed thatgetSession()would never returnnull; however, that's not relevant here.
Not that it is relevant here, but Psalm is able to handle this via the @psalm-assert annotation. See the first example in the docs. Annotating the throwIfNoSessionStarted like this will tell psalm that $this->session is an instance of Session:
/** @psalm-assert Session $this->session */
protected function throwIfNoSessionStarted(): void
[...]
src/Connection.php
Outdated
| @@ -277,7 +281,7 @@ protected function getDefaultQueryGrammar() | |||
| /** | |||
| * @inheritdoc | |||
| */ | |||
| protected function getDefaultSchemaGrammar() | |||
| protected function getDefaultSchemaGrammar(): Schema\Grammar | |||
There was a problem hiding this comment.
Isn't the addition of return type hints on non-private methods a potential BC break for inheriting classes?
There was a problem hiding this comment.
That is correct. Protected members in non-final classes have to be treated like public members when considering BC breaks.
src/Query/Builder.php
Outdated
| $result = $this->collection->DeleteMany($wheres); | ||
| $options = $this->inheritConnectionOptions(); | ||
|
|
||
| $result = $this->collection->DeleteMany($wheres, $options); |
There was a problem hiding this comment.
I realize method names are case-insensitive but we should use the canonical case:
| $result = $this->collection->DeleteMany($wheres, $options); | |
| $result = $this->collection->deleteMany($wheres, $options); |
| @@ -905,7 +919,7 @@ public function where($column, $operator = null, $value = null, $boolean = 'and' | |||
| * | |||
| * @return array | |||
| */ | |||
| protected function compileWheres() | |||
| protected function compileWheres(): array | |||
There was a problem hiding this comment.
Related to my earlier comment, this could be a BC break for inheriting classes.
| 'serverSelectionTryOnce' => false, | ||
| ], | ||
| ], | ||
|
|
||
| 'dsn_mongodb' => [ |
There was a problem hiding this comment.
Since this is a PHP file, do you think it makes sense to define the options array once at the top of the file and reference it in the three times it's used herein?
No description provided.