Conversation
| ->arg('$fixtureStoryResolver', service('.zenstruck_foundry.story.fixture_resolver')) | ||
| ->arg('$databaseResetters', tagged_iterator('.foundry.persistence.database_resetter')) | ||
| ->arg('$kernel', service('kernel')) | ||
| ->arg('$registry', service('doctrine')->nullOnInvalid()) |
There was a problem hiding this comment.
Instead of injecting the doctrine registry, could we inject .zenstruck_foundry.persistence_manager?
I'd suggest a transactional(callable) method on PersistenceStrategy: the ORM strategy wraps the callback in a transaction on its connections, and the Mongo strategy just calls it as a no-op operation (I'm planning to refactor this whole part of persistence some day, and the no-op operation won't exist anymore, but for now it is ok-ish I think.
PersistenceManager::transactional() would then nest the strategies, and the command would boil down to $this->persistenceManager->transactional(fn() => $this->loadStories(...)).
There was a problem hiding this comment.
Done: PersistenceStrategy::transactional() (the class is @internal), one transaction per ORM connection in AbstractORMPersistenceStrategy, a plain call in MongoPersistenceStrategy, and PersistenceManager::transactional() nests the strategies. The command only gets the persistence manager now.
| } catch (\Throwable $e) { | ||
| foreach ($connections as $connection) { | ||
| if ($connection->isTransactionActive()) { | ||
| $connection->rollBack(); |
There was a problem hiding this comment.
here, if an error occurs while rolling-back, the original error would be swallowed and hidden
could we do this in a finally block, the same way it is done here?
https://github.com/symfony/symfony/blob/8.2/src/Symfony/Bridge/Doctrine/Messenger/DoctrineTransactionMiddleware.php
There was a problem hiding this comment.
Moved to a finally, like DoctrineTransactionMiddleware: PHP then attaches the original exception as the previous one of the rollback error. OrmTransactionalTest::it_keeps_the_original_error_when_the_rollback_fails covers it, and fails with the former catch.
| With Doctrine ORM, the stories are loaded in a single transaction: if one of them fails, none of them is kept in the | ||
| database. |
There was a problem hiding this comment.
Could you reword it to say it applies per ORM connection, and that MongoDB is not covered?
There was a problem hiding this comment.
Reworded: a transaction per ORM connection, and MongoDB is not covered.
|
Added in the last commit, @nikophil: |
| // in "finally", so that an error while rolling back doesn't hide the original one | ||
| if (!$success) { | ||
| foreach ($connections as $connection) { | ||
| if ($connection->isTransactionActive()) { |
There was a problem hiding this comment.
The rollback relies on isTransactionActive(), which tells whether a transaction is active, not whether ours still is.
When the call is nested in an outer transaction (DAMA, or any caller already in a transaction), if the first connection commits (which only releases the savepoint, so it stays active at level 1) and the second one fails, this finally rolls back the first connection again, i.e. the outer transaction.
Could we track the connections we actually opened (add after beginTransaction(), remove after commit()) and only roll those back? It would also make isTransactionActive() unnecessary. The unit test doesn't catch it since the mock always returns true for isTransactionActive().
There was a problem hiding this comment.
Good catch, thanks! It now keeps the transactions it opened and hasn't committed yet, and rolls back only those, so isTransactionActive() is gone. it_only_rolls_back_the_transactions_it_still_has_open reproduces your scenario (it fails on the previous code).
| $connection->method('rollBack')->willThrowException(new \LogicException('rollback failed')); | ||
|
|
||
| try { | ||
| $this->strategy($connection)->transactional(static fn() => throw new \RuntimeException('story failed')); |
There was a problem hiding this comment.
nit: if no exception is thrown, $e is undefined and the test fails with a confusing error. A self::fail('...') right after this call would make it explicit.
There was a problem hiding this comment.
Added. PHPStan flagged it as unreachable since the callback always throws, so the callback now comes from a small failingStory() helper typed as returning a string.
| if ($input->getOption('no-transaction')) { | ||
| $this->loadStories($io, $resolvedStories); | ||
| } else { | ||
| // All the stories are loaded, or none of them: a story failing halfway must not |
There was a problem hiding this comment.
nit: this comment paraphrases what transactional() already says, I think it can be removed.
| * | ||
| * @return T | ||
| */ | ||
| public function transactional(callable $callback): mixed |
There was a problem hiding this comment.
nit: there's no direct test for this method, the nesting of the strategies is only covered indirectly by the mysql|mongo jobs. Not blocking, but a small unit test would document the behavior.
There was a problem hiding this comment.
Added PersistenceManagerTransactionalTest: two strategies, it checks the nesting order and that the callback runs once and its result comes back.
2 participants
Fixes #1078.
foundry:load-fixturesloads its stories in a transaction, so that a story failing halfway doesn't leave the stories loaded before it in the database. The transaction is opened on each ORM connection, MongoDB is not covered, and the database reset stays outside of it.