Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce sanity checks for IDs and fix overriding its type #1190

Merged
merged 4 commits into from
Aug 11, 2015
Merged

Introduce sanity checks for IDs and fix overriding its type #1190

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6fa807a
Introduce sanity checks for IDs and fix overriding its type
malarzm Aug 1, 2015
18b7fb2
Introduce ID sanity checks for embedded documents
malarzm Aug 1, 2015
3dd6a2f
MongoId::isValid was introduced in 1.5.0 so it's no good
malarzm Aug 1, 2015
ce8967c
Refactored determining identifier's type
malarzm Aug 10, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/Doctrine/ODM/MongoDB/Mapping/Annotations/Id.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,6 @@
final class Id extends AbstractField
{
public $id = true;
public $type = 'id';
public $type;
public $strategy = 'auto';
}
29 changes: 13 additions & 16 deletions lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadataInfo.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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']])) {
Expand Down Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion lib/Doctrine/ODM/MongoDB/Mapping/MappingException.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.");
}
}
24 changes: 21 additions & 3 deletions lib/Doctrine/ODM/MongoDB/UnitOfWork.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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])) {
Expand All @@ -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)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, this could happen with any generator strategy, including custom. It probably would be best if the generator itself could handle this check, including on whether to ignore it, throw an exception or quietly change the value.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially I agree but since AUTO strategy is the default one its misuse is the most problematic (see #819 or tests I needed to fix in this PR like this one), I think we can safely assume that if somebody is changing strategy he is clever enough to pass correct id (or will have his own checks in the type).

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);
Expand Down Expand Up @@ -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)
{
Expand Down
19 changes: 8 additions & 11 deletions tests/Doctrine/ODM/MongoDB/Tests/Functional/EmbeddedIdTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}

Expand Down
21 changes: 21 additions & 0 deletions tests/Doctrine/ODM/MongoDB/Tests/Functional/IdTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down
19 changes: 2 additions & 17 deletions tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH529Test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
}
2 changes: 1 addition & 1 deletion tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH816Test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH852Test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down
4 changes: 2 additions & 2 deletions tests/Doctrine/ODM/MongoDB/Tests/Mapping/ClassMetadataInfoTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
2 changes: 1 addition & 1 deletion tests/Documents/CmsPhonenumber.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
*/
class CmsPhonenumber
{
/** @ODM\Id(type="custom_id") */
/** @ODM\Id(strategy="NONE", type="custom_id") */
public $phonenumber;

/**
Expand Down