diff --git a/.github/workflows/continuous-integration.yml b/.github/workflows/continuous-integration.yml index 134fb0934..cce62ece4 100644 --- a/.github/workflows/continuous-integration.yml +++ b/.github/workflows/continuous-integration.yml @@ -22,6 +22,7 @@ jobs: - "8.2" - "8.3" mongodb-version: + - "7.0" - "6.0" - "5.0" - "4.4" @@ -34,24 +35,34 @@ jobs: symfony-version: - "stable" include: + # Test against lowest dependencies - dependencies: "lowest" php-version: "8.1" mongodb-version: "4.4" driver-version: "1.11.0" topology: "server" symfony-version: "stable" - - topology: "sharded_cluster" + # Test with highest dependencies + - topology: "server" + php-version: "8.2" + mongodb-version: "6.0" + driver-version: "stable" + dependencies: "highest" + symfony-version: "7" + # Test with a 4.4 replica set + - topology: "replica_set" php-version: "8.2" mongodb-version: "4.4" driver-version: "stable" dependencies: "highest" symfony-version: "stable" - - topology: "server" + # Test with a 4.4 sharded cluster + - topology: "sharded_cluster" php-version: "8.2" - mongodb-version: "6.0" + mongodb-version: "4.4" driver-version: "stable" dependencies: "highest" - symfony-version: "7" + symfony-version: "stable" steps: - name: "Checkout" diff --git a/lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php b/lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php index 373207d91..48ff319b5 100644 --- a/lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php +++ b/lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php @@ -35,6 +35,7 @@ use MongoDB\Driver\CursorInterface; use MongoDB\Driver\Exception\Exception as DriverException; use MongoDB\Driver\Exception\WriteException; +use MongoDB\Driver\Session; use MongoDB\Driver\WriteConcern; use MongoDB\GridFS\Bucket; use stdClass; @@ -1580,7 +1581,23 @@ private function getWriteOptions(array $options = []): array unset($writeOptions['w']); } - return $writeOptions; + return $this->isInTransaction($options) + ? $this->uow->stripTransactionOptions($writeOptions) + : $writeOptions; + } + + private function isInTransaction(array $options): bool + { + if (! isset($options['session'])) { + return false; + } + + $session = $options['session']; + if (! $session instanceof Session) { + return false; + } + + return $session->isInTransaction(); } /** diff --git a/lib/Doctrine/ODM/MongoDB/UnitOfWork.php b/lib/Doctrine/ODM/MongoDB/UnitOfWork.php index 96ba4c92a..8c6748a8b 100644 --- a/lib/Doctrine/ODM/MongoDB/UnitOfWork.php +++ b/lib/Doctrine/ODM/MongoDB/UnitOfWork.php @@ -25,13 +25,17 @@ use Doctrine\Persistence\PropertyChangedListener; use InvalidArgumentException; use MongoDB\BSON\UTCDateTime; +use MongoDB\Driver\Session; use MongoDB\Driver\WriteConcern; use ProxyManager\Proxy\GhostObjectInterface; use ReflectionProperty; use UnexpectedValueException; +use function array_diff_key; use function array_filter; +use function array_intersect_key; use function array_key_exists; +use function array_merge; use function assert; use function count; use function get_class; @@ -39,6 +43,7 @@ use function is_array; use function is_object; use function method_exists; +use function MongoDB\with_transaction; use function preg_match; use function serialize; use function spl_object_hash; @@ -61,6 +66,7 @@ * fsync?: bool, * safe?: int, * w?: int, + * withTransaction?: bool, * writeConcern?: WriteConcern * } */ @@ -92,6 +98,12 @@ final class UnitOfWork implements PropertyChangedListener /** @internal */ public const DEPRECATED_WRITE_OPTIONS = ['fsync', 'safe', 'w']; + private const TRANSACTION_OPTIONS = [ + 'maxCommitTimeMS' => 1, + 'readConcern' => 1, + 'readPreference' => 1, + 'writeConcern' => 1, + ]; /** * The identity map holds references to all managed documents. @@ -441,27 +453,18 @@ public function commit(array $options = []): void } } - // Raise onFlush $this->evm->dispatchEvent(Events::onFlush, new Event\OnFlushEventArgs($this->dm)); - foreach ($this->getClassesForCommitAction($this->scheduledDocumentUpserts) as $classAndDocuments) { - [$class, $documents] = $classAndDocuments; - $this->executeUpserts($class, $documents, $options); - } - - foreach ($this->getClassesForCommitAction($this->scheduledDocumentInsertions) as $classAndDocuments) { - [$class, $documents] = $classAndDocuments; - $this->executeInserts($class, $documents, $options); - } - - foreach ($this->getClassesForCommitAction($this->scheduledDocumentUpdates) as $classAndDocuments) { - [$class, $documents] = $classAndDocuments; - $this->executeUpdates($class, $documents, $options); - } - - foreach ($this->getClassesForCommitAction($this->scheduledDocumentDeletions, true) as $classAndDocuments) { - [$class, $documents] = $classAndDocuments; - $this->executeDeletions($class, $documents, $options); + if ($this->useTransaction($options)) { + with_transaction( + $this->dm->getClient()->startSession(), + function (Session $session) use ($options): void { + $this->doCommit(['session' => $session] + $this->stripTransactionOptions($options)); + }, + $this->getTransactionOptions($options), + ); + } else { + $this->doCommit($options); } // Raise postFlush @@ -3110,8 +3113,63 @@ public function isUninitializedObject(object $obj): bool }; } + /** @internal */ + public function stripTransactionOptions(array $options): array + { + return array_diff_key( + $options, + self::TRANSACTION_OPTIONS, + ); + } + private function objToStr(object $obj): string { return method_exists($obj, '__toString') ? (string) $obj : $obj::class . '@' . spl_object_hash($obj); } + + /** @psalm-param CommitOptions $options */ + private function doCommit(array $options): void + { + foreach ($this->getClassesForCommitAction($this->scheduledDocumentUpserts) as $classAndDocuments) { + [$class, $documents] = $classAndDocuments; + $this->executeUpserts($class, $documents, $options); + } + + foreach ($this->getClassesForCommitAction($this->scheduledDocumentInsertions) as $classAndDocuments) { + [$class, $documents] = $classAndDocuments; + $this->executeInserts($class, $documents, $options); + } + + foreach ($this->getClassesForCommitAction($this->scheduledDocumentUpdates) as $classAndDocuments) { + [$class, $documents] = $classAndDocuments; + $this->executeUpdates($class, $documents, $options); + } + + foreach ($this->getClassesForCommitAction($this->scheduledDocumentDeletions, true) as $classAndDocuments) { + [$class, $documents] = $classAndDocuments; + $this->executeDeletions($class, $documents, $options); + } + } + + /** @psalm-param CommitOptions $options */ + private function useTransaction(array $options): bool + { + if (isset($options['withTransaction'])) { + return $options['withTransaction']; + } + + return $this->dm->getConfiguration()->isTransactionalFlushEnabled(); + } + + /** @psalm-param CommitOptions $options */ + private function getTransactionOptions(array $options): array + { + return array_intersect_key( + array_merge( + $this->dm->getConfiguration()->getDefaultCommitOptions(), + $options, + ), + self::TRANSACTION_OPTIONS, + ); + } } diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 01a765530..65c41fc88 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -535,6 +535,11 @@ parameters: count: 1 path: lib/Doctrine/ODM/MongoDB/PersistentCollection.php + - + message: "#^Method Doctrine\\\\ODM\\\\MongoDB\\\\Persisters\\\\DocumentPersister\\:\\:isInTransaction\\(\\) has parameter \\$options with no value type specified in iterable type array\\.$#" + count: 1 + path: lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php + - message: "#^Method Doctrine\\\\ODM\\\\MongoDB\\\\Proxy\\\\Factory\\\\StaticProxyFactory\\:\\:createInitializer\\(\\) should return Closure\\(ProxyManager\\\\Proxy\\\\GhostObjectInterface\\&TDocument\\=, string\\=, array\\\\=, Closure\\|null\\=, array\\\\=\\)\\: bool but returns Closure\\(ProxyManager\\\\Proxy\\\\GhostObjectInterface, string, array, mixed, array\\)\\: true\\.$#" count: 1 @@ -590,6 +595,21 @@ parameters: count: 1 path: lib/Doctrine/ODM/MongoDB/Types/DateType.php + - + message: "#^Method Doctrine\\\\ODM\\\\MongoDB\\\\UnitOfWork\\:\\:getTransactionOptions\\(\\) return type has no value type specified in iterable type array\\.$#" + count: 1 + path: lib/Doctrine/ODM/MongoDB/UnitOfWork.php + + - + message: "#^Method Doctrine\\\\ODM\\\\MongoDB\\\\UnitOfWork\\:\\:stripTransactionOptions\\(\\) has parameter \\$options with no value type specified in iterable type array\\.$#" + count: 1 + path: lib/Doctrine/ODM/MongoDB/UnitOfWork.php + + - + message: "#^Method Doctrine\\\\ODM\\\\MongoDB\\\\UnitOfWork\\:\\:stripTransactionOptions\\(\\) return type has no value type specified in iterable type array\\.$#" + count: 1 + path: lib/Doctrine/ODM/MongoDB/UnitOfWork.php + - message: "#^Unable to resolve the template type T in call to method Doctrine\\\\ODM\\\\MongoDB\\\\DocumentManager\\:\\:getClassMetadata\\(\\)$#" count: 1 diff --git a/tests/Doctrine/ODM/MongoDB/Tests/BaseTestCase.php b/tests/Doctrine/ODM/MongoDB/Tests/BaseTestCase.php index 9471db757..9cc3827e9 100644 --- a/tests/Doctrine/ODM/MongoDB/Tests/BaseTestCase.php +++ b/tests/Doctrine/ODM/MongoDB/Tests/BaseTestCase.php @@ -11,6 +11,8 @@ use Doctrine\ODM\MongoDB\UnitOfWork; use Doctrine\Persistence\Mapping\Driver\MappingDriver; use MongoDB\Client; +use MongoDB\Driver\Manager; +use MongoDB\Driver\Server; use MongoDB\Model\DatabaseInfo; use PHPUnit\Framework\TestCase; @@ -34,6 +36,8 @@ abstract class BaseTestCase extends TestCase { + protected static ?bool $supportsTransactions; + protected static bool $allowsTransactions = true; protected ?DocumentManager $dm; protected UnitOfWork $uow; @@ -87,6 +91,9 @@ protected static function getConfiguration(): Configuration $config->addFilter('testFilter', Filter::class); $config->addFilter('testFilter2', Filter::class); + // Enable transactions if supported + $config->setUseTransactionalFlush(static::$allowsTransactions && self::supportsTransactions()); + return $config; } @@ -127,6 +134,32 @@ protected function getServerVersion(): string return $result['version']; } + protected function getPrimaryServer(): Server + { + return $this->dm->getClient()->getManager()->selectServer(); + } + + protected function skipTestIfNoTransactionSupport(): void + { + if (! self::supportsTransactions()) { + $this->markTestSkipped('Test requires a topology that supports transactions'); + } + } + + protected function skipTestIfTransactionalFlushDisabled(): void + { + if (! $this->dm?->getConfiguration()->isTransactionalFlushEnabled()) { + $this->markTestSkipped('Test only applies when transactional flush is enabled'); + } + } + + protected function skipTestIfTransactionalFlushEnabled(): void + { + if ($this->dm?->getConfiguration()->isTransactionalFlushEnabled()) { + $this->markTestSkipped('Test is not compatible with transactional flush'); + } + } + /** @psalm-param class-string $className */ protected function skipTestIfNotSharded(string $className): void { @@ -208,4 +241,16 @@ protected static function removeMultipleHosts(string $uri): string return substr_replace($uri, $singleHost, $pos, strlen($multipleHosts)); } + + protected static function supportsTransactions(): bool + { + return self::$supportsTransactions ??= self::detectTransactionSupport(); + } + + private static function detectTransactionSupport(): bool + { + $manager = new Manager(self::getUri()); + + return $manager->selectServer()->getType() !== Server::TYPE_STANDALONE; + } } diff --git a/tests/Doctrine/ODM/MongoDB/Tests/Functional/AtomicSetTest.php b/tests/Doctrine/ODM/MongoDB/Tests/Functional/AtomicSetTest.php index 4047de34c..dd5cdbc3a 100644 --- a/tests/Doctrine/ODM/MongoDB/Tests/Functional/AtomicSetTest.php +++ b/tests/Doctrine/ODM/MongoDB/Tests/Functional/AtomicSetTest.php @@ -25,6 +25,9 @@ */ class AtomicSetTest extends BaseTestCase { + // This test counts executed commands and thus doesn't work with transactions + protected static bool $allowsTransactions = false; + private CommandLogger $logger; public function setUp(): void diff --git a/tests/Doctrine/ODM/MongoDB/Tests/Functional/CollectionPersisterTest.php b/tests/Doctrine/ODM/MongoDB/Tests/Functional/CollectionPersisterTest.php index c2ef520fd..35b3c2745 100644 --- a/tests/Doctrine/ODM/MongoDB/Tests/Functional/CollectionPersisterTest.php +++ b/tests/Doctrine/ODM/MongoDB/Tests/Functional/CollectionPersisterTest.php @@ -15,6 +15,9 @@ class CollectionPersisterTest extends BaseTestCase { + // This test counts executed commands and thus doesn't work with transactions + protected static bool $allowsTransactions = false; + private CommandLogger $logger; public function setUp(): void diff --git a/tests/Doctrine/ODM/MongoDB/Tests/Functional/CommitImprovementTest.php b/tests/Doctrine/ODM/MongoDB/Tests/Functional/CommitImprovementTest.php index a16571c70..0ef938149 100644 --- a/tests/Doctrine/ODM/MongoDB/Tests/Functional/CommitImprovementTest.php +++ b/tests/Doctrine/ODM/MongoDB/Tests/Functional/CommitImprovementTest.php @@ -18,6 +18,9 @@ class CommitImprovementTest extends BaseTestCase { + // This test counts executed commands and thus doesn't work with transactions + protected static bool $allowsTransactions = false; + private CommandLogger $logger; public function setUp(): void diff --git a/tests/Doctrine/ODM/MongoDB/Tests/Functional/DocumentPersisterTest.php b/tests/Doctrine/ODM/MongoDB/Tests/Functional/DocumentPersisterTest.php index 5e36e9690..aa8313c52 100644 --- a/tests/Doctrine/ODM/MongoDB/Tests/Functional/DocumentPersisterTest.php +++ b/tests/Doctrine/ODM/MongoDB/Tests/Functional/DocumentPersisterTest.php @@ -632,6 +632,8 @@ public static function dataProviderTestWriteConcern(): array #[DataProvider('dataProviderTestWriteConcern')] public function testExecuteInsertsRespectsWriteConcern(string $class, $writeConcern): void { + $this->skipTestIfTransactionalFlushEnabled(); + $documentPersister = $this->uow->getDocumentPersister($class); $collection = $this->createMock(Collection::class); @@ -648,6 +650,28 @@ public function testExecuteInsertsRespectsWriteConcern(string $class, $writeConc $this->dm->flush(); } + /** @psalm-param class-string $class */ + #[DataProvider('dataProviderTestWriteConcern')] + public function testExecuteInsertsOmitsWriteConcernInTransaction(string $class): void + { + $this->skipTestIfTransactionalFlushDisabled(); + + $documentPersister = $this->uow->getDocumentPersister($class); + + $collection = $this->createMock(Collection::class); + $collection->expects($this->once()) + ->method('insertMany') + ->with($this->isType('array'), $this->logicalNot($this->arrayHasKey('writeConcern'))); + + $reflectionProperty = new ReflectionProperty($documentPersister, 'collection'); + $reflectionProperty->setAccessible(true); + $reflectionProperty->setValue($documentPersister, $collection); + + $testDocument = new $class(); + $this->dm->persist($testDocument); + $this->dm->flush(); + } + /** * @param int|string $writeConcern * @psalm-param class-string $class @@ -655,6 +679,8 @@ public function testExecuteInsertsRespectsWriteConcern(string $class, $writeConc #[DataProvider('dataProviderTestWriteConcern')] public function testExecuteUpsertsRespectsWriteConcern(string $class, $writeConcern): void { + $this->skipTestIfTransactionalFlushEnabled(); + $documentPersister = $this->uow->getDocumentPersister($class); $collection = $this->createMock(Collection::class); @@ -672,6 +698,29 @@ public function testExecuteUpsertsRespectsWriteConcern(string $class, $writeConc $this->dm->flush(); } + /** @psalm-param class-string $class */ + #[DataProvider('dataProviderTestWriteConcern')] + public function testExecuteUpsertsDoesNotUseWriteConcernInTransaction(string $class): void + { + $this->skipTestIfTransactionalFlushDisabled(); + + $documentPersister = $this->uow->getDocumentPersister($class); + + $collection = $this->createMock(Collection::class); + $collection->expects($this->once()) + ->method('updateOne') + ->with($this->isType('array'), $this->logicalNot($this->arrayHasKey('writeConcern'))); + + $reflectionProperty = new ReflectionProperty($documentPersister, 'collection'); + $reflectionProperty->setAccessible(true); + $reflectionProperty->setValue($documentPersister, $collection); + + $testDocument = new $class(); + $testDocument->id = new ObjectId(); + $this->dm->persist($testDocument); + $this->dm->flush(); + } + /** * @param int|string $writeConcern * @psalm-param class-string $class @@ -679,6 +728,8 @@ public function testExecuteUpsertsRespectsWriteConcern(string $class, $writeConc #[DataProvider('dataProviderTestWriteConcern')] public function testRemoveRespectsWriteConcern(string $class, $writeConcern): void { + $this->skipTestIfTransactionalFlushEnabled(); + $documentPersister = $this->uow->getDocumentPersister($class); $collection = $this->createMock(Collection::class); @@ -698,8 +749,35 @@ public function testRemoveRespectsWriteConcern(string $class, $writeConcern): vo $this->dm->flush(); } + /** @psalm-param class-string $class */ + #[DataProvider('dataProviderTestWriteConcern')] + public function testRemoveDoesNotUseWriteConcernInTransaction(string $class): void + { + $this->skipTestIfTransactionalFlushDisabled(); + + $documentPersister = $this->uow->getDocumentPersister($class); + + $collection = $this->createMock(Collection::class); + $collection->expects($this->once()) + ->method('deleteOne') + ->with($this->isType('array'), $this->logicalNot($this->arrayHasKey('writeConcern'))); + + $reflectionProperty = new ReflectionProperty($documentPersister, 'collection'); + $reflectionProperty->setAccessible(true); + $reflectionProperty->setValue($documentPersister, $collection); + + $testDocument = new $class(); + $this->dm->persist($testDocument); + $this->dm->flush(); + + $this->dm->remove($testDocument); + $this->dm->flush(); + } + public function testDefaultWriteConcernIsRespected(): void { + $this->skipTestIfTransactionalFlushEnabled(); + $class = DocumentPersisterTestDocument::class; $documentPersister = $this->uow->getDocumentPersister($class); @@ -719,8 +797,33 @@ public function testDefaultWriteConcernIsRespected(): void $this->dm->flush(); } + public function testDefaultWriteConcernIsIgnoredInTransaction(): void + { + $this->skipTestIfTransactionalFlushDisabled(); + + $class = DocumentPersisterTestDocument::class; + $documentPersister = $this->uow->getDocumentPersister($class); + + $collection = $this->createMock(Collection::class); + $collection->expects($this->once()) + ->method('insertMany') + ->with($this->isType('array'), $this->logicalNot($this->arrayHasKey('writeConcern'))); + + $reflectionProperty = new ReflectionProperty($documentPersister, 'collection'); + $reflectionProperty->setAccessible(true); + $reflectionProperty->setValue($documentPersister, $collection); + + $this->dm->getConfiguration()->setDefaultCommitOptions(['writeConcern' => new WriteConcern(1)]); + + $testDocument = new $class(); + $this->dm->persist($testDocument); + $this->dm->flush(); + } + public function testDefaultWriteConcernIsRespectedBackwardCompatibility(): void { + $this->skipTestIfTransactionalFlushEnabled(); + $class = DocumentPersisterTestDocument::class; $documentPersister = $this->uow->getDocumentPersister($class); diff --git a/tests/Doctrine/ODM/MongoDB/Tests/Functional/ReferencePrimerTest.php b/tests/Doctrine/ODM/MongoDB/Tests/Functional/ReferencePrimerTest.php index 11539f282..44c2c5c40 100644 --- a/tests/Doctrine/ODM/MongoDB/Tests/Functional/ReferencePrimerTest.php +++ b/tests/Doctrine/ODM/MongoDB/Tests/Functional/ReferencePrimerTest.php @@ -389,7 +389,9 @@ public function testPrimeReferencesInvokesPrimer(): void $invokedArgs[] = func_get_args(); }; - $readPreference = new ReadPreference(ReadPreference::RP_SECONDARY_PREFERRED); + // Note: using a secondary read preference here can cause issues when using transactions + // Using a primaryPreferred works just as well to check if the hint is passed on to the primer + $readPreference = new ReadPreference(ReadPreference::RP_PRIMARY_PREFERRED); $this->dm->createQueryBuilder(User::class) ->field('account')->prime($primer) ->field('groups')->prime($primer) diff --git a/tests/Doctrine/ODM/MongoDB/Tests/Functional/ShardKeyTest.php b/tests/Doctrine/ODM/MongoDB/Tests/Functional/ShardKeyTest.php index e5aa6de7b..c4eafc9f9 100644 --- a/tests/Doctrine/ODM/MongoDB/Tests/Functional/ShardKeyTest.php +++ b/tests/Doctrine/ODM/MongoDB/Tests/Functional/ShardKeyTest.php @@ -11,6 +11,7 @@ use MongoDB\BSON\ObjectId; use PHPUnit\Framework\Attributes\Group; +use function array_shift; use function assert; use function end; @@ -48,13 +49,15 @@ public function testUpdateAfterSave(): void $o = $this->dm->find($o::class, $o->id); assert($o instanceof ShardedOne); $o->title = 'test2'; + + $this->logger->clear(); $this->dm->flush(); - $queries = $this->logger->getAll(); - $lastQuery = end($queries); - self::assertSame('update', $lastQuery->getCommandName()); + $queries = $this->logger->getAll(); + $updateQuery = array_shift($queries); + self::assertSame('update', $updateQuery->getCommandName()); - $command = $lastQuery->getCommand(); + $command = $updateQuery->getCommand(); self::assertIsArray($command->updates); self::assertCount(1, $command->updates); self::assertEquals($o->key, $command->updates[0]->q->k); @@ -67,11 +70,11 @@ public function testUpsert(): void $this->dm->persist($o); $this->dm->flush(); - $queries = $this->logger->getAll(); - $lastQuery = end($queries); - self::assertSame('update', $lastQuery->getCommandName()); + $queries = $this->logger->getAll(); + $upsertQuery = array_shift($queries); + self::assertSame('update', $upsertQuery->getCommandName()); - $command = $lastQuery->getCommand(); + $command = $upsertQuery->getCommand(); self::assertIsArray($command->updates); self::assertCount(1, $command->updates); self::assertEquals($o->key, $command->updates[0]->q->k); @@ -83,14 +86,17 @@ public function testRemove(): void $o = new ShardedOne(); $this->dm->persist($o); $this->dm->flush(); + $this->dm->remove($o); + + $this->logger->clear(); $this->dm->flush(); - $queries = $this->logger->getAll(); - $lastQuery = end($queries); - self::assertSame('delete', $lastQuery->getCommandName()); + $queries = $this->logger->getAll(); + $removeQuery = array_shift($queries); + self::assertSame('delete', $removeQuery->getCommandName()); - $command = $lastQuery->getCommand(); + $command = $removeQuery->getCommand(); self::assertIsArray($command->deletes); self::assertCount(1, $command->deletes); self::assertEquals($o->key, $command->deletes[0]->q->k); diff --git a/tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH1138Test.php b/tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH1138Test.php index c06b34540..9e80fcffd 100644 --- a/tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH1138Test.php +++ b/tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH1138Test.php @@ -12,6 +12,9 @@ class GH1138Test extends BaseTestCase { + // This test counts executed commands and thus doesn't work with transactions + protected static bool $allowsTransactions = false; + private CommandLogger $logger; public function setUp(): void diff --git a/tests/Doctrine/ODM/MongoDB/Tests/UnitOfWorkCommitConsistencyTest.php b/tests/Doctrine/ODM/MongoDB/Tests/UnitOfWorkCommitConsistencyTest.php index ab01bbc85..71bc7c2a2 100644 --- a/tests/Doctrine/ODM/MongoDB/Tests/UnitOfWorkCommitConsistencyTest.php +++ b/tests/Doctrine/ODM/MongoDB/Tests/UnitOfWorkCommitConsistencyTest.php @@ -15,6 +15,9 @@ class UnitOfWorkCommitConsistencyTest extends BaseTestCase { + // This test requires transactions to be disabled + protected static bool $allowsTransactions = false; + public function tearDown(): void { $this->dm->getClient()->selectDatabase('admin')->command([ diff --git a/tests/Doctrine/ODM/MongoDB/Tests/UnitOfWorkTest.php b/tests/Doctrine/ODM/MongoDB/Tests/UnitOfWorkTest.php index 61a529e06..c3a22b308 100644 --- a/tests/Doctrine/ODM/MongoDB/Tests/UnitOfWorkTest.php +++ b/tests/Doctrine/ODM/MongoDB/Tests/UnitOfWorkTest.php @@ -8,6 +8,7 @@ use DateTime; use Doctrine\Common\Collections\ArrayCollection; use Doctrine\Common\Collections\Collection; +use Doctrine\ODM\MongoDB\APM\CommandLogger; use Doctrine\ODM\MongoDB\Mapping\Annotations as ODM; use Doctrine\ODM\MongoDB\Mapping\ClassMetadata; use Doctrine\ODM\MongoDB\MongoDBException; @@ -24,11 +25,15 @@ use Documents\Functional\NotSaved; use Documents\User; use MongoDB\BSON\ObjectId; +use MongoDB\Collection as MongoDBCollection; +use MongoDB\Driver\WriteConcern; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\DoesNotPerformAssertions; use ProxyManager\Proxy\GhostObjectInterface; +use ReflectionProperty; use Throwable; +use function end; use function spl_object_hash; use function sprintf; @@ -548,6 +553,57 @@ public function testCommitsInProgressIsUpdatedOnException(): void $this->fail('This should never be reached, an exception should have been thrown.'); } + + public function testTransactionalCommitOmitsWriteConcernInOperation(): void + { + $this->skipTestIfNoTransactionSupport(); + + // Force transaction config to be enabled + $this->dm->getConfiguration()->setUseTransactionalFlush(true); + + $collection = $this->createMock(MongoDBCollection::class); + $collection->expects($this->once()) + ->method('insertMany') + ->with($this->isType('array'), $this->logicalNot($this->arrayHasKey('writeConcern'))); + + $documentPersister = $this->uow->getDocumentPersister(ForumUser::class); + + $reflectionProperty = new ReflectionProperty($documentPersister, 'collection'); + $reflectionProperty->setAccessible(true); + $reflectionProperty->setValue($documentPersister, $collection); + + $user = new ForumUser(); + $user->username = '12345'; + $this->uow->persist($user); + + $this->uow->commit(['writeConcern' => new WriteConcern(1)]); + } + + public function testTransactionalCommitUsesWriteConcernInCommitCommand(): void + { + $this->skipTestIfNoTransactionSupport(); + + // Force transaction config to be enabled + $this->dm->getConfiguration()->setUseTransactionalFlush(true); + + $user = new ForumUser(); + $user->username = '12345'; + $this->uow->persist($user); + + $logger = new CommandLogger(); + $logger->register(); + + $this->uow->commit(['writeConcern' => new WriteConcern('majority')]); + + $logger->unregister(); + + $commands = $logger->getAll(); + $commitCommand = end($commands); + + $this->assertSame('commitTransaction', $commitCommand->getCommandName()); + $this->assertObjectHasProperty('writeConcern', $commitCommand->getCommand()); + $this->assertEquals((object) ['w' => 'majority'], $commitCommand->getCommand()->writeConcern); + } } class ParentAssociationTest diff --git a/tests/Doctrine/ODM/MongoDB/Tests/UnitOfWorkTransactionalCommitConsistencyTest.php b/tests/Doctrine/ODM/MongoDB/Tests/UnitOfWorkTransactionalCommitConsistencyTest.php new file mode 100644 index 000000000..d355e54a2 --- /dev/null +++ b/tests/Doctrine/ODM/MongoDB/Tests/UnitOfWorkTransactionalCommitConsistencyTest.php @@ -0,0 +1,562 @@ +skipTestIfNoTransactionSupport(); + } + + public function tearDown(): void + { + $this->dm->getClient()->selectDatabase('admin')->command([ + 'configureFailPoint' => 'failCommand', + 'mode' => 'off', + ]); + + parent::tearDown(); + } + + public function testFatalInsertError(): void + { + $firstUser = new ForumUser(); + $firstUser->username = 'alcaeus'; + $this->uow->persist($firstUser); + + $secondUser = new ForumUser(); + $secondUser->username = 'jmikola'; + $this->uow->persist($secondUser); + + $friendUser = new FriendUser('GromNaN'); + $this->uow->persist($friendUser); + + $this->createFatalFailPoint('insert'); + + try { + $this->uow->commit(); + self::fail('Expected exception when committing'); + } catch (Throwable $e) { + self::assertInstanceOf(BulkWriteException::class, $e); + self::assertSame(192, $e->getCode()); + } + + self::assertSame( + 0, + $this->dm->getDocumentCollection(ForumUser::class)->countDocuments(), + ); + + self::assertSame( + 0, + $this->dm->getDocumentCollection(FriendUser::class)->countDocuments(), + ); + + self::assertTrue($this->uow->isScheduledForInsert($firstUser)); + self::assertNotEquals([], $this->uow->getDocumentChangeSet($firstUser)); + + self::assertTrue($this->uow->isScheduledForInsert($secondUser)); + self::assertNotEquals([], $this->uow->getDocumentChangeSet($secondUser)); + + self::assertTrue($this->uow->isScheduledForInsert($friendUser)); + self::assertNotEquals([], $this->uow->getDocumentChangeSet($friendUser)); + } + + public function testTransientInsertError(): void + { + $firstUser = new ForumUser(); + $firstUser->username = 'alcaeus'; + $this->uow->persist($firstUser); + + $secondUser = new ForumUser(); + $secondUser->username = 'jmikola'; + $this->uow->persist($secondUser); + + $friendUser = new FriendUser('GromNaN'); + $this->uow->persist($friendUser); + + // Add a failpoint that triggers a transient error. The transaction will be retried and succeeds + $this->createTransientFailPoint('insert'); + + $this->uow->commit(); + + self::assertSame( + 2, + $this->dm->getDocumentCollection(ForumUser::class)->countDocuments(), + ); + + self::assertSame( + 1, + $this->dm->getDocumentCollection(FriendUser::class)->countDocuments(), + ); + + self::assertFalse($this->uow->isScheduledForInsert($firstUser)); + self::assertEquals([], $this->uow->getDocumentChangeSet($firstUser)); + + self::assertFalse($this->uow->isScheduledForInsert($secondUser)); + self::assertEquals([], $this->uow->getDocumentChangeSet($secondUser)); + + self::assertFalse($this->uow->isScheduledForInsert($friendUser)); + self::assertEquals([], $this->uow->getDocumentChangeSet($friendUser)); + } + + public function testDuplicateKeyError(): void + { + // Create a unique index on the collection to let the second insert fail + $collection = $this->dm->getDocumentCollection(ForumUser::class); + $collection->createIndex(['username' => 1], ['unique' => true]); + + $firstUser = new ForumUser(); + $firstUser->username = 'alcaeus'; + $this->uow->persist($firstUser); + + $secondUser = new ForumUser(); + $secondUser->username = 'alcaeus'; + $this->uow->persist($secondUser); + + $thirdUser = new ForumUser(); + $thirdUser->username = 'jmikola'; + $this->uow->persist($thirdUser); + + try { + $this->uow->commit(); + self::fail('Expected exception when committing'); + } catch (Throwable $e) { + self::assertInstanceOf(BulkWriteException::class, $e); + self::assertSame(11000, $e->getCode()); // Duplicate key + } + + // No users inserted + self::assertSame( + 0, + $this->dm->getDocumentCollection(ForumUser::class)->countDocuments(), + ); + + self::assertTrue($this->uow->isScheduledForInsert($firstUser)); + self::assertNotEquals([], $this->uow->getDocumentChangeSet($firstUser)); + + self::assertTrue($this->uow->isScheduledForInsert($secondUser)); + self::assertNotEquals([], $this->uow->getDocumentChangeSet($secondUser)); + + self::assertTrue($this->uow->isScheduledForInsert($thirdUser)); + self::assertNotEquals([], $this->uow->getDocumentChangeSet($thirdUser)); + } + + public function testFatalInsertErrorWithEmbeddedDocument(): void + { + // Create a unique index on the collection to let the second insert fail + $collection = $this->dm->getDocumentCollection(User::class); + $collection->createIndex(['username' => 1], ['unique' => true]); + + $firstAddress = new Address(); + $firstAddress->setCity('Olching'); + $firstUser = new User(); + $firstUser->setUsername('alcaeus'); + $firstUser->setAddress($firstAddress); + + $secondAddress = new Address(); + $secondAddress->setCity('Olching'); + $secondUser = new User(); + $secondUser->setUsername('alcaeus'); + $secondUser->setAddress($secondAddress); + + $this->uow->persist($firstUser); + $this->uow->persist($secondUser); + + try { + $this->uow->commit(); + self::fail('Expected exception when committing'); + } catch (Throwable $e) { + self::assertInstanceOf(BulkWriteException::class, $e); + self::assertSame(11000, $e->getCode()); + } + + self::assertSame(0, $collection->countDocuments()); + + $this->assertTrue($this->uow->isScheduledForInsert($firstUser)); + $this->assertNotEquals([], $this->uow->getDocumentChangeSet($firstUser)); + $this->assertTrue($this->uow->isScheduledForInsert($firstAddress)); + $this->assertNotEquals([], $this->uow->getDocumentChangeSet($firstAddress)); + + $this->assertTrue($this->uow->isScheduledForInsert($secondUser)); + $this->assertNotEquals([], $this->uow->getDocumentChangeSet($secondUser)); + $this->assertTrue($this->uow->isScheduledForInsert($secondAddress)); + $this->assertNotEquals([], $this->uow->getDocumentChangeSet($secondAddress)); + } + + public function testFatalUpsertError(): void + { + $user = new ForumUser(); + $user->id = new ObjectId(); // Specifying an identifier makes this an upsert + $user->username = 'alcaeus'; + $this->uow->persist($user); + + $this->createFatalFailPoint('update'); + + try { + $this->uow->commit(); + self::fail('Expected exception when committing'); + } catch (Throwable $e) { + self::assertInstanceOf(BulkWriteException::class, $e); + self::assertSame(192, $e->getCode()); + } + + // No document was inserted + self::assertSame( + 0, + $this->dm->getDocumentCollection(ForumUser::class)->countDocuments(), + ); + + self::assertTrue($this->uow->isScheduledForUpsert($user)); + self::assertNotEquals([], $this->uow->getDocumentChangeSet($user)); + } + + public function testTransientUpsertError(): void + { + $user = new ForumUser(); + $user->id = new ObjectId(); // Specifying an identifier makes this an upsert + $user->username = 'alcaeus'; + $this->uow->persist($user); + + $this->createTransientFailPoint('update'); + + $this->uow->commit(); + + self::assertSame( + 1, + $this->dm->getDocumentCollection(ForumUser::class)->countDocuments(), + ); + + self::assertFalse($this->uow->isScheduledForUpsert($user)); + self::assertEquals([], $this->uow->getDocumentChangeSet($user)); + } + + public function testFatalUpdateError(): void + { + $user = new ForumUser(); + $user->username = 'alcaeus'; + $this->uow->persist($user); + $this->uow->commit(); + + $user->username = 'jmikola'; + + $this->createFatalFailPoint('update'); + + try { + $this->uow->commit(); + self::fail('Expected exception when committing'); + } catch (Throwable $e) { + self::assertInstanceOf(BulkWriteException::class, $e); + self::assertSame(192, $e->getCode()); + } + + self::assertSame( + 1, + $this->dm->getDocumentCollection(ForumUser::class)->countDocuments(['username' => 'alcaeus']), + ); + + self::assertTrue($this->uow->isScheduledForUpdate($user)); + self::assertNotEquals([], $this->uow->getDocumentChangeSet($user)); + } + + public function testTransientUpdateError(): void + { + $user = new ForumUser(); + $user->username = 'alcaeus'; + $this->uow->persist($user); + $this->uow->commit(); + + $user->username = 'jmikola'; + + $this->createTransientFailPoint('update'); + + $this->uow->commit(); + + self::assertSame( + 1, + $this->dm->getDocumentCollection(ForumUser::class)->countDocuments(['username' => 'jmikola']), + ); + + self::assertFalse($this->uow->isScheduledForUpdate($user)); + self::assertEquals([], $this->uow->getDocumentChangeSet($user)); + } + + public function testFatalUpdateErrorWithNewEmbeddedDocument(): void + { + $user = new User(); + $user->setUsername('alcaeus'); + + $this->uow->persist($user); + $this->uow->commit(); + + $address = new Address(); + $address->setCity('Olching'); + $user->setAddress($address); + + $this->createFatalFailPoint('update'); + + try { + $this->uow->commit(); + self::fail('Expected exception when committing'); + } catch (Throwable $e) { + self::assertInstanceOf(BulkWriteException::class, $e); + self::assertSame(192, $e->getCode()); + } + + $this->assertTrue($this->uow->isScheduledForUpdate($user)); + $this->assertNotEquals([], $this->uow->getDocumentChangeSet($user)); + $this->assertTrue($this->uow->isScheduledForInsert($address)); + $this->assertNotEquals([], $this->uow->getDocumentChangeSet($address)); + } + + public function testTransientUpdateErrorWithNewEmbeddedDocument(): void + { + $user = new User(); + $user->setUsername('alcaeus'); + + $this->uow->persist($user); + $this->uow->commit(); + + $address = new Address(); + $address->setCity('Olching'); + $user->setAddress($address); + + $this->createTransientFailPoint('update'); + + $this->uow->commit(); + + $this->assertFalse($this->uow->isScheduledForUpdate($user)); + $this->assertEquals([], $this->uow->getDocumentChangeSet($user)); + $this->assertFalse($this->uow->isScheduledForInsert($address)); + $this->assertEquals([], $this->uow->getDocumentChangeSet($address)); + } + + public function testFatalUpdateErrorOfEmbeddedDocument(): void + { + $address = new Address(); + $address->setCity('Olching'); + + $user = new User(); + $user->setUsername('alcaeus'); + $user->setAddress($address); + + $this->uow->persist($user); + $this->uow->commit(); + + $address->setCity('Munich'); + + $this->createFatalFailPoint('update'); + + try { + $this->uow->commit(); + self::fail('Expected exception when committing'); + } catch (Throwable $e) { + self::assertInstanceOf(BulkWriteException::class, $e); + self::assertSame(192, $e->getCode()); + } + + $this->assertTrue($this->uow->isScheduledForUpdate($user)); + $this->assertNotEquals([], $this->uow->getDocumentChangeSet($user)); + $this->assertTrue($this->uow->isScheduledForUpdate($address)); + $this->assertNotEquals([], $this->uow->getDocumentChangeSet($address)); + } + + public function testTransientUpdateErrorOfEmbeddedDocument(): void + { + $address = new Address(); + $address->setCity('Olching'); + + $user = new User(); + $user->setUsername('alcaeus'); + $user->setAddress($address); + + $this->uow->persist($user); + $this->uow->commit(); + + $address->setCity('Munich'); + + $this->createTransientFailPoint('update'); + + $this->uow->commit(); + + $this->assertFalse($this->uow->isScheduledForUpdate($user)); + $this->assertEquals([], $this->uow->getDocumentChangeSet($user)); + $this->assertFalse($this->uow->isScheduledForUpdate($address)); + $this->assertEquals([], $this->uow->getDocumentChangeSet($address)); + } + + public function testFatalUpdateErrorWithRemovedEmbeddedDocument(): void + { + $address = new Address(); + $address->setCity('Olching'); + + $user = new User(); + $user->setUsername('alcaeus'); + $user->setAddress($address); + + $this->uow->persist($user); + $this->uow->commit(); + + $user->removeAddress(); + + $this->createFatalFailPoint('update'); + + try { + $this->uow->commit(); + self::fail('Expected exception when committing'); + } catch (Throwable $e) { + self::assertInstanceOf(BulkWriteException::class, $e); + self::assertSame(192, $e->getCode()); + } + + $this->assertTrue($this->uow->isScheduledForUpdate($user)); + $this->assertNotEquals([], $this->uow->getDocumentChangeSet($user)); + $this->assertTrue($this->uow->isScheduledForDelete($address)); + + // As $address is orphaned after changeset computation, it is removed from the identity map + $this->assertFalse($this->uow->isInIdentityMap($address)); + } + + public function testTransientUpdateErrorWithRemovedEmbeddedDocument(): void + { + $address = new Address(); + $address->setCity('Olching'); + + $user = new User(); + $user->setUsername('alcaeus'); + $user->setAddress($address); + + $this->uow->persist($user); + $this->uow->commit(); + + $user->removeAddress(); + + $this->createTransientFailPoint('update'); + + $this->uow->commit(); + + $this->assertFalse($this->uow->isScheduledForUpdate($user)); + $this->assertEquals([], $this->uow->getDocumentChangeSet($user)); + $this->assertFalse($this->uow->isScheduledForDelete($address)); + $this->assertFalse($this->uow->isInIdentityMap($address)); + } + + public function testFatalDeleteErrorWithEmbeddedDocument(): void + { + $address = new Address(); + $address->setCity('Olching'); + + $user = new User(); + $user->setUsername('alcaeus'); + $user->setAddress($address); + + $this->uow->persist($user); + $this->uow->commit(); + + $this->uow->remove($user); + + $this->createFatalFailPoint('delete'); + + try { + $this->uow->commit(); + self::fail('Expected exception when committing'); + } catch (Throwable $e) { + self::assertInstanceOf(BulkWriteException::class, $e); + self::assertSame(192, $e->getCode()); + } + + // The document still exists, the deletion is still scheduled + self::assertSame( + 1, + $this->dm->getDocumentCollection(User::class)->countDocuments(['username' => 'alcaeus']), + ); + + self::assertTrue($this->uow->isScheduledForDelete($user)); + self::assertTrue($this->uow->isScheduledForDelete($address)); + } + + public function testTransientDeleteErrorWithEmbeddedDocument(): void + { + $address = new Address(); + $address->setCity('Olching'); + + $user = new User(); + $user->setUsername('alcaeus'); + $user->setAddress($address); + + $this->uow->persist($user); + $this->uow->commit(); + + $this->uow->remove($user); + + $this->createTransientFailPoint('delete'); + + $this->uow->commit(); + + self::assertSame( + 0, + $this->dm->getDocumentCollection(User::class)->countDocuments(['username' => 'alcaeus']), + ); + + self::assertFalse($this->uow->isScheduledForDelete($address)); + self::assertFalse($this->uow->isScheduledForDelete($user)); + } + + /** Create a document manager with a single host to ensure failpoints target the correct server */ + protected static function createTestDocumentManager(): DocumentManager + { + $config = static::getConfiguration(); + $client = new Client(self::getUri(false), [], ['typeMap' => ['root' => 'array', 'document' => 'array']]); + + return DocumentManager::create($client, $config); + } + + protected static function getConfiguration(): Configuration + { + $configuration = parent::getConfiguration(); + $configuration->setUseTransactionalFlush(true); + + return $configuration; + } + + private function createTransientFailPoint(string $failCommand): void + { + $this->dm->getClient()->selectDatabase('admin')->command([ + 'configureFailPoint' => 'failCommand', + // Trigger the error twice, working around retryable writes + 'mode' => ['times' => 2], + 'data' => [ + 'errorCode' => 192, // FailPointEnabled + 'errorLabels' => ['TransientTransactionError'], + 'failCommands' => [$failCommand], + ], + ]); + } + + private function createFatalFailPoint(string $failCommand): void + { + $this->dm->getClient()->selectDatabase('admin')->command([ + 'configureFailPoint' => 'failCommand', + 'mode' => ['times' => 1], + 'data' => [ + 'errorCode' => 192, // FailPointEnabled + 'failCommands' => [$failCommand], + ], + ]); + } +}