Merge pull request #1190 from malarzm/id-checks

Introduce sanity checks for IDs and fix overriding its type
doctrine · Aug 11, 2015 · 23aca0f · 23aca0f
2 parents 7fe7b97 + ce8967c
commit 23aca0f
Show file tree

Hide file tree

Showing 11 changed files with 72 additions and 54 deletions.
diff --git 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
@@ -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';
-                    } else {
-                        $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
@@ -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/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,22 @@ 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) {
+                throw new \InvalidArgumentException(sprintf(
+                    "%s uses NONE identifier generation strategy but no identifier was provided when persisting.",
+                    get_class($document)
+                ));
+            }
+
+            // \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)
+                ));
+            }
 
             if ($class->generatorType !== ClassMetadata::GENERATOR_TYPE_NONE && $idValue === null) {
                 $idValue = $class->idGenerator->generate($this->dm, $document);
@@ -1775,6 +1791,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/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);
-        }
     }
 }
 

diff --git 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
@@ -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
@@ -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
@@ -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/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');

diff --git 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;
 
     /**