From 6fa807a449c1390326be3824c207bdcc50b73a41 Mon Sep 17 00:00:00 2001 From: Maciej Malarz Date: Sat, 1 Aug 2015 20:27:22 +0200 Subject: [PATCH 1/4] Introduce sanity checks for IDs and fix overriding its type --- .../ODM/MongoDB/Mapping/ClassMetadataInfo.php | 2 +- lib/Doctrine/ODM/MongoDB/UnitOfWork.php | 23 ++++++++++++++++--- .../ODM/MongoDB/Tests/Functional/IdTest.php | 21 +++++++++++++++++ .../Tests/Functional/Ticket/GH529Test.php | 19 ++------------- .../Tests/Functional/Ticket/GH816Test.php | 2 +- .../Tests/Functional/Ticket/GH852Test.php | 2 +- tests/Documents/CmsPhonenumber.php | 2 +- 7 files changed, 47 insertions(+), 24 deletions(-) diff --git a/lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadataInfo.php b/lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadataInfo.php index 5b89e7b702..dbc4ac1511 100644 --- a/lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadataInfo.php +++ b/lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadataInfo.php @@ -1136,7 +1136,7 @@ public function mapField(array $mapping) $mapping['type'] = $this->generatorOptions['type']; } elseif ($generatorType === ClassMetadata::GENERATOR_TYPE_INCREMENT) { $mapping['type'] = 'int_id'; - } else { + } elseif (empty($mapping['type']) || $mapping['type'] === 'id') { $mapping['type'] = 'custom_id'; } diff --git a/lib/Doctrine/ODM/MongoDB/UnitOfWork.php b/lib/Doctrine/ODM/MongoDB/UnitOfWork.php index 2e4bb118cb..1a4abf5a73 100644 --- a/lib/Doctrine/ODM/MongoDB/UnitOfWork.php +++ b/lib/Doctrine/ODM/MongoDB/UnitOfWork.php @@ -1003,10 +1003,11 @@ public function recomputeSingleDocumentChangeSet(ClassMetadata $class, $document } /** - * @param $class + * @param ClassMetadata $class * @param object $document + * @throws \InvalidArgumentException If there is something wrong with document's identifier. */ - private function persistNew($class, $document) + private function persistNew(ClassMetadata $class, $document) { $oid = spl_object_hash($document); if ( ! empty($class->lifecycleCallbacks[Events::prePersist])) { @@ -1019,7 +1020,21 @@ private function persistNew($class, $document) $upsert = false; if ($class->identifier) { $idValue = $class->getIdentifierValue($document); - $upsert = !$class->isEmbeddedDocument && $idValue !== null; + $upsert = ! $class->isEmbeddedDocument && $idValue !== null; + + if ($class->generatorType === ClassMetadata::GENERATOR_TYPE_NONE && $idValue === null && ! $class->isEmbeddedDocument) { + throw new \InvalidArgumentException(sprintf( + "%s uses NONE identifier generation strategy but no identifier was provided when persisting.", + get_class($document) + )); + } + + if ($class->generatorType === ClassMetadata::GENERATOR_TYPE_AUTO && $idValue !== null && ! \MongoId::isValid($idValue)) { + throw new \InvalidArgumentException(sprintf( + "%s uses AUTO identifier generation strategy but provided identifier is not valid MongoId.", + get_class($document) + )); + } if ($class->generatorType !== ClassMetadata::GENERATOR_TYPE_NONE && $idValue === null) { $idValue = $class->idGenerator->generate($this->dm, $document); @@ -1775,6 +1790,8 @@ public function containsId($id, $rootClassName) * Persists a document as part of the current unit of work. * * @param object $document The document to persist. + * @throws MongoDBException If trying to persist MappedSuperclass. + * @throws \InvalidArgumentException If there is something wrong with document's identifier. */ public function persist($document) { diff --git a/tests/Doctrine/ODM/MongoDB/Tests/Functional/IdTest.php b/tests/Doctrine/ODM/MongoDB/Tests/Functional/IdTest.php index bc25a12f24..3eccf8bf2e 100644 --- a/tests/Doctrine/ODM/MongoDB/Tests/Functional/IdTest.php +++ b/tests/Doctrine/ODM/MongoDB/Tests/Functional/IdTest.php @@ -312,6 +312,27 @@ public function getTestBinIdsData() ); } + /** + * @expectedException \InvalidArgumentException + * @expectedExceptionMessage Doctrine\ODM\MongoDB\Tests\Functional\CustomIdUser uses NONE identifier generation strategy but no identifier was provided when persisting. + */ + public function testStrategyNoneAndNoIdThrowsException() + { + $this->dm->persist(new CustomIdUser('Maciej')); + } + + /** + * @expectedException \InvalidArgumentException + * @expectedExceptionMessage Doctrine\ODM\MongoDB\Tests\Functional\TestIdTypesIdAutoUser uses AUTO identifier generation strategy but provided identifier is not valid MongoId. + */ + public function testStrategyAutoWithNotValidIdThrowsException() + { + $this->createIdTestClass('id', 'auto'); + $user = new TestIdTypesIdAutoUser(); + $user->id = 1; + $this->dm->persist($user); + } + private function createIdTestClass($type, $strategy) { $shortClassName = sprintf('TestIdTypes%s%sUser', ucfirst($type), ucfirst($strategy)); diff --git a/tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH529Test.php b/tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH529Test.php index c6db5427e4..9dc83ad553 100644 --- a/tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH529Test.php +++ b/tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH529Test.php @@ -22,21 +22,6 @@ public function testAutoIdWithConsistentValues() $this->assertEquals($mongoId, $doc->id); } - public function testAutoIdWithInconsistentValues() - { - $doc = new GH529AutoIdDocument(); - $doc->id = 'foo'; - - $this->dm->persist($doc); - $this->dm->flush(); - $this->dm->clear(); - - $doc = $this->dm->getRepository(get_class($doc))->findOneBy(array()); - - $this->assertNotNull($doc); - $this->assertNotEquals('foo', $doc->id); - } - public function testCustomIdType() { /* All values are consistent for CustomIdType, since the PHP and DB @@ -96,13 +81,13 @@ class GH529AutoIdDocument /** @ODM\Document */ class GH529CustomIdDocument { - /** @ODM\Id(type="custom_id") */ + /** @ODM\Id(strategy="none", type="custom_id") */ public $id; } /** @ODM\Document */ class GH529IntIdDocument { - /** @ODM\Id(type="int") */ + /** @ODM\Id(strategy="none", type="int") */ public $id; } diff --git a/tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH816Test.php b/tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH816Test.php index 83f3e98359..5a687cd319 100644 --- a/tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH816Test.php +++ b/tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH816Test.php @@ -9,7 +9,7 @@ class GH816Test extends \Doctrine\ODM\MongoDB\Tests\BaseTest public function testPersistAfterDetachWithIdSet() { $d=new GH816Document(); - $d->_id="Test"; + $d->_id=new \MongoId(); $this->assertSame(array(), $this->dm->getRepository('Doctrine\ODM\MongoDB\Tests\GH816Document')->findAll()); $this->dm->persist($d); $this->dm->detach($d); diff --git a/tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH852Test.php b/tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH852Test.php index 34c63c3beb..0acd468be6 100644 --- a/tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH852Test.php +++ b/tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH852Test.php @@ -99,7 +99,7 @@ public function provideIdGenerators() /** @ODM\Document */ class GH852Document { - /** @ODM\Id(type="custom_id") */ + /** @ODM\Id(strategy="NONE", type="custom_id") */ public $id; /** @ODM\String */ diff --git a/tests/Documents/CmsPhonenumber.php b/tests/Documents/CmsPhonenumber.php index 78c7f4a2c0..f06217038b 100644 --- a/tests/Documents/CmsPhonenumber.php +++ b/tests/Documents/CmsPhonenumber.php @@ -9,7 +9,7 @@ */ class CmsPhonenumber { - /** @ODM\Id(type="custom_id") */ + /** @ODM\Id(strategy="NONE", type="custom_id") */ public $phonenumber; /** From 18b7fb265b4c899ce39324d43ebf45a9524f1593 Mon Sep 17 00:00:00 2001 From: Maciej Malarz Date: Sat, 1 Aug 2015 20:35:03 +0200 Subject: [PATCH 2/4] Introduce ID sanity checks for embedded documents --- lib/Doctrine/ODM/MongoDB/UnitOfWork.php | 2 +- .../Tests/Functional/EmbeddedIdTest.php | 19 ++++++++----------- 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/lib/Doctrine/ODM/MongoDB/UnitOfWork.php b/lib/Doctrine/ODM/MongoDB/UnitOfWork.php index 1a4abf5a73..df2dde9051 100644 --- a/lib/Doctrine/ODM/MongoDB/UnitOfWork.php +++ b/lib/Doctrine/ODM/MongoDB/UnitOfWork.php @@ -1022,7 +1022,7 @@ private function persistNew(ClassMetadata $class, $document) $idValue = $class->getIdentifierValue($document); $upsert = ! $class->isEmbeddedDocument && $idValue !== null; - if ($class->generatorType === ClassMetadata::GENERATOR_TYPE_NONE && $idValue === null && ! $class->isEmbeddedDocument) { + if ($class->generatorType === ClassMetadata::GENERATOR_TYPE_NONE && $idValue === null) { throw new \InvalidArgumentException(sprintf( "%s uses NONE identifier generation strategy but no identifier was provided when persisting.", get_class($document) diff --git a/tests/Doctrine/ODM/MongoDB/Tests/Functional/EmbeddedIdTest.php b/tests/Doctrine/ODM/MongoDB/Tests/Functional/EmbeddedIdTest.php index 82941931a7..78bb46ca16 100644 --- a/tests/Doctrine/ODM/MongoDB/Tests/Functional/EmbeddedIdTest.php +++ b/tests/Doctrine/ODM/MongoDB/Tests/Functional/EmbeddedIdTest.php @@ -27,31 +27,28 @@ public function testEmbeddedIdsAreNotOverwritten() $this->assertEquals($id, $test->id); } + /** + * @expectedException \InvalidArgumentException + * @expectedExceptionMessage Doctrine\ODM\MongoDB\Tests\Functional\DefaultIdStrategyNoneEmbeddedDocument uses NONE identifier generation strategy but no identifier was provided when persisting. + */ public function testEmbedOneDocumentWithMissingIdentifier() { $user = new EmbeddedStrategyNoneIdTestUser(); $user->embedOne = new DefaultIdStrategyNoneEmbeddedDocument(); $this->dm->persist($user); - $this->dm->flush(); - $this->dm->clear(); - - $user = $this->dm->find(get_class($user), $user->id); } + /** + * @expectedException \InvalidArgumentException + * @expectedExceptionMessage Doctrine\ODM\MongoDB\Tests\Functional\DefaultIdStrategyNoneEmbeddedDocument uses NONE identifier generation strategy but no identifier was provided when persisting. + */ public function testEmbedManyDocumentWithMissingIdentifier() { $user = new EmbeddedStrategyNoneIdTestUser(); $user->embedMany[] = new DefaultIdStrategyNoneEmbeddedDocument(); $this->dm->persist($user); - $this->dm->flush(); - $this->dm->clear(); - - $user = $this->dm->find(get_class($user), $user->id); - foreach ($user->embedMany as $embed) { - $this->assertNull($embed->id); - } } } From 3dd6a2f10e14f389d760fb9857d7dbeca9199972 Mon Sep 17 00:00:00 2001 From: Maciej Malarz Date: Sat, 1 Aug 2015 20:56:52 +0200 Subject: [PATCH 3/4] MongoId::isValid was introduced in 1.5.0 so it's no good --- lib/Doctrine/ODM/MongoDB/UnitOfWork.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/Doctrine/ODM/MongoDB/UnitOfWork.php b/lib/Doctrine/ODM/MongoDB/UnitOfWork.php index df2dde9051..87eeb0d54d 100644 --- a/lib/Doctrine/ODM/MongoDB/UnitOfWork.php +++ b/lib/Doctrine/ODM/MongoDB/UnitOfWork.php @@ -1029,7 +1029,8 @@ private function persistNew(ClassMetadata $class, $document) )); } - if ($class->generatorType === ClassMetadata::GENERATOR_TYPE_AUTO && $idValue !== null && ! \MongoId::isValid($idValue)) { + // \MongoId::isValid($idValue) was introduced in 1.5.0 so it's no good + if ($class->generatorType === ClassMetadata::GENERATOR_TYPE_AUTO && $idValue !== null && ! preg_match('/^[0-9a-f]{24}$/', $idValue)) { throw new \InvalidArgumentException(sprintf( "%s uses AUTO identifier generation strategy but provided identifier is not valid MongoId.", get_class($document) From ce8967c0ef7d329818f86ad4022e0f779f7e7192 Mon Sep 17 00:00:00 2001 From: Maciej Malarz Date: Mon, 10 Aug 2015 21:48:11 +0200 Subject: [PATCH 4/4] Refactored determining identifier's type --- .../ODM/MongoDB/Mapping/Annotations/Id.php | 2 +- .../ODM/MongoDB/Mapping/ClassMetadataInfo.php | 29 +++++++++---------- .../ODM/MongoDB/Mapping/MappingException.php | 2 +- .../Tests/Mapping/ClassMetadataInfoTest.php | 4 +-- 4 files changed, 17 insertions(+), 20 deletions(-) diff --git a/lib/Doctrine/ODM/MongoDB/Mapping/Annotations/Id.php b/lib/Doctrine/ODM/MongoDB/Mapping/Annotations/Id.php index 53a90ff5b0..e1197c1e72 100644 --- a/lib/Doctrine/ODM/MongoDB/Mapping/Annotations/Id.php +++ b/lib/Doctrine/ODM/MongoDB/Mapping/Annotations/Id.php @@ -23,6 +23,6 @@ final class Id extends AbstractField { public $id = true; - public $type = 'id'; + public $type; public $strategy = 'auto'; } diff --git a/lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadataInfo.php b/lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadataInfo.php index dbc4ac1511..bb794d7545 100644 --- a/lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadataInfo.php +++ b/lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadataInfo.php @@ -1070,7 +1070,7 @@ public function mapField(array $mapping) if ( ! isset($mapping['name'])) { $mapping['name'] = $mapping['fieldName']; } - if ($this->identifier === $mapping['name'] && $mapping['type'] !== 'id') { + if ($this->identifier === $mapping['name'] && empty($mapping['id'])) { throw MappingException::mustNotChangeIdentifierFieldsType($this->name, $mapping['name']); } if (isset($this->fieldMappings[$mapping['fieldName']])) { @@ -1125,26 +1125,23 @@ public function mapField(array $mapping) } if (isset($mapping['id']) && $mapping['id'] === true) { $mapping['name'] = '_id'; - $mapping['type'] = isset($mapping['type']) ? $mapping['type'] : 'id'; $this->identifier = $mapping['fieldName']; if (isset($mapping['strategy'])) { - $this->generatorOptions = isset($mapping['options']) ? $mapping['options'] : array(); - - $generatorType = constant('Doctrine\ODM\MongoDB\Mapping\ClassMetadata::GENERATOR_TYPE_' . strtoupper($mapping['strategy'])); - if ($generatorType !== self::GENERATOR_TYPE_AUTO) { - if (isset($this->generatorOptions['type'])) { + $this->generatorType = constant('Doctrine\ODM\MongoDB\Mapping\ClassMetadata::GENERATOR_TYPE_' . strtoupper($mapping['strategy'])); + } + $this->generatorOptions = isset($mapping['options']) ? $mapping['options'] : array(); + switch ($this->generatorType) { + case self::GENERATOR_TYPE_AUTO: + $mapping['type'] = 'id'; + break; + default: + if ( ! empty($this->generatorOptions['type'])) { $mapping['type'] = $this->generatorOptions['type']; - } elseif ($generatorType === ClassMetadata::GENERATOR_TYPE_INCREMENT) { - $mapping['type'] = 'int_id'; - } elseif (empty($mapping['type']) || $mapping['type'] === 'id') { - $mapping['type'] = 'custom_id'; + } elseif (empty($mapping['type'])) { + $mapping['type'] = $this->generatorType === self::GENERATOR_TYPE_INCREMENT ? 'int_id' : 'custom_id'; } - - unset($this->generatorOptions['type']); - } - - $this->generatorType = $generatorType; } + unset($this->generatorOptions['type']); } if ( ! isset($mapping['nullable'])) { $mapping['nullable'] = false; diff --git a/lib/Doctrine/ODM/MongoDB/Mapping/MappingException.php b/lib/Doctrine/ODM/MongoDB/Mapping/MappingException.php index 4ba9c3a007..9ac1e45caa 100644 --- a/lib/Doctrine/ODM/MongoDB/Mapping/MappingException.php +++ b/lib/Doctrine/ODM/MongoDB/Mapping/MappingException.php @@ -242,6 +242,6 @@ public static function owningAndInverseReferencesRequireTargetDocument($classNam */ public static function mustNotChangeIdentifierFieldsType($className, $fieldName) { - return new self("You must not change identifier field's type: $className::$fieldName"); + return new self("$className::$fieldName was declared an identifier and must stay this way."); } } diff --git a/tests/Doctrine/ODM/MongoDB/Tests/Mapping/ClassMetadataInfoTest.php b/tests/Doctrine/ODM/MongoDB/Tests/Mapping/ClassMetadataInfoTest.php index 83358b9188..24f018b106 100644 --- a/tests/Doctrine/ODM/MongoDB/Tests/Mapping/ClassMetadataInfoTest.php +++ b/tests/Doctrine/ODM/MongoDB/Tests/Mapping/ClassMetadataInfoTest.php @@ -301,9 +301,9 @@ public function testAddInheritedAssociationMapping() /** * @expectedException \Doctrine\ODM\MongoDB\Mapping\MappingException - * @expectedExceptionMessage You must not change identifier field's type: stdClass::id + * @expectedExceptionMessage stdClass::id was declared an identifier and must stay this way. */ - public function testIdFieldsTypeMustNotBeOverrriden() + public function testIdFieldsTypeMustNotBeOverridden() { $cm = new ClassMetadataInfo('stdClass'); $cm->setIdentifier('id');