From f04d413ad6a13db90f14502e70f329740aba1b8b Mon Sep 17 00:00:00 2001 From: Andreas Braun Date: Tue, 27 Mar 2018 07:05:57 +0200 Subject: [PATCH 1/2] Fix upsert behavior of nullable fields This brings consistency to the upsert handling of fields with a null values. Non-nullable fields were not unset from the database to prevent loss of data, but nullable fields had their null value set in the daatabase. Since the object being upserted may be an incomplete object that doesn't completely represent the database state, we only write null values to the database if the upsert results in a document insertion. --- .../ODM/MongoDB/Persisters/PersistenceBuilder.php | 8 +++++--- .../ODM/MongoDB/Tests/Functional/FunctionalTest.php | 5 +++++ .../MongoDB/Tests/Persisters/PersistenceBuilderTest.php | 9 +++++++-- tests/Documents/CmsComment.php | 5 +++++ tests/Documents/UserUpsert.php | 3 +++ tests/Documents/UserUpsertIdStrategyNone.php | 3 +++ 6 files changed, 28 insertions(+), 5 deletions(-) diff --git a/lib/Doctrine/ODM/MongoDB/Persisters/PersistenceBuilder.php b/lib/Doctrine/ODM/MongoDB/Persisters/PersistenceBuilder.php index caa90be3e9..7d65b9fe10 100644 --- a/lib/Doctrine/ODM/MongoDB/Persisters/PersistenceBuilder.php +++ b/lib/Doctrine/ODM/MongoDB/Persisters/PersistenceBuilder.php @@ -243,16 +243,18 @@ public function prepareUpsertData($document) // Scalar fields if ( ! isset($mapping['association'])) { - if ($new !== null || $mapping['nullable'] === true) { - if ($new !== null && empty($mapping['id']) && isset($mapping['strategy']) && $mapping['strategy'] === ClassMetadataInfo::STORAGE_STRATEGY_INCREMENT) { + if ($new !== null) { + if (empty($mapping['id']) && isset($mapping['strategy']) && $mapping['strategy'] === ClassMetadataInfo::STORAGE_STRATEGY_INCREMENT) { $operator = '$inc'; $value = Type::getType($mapping['type'])->convertToDatabaseValue($new - $old); } else { $operator = '$set'; - $value = $new === null ? null : Type::getType($mapping['type'])->convertToDatabaseValue($new); + $value = Type::getType($mapping['type'])->convertToDatabaseValue($new); } $updateData[$operator][$mapping['name']] = $value; + } elseif ($mapping['nullable'] === true) { + $updateData['$setOnInsert'][$mapping['name']] = null; } // @EmbedOne diff --git a/tests/Doctrine/ODM/MongoDB/Tests/Functional/FunctionalTest.php b/tests/Doctrine/ODM/MongoDB/Tests/Functional/FunctionalTest.php index 9941e6160a..25263ff6fa 100644 --- a/tests/Doctrine/ODM/MongoDB/Tests/Functional/FunctionalTest.php +++ b/tests/Doctrine/ODM/MongoDB/Tests/Functional/FunctionalTest.php @@ -62,6 +62,8 @@ public function testUpsertObject($className, $id, $discriminator) $this->assertEquals((string) $id, (string) $check['_id']); $this->assertEquals($group->getId(), (string) $check['groups'][0]['$id']); $this->assertEquals($discriminator, $check['discriminator']); + $this->assertArrayHasKey('nullableField', $check); + $this->assertNull($check['nullableField']); $group2 = new \Documents\Group('Group'); @@ -70,6 +72,7 @@ public function testUpsertObject($className, $id, $discriminator) $user->hits = 5; $user->count = 2; $user->groups = array($group2); + $user->nullableField = 'foo'; $this->dm->persist($user); $this->dm->flush(); $this->dm->clear(); @@ -83,6 +86,7 @@ public function testUpsertObject($className, $id, $discriminator) $this->assertEquals($group2->getId(), (string) $check['groups'][1]['$id']); $this->assertArrayHasKey('username', $check); $this->assertEquals('test', $check['username']); + $this->assertEquals('foo', $check['nullableField']); $user = new $className(); $user->id = $id; @@ -99,6 +103,7 @@ public function testUpsertObject($className, $id, $discriminator) $this->assertEquals($group2->getId(), (string) $check['groups'][1]['$id']); $this->assertArrayHasKey('username', $check); $this->assertEquals('test', $check['username']); + $this->assertEquals('foo', $check['nullableField']); } public function testInheritedAssociationMappings() diff --git a/tests/Doctrine/ODM/MongoDB/Tests/Persisters/PersistenceBuilderTest.php b/tests/Doctrine/ODM/MongoDB/Tests/Persisters/PersistenceBuilderTest.php index ad8c5adee2..11a2f68b76 100644 --- a/tests/Doctrine/ODM/MongoDB/Tests/Persisters/PersistenceBuilderTest.php +++ b/tests/Doctrine/ODM/MongoDB/Tests/Persisters/PersistenceBuilderTest.php @@ -150,7 +150,8 @@ public function testPrepareInsertDataWithCreatedReferenceOne() '$db' => DOCTRINE_MONGODB_DATABASE, '$id' => new \MongoId($article->id), '$ref' => 'CmsArticle' - ) + ), + 'nullableField' => null, ); $this->assertDocumentInsertData($expectedData, $this->pb->prepareInsertData($comment)); } @@ -175,7 +176,8 @@ public function testPrepareInsertDataWithFetchedReferenceOne() '$db' => DOCTRINE_MONGODB_DATABASE, '$id' => new \MongoId($article->id), '$ref' => 'CmsArticle' - ) + ), + 'nullableField' => null, ); $this->assertDocumentInsertData($expectedData, $this->pb->prepareInsertData($comment)); } @@ -207,6 +209,9 @@ public function testPrepareUpsertData() '$ref' => 'CmsArticle' ), '_id' => new \MongoId($comment->id), + ), + '$setOnInsert' => array( + 'nullableField' => null, ) ); $this->assertEquals($expectedData, $this->pb->prepareUpsertData($comment)); diff --git a/tests/Documents/CmsComment.php b/tests/Documents/CmsComment.php index ad7bde8d5b..90835d1170 100644 --- a/tests/Documents/CmsComment.php +++ b/tests/Documents/CmsComment.php @@ -32,6 +32,11 @@ class CmsComment /** @ODM\Field(name="ip", type="string") */ public $authorIp; + /** + * @ODM\Field(type="string", nullable=true) + */ + public $nullableField; + public function setArticle(CmsArticle $article) { $this->article = $article; } diff --git a/tests/Documents/UserUpsert.php b/tests/Documents/UserUpsert.php index 967c98ae8d..1cac5915fa 100644 --- a/tests/Documents/UserUpsert.php +++ b/tests/Documents/UserUpsert.php @@ -29,4 +29,7 @@ class UserUpsert /** @ODM\ReferenceMany(targetDocument="Group", cascade={"all"}) */ public $groups; + + /** @ODM\Field(type="string", nullable=true) */ + public $nullableField; } diff --git a/tests/Documents/UserUpsertIdStrategyNone.php b/tests/Documents/UserUpsertIdStrategyNone.php index 01e3ee6255..cc30df14c7 100644 --- a/tests/Documents/UserUpsertIdStrategyNone.php +++ b/tests/Documents/UserUpsertIdStrategyNone.php @@ -28,4 +28,7 @@ class UserUpsertIdStrategyNone /** @ODM\ReferenceMany(targetDocument="Group", cascade={"all"}) */ public $groups; + + /** @ODM\Field(type="string", nullable=true) */ + public $nullableField; } From 151f30ddd4cbba05addf56ad87ad1d6a30a33a0b Mon Sep 17 00:00:00 2001 From: Andreas Braun Date: Tue, 27 Mar 2018 07:11:42 +0200 Subject: [PATCH 2/2] Harden handling of empty modifiers in upserts --- .../ODM/MongoDB/Persisters/DocumentPersister.php | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php b/lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php index 03600de7aa..9759c146ab 100644 --- a/lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php +++ b/lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php @@ -329,11 +329,15 @@ private function executeUpsert($document, array $options) foreach (array_keys($criteria) as $field) { unset($data['$set'][$field]); + unset($data['$inc'][$field]); + unset($data['$setOnInsert'][$field]); } - // Do not send an empty $set modifier - if (empty($data['$set'])) { - unset($data['$set']); + // Do not send empty update operators + foreach (['$set', '$inc', '$setOnInsert'] as $operator) { + if (empty($data[$operator])) { + unset($data[$operator]); + } } /* If there are no modifiers remaining, we're upserting a document with