From 887a62ed06834bed7e7253852ae12de142d2d162 Mon Sep 17 00:00:00 2001 From: buffcode Date: Wed, 3 Feb 2021 15:01:35 +0100 Subject: [PATCH 01/16] Fix locking when ClassMetadata is unserialized Caching / unserializing ClassMetadata broke locking functionality Fixes #2278 --- lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadata.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadata.php b/lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadata.php index 891bc51bcb..8d167dc92e 100644 --- a/lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadata.php +++ b/lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadata.php @@ -2129,6 +2129,11 @@ public function __sleep() $serialized[] = 'versionField'; } + if ($this->isLockable) { + $serialized[] = 'isLockable'; + $serialized[] = 'lockField'; + } + if ($this->lifecycleCallbacks) { $serialized[] = 'lifecycleCallbacks'; } From a97aab0c4703ad17eac1efaa9bbb66ad844990ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Laurens=20St=C3=B6tzel?= Date: Thu, 4 Feb 2021 01:19:12 +0100 Subject: [PATCH 02/16] Test serialization of lock/version fields --- .../ODM/MongoDB/Tests/Mapping/ClassMetadataTest.php | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/Doctrine/ODM/MongoDB/Tests/Mapping/ClassMetadataTest.php b/tests/Doctrine/ODM/MongoDB/Tests/Mapping/ClassMetadataTest.php index b6f6a3f1dc..37630ed48e 100644 --- a/tests/Doctrine/ODM/MongoDB/Tests/Mapping/ClassMetadataTest.php +++ b/tests/Doctrine/ODM/MongoDB/Tests/Mapping/ClassMetadataTest.php @@ -57,6 +57,10 @@ public function testClassMetadataInstanceSerialization() $cm->setCollectionCapped(true); $cm->setCollectionMax(1000); $cm->setCollectionSize(500); + $cm->setLockable(true); + $cm->setLockField('lock'); + $cm->setVersioned(true); + $cm->setVersionField('version'); $this->assertIsArray($cm->getFieldMapping('phonenumbers')); $this->assertCount(1, $cm->fieldMappings); $this->assertCount(1, $cm->associationMappings); @@ -82,6 +86,10 @@ public function testClassMetadataInstanceSerialization() $this->assertTrue($cm->getCollectionCapped()); $this->assertEquals(1000, $cm->getCollectionMax()); $this->assertEquals(500, $cm->getCollectionSize()); + $this->assertEquals(true, $cm->isLockable); + $this->assertEquals('lock', $cm->lockField); + $this->assertEquals(true, $cm->isVersioned); + $this->assertEquals('version', $cm->versionField); } public function testOwningSideAndInverseSide() From b243e4b204f6807df6c809999bae1bf306c521b7 Mon Sep 17 00:00:00 2001 From: jeeiex <78592605+jeeiex@users.noreply.github.com> Date: Fri, 5 Feb 2021 09:59:48 +0000 Subject: [PATCH 03/16] Update working-with-objects.rst Detach doc text from code block --- docs/en/reference/working-with-objects.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/en/reference/working-with-objects.rst b/docs/en/reference/working-with-objects.rst index 66802731b4..edf1ed191c 100644 --- a/docs/en/reference/working-with-objects.rst +++ b/docs/en/reference/working-with-objects.rst @@ -291,8 +291,8 @@ Example: // $document now refers to the fully managed copy returned by the merge operation. // The DocumentManager $dm now manages the persistence of $document as usual. - The semantics of the merge operation, applied to a document X, are - as follows: +The semantics of the merge operation, applied to a document X, are +as follows: - If X is a detached document, the state of X is copied onto a From 5a0af40c6887f56feef28d0b87c5f59788400634 Mon Sep 17 00:00:00 2001 From: jeeiex <78592605+jeeiex@users.noreply.github.com> Date: Fri, 5 Feb 2021 11:14:19 +0000 Subject: [PATCH 04/16] Update storage-strategies.rst --- docs/en/reference/storage-strategies.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/en/reference/storage-strategies.rst b/docs/en/reference/storage-strategies.rst index 58944087da..f5c94cbfc8 100644 --- a/docs/en/reference/storage-strategies.rst +++ b/docs/en/reference/storage-strategies.rst @@ -6,8 +6,8 @@ Storage Strategies Doctrine MongoDB ODM implements several different strategies for persisting changes to mapped fields. These strategies apply to the following mapping types: -- :ref:`int` -- :ref:`float` +- `int` +- `float` - :ref:`embed_many` - :ref:`reference_many` From 1276c9c391ad091c3d083cbca0f919a6d4c0de85 Mon Sep 17 00:00:00 2001 From: Andreas Braun Date: Fri, 26 Feb 2021 09:39:57 +0100 Subject: [PATCH 05/16] Fix invalid strict comparison when validating mappings --- .../MongoDB/Tools/Console/Command/Schema/ValidateCommand.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/Doctrine/ODM/MongoDB/Tools/Console/Command/Schema/ValidateCommand.php b/lib/Doctrine/ODM/MongoDB/Tools/Console/Command/Schema/ValidateCommand.php index f2267b9bf1..59d3e691b9 100644 --- a/lib/Doctrine/ODM/MongoDB/Tools/Console/Command/Schema/ValidateCommand.php +++ b/lib/Doctrine/ODM/MongoDB/Tools/Console/Command/Schema/ValidateCommand.php @@ -44,7 +44,9 @@ protected function execute(InputInterface $input, OutputInterface $output) $errors = 0; foreach ($metadataFactory->getAllMetadata() as $meta) { - if ($meta === unserialize(serialize($meta))) { + // Don't use === to compare as that will always evaluate to false since we receive a different object + // phpcs:ignore SlevomatCodingStandard.Operators.DisallowEqualOperators.DisallowedEqualOperator + if ($meta == unserialize(serialize($meta))) { continue; } From 8069ded252ff457c481b4da7464ff9db00be41ba Mon Sep 17 00:00:00 2001 From: Gocha Ossinkine Date: Wed, 10 Mar 2021 17:36:19 +0200 Subject: [PATCH 06/16] Correctly handle write concern specified in defaultCommitOptions (#2294) --- lib/Doctrine/ODM/MongoDB/Configuration.php | 7 ++++- .../MongoDB/Persisters/DocumentPersister.php | 19 ++++++++++-- .../Functional/DocumentPersisterTest.php | 30 ++++++++++++++++--- 3 files changed, 49 insertions(+), 7 deletions(-) diff --git a/lib/Doctrine/ODM/MongoDB/Configuration.php b/lib/Doctrine/ODM/MongoDB/Configuration.php index 8273d5d909..f2aa7e091c 100644 --- a/lib/Doctrine/ODM/MongoDB/Configuration.php +++ b/lib/Doctrine/ODM/MongoDB/Configuration.php @@ -21,6 +21,7 @@ use Doctrine\Persistence\Mapping\Driver\MappingDriver; use Doctrine\Persistence\ObjectRepository; use InvalidArgumentException; +use MongoDB\Driver\WriteConcern; use ProxyManager\Configuration as ProxyManagerConfiguration; use ProxyManager\Factory\LazyLoadingGhostFactory; use ProxyManager\GeneratorStrategy\EvaluatingGeneratorStrategy; @@ -347,7 +348,11 @@ public function getClassMetadataFactoryName(): string public function getDefaultCommitOptions(): array { - return $this->attributes['defaultCommitOptions'] ?? ['w' => 1]; + if (! isset($this->attributes['defaultCommitOptions'])) { + $this->attributes['defaultCommitOptions'] = ['writeConcern' => new WriteConcern(1)]; + } + + return $this->attributes['defaultCommitOptions']; } public function setDefaultCommitOptions(array $defaultCommitOptions): void diff --git a/lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php b/lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php index 1d5ce672e3..ca45cbcba4 100644 --- a/lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php +++ b/lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php @@ -32,6 +32,7 @@ use MongoDB\Driver\Cursor; use MongoDB\Driver\Exception\Exception as DriverException; use MongoDB\Driver\Exception\WriteException; +use MongoDB\Driver\WriteConcern; use MongoDB\GridFS\Bucket; use ProxyManager\Proxy\GhostObjectInterface; use stdClass; @@ -39,6 +40,7 @@ use function array_combine; use function array_fill; use function array_intersect_key; +use function array_key_exists; use function array_keys; use function array_map; use function array_merge; @@ -61,6 +63,7 @@ use function sprintf; use function strpos; use function strtolower; +use function trigger_deprecation; /** * The DocumentPersister is responsible for persisting documents. @@ -1493,10 +1496,22 @@ private function getWriteOptions(array $options = []): array $defaultOptions = $this->dm->getConfiguration()->getDefaultCommitOptions(); $documentOptions = []; if ($this->class->hasWriteConcern()) { - $documentOptions['w'] = $this->class->getWriteConcern(); + $documentOptions['writeConcern'] = new WriteConcern($this->class->getWriteConcern()); } - return array_merge($defaultOptions, $documentOptions, $options); + $writeOptions = array_merge($defaultOptions, $documentOptions, $options); + if (array_key_exists('w', $writeOptions)) { + trigger_deprecation( + 'doctrine/mongodb-odm', + '2.2', + 'The "w" option as commit option is deprecated, please pass "%s" object in "writeConcern" option.', + WriteConcern::class + ); + $writeOptions['writeConcern'] = new WriteConcern($writeOptions['w']); + unset($writeOptions['w']); + } + + return $writeOptions; } private function prepareReference(string $fieldName, $value, array $mapping, bool $inNewObj): array diff --git a/tests/Doctrine/ODM/MongoDB/Tests/Functional/DocumentPersisterTest.php b/tests/Doctrine/ODM/MongoDB/Tests/Functional/DocumentPersisterTest.php index 56c8658294..d72f41f5dd 100644 --- a/tests/Doctrine/ODM/MongoDB/Tests/Functional/DocumentPersisterTest.php +++ b/tests/Doctrine/ODM/MongoDB/Tests/Functional/DocumentPersisterTest.php @@ -19,6 +19,7 @@ use MongoDB\BSON\ObjectId; use MongoDB\BSON\UTCDateTime; use MongoDB\Collection; +use MongoDB\Driver\WriteConcern; use ReflectionProperty; use function get_class; @@ -644,7 +645,7 @@ public function testExecuteInsertsRespectsWriteConcern($class, $writeConcern) $collection = $this->createMock(Collection::class); $collection->expects($this->once()) ->method('insertMany') - ->with($this->isType('array'), $this->logicalAnd($this->arrayHasKey('w'), $this->containsEqual($writeConcern))); + ->with($this->isType('array'), $this->logicalAnd($this->arrayHasKey('writeConcern'), $this->containsEqual(new WriteConcern($writeConcern)))); $reflectionProperty = new ReflectionProperty($documentPersister, 'collection'); $reflectionProperty->setAccessible(true); @@ -668,7 +669,7 @@ public function testExecuteUpsertsRespectsWriteConcern($class, $writeConcern) $collection = $this->createMock(Collection::class); $collection->expects($this->once()) ->method('updateOne') - ->with($this->isType('array'), $this->isType('array'), $this->logicalAnd($this->arrayHasKey('w'), $this->containsEqual($writeConcern))); + ->with($this->isType('array'), $this->isType('array'), $this->logicalAnd($this->arrayHasKey('writeConcern'), $this->containsEqual(new WriteConcern($writeConcern)))); $reflectionProperty = new ReflectionProperty($documentPersister, 'collection'); $reflectionProperty->setAccessible(true); @@ -693,7 +694,7 @@ public function testRemoveRespectsWriteConcern($class, $writeConcern) $collection = $this->createMock(Collection::class); $collection->expects($this->once()) ->method('deleteOne') - ->with($this->isType('array'), $this->logicalAnd($this->arrayHasKey('w'), $this->containsEqual($writeConcern))); + ->with($this->isType('array'), $this->logicalAnd($this->arrayHasKey('writeConcern'), $this->containsEqual(new WriteConcern($writeConcern)))); $reflectionProperty = new ReflectionProperty($documentPersister, 'collection'); $reflectionProperty->setAccessible(true); @@ -715,7 +716,28 @@ public function testDefaultWriteConcernIsRespected() $collection = $this->createMock(Collection::class); $collection->expects($this->once()) ->method('insertMany') - ->with($this->isType('array'), $this->equalTo(['w' => 0])); + ->with($this->isType('array'), $this->equalTo(['writeConcern' => new WriteConcern(0)])); + + $reflectionProperty = new ReflectionProperty($documentPersister, 'collection'); + $reflectionProperty->setAccessible(true); + $reflectionProperty->setValue($documentPersister, $collection); + + $this->dm->getConfiguration()->setDefaultCommitOptions(['writeConcern' => new WriteConcern(0)]); + + $testDocument = new $class(); + $this->dm->persist($testDocument); + $this->dm->flush(); + } + + public function testDefaultWriteConcernIsRespectedBackwardCompatibility() + { + $class = DocumentPersisterTestDocument::class; + $documentPersister = $this->uow->getDocumentPersister($class); + + $collection = $this->createMock(Collection::class); + $collection->expects($this->once()) + ->method('insertMany') + ->with($this->isType('array'), $this->equalTo(['writeConcern' => new WriteConcern(0)])); $reflectionProperty = new ReflectionProperty($documentPersister, 'collection'); $reflectionProperty->setAccessible(true); From 841221bfe8d82805217dd9f909b0246612b289a6 Mon Sep 17 00:00:00 2001 From: Ryan RAJKOMAR Date: Thu, 4 Feb 2021 14:44:05 +0100 Subject: [PATCH 07/16] Fix documentation for uploadFromFile --- docs/en/reference/storing-files-with-gridfs.rst | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/docs/en/reference/storing-files-with-gridfs.rst b/docs/en/reference/storing-files-with-gridfs.rst index 586c632727..ac98114a11 100644 --- a/docs/en/reference/storing-files-with-gridfs.rst +++ b/docs/en/reference/storing-files-with-gridfs.rst @@ -163,7 +163,7 @@ can also open an upload stream and write contents yourself. getRepository(Documents\Image::class); - $file = $repository->uploadFromFile('image.jpg', '/tmp/path/to/image'); + $file = $repository->uploadFromFile('/tmp/path/to/image', 'image.jpg'); When using the default GridFS repository implementation, the ``uploadFromFile`` and ``uploadFromStream`` methods return a proxy object of the file you just @@ -184,7 +184,7 @@ can pass an ``UploadOptions`` object as the last argument to the $uploadOptions->chunkSizeBytes = 1024 * 1024; $repository = $documentManager->getRepository(Documents\Image::class); - $file = $repository->uploadFromFile('image.jpg', '/tmp/path/to/image', $uploadOptions); + $file = $repository->uploadFromFile('/tmp/path/to/image', 'image.jpg', $uploadOptions); Reading files from GridFS buckets --------------------------------- @@ -208,8 +208,13 @@ uploading: metadata = new Documents\ImageMetadata('image/jpeg'); + $repository = $documentManager->getRepository(Documents\Image::class); - $file = $repository->uploadFromFile('image.jpg', '/tmp/path/to/image', new Documents\ImageMetadata('image/jpeg')); + $file = $repository->uploadFromFile('/tmp/path/to/image', 'image.jpg', $uploadOptions); $stream = fopen('tmp/path/to/copy', 'w+'); try { @@ -236,7 +241,7 @@ a stream from where you can read file contents: $uploadOptions->metadata = new Documents\ImageMetadata('image/jpeg'); $repository = $documentManager->getRepository(Documents\Image::class); - $file = $repository->uploadFromFile('image.jpg', '/tmp/path/to/image', $uploadOptions); + $file = $repository->uploadFromFile('/tmp/path/to/image', 'image.jpg', $uploadOptions); $stream = $repository->openDownloadStream($file->getId()); try { From a3efe06270e0652181b1f7966ab40b2881f2d436 Mon Sep 17 00:00:00 2001 From: wuchen90 Date: Mon, 15 Mar 2021 12:45:20 +0100 Subject: [PATCH 08/16] Fix mapping of the nullable option for XML driver --- .../ODM/MongoDB/Mapping/Driver/XmlDriver.php | 4 +- .../Mapping/Driver/AbstractDriverTest.php | 116 ++++++++++++++++++ .../fixtures/NullableFieldsDocument.php | 20 +++ ...stDocuments.NullableFieldsDocument.dcm.xml | 16 +++ 4 files changed, 155 insertions(+), 1 deletion(-) create mode 100644 tests/Doctrine/ODM/MongoDB/Tests/Mapping/Driver/fixtures/NullableFieldsDocument.php create mode 100644 tests/Doctrine/ODM/MongoDB/Tests/Mapping/Driver/fixtures/xml/TestDocuments.NullableFieldsDocument.dcm.xml diff --git a/lib/Doctrine/ODM/MongoDB/Mapping/Driver/XmlDriver.php b/lib/Doctrine/ODM/MongoDB/Mapping/Driver/XmlDriver.php index 3ed93b97bb..691ac8fef3 100644 --- a/lib/Doctrine/ODM/MongoDB/Mapping/Driver/XmlDriver.php +++ b/lib/Doctrine/ODM/MongoDB/Mapping/Driver/XmlDriver.php @@ -243,7 +243,7 @@ public function loadMetadataForClass($className, \Doctrine\Persistence\Mapping\C $attributes = $field->attributes(); foreach ($attributes as $key => $value) { $mapping[$key] = (string) $value; - $booleanAttributes = ['reference', 'embed', 'unique', 'sparse']; + $booleanAttributes = ['reference', 'embed', 'unique', 'sparse', 'nullable']; if (! in_array($key, $booleanAttributes)) { continue; } @@ -362,6 +362,7 @@ private function addEmbedMapping(ClassMetadata $class, SimpleXMLElement $embed, 'collectionClass' => isset($attributes['collection-class']) ? (string) $attributes['collection-class'] : null, 'name' => (string) $attributes['field'], 'strategy' => (string) ($attributes['strategy'] ?? $defaultStrategy), + 'nullable' => isset($attributes['nullable']) ? ((string) $attributes['nullable'] === 'true') : false, ]; if (isset($attributes['field-name'])) { $mapping['fieldName'] = (string) $attributes['field-name']; @@ -419,6 +420,7 @@ private function addReferenceMapping(ClassMetadata $class, $reference, string $t 'limit' => isset($attributes['limit']) ? (int) $attributes['limit'] : null, 'skip' => isset($attributes['skip']) ? (int) $attributes['skip'] : null, 'prime' => [], + 'nullable' => isset($attributes['nullable']) ? ((string) $attributes['nullable'] === 'true') : false, ]; if (isset($attributes['field-name'])) { diff --git a/tests/Doctrine/ODM/MongoDB/Tests/Mapping/Driver/AbstractDriverTest.php b/tests/Doctrine/ODM/MongoDB/Tests/Mapping/Driver/AbstractDriverTest.php index 49afb70a0d..7aa7323476 100644 --- a/tests/Doctrine/ODM/MongoDB/Tests/Mapping/Driver/AbstractDriverTest.php +++ b/tests/Doctrine/ODM/MongoDB/Tests/Mapping/Driver/AbstractDriverTest.php @@ -12,6 +12,7 @@ use Documents\Profile; use PHPUnit\Framework\TestCase; use TestDocuments\EmbeddedDocument; +use TestDocuments\NullableFieldsDocument; use TestDocuments\PartialFilterDocument; use TestDocuments\PrimedCollectionDocument; use TestDocuments\QueryResultDocument; @@ -392,4 +393,119 @@ public function testCollectionPrimers() 'prime' => ['references'], ], $classMetadata->fieldMappings['inverseMappedBy']); } + + public function testNullableFieldsMapping() + { + $classMetadata = new ClassMetadata(NullableFieldsDocument::class); + $this->driver->loadMetadataForClass(NullableFieldsDocument::class, $classMetadata); + + $this->assertEquals([ + 'fieldName' => 'username', + 'name' => 'username', + 'type' => 'string', + 'isCascadeDetach' => false, + 'isCascadeMerge' => false, + 'isCascadePersist' => false, + 'isCascadeRefresh' => false, + 'isCascadeRemove' => false, + 'isInverseSide' => false, + 'isOwningSide' => true, + 'nullable' => true, + 'strategy' => ClassMetadata::STORAGE_STRATEGY_SET, + ], $classMetadata->fieldMappings['username']); + + $this->assertEquals([ + 'association' => ClassMetadata::EMBED_ONE, + 'fieldName' => 'address', + 'name' => 'address', + 'type' => ClassMetadata::ONE, + 'embedded' => true, + 'targetDocument' => Address::class, + 'collectionClass' => null, + 'isCascadeDetach' => true, + 'isCascadeMerge' => true, + 'isCascadePersist' => true, + 'isCascadeRefresh' => true, + 'isCascadeRemove' => true, + 'isInverseSide' => false, + 'isOwningSide' => true, + 'nullable' => true, + 'strategy' => ClassMetadata::STORAGE_STRATEGY_SET, + ], $classMetadata->fieldMappings['address']); + + $this->assertEquals([ + 'association' => ClassMetadata::EMBED_MANY, + 'fieldName' => 'phonenumbers', + 'name' => 'phonenumbers', + 'type' => ClassMetadata::MANY, + 'embedded' => true, + 'targetDocument' => Phonenumber::class, + 'collectionClass' => null, + 'isCascadeDetach' => true, + 'isCascadeMerge' => true, + 'isCascadePersist' => true, + 'isCascadeRefresh' => true, + 'isCascadeRemove' => true, + 'isInverseSide' => false, + 'isOwningSide' => true, + 'nullable' => true, + 'strategy' => ClassMetadata::STORAGE_STRATEGY_PUSH_ALL, + ], $classMetadata->fieldMappings['phonenumbers']); + + $this->assertEquals([ + 'association' => ClassMetadata::REFERENCE_ONE, + 'fieldName' => 'profile', + 'name' => 'profile', + 'type' => ClassMetadata::ONE, + 'reference' => true, + 'storeAs' => ClassMetadata::REFERENCE_STORE_AS_DB_REF, + 'targetDocument' => Profile::class, + 'collectionClass' => null, + 'cascade' => [], + 'isCascadeDetach' => false, + 'isCascadeMerge' => false, + 'isCascadePersist' => false, + 'isCascadeRefresh' => false, + 'isCascadeRemove' => false, + 'isInverseSide' => false, + 'isOwningSide' => true, + 'nullable' => true, + 'strategy' => ClassMetadata::STORAGE_STRATEGY_SET, + 'inversedBy' => null, + 'mappedBy' => null, + 'repositoryMethod' => null, + 'limit' => null, + 'skip' => null, + 'orphanRemoval' => false, + 'prime' => [], + ], $classMetadata->fieldMappings['profile']); + + $this->assertEquals([ + 'association' => ClassMetadata::REFERENCE_MANY, + 'fieldName' => 'groups', + 'name' => 'groups', + 'type' => ClassMetadata::MANY, + 'reference' => true, + 'storeAs' => ClassMetadata::REFERENCE_STORE_AS_DB_REF, + 'targetDocument' => Group::class, + 'collectionClass' => null, + 'cascade' => [], + 'isCascadeDetach' => false, + 'isCascadeMerge' => false, + 'isCascadePersist' => false, + 'isCascadeRefresh' => false, + 'isCascadeRemove' => false, + 'isInverseSide' => false, + 'isOwningSide' => true, + 'nullable' => true, + 'strategy' => ClassMetadata::STORAGE_STRATEGY_PUSH_ALL, + 'inversedBy' => null, + 'mappedBy' => null, + 'repositoryMethod' => null, + 'limit' => null, + 'skip' => null, + 'orphanRemoval' => false, + 'prime' => [], + ], $classMetadata->fieldMappings['groups']); + } } diff --git a/tests/Doctrine/ODM/MongoDB/Tests/Mapping/Driver/fixtures/NullableFieldsDocument.php b/tests/Doctrine/ODM/MongoDB/Tests/Mapping/Driver/fixtures/NullableFieldsDocument.php new file mode 100644 index 0000000000..fcd94015f6 --- /dev/null +++ b/tests/Doctrine/ODM/MongoDB/Tests/Mapping/Driver/fixtures/NullableFieldsDocument.php @@ -0,0 +1,20 @@ + + + + + + + + + + + + + From 05c9a252ef93aaea42cca221d96bf4eb8451674e Mon Sep 17 00:00:00 2001 From: Andreas Braun Date: Tue, 13 Apr 2021 10:51:37 +0200 Subject: [PATCH 09/16] Fix query preparation when in elemMatch (#2299) --- lib/Doctrine/ODM/MongoDB/Query/Expr.php | 77 +++++++++++++------ .../Tests/Functional/Ticket/GH1674Test.php | 57 ++++++++++++++ 2 files changed, 109 insertions(+), 25 deletions(-) create mode 100644 tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH1674Test.php diff --git a/lib/Doctrine/ODM/MongoDB/Query/Expr.php b/lib/Doctrine/ODM/MongoDB/Query/Expr.php index bd9d8c079b..e924a66fc8 100644 --- a/lib/Doctrine/ODM/MongoDB/Query/Expr.php +++ b/lib/Doctrine/ODM/MongoDB/Query/Expr.php @@ -95,12 +95,7 @@ public function addAnd($expression, ...$expressions): self $this->query['$and'] = array_merge( $this->query['$and'], - array_map( - static function ($expression) { - return $expression instanceof Expr ? $expression->getQuery() : $expression; - }, - func_get_args() - ) + func_get_args() ); return $this; @@ -123,9 +118,7 @@ public function addNor($expression, ...$expressions): self $this->query['$nor'] = array_merge( $this->query['$nor'], - array_map(static function ($expression) { - return $expression instanceof Expr ? $expression->getQuery() : $expression; - }, func_get_args()) + func_get_args() ); return $this; @@ -148,9 +141,7 @@ public function addOr($expression, ...$expressions): self $this->query['$or'] = array_merge( $this->query['$or'], - array_map(static function ($expression) { - return $expression instanceof Expr ? $expression->getQuery() : $expression; - }, func_get_args()) + func_get_args() ); return $this; @@ -175,12 +166,8 @@ public function addOr($expression, ...$expressions): self */ public function addToSet($valueOrExpression): self { - if ($valueOrExpression instanceof Expr) { - $valueOrExpression = $valueOrExpression->getQuery(); - } - $this->requiresCurrentField(); - $this->newObj['$addToSet'][$this->currentField] = $valueOrExpression; + $this->newObj['$addToSet'][$this->currentField] = static::convertExpression($valueOrExpression, $this->class); return $this; } @@ -414,7 +401,7 @@ public function each(array $values): self */ public function elemMatch($expression): self { - return $this->operator('$elemMatch', $expression instanceof Expr ? $expression->getQuery() : $expression); + return $this->operator('$elemMatch', $expression); } /** @@ -601,7 +588,7 @@ public function getQuery(): array { return $this->dm->getUnitOfWork() ->getDocumentPersister($this->class->name) - ->prepareQueryOrNewObj($this->query); + ->prepareQueryOrNewObj($this->convertExpressions($this->query)); } /** @@ -878,7 +865,7 @@ public function nearSphere($x, $y = null): self */ public function not($expression): self { - return $this->operator('$not', $expression instanceof Expr ? $expression->getQuery() : $expression); + return $this->operator('$not', $expression); } /** @@ -978,12 +965,8 @@ public function position(int $position): self */ public function pull($valueOrExpression): self { - if ($valueOrExpression instanceof Expr) { - $valueOrExpression = $valueOrExpression->getQuery(); - } - $this->requiresCurrentField(); - $this->newObj['$pull'][$this->currentField] = $valueOrExpression; + $this->newObj['$pull'][$this->currentField] = static::convertExpression($valueOrExpression, $this->class); return $this; } @@ -1420,4 +1403,48 @@ private function wrapEqualityCriteria(): void $query = ['$in' => [$query]]; } + + private function convertExpressions(array $query, ?ClassMetadata $classMetadata = null): array + { + if ($classMetadata === null) { + $classMetadata = $this->class; + } + + $convertedQuery = []; + foreach ($query as $key => $value) { + if (is_string($key) && $classMetadata->hasAssociation($key)) { + $targetDocument = $classMetadata->getAssociationTargetClass($key); + + if ($targetDocument) { + $fieldMetadata = $this->dm->getClassMetadata($targetDocument); + } + } + + if (is_array($value)) { + $convertedQuery[$key] = $this->convertExpressions($value, $fieldMetadata ?? $classMetadata); + continue; + } + + $convertedQuery[$key] = static::convertExpression($value, $fieldMetadata ?? $classMetadata); + } + + return $convertedQuery; + } + + /** + * Converts expression objects to query arrays. Non-expression values are + * returned unmodified. + * + * @param Expr|mixed $expression + */ + private static function convertExpression($expression, ClassMetadata $classMetadata) + { + if (! $expression instanceof Expr) { + return $expression; + } + + $expression->setClassMetadata($classMetadata); + + return $expression->getQuery(); + } } diff --git a/tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH1674Test.php b/tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH1674Test.php new file mode 100644 index 0000000000..71e4179207 --- /dev/null +++ b/tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH1674Test.php @@ -0,0 +1,57 @@ +dm->createQueryBuilder(GH1674Document::class); + $builder + ->field('embedded') + ->elemMatch( + $builder->expr() + ->field('id') + ->equals(1) + ); + + $this->assertSame( + [ + 'embedded' => [ + '$elemMatch' => ['id' => '1'], + ], + ], + $builder->getQueryArray() + ); + } +} + +/** @ODM\Document */ +class GH1674Document +{ + /** @ODM\Id */ + protected $id; + + /** @ODM\EmbedMany(targetDocument=GH1674Embedded::class) */ + protected $embedded; + + public function __construct() + { + $this->id = new ObjectId(); + $this->embedded = new ArrayCollection(); + } +} + +/** @ODM\EmbeddedDocument */ +class GH1674Embedded +{ + /** @ODM\Field */ + public $id = []; +} From 498478488c7d8539239e697b2260b4944b2d7b7a Mon Sep 17 00:00:00 2001 From: Andreas Braun Date: Tue, 13 Apr 2021 11:15:29 +0200 Subject: [PATCH 10/16] Fix preparation of $elemMatch operators in queries (#2298) --- .../MongoDB/Persisters/DocumentPersister.php | 7 ++- .../Functional/DocumentPersisterTest.php | 16 +++---- .../Tests/Functional/Ticket/GH2251Test.php | 48 +++++++++++++++++++ 3 files changed, 62 insertions(+), 9 deletions(-) create mode 100644 tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH2251Test.php diff --git a/lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php b/lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php index ca45cbcba4..dc481a414f 100644 --- a/lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php +++ b/lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php @@ -1321,7 +1321,12 @@ private function prepareQueryExpression(array $expression, ClassMetadata $class) continue; } - // Recursively process expressions within a $not operator + // Recursively process expressions within a $not or $elemMatch operator + if ($k === '$elemMatch' && is_array($v)) { + $expression[$k] = $this->prepareQueryOrNewObj($v, false); + continue; + } + if ($k === '$not' && is_array($v)) { $expression[$k] = $this->prepareQueryExpression($v, $class); continue; diff --git a/tests/Doctrine/ODM/MongoDB/Tests/Functional/DocumentPersisterTest.php b/tests/Doctrine/ODM/MongoDB/Tests/Functional/DocumentPersisterTest.php index d72f41f5dd..bc69e0d16d 100644 --- a/tests/Doctrine/ODM/MongoDB/Tests/Functional/DocumentPersisterTest.php +++ b/tests/Doctrine/ODM/MongoDB/Tests/Functional/DocumentPersisterTest.php @@ -198,7 +198,7 @@ public function testPrepareQueryOrNewObjWithHashIdAndInOperators($hashId) $this->assertEquals($expected, $documentPersister->prepareQueryOrNewObj($value)); $value = ['_id' => ['$elemMatch' => $hashId]]; - $expected = ['_id' => ['$elemMatch' => (object) $hashId]]; + $expected = ['_id' => ['$elemMatch' => $hashId]]; $this->assertEquals($expected, $documentPersister->prepareQueryOrNewObj($value)); @@ -208,7 +208,7 @@ public function testPrepareQueryOrNewObjWithHashIdAndInOperators($hashId) $this->assertEquals($expected, $documentPersister->prepareQueryOrNewObj($value)); $value = ['_id' => ['$not' => ['$elemMatch' => $hashId]]]; - $expected = ['_id' => ['$not' => ['$elemMatch' => (object) $hashId]]]; + $expected = ['_id' => ['$not' => ['$elemMatch' => $hashId]]]; $this->assertEquals($expected, $documentPersister->prepareQueryOrNewObj($value)); @@ -396,7 +396,7 @@ public function testPrepareQueryOrNewObjWithSimpleReferenceToTargetDocumentWithH $this->assertEquals($expected, $documentPersister->prepareQueryOrNewObj($value)); $value = ['simpleRef' => ['$elemMatch' => $hashId]]; - $expected = ['simpleRef' => ['$elemMatch' => (object) $hashId]]; + $expected = ['simpleRef' => ['$elemMatch' => $hashId]]; $this->assertEquals($expected, $documentPersister->prepareQueryOrNewObj($value)); @@ -406,7 +406,7 @@ public function testPrepareQueryOrNewObjWithSimpleReferenceToTargetDocumentWithH $this->assertEquals($expected, $documentPersister->prepareQueryOrNewObj($value)); $value = ['simpleRef' => ['$not' => ['$elemMatch' => $hashId]]]; - $expected = ['simpleRef' => ['$not' => ['$elemMatch' => (object) $hashId]]]; + $expected = ['simpleRef' => ['$not' => ['$elemMatch' => $hashId]]]; $this->assertEquals($expected, $documentPersister->prepareQueryOrNewObj($value)); @@ -473,7 +473,7 @@ public function testPrepareQueryOrNewObjWithDBRefReferenceToTargetDocumentWithHa $this->assertEquals($expected, $documentPersister->prepareQueryOrNewObj($value)); $value = ['complexRef.id' => ['$elemMatch' => $hashId]]; - $expected = ['complexRef.$id' => ['$elemMatch' => (object) $hashId]]; + $expected = ['complexRef.$id' => ['$elemMatch' => $hashId]]; $this->assertEquals($expected, $documentPersister->prepareQueryOrNewObj($value)); @@ -483,7 +483,7 @@ public function testPrepareQueryOrNewObjWithDBRefReferenceToTargetDocumentWithHa $this->assertEquals($expected, $documentPersister->prepareQueryOrNewObj($value)); $value = ['complexRef.id' => ['$not' => ['$elemMatch' => $hashId]]]; - $expected = ['complexRef.$id' => ['$not' => ['$elemMatch' => (object) $hashId]]]; + $expected = ['complexRef.$id' => ['$not' => ['$elemMatch' => $hashId]]]; $this->assertEquals($expected, $documentPersister->prepareQueryOrNewObj($value)); @@ -587,7 +587,7 @@ public function testPrepareQueryOrNewObjWithEmbeddedReferenceToTargetDocumentWit $this->assertEquals($expected, $documentPersister->prepareQueryOrNewObj($value)); $value = ['embeddedRef.id' => ['$elemMatch' => $hashId]]; - $expected = ['embeddedRef.id' => ['$elemMatch' => (object) $hashId]]; + $expected = ['embeddedRef.id' => ['$elemMatch' => $hashId]]; $this->assertEquals($expected, $documentPersister->prepareQueryOrNewObj($value)); @@ -597,7 +597,7 @@ public function testPrepareQueryOrNewObjWithEmbeddedReferenceToTargetDocumentWit $this->assertEquals($expected, $documentPersister->prepareQueryOrNewObj($value)); $value = ['embeddedRef.id' => ['$not' => ['$elemMatch' => $hashId]]]; - $expected = ['embeddedRef.id' => ['$not' => ['$elemMatch' => (object) $hashId]]]; + $expected = ['embeddedRef.id' => ['$not' => ['$elemMatch' => $hashId]]]; $this->assertEquals($expected, $documentPersister->prepareQueryOrNewObj($value)); diff --git a/tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH2251Test.php b/tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH2251Test.php new file mode 100644 index 0000000000..47ee6f0035 --- /dev/null +++ b/tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH2251Test.php @@ -0,0 +1,48 @@ +dm->createQueryBuilder(User::class); + + $objectIds = [ + new ObjectId('5fae9a775ef4492e3c72b3f3'), + new ObjectId('5fae9a775ef4492e3c72b3f4'), + ]; + + $notIn = $builder->expr()->notIn($objectIds); + $elemMatch = $builder->expr() + ->field($fieldName) + ->elemMatch($notIn); + + $builder->addNor( + $elemMatch + ); + + $this->assertSame( + [ + '$nor' => [ + [ + $fieldName => [ + '$elemMatch' => ['$nin' => $objectIds], + ], + ], + ], + ], + $builder->getQueryArray() + ); + } +} From 18d9ac7e2268016e41b8bf0dfafc1276ed18b31e Mon Sep 17 00:00:00 2001 From: Andreas Braun Date: Wed, 14 Apr 2021 19:14:29 +0200 Subject: [PATCH 11/16] Fix using null values in partial filter expressions (#2300) --- .../ODM/MongoDB/Mapping/Driver/XmlDriver.php | 63 ++++++++++--------- .../Mapping/Driver/AbstractDriverTest.php | 1 + ...estDocuments.PartialFilterDocument.dcm.xml | 1 + 3 files changed, 36 insertions(+), 29 deletions(-) diff --git a/lib/Doctrine/ODM/MongoDB/Mapping/Driver/XmlDriver.php b/lib/Doctrine/ODM/MongoDB/Mapping/Driver/XmlDriver.php index 691ac8fef3..52fd4b9a87 100644 --- a/lib/Doctrine/ODM/MongoDB/Mapping/Driver/XmlDriver.php +++ b/lib/Doctrine/ODM/MongoDB/Mapping/Driver/XmlDriver.php @@ -505,16 +505,7 @@ private function addIndex(ClassMetadata $class, SimpleXMLElement $xmlIndex): voi if (isset($xmlIndex->{'option'})) { foreach ($xmlIndex->{'option'} as $option) { - $value = (string) $option['value']; - if ($value === 'true') { - $value = true; - } elseif ($value === 'false') { - $value = false; - } elseif (is_numeric($value)) { - $value = preg_match('/^[-]?\d+$/', $value) ? (int) $value : (float) $value; - } - - $options[(string) $option['name']] = $value; + $options[(string) $option['name']] = $this->convertXMLElementValue((string) $option['value']); } } @@ -564,15 +555,7 @@ private function getPartialFilterExpression(SimpleXMLElement $fields): array $value = $nestedExpression; } else { - $value = trim((string) $field['value']); - } - - if ($value === 'true') { - $value = true; - } elseif ($value === 'false') { - $value = false; - } elseif (is_numeric($value)) { - $value = preg_match('/^[-]?\d+$/', $value) ? (int) $value : (float) $value; + $value = $this->convertXMLElementValue((string) $field['value']); } $partialFilterExpression[(string) $field['name']] = $operator ? ['$' . $operator => $value] : $value; @@ -581,6 +564,37 @@ private function getPartialFilterExpression(SimpleXMLElement $fields): array return $partialFilterExpression; } + /** + * Converts XML strings to scalar values. + * + * Special strings "false", "true", and "null" are converted to their + * respective values. Numeric strings are cast to int or float depending on + * whether they contain decimal separators or not. + * + * @return scalar|null + */ + private function convertXMLElementValue(string $value) + { + $value = trim($value); + + switch ($value) { + case 'true': + return true; + + case 'false': + return false; + + case 'null': + return null; + } + + if (! is_numeric($value)) { + return $value; + } + + return preg_match('/^[-]?\d+$/', $value) ? (int) $value : (float) $value; + } + private function setShardKey(ClassMetadata $class, SimpleXMLElement $xmlShardkey): void { $attributes = $xmlShardkey->attributes(); @@ -601,16 +615,7 @@ private function setShardKey(ClassMetadata $class, SimpleXMLElement $xmlShardkey if (isset($xmlShardkey->{'option'})) { foreach ($xmlShardkey->{'option'} as $option) { - $value = (string) $option['value']; - if ($value === 'true') { - $value = true; - } elseif ($value === 'false') { - $value = false; - } elseif (is_numeric($value)) { - $value = preg_match('/^[-]?\d+$/', $value) ? (int) $value : (float) $value; - } - - $options[(string) $option['name']] = $value; + $options[(string) $option['name']] = $this->convertXMLElementValue((string) $option['value']); } } diff --git a/tests/Doctrine/ODM/MongoDB/Tests/Mapping/Driver/AbstractDriverTest.php b/tests/Doctrine/ODM/MongoDB/Tests/Mapping/Driver/AbstractDriverTest.php index 7aa7323476..d46eda76f2 100644 --- a/tests/Doctrine/ODM/MongoDB/Tests/Mapping/Driver/AbstractDriverTest.php +++ b/tests/Doctrine/ODM/MongoDB/Tests/Mapping/Driver/AbstractDriverTest.php @@ -307,6 +307,7 @@ public function testPartialFilterExpressions() 'partialFilterExpression' => [ 'version' => ['$gt' => 1], 'discr' => ['$eq' => 'default'], + 'parent' => ['$eq' => null], ], ], ], diff --git a/tests/Doctrine/ODM/MongoDB/Tests/Mapping/Driver/fixtures/xml/TestDocuments.PartialFilterDocument.dcm.xml b/tests/Doctrine/ODM/MongoDB/Tests/Mapping/Driver/fixtures/xml/TestDocuments.PartialFilterDocument.dcm.xml index 9075873e0b..82f8a3ef0b 100644 --- a/tests/Doctrine/ODM/MongoDB/Tests/Mapping/Driver/fixtures/xml/TestDocuments.PartialFilterDocument.dcm.xml +++ b/tests/Doctrine/ODM/MongoDB/Tests/Mapping/Driver/fixtures/xml/TestDocuments.PartialFilterDocument.dcm.xml @@ -13,6 +13,7 @@ + From 5ca00af8b053404f1ae1a0662d702d1959f9a49d Mon Sep 17 00:00:00 2001 From: Andreas Braun Date: Thu, 22 Apr 2021 13:54:32 +0200 Subject: [PATCH 12/16] Fix errors with nullable typed associations (#2302) * Fix initialising nullable associations * Fix error when merging documents with uninitialised typed properties --- .../ODM/MongoDB/Hydrator/HydratorFactory.php | 49 ++++++----- lib/Doctrine/ODM/MongoDB/UnitOfWork.php | 5 ++ .../Tests/Functional/TypedPropertiesTest.php | 86 ++++++++++++++----- tests/Documents74/TypedDocument.php | 49 ++++------- 4 files changed, 114 insertions(+), 75 deletions(-) diff --git a/lib/Doctrine/ODM/MongoDB/Hydrator/HydratorFactory.php b/lib/Doctrine/ODM/MongoDB/Hydrator/HydratorFactory.php index c9019488e2..d3bffd9906 100644 --- a/lib/Doctrine/ODM/MongoDB/Hydrator/HydratorFactory.php +++ b/lib/Doctrine/ODM/MongoDB/Hydrator/HydratorFactory.php @@ -250,18 +250,20 @@ private function generateHydratorClass(ClassMetadata $class, string $hydratorCla <<class->fieldMappings['%2\$s']['nullable']) && array_key_exists('%1\$s', \$data))) { + \$return = \$data['%1\$s']; + if (\$return !== null) { + if (\$this->class->fieldMappings['%2\$s']['storeAs'] !== ClassMetadata::REFERENCE_STORE_AS_ID && ! is_array(\$return)) { + throw HydratorException::associationTypeMismatch('%3\$s', '%1\$s', 'array', gettype(\$return)); + } - if (\$this->class->fieldMappings['%2\$s']['storeAs'] !== ClassMetadata::REFERENCE_STORE_AS_ID && ! is_array(\$reference)) { - throw HydratorException::associationTypeMismatch('%3\$s', '%1\$s', 'array', gettype(\$reference)); + \$className = \$this->unitOfWork->getClassNameForAssociation(\$this->class->fieldMappings['%2\$s'], \$return); + \$identifier = ClassMetadata::getReferenceId(\$return, \$this->class->fieldMappings['%2\$s']['storeAs']); + \$targetMetadata = \$this->dm->getClassMetadata(\$className); + \$id = \$targetMetadata->getPHPIdentifierValue(\$identifier); + \$return = \$this->dm->getReference(\$className, \$id); } - \$className = \$this->unitOfWork->getClassNameForAssociation(\$this->class->fieldMappings['%2\$s'], \$reference); - \$identifier = ClassMetadata::getReferenceId(\$reference, \$this->class->fieldMappings['%2\$s']['storeAs']); - \$targetMetadata = \$this->dm->getClassMetadata(\$className); - \$id = \$targetMetadata->getPHPIdentifierValue(\$identifier); - \$return = \$this->dm->getReference(\$className, \$id); \$this->class->reflFields['%2\$s']->setValue(\$document, \$return); \$hydratedData['%2\$s'] = \$return; } @@ -344,24 +346,27 @@ private function generateHydratorClass(ClassMetadata $class, string $hydratorCla <<class->fieldMappings['%2\$s']['nullable']) && array_key_exists('%1\$s', \$data))) { + \$return = \$data['%1\$s']; + if (\$return !== null) { + \$embeddedDocument = \$return; - if (! is_array(\$embeddedDocument)) { - throw HydratorException::associationTypeMismatch('%3\$s', '%1\$s', 'array', gettype(\$embeddedDocument)); - } + if (! is_array(\$embeddedDocument)) { + throw HydratorException::associationTypeMismatch('%3\$s', '%1\$s', 'array', gettype(\$embeddedDocument)); + } - \$className = \$this->unitOfWork->getClassNameForAssociation(\$this->class->fieldMappings['%2\$s'], \$embeddedDocument); - \$embeddedMetadata = \$this->dm->getClassMetadata(\$className); - \$return = \$embeddedMetadata->newInstance(); + \$className = \$this->unitOfWork->getClassNameForAssociation(\$this->class->fieldMappings['%2\$s'], \$embeddedDocument); + \$embeddedMetadata = \$this->dm->getClassMetadata(\$className); + \$return = \$embeddedMetadata->newInstance(); - \$this->unitOfWork->setParentAssociation(\$return, \$this->class->fieldMappings['%2\$s'], \$document, '%1\$s'); + \$this->unitOfWork->setParentAssociation(\$return, \$this->class->fieldMappings['%2\$s'], \$document, '%1\$s'); - \$embeddedData = \$this->dm->getHydratorFactory()->hydrate(\$return, \$embeddedDocument, \$hints); - \$embeddedId = \$embeddedMetadata->identifier && isset(\$embeddedData[\$embeddedMetadata->identifier]) ? \$embeddedData[\$embeddedMetadata->identifier] : null; + \$embeddedData = \$this->dm->getHydratorFactory()->hydrate(\$return, \$embeddedDocument, \$hints); + \$embeddedId = \$embeddedMetadata->identifier && isset(\$embeddedData[\$embeddedMetadata->identifier]) ? \$embeddedData[\$embeddedMetadata->identifier] : null; - if (empty(\$hints[Query::HINT_READ_ONLY])) { - \$this->unitOfWork->registerManaged(\$return, \$embeddedId, \$embeddedData); + if (empty(\$hints[Query::HINT_READ_ONLY])) { + \$this->unitOfWork->registerManaged(\$return, \$embeddedId, \$embeddedData); + } } \$this->class->reflFields['%2\$s']->setValue(\$document, \$return); diff --git a/lib/Doctrine/ODM/MongoDB/UnitOfWork.php b/lib/Doctrine/ODM/MongoDB/UnitOfWork.php index 1bca49dde4..711f5bc22d 100644 --- a/lib/Doctrine/ODM/MongoDB/UnitOfWork.php +++ b/lib/Doctrine/ODM/MongoDB/UnitOfWork.php @@ -1830,6 +1830,11 @@ private function doMerge(object $document, array &$visited, ?object $prevManaged $name = $nativeReflection->name; $prop = $this->reflectionService->getAccessibleProperty($class->name, $name); assert($prop instanceof ReflectionProperty); + + if (method_exists($prop, 'isInitialized') && ! $prop->isInitialized($document)) { + continue; + } + if (! isset($class->associationMappings[$name])) { if (! $class->isIdentifier($name)) { $prop->setValue($managedCopy, $prop->getValue($document)); diff --git a/tests/Doctrine/ODM/MongoDB/Tests/Functional/TypedPropertiesTest.php b/tests/Doctrine/ODM/MongoDB/Tests/Functional/TypedPropertiesTest.php index eb8e2a2f0f..940cb3c1b3 100644 --- a/tests/Doctrine/ODM/MongoDB/Tests/Functional/TypedPropertiesTest.php +++ b/tests/Doctrine/ODM/MongoDB/Tests/Functional/TypedPropertiesTest.php @@ -26,52 +26,96 @@ public function setUp(): void public function testPersistNew(): void { - $doc = new TypedDocument(); - $doc->setName('Maciej'); - $doc->setEmbedOne(new TypedEmbeddedDocument('The answer', 42)); + $ref = new TypedDocument(); + $ref->name = 'alcaeus'; + $this->dm->persist($ref); + + $doc = new TypedDocument(); + $doc->name = 'Maciej'; + $doc->embedOne = new TypedEmbeddedDocument('The answer', 42); + $doc->referenceOne = $ref; $doc->getEmbedMany()->add(new TypedEmbeddedDocument('Lucky number', 7)); $this->dm->persist($doc); $this->dm->flush(); $this->dm->clear(); - $saved = $this->dm->find(TypedDocument::class, $doc->getId()); + $ref = $this->dm->find(TypedDocument::class, $ref->id); + $saved = $this->dm->find(TypedDocument::class, $doc->id); assert($saved instanceof TypedDocument); - $this->assertEquals($doc->getId(), $saved->getId()); - $this->assertSame($doc->getName(), $saved->getName()); - $this->assertEquals($doc->getEmbedOne(), $saved->getEmbedOne()); + $this->assertEquals($doc->id, $saved->id); + $this->assertSame($doc->name, $saved->name); + $this->assertEquals($doc->embedOne, $saved->embedOne); + $this->assertSame($ref, $saved->referenceOne); $this->assertEquals($doc->getEmbedMany()->getValues(), $saved->getEmbedMany()->getValues()); } public function testMerge(): void { - $doc = new TypedDocument(); - $doc->setId((string) new ObjectId()); - $doc->setName('Maciej'); - $doc->setEmbedOne(new TypedEmbeddedDocument('The answer', 42)); + $ref = new TypedDocument(); + $ref->name = 'alcaeus'; + $this->dm->persist($ref); + + $doc = new TypedDocument(); + $doc->id = (string) new ObjectId(); + $doc->name = 'Maciej'; + $doc->embedOne = new TypedEmbeddedDocument('The answer', 42); + $doc->referenceOne = $ref; + $doc->getEmbedMany()->add(new TypedEmbeddedDocument('Lucky number', 7)); + + $merged = $this->dm->merge($doc); + assert($merged instanceof TypedDocument); + $this->assertEquals($doc->id, $merged->id); + $this->assertSame($doc->name, $merged->name); + $this->assertEquals($doc->embedOne, $merged->embedOne); + $this->assertEquals($doc->referenceOne, $merged->referenceOne); + $this->assertEquals($doc->getEmbedMany()->getValues(), $merged->getEmbedMany()->getValues()); + } + + public function testMergeWithUninitializedAssociations(): void + { + $doc = new TypedDocument(); + $doc->id = (string) new ObjectId(); + $doc->name = 'Maciej'; $doc->getEmbedMany()->add(new TypedEmbeddedDocument('Lucky number', 7)); $merged = $this->dm->merge($doc); - $this->assertEquals($doc->getId(), $merged->getId()); - $this->assertSame($doc->getName(), $merged->getName()); - $this->assertEquals($doc->getEmbedOne(), $merged->getEmbedOne()); + assert($merged instanceof TypedDocument); + $this->assertEquals($doc->id, $merged->id); + $this->assertSame($doc->name, $merged->name); $this->assertEquals($doc->getEmbedMany()->getValues(), $merged->getEmbedMany()->getValues()); } public function testProxying(): void { - $doc = new TypedDocument(); - $doc->setName('Maciej'); - $doc->setEmbedOne(new TypedEmbeddedDocument('The answer', 42)); + $doc = new TypedDocument(); + $doc->name = 'Maciej'; + $doc->embedOne = new TypedEmbeddedDocument('The answer', 42); $doc->getEmbedMany()->add(new TypedEmbeddedDocument('Lucky number', 7)); $this->dm->persist($doc); $this->dm->flush(); $this->dm->clear(); - $proxy = $this->dm->getReference(TypedDocument::class, $doc->getId()); + $proxy = $this->dm->getReference(TypedDocument::class, $doc->id); assert($proxy instanceof TypedDocument); - $this->assertEquals($doc->getId(), $proxy->getId()); - $this->assertSame($doc->getName(), $proxy->getName()); - $this->assertEquals($doc->getEmbedOne(), $proxy->getEmbedOne()); + $this->assertEquals($doc->id, $proxy->id); + $this->assertSame($doc->name, $proxy->name); + $this->assertEquals($doc->embedOne, $proxy->embedOne); $this->assertEquals($doc->getEmbedMany()->getValues(), $proxy->getEmbedMany()->getValues()); } + + public function testNullableProperties(): void + { + $doc = new TypedDocument(); + $doc->name = 'webmozart'; + $this->dm->persist($doc); + $this->dm->flush(); + $this->dm->clear(); + + $saved = $this->dm->find(TypedDocument::class, $doc->id); + assert($saved instanceof TypedDocument); + $this->assertNull($saved->nullableEmbedOne); + $this->assertNull($saved->initializedNullableEmbedOne); + $this->assertNull($saved->nullableReferenceOne); + $this->assertNull($saved->initializedNullableReferenceOne); + } } diff --git a/tests/Documents74/TypedDocument.php b/tests/Documents74/TypedDocument.php index 108b984ee8..8dedacead0 100644 --- a/tests/Documents74/TypedDocument.php +++ b/tests/Documents74/TypedDocument.php @@ -14,50 +14,35 @@ class TypedDocument { /** @ODM\Id() */ - private string $id; + public string $id; /** @ODM\Field(type="string") */ - private string $name; + public string $name; /** @ODM\EmbedOne(targetDocument=TypedEmbeddedDocument::class) */ - private TypedEmbeddedDocument $embedOne; + public TypedEmbeddedDocument $embedOne; - /** @ODM\EmbedMany(targetDocument=TypedEmbeddedDocument::class) */ - private Collection $embedMany; + /** @ODM\EmbedOne(targetDocument=TypedEmbeddedDocument::class, nullable=true) */ + public ?TypedEmbeddedDocument $nullableEmbedOne; - public function __construct() - { - $this->embedMany = new ArrayCollection(); - } + /** @ODM\EmbedOne(targetDocument=TypedEmbeddedDocument::class, nullable=true) */ + public ?TypedEmbeddedDocument $initializedNullableEmbedOne = null; - public function getId(): string - { - return $this->id; - } + /** @ODM\ReferenceOne(targetDocument=TypedDocument::class) */ + public TypedDocument $referenceOne; - public function setId(string $id): void - { - $this->id = $id; - } + /** @ODM\ReferenceOne(targetDocument=TypedDocument::class, nullable=true) */ + public ?TypedDocument $nullableReferenceOne; - public function getName(): string - { - return $this->name; - } + /** @ODM\ReferenceOne(targetDocument=TypedDocument::class, nullable=true) */ + public ?TypedDocument $initializedNullableReferenceOne = null; - public function setName(string $name): void - { - $this->name = $name; - } - - public function getEmbedOne(): TypedEmbeddedDocument - { - return $this->embedOne; - } + /** @ODM\EmbedMany(targetDocument=TypedEmbeddedDocument::class) */ + private Collection $embedMany; - public function setEmbedOne(TypedEmbeddedDocument $embedOne): void + public function __construct() { - $this->embedOne = $embedOne; + $this->embedMany = new ArrayCollection(); } public function getEmbedMany(): Collection From f225a0cfc5ba878aba3e32b7f5fd70facc2d4eee Mon Sep 17 00:00:00 2001 From: Fran Moreno Date: Thu, 20 May 2021 17:10:26 +0200 Subject: [PATCH 13/16] Allow mixed value in $not operator (#2307) --- lib/Doctrine/ODM/MongoDB/Query/Builder.php | 6 ++--- lib/Doctrine/ODM/MongoDB/Query/Expr.php | 6 ++--- .../MongoDB/Tests/Functional/QueryTest.php | 22 +++++++++++++++++++ .../ODM/MongoDB/Tests/Query/BuilderTest.php | 14 ++++++++++++ 4 files changed, 42 insertions(+), 6 deletions(-) diff --git a/lib/Doctrine/ODM/MongoDB/Query/Builder.php b/lib/Doctrine/ODM/MongoDB/Query/Builder.php index a800303a58..2c959178ce 100644 --- a/lib/Doctrine/ODM/MongoDB/Query/Builder.php +++ b/lib/Doctrine/ODM/MongoDB/Query/Builder.php @@ -1028,11 +1028,11 @@ public function nearSphere($x, $y = null): self * @see Expr::not() * @see https://docs.mongodb.com/manual/reference/operator/not/ * - * @param array|Expr $expression + * @param array|Expr|mixed $valueOrExpression */ - public function not($expression): self + public function not($valueOrExpression): self { - $this->expr->not($expression); + $this->expr->not($valueOrExpression); return $this; } diff --git a/lib/Doctrine/ODM/MongoDB/Query/Expr.php b/lib/Doctrine/ODM/MongoDB/Query/Expr.php index e924a66fc8..86e47f4dfd 100644 --- a/lib/Doctrine/ODM/MongoDB/Query/Expr.php +++ b/lib/Doctrine/ODM/MongoDB/Query/Expr.php @@ -861,11 +861,11 @@ public function nearSphere($x, $y = null): self * @see Builder::not() * @see https://docs.mongodb.com/manual/reference/operator/not/ * - * @param array|Expr $expression + * @param array|Expr|mixed $valueOrExpression */ - public function not($expression): self + public function not($valueOrExpression): self { - return $this->operator('$not', $expression); + return $this->operator('$not', $valueOrExpression); } /** diff --git a/tests/Doctrine/ODM/MongoDB/Tests/Functional/QueryTest.php b/tests/Doctrine/ODM/MongoDB/Tests/Functional/QueryTest.php index 9bb301d14b..1b16577719 100644 --- a/tests/Doctrine/ODM/MongoDB/Tests/Functional/QueryTest.php +++ b/tests/Doctrine/ODM/MongoDB/Tests/Functional/QueryTest.php @@ -15,6 +15,7 @@ use InvalidArgumentException; use IteratorAggregate; use MongoDB\BSON\ObjectId; +use MongoDB\BSON\Regex; use MongoDB\BSON\UTCDateTime; use function array_values; @@ -97,6 +98,27 @@ public function testAddNot() $this->assertNotNull($user); } + public function testNotAllowsRegex() + { + $user = new User(); + $user->setUsername('boo'); + + $this->dm->persist($user); + $this->dm->flush(); + + $qb = $this->dm->createQueryBuilder(User::class); + $qb->field('username')->not(new Regex('Boo', 'i')); + $query = $qb->getQuery(); + $user = $query->getSingleResult(); + $this->assertNull($user); + + $qb = $this->dm->createQueryBuilder(User::class); + $qb->field('username')->not(new Regex('Boo')); + $query = $qb->getQuery(); + $user = $query->getSingleResult(); + $this->assertNotNull($user); + } + public function testDistinct() { $user = new User(); diff --git a/tests/Doctrine/ODM/MongoDB/Tests/Query/BuilderTest.php b/tests/Doctrine/ODM/MongoDB/Tests/Query/BuilderTest.php index e3dfc629c5..491f4aaad7 100644 --- a/tests/Doctrine/ODM/MongoDB/Tests/Query/BuilderTest.php +++ b/tests/Doctrine/ODM/MongoDB/Tests/Query/BuilderTest.php @@ -19,6 +19,7 @@ use InvalidArgumentException; use IteratorAggregate; use MongoDB\BSON\ObjectId; +use MongoDB\BSON\Regex; use MongoDB\Driver\ReadPreference; use ReflectionProperty; @@ -299,6 +300,19 @@ public function testAddNot() $this->assertEquals($expected, $qb->getQueryArray()); } + public function testNotAllowsRegex() + { + $qb = $this->getTestQueryBuilder(); + $qb->field('username')->not(new Regex('Boo', 'i')); + + $expected = [ + 'username' => [ + '$not' => new Regex('Boo', 'i'), + ], + ]; + $this->assertEquals($expected, $qb->getQueryArray()); + } + public function testFindQuery() { $qb = $this->getTestQueryBuilder() From ccf26d9e023387e9aae86951186c043b3222fe70 Mon Sep 17 00:00:00 2001 From: Andreas Braun Date: Wed, 2 Jun 2021 06:05:27 +0200 Subject: [PATCH 14/16] [2.2] Fix builds (#2319) --- .github/workflows/coding-standards.yml | 5 ++ .github/workflows/continuous-integration.yml | 12 ++-- .github/workflows/performance.yml | 6 ++ .github/workflows/static-analysis.yml | 12 ++++ composer.json | 2 +- phpbench.json | 4 +- phpstan-baseline.neon | 61 +++++++++++++------- psalm-baseline.xml | 31 ++++++++++ psalm.xml | 1 + 9 files changed, 104 insertions(+), 30 deletions(-) create mode 100644 psalm-baseline.xml diff --git a/.github/workflows/coding-standards.yml b/.github/workflows/coding-standards.yml index 9dc3a9a6a9..02dca84361 100644 --- a/.github/workflows/coding-standards.yml +++ b/.github/workflows/coding-standards.yml @@ -50,6 +50,11 @@ jobs: - name: "Install dependencies with Composer" uses: "ramsey/composer-install@v1" + - name: "Upload composer.lock as build artifact" + uses: actions/upload-artifact@v2 + with: + name: composer.lock + path: composer.lock # https://github.com/doctrine/.github/issues/3 - name: "Run PHP_CodeSniffer" diff --git a/.github/workflows/continuous-integration.yml b/.github/workflows/continuous-integration.yml index 88546fb2cd..e495b70cc6 100644 --- a/.github/workflows/continuous-integration.yml +++ b/.github/workflows/continuous-integration.yml @@ -12,12 +12,10 @@ env: jobs: phpunit: name: "PHPUnit" - runs-on: "${{ matrix.os }}" + runs-on: "ubuntu-18.04" strategy: matrix: - os: - - "ubuntu-18.04" php-version: - "7.2" - "7.3" @@ -35,13 +33,11 @@ jobs: - "highest" include: - deps: "lowest" - os: "ubuntu-16.04" php-version: "7.2" mongodb-version: "3.6" driver-version: "1.5.0" topology: "server" - topology: "sharded_cluster" - os: "ubuntu-18.04" php-version: "8.0" mongodb-version: "4.4" driver-version: "stable" @@ -86,6 +82,12 @@ jobs: dependency-versions: "${{ matrix.dependencies }}" composer-options: "--prefer-dist" + - name: "Upload composer.lock as build artifact" + uses: actions/upload-artifact@v2 + with: + name: composer.lock + path: composer.lock + - id: setup-mongodb uses: mongodb-labs/drivers-evergreen-tools@master with: diff --git a/.github/workflows/performance.yml b/.github/workflows/performance.yml index 1e9792830b..3f0325a942 100644 --- a/.github/workflows/performance.yml +++ b/.github/workflows/performance.yml @@ -54,6 +54,12 @@ jobs: - name: "Install dependencies with Composer" uses: "ramsey/composer-install@v1" + - name: "Upload composer.lock as build artifact" + uses: actions/upload-artifact@v2 + with: + name: composer.lock + path: composer.lock + # https://github.com/doctrine/.github/issues/3 - name: "Run PHP_CodeSniffer" run: "vendor/bin/phpbench run --report=default --revs=100 --iterations=5 --report=aggregate" diff --git a/.github/workflows/static-analysis.yml b/.github/workflows/static-analysis.yml index 5109efe477..e0ad865ade 100644 --- a/.github/workflows/static-analysis.yml +++ b/.github/workflows/static-analysis.yml @@ -49,6 +49,12 @@ jobs: - name: "Install dependencies with Composer" uses: "ramsey/composer-install@v1" + - name: "Upload composer.lock as build artifact" + uses: actions/upload-artifact@v2 + with: + name: composer.lock + path: composer.lock + - name: "Run a static analysis with phpstan/phpstan" run: "vendor/bin/phpstan analyse --error-format=github" @@ -75,5 +81,11 @@ jobs: - name: "Install dependencies with Composer" uses: "ramsey/composer-install@v1" + - name: "Upload composer.lock as build artifact" + uses: actions/upload-artifact@v2 + with: + name: composer.lock + path: composer.lock + - name: "Run a static analysis with vimeo/psalm" run: "vendor/bin/psalm --show-info=false --stats --output-format=github --threads=$(nproc) --php-version=${{ matrix.php-version }}" diff --git a/composer.json b/composer.json index 6b2d35d8b3..006f85f2e6 100644 --- a/composer.json +++ b/composer.json @@ -41,7 +41,7 @@ "ext-bcmath": "*", "doctrine/coding-standard": "^8.2", "jmikola/geojson": "^1.0", - "phpbench/phpbench": "^1.0.0-alpha3", + "phpbench/phpbench": "^1.0.0", "phpstan/phpstan": "^0.12.32", "phpunit/phpunit": "^8.5 || ^9", "squizlabs/php_codesniffer": "^3.5", diff --git a/phpbench.json b/phpbench.json index 8a9607dd98..14b72df1c3 100644 --- a/phpbench.json +++ b/phpbench.json @@ -1,4 +1,4 @@ { - "bootstrap": "tests/bootstrap.php", - "path": "benchmark" + "runner.bootstrap": "tests/bootstrap.php", + "runner.path": "benchmark" } diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 37516471fd..45d34759ca 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -1,27 +1,44 @@ parameters: - ignoreErrors: - # Adding a parameter would be BC-break - - - message: "#^PHPDoc tag @param references unknown parameter\\: \\$applyFilters$#" - count: 1 - path: lib/Doctrine/ODM/MongoDB/Aggregation/Builder.php + ignoreErrors: + # Adding a parameter would be BC-break + - + message: "#^PHPDoc tag @param references unknown parameter\\: \\$applyFilters$#" + count: 1 + path: lib/Doctrine/ODM/MongoDB/Aggregation/Builder.php - # Making classes final as suggested would be a BC-break - - - message: "#^Unsafe usage of new static\\(\\)\\.$#" - paths: - - lib/Doctrine/ODM/MongoDB/Aggregation/Expr.php - - lib/Doctrine/ODM/MongoDB/DocumentManager.php + # Making classes final as suggested would be a BC-break + - + message: "#^Unsafe usage of new static\\(\\)\\.$#" + count: 1 + path: lib/Doctrine/ODM/MongoDB/Aggregation/Expr.php - # The limit option in GeoNear has been removed in MongoDB 4.2 in favor of $limit stage - - - message: "#^Return type \\(Doctrine\\\\ODM\\\\MongoDB\\\\Aggregation\\\\Stage\\\\GeoNear\\) of method Doctrine\\\\ODM\\\\MongoDB\\\\Aggregation\\\\Stage\\\\GeoNear\\:\\:limit\\(\\) should be compatible with return type \\(Doctrine\\\\ODM\\\\MongoDB\\\\Aggregation\\\\Stage\\\\Limit\\) of method Doctrine\\\\ODM\\\\MongoDB\\\\Aggregation\\\\Stage\\:\\:limit\\(\\)$#" - count: 1 - path: lib/Doctrine/ODM/MongoDB/Aggregation/Stage/GeoNear.php + # The limit option in GeoNear has been removed in MongoDB 4.2 in favor of $limit stage + - + message: "#^Return type \\(Doctrine\\\\ODM\\\\MongoDB\\\\Aggregation\\\\Stage\\\\GeoNear\\) of method Doctrine\\\\ODM\\\\MongoDB\\\\Aggregation\\\\Stage\\\\GeoNear\\:\\:limit\\(\\) should be compatible with return type \\(Doctrine\\\\ODM\\\\MongoDB\\\\Aggregation\\\\Stage\\\\Limit\\) of method Doctrine\\\\ODM\\\\MongoDB\\\\Aggregation\\\\Stage\\:\\:limit\\(\\)$#" + count: 1 + path: lib/Doctrine/ODM/MongoDB/Aggregation/Stage/GeoNear.php - # Union types cannot be added yet - - - message: "#^Result of && is always false\\.$#" - count: 1 - path: lib/Doctrine/ODM/MongoDB/Query/Builder.php + # Fixed in 2.3 + - + message: "#^Method Doctrine\\\\ODM\\\\MongoDB\\\\DocumentManager\\:\\:find\\(\\) should return T of object\\|null but returns object\\|null\\.$#" + count: 1 + path: lib/Doctrine/ODM/MongoDB/DocumentManager.php + + # Fixed in 2.3 + - + message: "#^Return type \\(Doctrine\\\\ODM\\\\MongoDB\\\\Mapping\\\\ClassMetadataFactory\\) of method Doctrine\\\\ODM\\\\MongoDB\\\\DocumentManager\\:\\:getMetadataFactory\\(\\) should be compatible with return type \\(Doctrine\\\\Persistence\\\\Mapping\\\\ClassMetadataFactory\\\\>\\) of method Doctrine\\\\Persistence\\\\ObjectManager\\:\\:getMetadataFactory\\(\\)$#" + count: 1 + path: lib/Doctrine/ODM/MongoDB/DocumentManager.php + + # Making classes final as suggested would be a BC-break + - + message: "#^Unsafe usage of new static\\(\\)\\.$#" + count: 1 + path: lib/Doctrine/ODM/MongoDB/DocumentManager.php + + # Union types cannot be added yet + - + message: "#^Result of && is always false\\.$#" + count: 1 + path: lib/Doctrine/ODM/MongoDB/Query/Builder.php diff --git a/psalm-baseline.xml b/psalm-baseline.xml new file mode 100644 index 0000000000..8e22801a5e --- /dev/null +++ b/psalm-baseline.xml @@ -0,0 +1,31 @@ + + + + + $document + $document + $document + $document + $document + $document + $documentName + $documentName + + + + + $fieldName + $fieldName + + + + + $value + + + + + $compositeExpr + + + diff --git a/psalm.xml b/psalm.xml index c7a71ee0d0..265650b45d 100644 --- a/psalm.xml +++ b/psalm.xml @@ -5,6 +5,7 @@ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns="https://getpsalm.org/schema/config" xsi:schemaLocation="https://getpsalm.org/schema/config vendor/vimeo/psalm/config.xsd" + errorBaseline="psalm-baseline.xml" > From 5aa06c040833c2de4dcb2cc5bd023265db3b8d9c Mon Sep 17 00:00:00 2001 From: Andreas Braun Date: Tue, 15 Jun 2021 10:36:48 +0200 Subject: [PATCH 15/16] Fix wrong handling for nullable fields in upsert and update (#2318) * Comprehensively test nullable behaviour for embedOne Co-authored-by: wuchen90 * Fix handling of nullable fields for upsert Co-authored-by: wuchen90 --- .../MongoDB/Persisters/PersistenceBuilder.php | 98 +++++++------- .../MongoDB/Tests/Functional/EmbeddedTest.php | 1 + .../Tests/Functional/Ticket/GH2310Test.php | 124 ++++++++++++++++++ .../MongoDB/Tests/Functional/UpsertTest.php | 71 +++++++++- tests/Documents/User.php | 11 +- tests/Documents74/GH2310Container.php | 34 +++++ 6 files changed, 280 insertions(+), 59 deletions(-) create mode 100644 tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH2310Test.php create mode 100644 tests/Documents74/GH2310Container.php diff --git a/lib/Doctrine/ODM/MongoDB/Persisters/PersistenceBuilder.php b/lib/Doctrine/ODM/MongoDB/Persisters/PersistenceBuilder.php index adf1c9ef3b..4d405a1a03 100644 --- a/lib/Doctrine/ODM/MongoDB/Persisters/PersistenceBuilder.php +++ b/lib/Doctrine/ODM/MongoDB/Persisters/PersistenceBuilder.php @@ -68,14 +68,11 @@ public function prepareInsertData($document) foreach ($class->fieldMappings as $mapping) { $new = $changeset[$mapping['fieldName']][1] ?? null; - if ($new === null && $mapping['nullable']) { - $insertData[$mapping['name']] = null; - } - - /* Nothing more to do for null values, since we're either storing - * them (if nullable was true) or not. - */ if ($new === null) { + if ($mapping['nullable']) { + $insertData[$mapping['name']] = null; + } + continue; } @@ -143,34 +140,36 @@ public function prepareUpdateData($document) [$old, $new] = $change; + if ($new === null) { + if ($mapping['nullable'] === true) { + $updateData['$set'][$mapping['name']] = null; + } else { + $updateData['$unset'][$mapping['name']] = true; + } + + continue; + } + // Scalar fields if (! isset($mapping['association'])) { - if ($new === null && $mapping['nullable'] !== true) { - $updateData['$unset'][$mapping['name']] = true; + if (isset($mapping['strategy']) && $mapping['strategy'] === ClassMetadata::STORAGE_STRATEGY_INCREMENT) { + $operator = '$inc'; + $type = Type::getType($mapping['type']); + assert($type instanceof Incrementable); + $value = $type->convertToDatabaseValue($type->diff($old, $new)); } else { - if ($new !== null && isset($mapping['strategy']) && $mapping['strategy'] === ClassMetadata::STORAGE_STRATEGY_INCREMENT) { - $operator = '$inc'; - $type = Type::getType($mapping['type']); - assert($type instanceof Incrementable); - $value = $type->convertToDatabaseValue($type->diff($old, $new)); - } else { - $operator = '$set'; - $value = $new === null ? null : Type::getType($mapping['type'])->convertToDatabaseValue($new); - } - - $updateData[$operator][$mapping['name']] = $value; + $operator = '$set'; + $value = Type::getType($mapping['type'])->convertToDatabaseValue($new); } + $updateData[$operator][$mapping['name']] = $value; + // @EmbedOne } elseif (isset($mapping['association']) && $mapping['association'] === ClassMetadata::EMBED_ONE) { // If we have a new embedded document then lets set the whole thing - if ($new && $this->uow->isScheduledForInsert($new)) { + if ($this->uow->isScheduledForInsert($new)) { $updateData['$set'][$mapping['name']] = $this->prepareEmbeddedDocumentValue($mapping, $new); - // If we don't have a new value then lets unset the embedded document - } elseif (! $new) { - $updateData['$unset'][$mapping['name']] = true; - // Update existing embedded document } else { $update = $this->prepareUpdateData($new); @@ -182,7 +181,7 @@ public function prepareUpdateData($document) } // @ReferenceMany, @EmbedMany - } elseif (isset($mapping['association']) && $mapping['type'] === 'many' && $new) { + } elseif (isset($mapping['association']) && $mapping['type'] === 'many') { if (CollectionHelper::isAtomic($mapping['strategy']) && $this->uow->isCollectionScheduledForUpdate($new)) { $updateData['$set'][$mapping['name']] = $this->prepareAssociatedCollectionValue($new, true); } elseif (CollectionHelper::isAtomic($mapping['strategy']) && $this->uow->isCollectionScheduledForDeletion($new)) { @@ -208,11 +207,7 @@ public function prepareUpdateData($document) // @ReferenceOne } elseif (isset($mapping['association']) && $mapping['association'] === ClassMetadata::REFERENCE_ONE) { - if (isset($new) || $mapping['nullable'] === true) { - $updateData['$set'][$mapping['name']] = $new === null ? null : $this->prepareReferencedDocumentValue($mapping, $new); - } else { - $updateData['$unset'][$mapping['name']] = true; - } + $updateData['$set'][$mapping['name']] = $this->prepareReferencedDocumentValue($mapping, $new); } } @@ -250,31 +245,36 @@ public function prepareUpsertData($document) [$old, $new] = $change; + // Fields with a null value should only be written for inserts + if ($new === null) { + if ($mapping['nullable'] === true) { + $updateData['$setOnInsert'][$mapping['name']] = null; + } + + continue; + } + // Scalar fields if (! isset($mapping['association'])) { - if ($new !== null) { - if (empty($mapping['id']) && isset($mapping['strategy']) && $mapping['strategy'] === ClassMetadata::STORAGE_STRATEGY_INCREMENT) { - $operator = '$inc'; - $type = Type::getType($mapping['type']); - assert($type instanceof Incrementable); - $value = $type->convertToDatabaseValue($type->diff($old, $new)); - } else { - $operator = '$set'; - $value = Type::getType($mapping['type'])->convertToDatabaseValue($new); - } - - $updateData[$operator][$mapping['name']] = $value; - } elseif ($mapping['nullable'] === true) { - $updateData['$setOnInsert'][$mapping['name']] = null; + if (empty($mapping['id']) && isset($mapping['strategy']) && $mapping['strategy'] === ClassMetadata::STORAGE_STRATEGY_INCREMENT) { + $operator = '$inc'; + $type = Type::getType($mapping['type']); + assert($type instanceof Incrementable); + $value = $type->convertToDatabaseValue($type->diff($old, $new)); + } else { + $operator = '$set'; + $value = Type::getType($mapping['type'])->convertToDatabaseValue($new); } + $updateData[$operator][$mapping['name']] = $value; + // @EmbedOne } elseif (isset($mapping['association']) && $mapping['association'] === ClassMetadata::EMBED_ONE) { // If we don't have a new value then do nothing on upsert // If we have a new embedded document then lets set the whole thing - if ($new && $this->uow->isScheduledForInsert($new)) { + if ($this->uow->isScheduledForInsert($new)) { $updateData['$set'][$mapping['name']] = $this->prepareEmbeddedDocumentValue($mapping, $new); - } elseif ($new) { + } else { // Update existing embedded document $update = $this->prepareUpsertData($new); foreach ($update as $cmd => $values) { @@ -286,9 +286,7 @@ public function prepareUpsertData($document) // @ReferenceOne } elseif (isset($mapping['association']) && $mapping['association'] === ClassMetadata::REFERENCE_ONE) { - if (isset($new) || $mapping['nullable'] === true) { - $updateData['$set'][$mapping['name']] = $new === null ? null : $this->prepareReferencedDocumentValue($mapping, $new); - } + $updateData['$set'][$mapping['name']] = $this->prepareReferencedDocumentValue($mapping, $new); // @ReferenceMany, @EmbedMany } elseif ( diff --git a/tests/Doctrine/ODM/MongoDB/Tests/Functional/EmbeddedTest.php b/tests/Doctrine/ODM/MongoDB/Tests/Functional/EmbeddedTest.php index 1a54516063..8d1c08ae42 100644 --- a/tests/Doctrine/ODM/MongoDB/Tests/Functional/EmbeddedTest.php +++ b/tests/Doctrine/ODM/MongoDB/Tests/Functional/EmbeddedTest.php @@ -418,6 +418,7 @@ public function testRemoveEmbeddedDocument() $check = $this->dm->getDocumentCollection(User::class)->findOne(); $this->assertEmpty($check['phonenumbers']); + $this->assertNull($check['addressNullable']); $this->assertArrayNotHasKey('address', $check); } diff --git a/tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH2310Test.php b/tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH2310Test.php new file mode 100644 index 0000000000..e00301c8a3 --- /dev/null +++ b/tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH2310Test.php @@ -0,0 +1,124 @@ +dm->persist($document); + $this->dm->flush(); + $this->dm->clear(); + + $repository = $this->dm->getRepository(GH2310Container::class); + $result = $repository->find($document->id); + + self::assertInstanceOf(GH2310Container::class, $result); + self::assertSame($document->id, $result->id); + self::assertNull($result->embedded); + } + + public function testAggregatorBuilderWithNullableEmbeddedAfterUpsert(): void + { + $document = new GH2310Container((string) new ObjectId(), null); + $this->dm->persist($document); + $this->dm->flush(); + $this->dm->clear(); + + $aggBuilder = $this->dm->createAggregationBuilder(GH2310Container::class); + $aggBuilder->match()->field('id')->equals($document->id); + $result = $aggBuilder->hydrate(GH2310Container::class)->getAggregation()->getIterator()->current(); + + self::assertInstanceOf(GH2310Container::class, $result); + self::assertSame($document->id, $result->id); + self::assertNull($result->embedded); + } + + public function testFindWithNullableEmbeddedAfterInsert(): void + { + $document = new GH2310Container(null, null); + $this->dm->persist($document); + $this->dm->flush(); + + $repository = $this->dm->getRepository(GH2310Container::class); + $result = $repository->find($document->id); + + self::assertInstanceOf(GH2310Container::class, $result); + self::assertSame($document->id, $result->id); + self::assertNull($result->embedded); + } + + public function testAggregatorBuilderWithNullableEmbeddedAfterInsert(): void + { + $document = new GH2310Container(null, null); + $this->dm->persist($document); + $this->dm->flush(); + + $aggBuilder = $this->dm->createAggregationBuilder(GH2310Container::class); + $aggBuilder->match()->field('id')->equals($document->id); + $result = $aggBuilder->hydrate(GH2310Container::class)->getAggregation()->getIterator()->current(); + + self::assertInstanceOf(GH2310Container::class, $result); + self::assertSame($document->id, $result->id); + self::assertNull($result->embedded); + } + + public function testFindWithNullableEmbeddedAfterUpdate(): void + { + $document = new GH2310Container(null, new GH2310Embedded()); + $this->dm->persist($document); + $this->dm->flush(); + + // Update embedded document to trigger nullable behaviour + $document->embedded = null; + $this->dm->flush(); + $this->dm->clear(); + + $repository = $this->dm->getRepository(GH2310Container::class); + $result = $repository->find($document->id); + + self::assertInstanceOf(GH2310Container::class, $result); + self::assertSame($document->id, $result->id); + self::assertNull($result->embedded); + } + + public function testAggregatorBuilderWithNullableEmbeddedAfterUpdate(): void + { + $document = new GH2310Container(null, new GH2310Embedded()); + $this->dm->persist($document); + $this->dm->flush(); + + // Update embedded document to trigger nullable behaviour + $document->embedded = null; + $this->dm->flush(); + $this->dm->clear(); + + $aggBuilder = $this->dm->createAggregationBuilder(GH2310Container::class); + $aggBuilder->match()->field('id')->equals($document->id); + $result = $aggBuilder->hydrate(GH2310Container::class)->getAggregation()->getIterator()->current(); + + self::assertInstanceOf(GH2310Container::class, $result); + self::assertSame($document->id, $result->id); + self::assertNull($result->embedded); + } +} diff --git a/tests/Doctrine/ODM/MongoDB/Tests/Functional/UpsertTest.php b/tests/Doctrine/ODM/MongoDB/Tests/Functional/UpsertTest.php index 3449292ff7..dbbc148450 100644 --- a/tests/Doctrine/ODM/MongoDB/Tests/Functional/UpsertTest.php +++ b/tests/Doctrine/ODM/MongoDB/Tests/Functional/UpsertTest.php @@ -8,6 +8,8 @@ use Doctrine\ODM\MongoDB\Tests\BaseTest; use MongoDB\BSON\ObjectId; +use function assert; + class UpsertTest extends BaseTest { /** @@ -17,11 +19,9 @@ class UpsertTest extends BaseTest */ public function testUpsertEmbedManyDoesNotCreateObject() { - $test = new UpsertTestUser(); - $test->id = (string) new ObjectId(); + $test = new UpsertTestUser(); $embedded = new UpsertTestUserEmbedded(); - $embedded->id = (string) new ObjectId(); $embedded->test = 'test'; $test->embedMany[] = $embedded; @@ -36,6 +36,54 @@ public function testUpsertEmbedManyDoesNotCreateObject() $this->dm->flush(); } + + public function testUpsertDoesNotOverwriteNullableFieldsOnNull() + { + $test = new UpsertTestUser(); + + $test->nullableField = 'value'; + $test->nullableReferenceOne = new UpsertTestUser(); + $test->nullableEmbedOne = new UpsertTestUserEmbedded(); + + $this->dm->persist($test); + $this->dm->flush(); + $this->dm->clear(); + + $upsert = new UpsertTestUser(); + + // Re-use old ID but don't set any other values + $upsert->id = $test->id; + + $this->dm->persist($upsert); + $this->dm->flush(); + $this->dm->clear(); + + $upsertResult = $this->dm->find(UpsertTestUser::class, $test->id); + assert($upsertResult instanceof $upsertResult); + self::assertNotNull($upsertResult->nullableField); + self::assertNotNull($upsertResult->nullableReferenceOne); + self::assertNotNull($upsertResult->nullableEmbedOne); + } + + public function testUpsertsWritesNullableFieldsOnInsert() + { + $test = new UpsertTestUser(); + $this->dm->persist($test); + $this->dm->flush(); + + $collection = $this->dm->getDocumentCollection(UpsertTestUser::class); + $result = $collection->findOne(['_id' => new ObjectId($test->id)]); + + self::assertEquals( + [ + '_id' => new ObjectId($test->id), + 'nullableField' => null, + 'nullableReferenceOne' => null, + 'nullableEmbedOne' => null, + ], + $result + ); + } } /** @ODM\Document */ @@ -44,16 +92,27 @@ class UpsertTestUser /** @ODM\Id */ public $id; + /** @ODM\Field(nullable=true) */ + public $nullableField; + + /** @ODM\EmbedOne(targetDocument=UpsertTestUserEmbedded::class, nullable=true) */ + public $nullableEmbedOne; + + /** @ODM\ReferenceOne(targetDocument=UpsertTestUser::class, cascade="persist", nullable=true) */ + public $nullableReferenceOne; + /** @ODM\EmbedMany(targetDocument=UpsertTestUserEmbedded::class) */ public $embedMany; + + public function __construct() + { + $this->id = (string) new ObjectId(); + } } /** @ODM\EmbeddedDocument */ class UpsertTestUserEmbedded { - /** @ODM\Id */ - public $id; - /** @ODM\Field(type="string") */ public $test; } diff --git a/tests/Documents/User.php b/tests/Documents/User.php index 7b11d9f624..00340678f4 100644 --- a/tests/Documents/User.php +++ b/tests/Documents/User.php @@ -28,9 +28,12 @@ class User extends BaseDocument /** @ODM\Field(type="date") */ protected $createdAt; - /** @ODM\EmbedOne(targetDocument=Address::class, nullable=true) */ + /** @ODM\EmbedOne(targetDocument=Address::class) */ protected $address; + /** @ODM\EmbedOne(targetDocument=Address::class, nullable=true) */ + protected $addressNullable; + /** @ODM\ReferenceOne(targetDocument=Profile::class, cascade={"all"}) */ protected $profile; @@ -177,12 +180,14 @@ public function getAddress() public function setAddress(?Address $address = null) { - $this->address = $address; + $this->address = $address; + $this->addressNullable = $address ? clone $address : $address; } public function removeAddress() { - $this->address = null; + $this->address = null; + $this->addressNullable = null; } public function setProfile(Profile $profile) diff --git a/tests/Documents74/GH2310Container.php b/tests/Documents74/GH2310Container.php new file mode 100644 index 0000000000..bfa5c7fa23 --- /dev/null +++ b/tests/Documents74/GH2310Container.php @@ -0,0 +1,34 @@ +id = $id; + $this->embedded = $embedded; + } +} + +/** + * @ODM\EmbeddedDocument + */ +class GH2310Embedded +{ + /** @ODM\Field(type="integer") */ + public int $value; +} From f20b3cd29d17c38115c39dc1516ead1c15e918b7 Mon Sep 17 00:00:00 2001 From: Bernhard Schussek Date: Tue, 29 Jun 2021 13:07:17 +0200 Subject: [PATCH 16/16] Fix handling of upserts during scheduling for deletion (#2334) * Fix handling of upserts during scheduling for deletion * Added test --- lib/Doctrine/ODM/MongoDB/UnitOfWork.php | 4 ++++ .../ODM/MongoDB/Tests/UnitOfWorkTest.php | 18 ++++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/lib/Doctrine/ODM/MongoDB/UnitOfWork.php b/lib/Doctrine/ODM/MongoDB/UnitOfWork.php index 711f5bc22d..33ba0459cb 100644 --- a/lib/Doctrine/ODM/MongoDB/UnitOfWork.php +++ b/lib/Doctrine/ODM/MongoDB/UnitOfWork.php @@ -1357,6 +1357,10 @@ public function scheduleForDelete(object $document, bool $isView = false): void unset($this->documentUpdates[$oid]); } + if (isset($this->documentUpserts[$oid])) { + unset($this->documentUpserts[$oid]); + } + if (isset($this->documentDeletions[$oid])) { return; } diff --git a/tests/Doctrine/ODM/MongoDB/Tests/UnitOfWorkTest.php b/tests/Doctrine/ODM/MongoDB/Tests/UnitOfWorkTest.php index ae2e6080b4..9bcbfbe65f 100644 --- a/tests/Doctrine/ODM/MongoDB/Tests/UnitOfWorkTest.php +++ b/tests/Doctrine/ODM/MongoDB/Tests/UnitOfWorkTest.php @@ -116,6 +116,24 @@ public function testRegisterRemovedOnNewEntityIsIgnored() $this->assertFalse($this->uow->isScheduledForDelete($user)); } + public function testScheduleForDeleteShouldUnregisterScheduledUpserts() + { + $class = $this->dm->getClassMetadata(ForumUser::class); + $user = new ForumUser(); + $user->id = new ObjectId(); + $this->assertFalse($this->uow->isScheduledForInsert($user)); + $this->assertFalse($this->uow->isScheduledForUpsert($user)); + $this->assertFalse($this->uow->isScheduledForDelete($user)); + $this->uow->scheduleForUpsert($class, $user); + $this->assertFalse($this->uow->isScheduledForInsert($user)); + $this->assertTrue($this->uow->isScheduledForUpsert($user)); + $this->assertFalse($this->uow->isScheduledForDelete($user)); + $this->uow->scheduleForDelete($user); + $this->assertFalse($this->uow->isScheduledForInsert($user)); + $this->assertFalse($this->uow->isScheduledForUpsert($user)); + $this->assertTrue($this->uow->isScheduledForDelete($user)); + } + public function testThrowsOnPersistOfMappedSuperclass() { $this->expectException(MongoDBException::class);