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.

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

[WIP] Feature sharding #325

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@ final class DiscriminatorField extends Annotation
final class DiscriminatorMap extends Annotation {}
/** @Annotation */
final class DiscriminatorValue extends Annotation {}
/** @Annotation */
final class QueryFields extends Annotation {}
Copy link
Member

Choose a reason for hiding this comment

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

I think it would make more sense for this to be named ShardKeys, unless there is some alternative use case I'm missing. Additionally, we can consider mapping one or more fields with @ShardKey down the line. Specifying an array of MongoDB field names (they're not property names, right?) as a class-level annotation works for now, though.

Copy link
Author

Choose a reason for hiding this comment

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

Ok. I will change this to @ShardKey

Copy link
Member

Choose a reason for hiding this comment

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

The class-level annotation should be @ShardKeys, since users may specify one or more fields. The field-level annotation can be @ShardKey later on.


/** @Annotation */
final class Indexes extends Annotation {}
Expand Down
4 changes: 4 additions & 0 deletions lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadata.php
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,10 @@ public function __sleep()
$serialized[] = 'file';
}

if($this->hasQueryFields()) {
$serialized[] = 'queryFields';
}

return $serialized;
}

Expand Down
41 changes: 40 additions & 1 deletion lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadataInfo.php
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,11 @@ class ClassMetadataInfo implements \Doctrine\Common\Persistence\Mapping\ClassMet
*/
public $requireIndexes = false;

/**
* READ-ONLY: The fields for the query for the document collection.
*/
public $queryFields = array();
Copy link
Member

Choose a reason for hiding this comment

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

Naming this shardKeys would also help readability (same point as my comment about the annotation).


/**
* READ-ONLY: The name of the document class.
*/
Expand Down Expand Up @@ -681,6 +686,40 @@ public function hasIndexes()
return $this->indexes ? true : false;
}

/**
* Returns the custom query fields for this Document.
*
* @return array $fields The fields for the query.
*/
public function getQueryFields()
{
return $this->queryFields;
}

/**
* Checks whether this document has custom query fields or not.
*
* @return boolean
*/
public function hasQueryFields()
{
return $this->queryFields ? true : false;
Copy link
Contributor

Choose a reason for hiding this comment

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

couldnt this be return (boolean) $this->queryFields ?

Copy link
Author

Choose a reason for hiding this comment

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

fixed in the next commits

}

/**
* Sets the custom query fields for this Document.
*
* @param array $fields Array of fields for the query.
*/
public function setQueryFields($fields)
{
if(!is_array($fields)) {
$fields = array($fields);
}

$this->queryFields = $fields;
Copy link
Contributor

Choose a reason for hiding this comment

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

$this->queryFields = is_array($fields) ? $fields : array($fields);

Imo easier to read.

Copy link
Author

Choose a reason for hiding this comment

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

fixed in the next commits

Copy link

Choose a reason for hiding this comment

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

Yes easier, but using if () provide better performance, what is more important?

}

/**
* Sets the change tracking policy used by this class.
*
Expand Down Expand Up @@ -1289,7 +1328,7 @@ public function getIdentifierValue($document)
if ($document instanceof Proxy && !$document->__isInitialized()) {
return $document->__identifier__;
}
return (string) $this->reflFields[$this->identifier]->getValue($document);
return $this->reflFields[$this->identifier]->getValue($document);
}

/**
Expand Down
2 changes: 2 additions & 0 deletions lib/Doctrine/ODM/MongoDB/Mapping/Driver/AnnotationDriver.php
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,8 @@ public function loadMetadataForClass($className, ClassMetadataInfo $class)
foreach (is_array($annot->value) ? $annot->value : array($annot->value) as $index) {
$this->addIndex($class, $index);
}
} elseif ($annot instanceof ODM\QueryFields) {
$class->setQueryFields($annot->value);
} elseif ($annot instanceof ODM\InheritanceType) {
$class->setInheritanceType(constant('Doctrine\\ODM\\MongoDB\\Mapping\\ClassMetadata::INHERITANCE_TYPE_'.$annot->value));
} elseif ($annot instanceof ODM\DiscriminatorField) {
Expand Down
12 changes: 12 additions & 0 deletions lib/Doctrine/ODM/MongoDB/Mapping/Driver/XmlDriver.php
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,9 @@ public function loadMetadataForClass($className, ClassMetadataInfo $class)
}
$class->setDiscriminatorMap($map);
}
if (isset($xmlRoot->{'query-fields'})) {
$this->setQueryFields($class, $xmlRoot->{'query-fields'});
}
if (isset($xmlRoot->{'indexes'})) {
foreach($xmlRoot->{'indexes'}->{'index'} as $index) {
$this->addIndex($class, $index);
Expand Down Expand Up @@ -301,6 +304,15 @@ private function addIndex(ClassMetadataInfo $class, SimpleXmlElement $xmlIndex)
$class->addIndex($index['keys'], $index['options']);
}

private function setQueryFields(ClassMetadataInfo $class, SimpleXmlElement $xmlIndex)
{
$fields = array();
foreach ($xmlIndex->{'key'} as $key) {
$fields[(string) $key['name']] = (string) $key['name'];
}
$class->setQueryFields(array_values($fields));
}

protected function loadMappingFile($file)
{
$result = array();
Expand Down
3 changes: 3 additions & 0 deletions lib/Doctrine/ODM/MongoDB/Mapping/Driver/YamlDriver.php
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@ public function loadMetadataForClass($className, ClassMetadataInfo $class)
} elseif ($element['type'] === 'embeddedDocument') {
$class->isEmbeddedDocument = true;
}
if (isset($element['queryFields'])) {
$class->setQueryFields($element['queryFields']);
}
if (isset($element['indexes'])) {
foreach($element['indexes'] as $index) {
$class->addIndex($index['keys'], isset($index['options']) ? $index['options'] : array());
Expand Down
73 changes: 52 additions & 21 deletions lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php
Original file line number Diff line number Diff line change
Expand Up @@ -240,13 +240,17 @@ public function executeInserts(array $options = array())
$upsertOptions = $options;
$upsertOptions['upsert'] = true;
foreach ($upserts as $oid => $data) {
$criteria = array('_id' => $data[$this->cmd.'set']['_id']);
unset($data[$this->cmd.'set']['_id']);
$query = $this->getDocumentQuery($this->queuedInserts[$oid]);
foreach ($query as $field => $value) {
if (isset($data[$this->cmd.'set'][$field])) {
unset($data[$this->cmd.'set'][$field]);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I assume this is for ensuring that shard keys aren't modified, so you'll end up replacing this with an exception?

Can we simply read the shard key fields from the class metadata here? Calling getDocumentQuery() seems like unnecessary overhead just to get some field names.

Copy link
Author

Choose a reason for hiding this comment

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

yes in the next update it throws exceptions

// stupid php
if (empty($data[$this->cmd.'set'])) {
$data[$this->cmd.'set'] = new \stdClass;
}
$this->collection->update($criteria, $data, $upsertOptions);
$this->collection->update($query, $data, $upsertOptions);
}
}

Expand All @@ -263,13 +267,10 @@ public function executeInserts(array $options = array())
*/
public function update($document, array $options = array())
{
$id = $this->uow->getDocumentIdentifier($document);
$update = $this->pb->prepareUpdateData($document);

if ( ! empty($update)) {

$id = $this->class->getDatabaseIdentifierValue($id);
$query = array('_id' => $id);
$query = $this->getDocumentQuery($document);

// Include versioning logic to set the new version value in the database
// and to ensure the version has not changed since this document object instance
Expand Down Expand Up @@ -320,8 +321,7 @@ public function update($document, array $options = array())
*/
public function delete($document, array $options = array())
{
$id = $this->uow->getDocumentIdentifier($document);
$query = array('_id' => $this->class->getDatabaseIdentifierValue($id));
$query = $this->getDocumentQuery($document);

if ($this->class->isLockable) {
$query[$this->class->lockField] = array($this->cmd . 'exists' => false);
Expand All @@ -338,13 +338,12 @@ public function delete($document, array $options = array())
/**
* Refreshes a managed document.
*
* @param array $id The identifier of the document.
* @param object $document The document to refresh.
*/
public function refresh($id, $document)
public function refresh($document)
Copy link
Member

Choose a reason for hiding this comment

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

Does the removal of the first argument signify a BC break, or would this public method never be called by the user. I assume perhaps it's further down the line from DocumentManager::refresh(), so users wouldn't call this.

Copy link
Author

Choose a reason for hiding this comment

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

IMO its ok. It was used only one time in the core. I don't thinks users used it.

{
$class = $this->dm->getClassMetadata(get_class($document));
$data = $this->collection->findOne(array('_id' => $id));
$query = $this->getDocumentQuery($document);
$data = $this->collection->findOne($query);
$data = $this->hydratorFactory->hydrate($document, $data);
$this->uow->setOriginalDocumentData($document, $data);
}
Expand Down Expand Up @@ -449,8 +448,8 @@ private function wrapCursor(BaseCursor $cursor)
*/
public function exists($document)
{
$id = $this->class->getIdentifierObject($document);
return (bool) $this->collection->findOne(array(array('_id' => $id)), array('_id'));
$query = $this->getDocumentQuery($document);
return (bool) $this->collection->findOne($query, array('_id'));
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the second argument be array('_id' => 1)? I realize that's not your fault, but this would be a good place to correct it.

}

/**
Expand All @@ -461,10 +460,9 @@ public function exists($document)
*/
public function lock($document, $lockMode)
{
$id = $this->uow->getDocumentIdentifier($document);
$criteria = array('_id' => $this->class->getDatabaseIdentifierValue($id));
$query = $this->getDocumentQuery($document);
$lockMapping = $this->class->fieldMappings[$this->class->lockField];
$this->collection->update($criteria, array($this->cmd.'set' => array($lockMapping['name'] => $lockMode)));
$this->collection->update($query, array($this->cmd.'set' => array($lockMapping['name'] => $lockMode)));
$this->class->reflFields[$this->class->lockField]->setValue($document, $lockMode);
}

Expand All @@ -475,10 +473,9 @@ public function lock($document, $lockMode)
*/
public function unlock($document)
{
$id = $this->uow->getDocumentIdentifier($document);
$criteria = array('_id' => $this->class->getDatabaseIdentifierValue($id));
$query = $this->getDocumentQuery($document);
$lockMapping = $this->class->fieldMappings[$this->class->lockField];
$this->collection->update($criteria, array($this->cmd.'unset' => array($lockMapping['name'] => true)));
$this->collection->update($query, array($this->cmd.'unset' => array($lockMapping['name'] => true)));
$this->class->reflFields[$this->class->lockField]->setValue($document, null);
}

Expand Down Expand Up @@ -740,6 +737,40 @@ private function loadReferenceManyWithRepositoryMethod(PersistentCollection $col
}
}

/**
* Gets the query for the document.
*
* @param mixed $document
*
* @return array $query
*/
public function getDocumentQuery($document)
{
if ($this->class->hasQueryFields()) {
$changeSet = $this->uow->getDocumentChangeSet($document);
$query = array();
foreach ($this->class->queryFields as $field) {
if (isset($changeSet[$field])) {
if (null !== $changeSet[$field][0]) {
$query[$field] = $changeSet[$field][0];
} else {
// upsert new document
$query[$field] = $changeSet[$field][1];
}
} else {
$query[$field] = $this->class->getFieldValue($document, $field);
}
}

$query = $this->prepareQuery($query);
} else {
$id = $this->uow->getDocumentIdentifier($document);
$query = array('_id' => $this->class->getDatabaseIdentifierValue($id));
}

return $query;
}

/**
* Prepares a sort array and converts PHP property names to MongoDB field names.
*
Expand Down
3 changes: 1 addition & 2 deletions lib/Doctrine/ODM/MongoDB/UnitOfWork.php
Original file line number Diff line number Diff line change
Expand Up @@ -2100,8 +2100,7 @@ private function doRefresh($document, array &$visited)

$class = $this->dm->getClassMetadata(get_class($document));
if ($this->getDocumentState($document) == self::STATE_MANAGED) {
$id = $class->getDatabaseIdentifierValue($this->documentIdentifiers[$oid]);
$this->getDocumentPersister($class->name)->refresh($id, $document);
$this->getDocumentPersister($class->name)->refresh($document);
} else {
throw new \InvalidArgumentException("Document is not MANAGED.");
}
Expand Down
8 changes: 6 additions & 2 deletions tests/Doctrine/ODM/MongoDB/Tests/Functional/QueryTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -149,8 +149,12 @@ public function testUpdateQuery()
$query = $qb->getQuery();
$result = $query->execute();

$this->dm->refresh($this->user);
$this->assertEquals('crap', $this->user->getUsername());
$qb = $this->dm->createQueryBuilder('Documents\User')
->find()
->field('username')->equals('crap');
$query = $qb->getQuery();
$user = $query->getSingleResult();
$this->assertNotNull($user);
}

public function testUpsertUpdateQuery()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -217,12 +217,25 @@ public function testIndexes($class)

return $class;
}

/**
* @depends testCustomFieldName
* @param ClassMetadata $class
*/
public function testQueryFields($class)
{
$this->assertTrue(isset($class->queryFields));
$this->assertEquals(array('id', 'name'), $class->queryFields);

return $class;
}
}

/**
* @ODM\Document(collection="cms_users")
* @ODM\DiscriminatorField(fieldName="discr")
* @ODM\DiscriminatorMap({"default"="Doctrine\ODM\MongoDB\Tests\Mapping\User"})
* @ODM\QueryFields({"id", "name"})
*/
class User
{
Expand Down Expand Up @@ -366,5 +379,6 @@ public static function loadMetadata(ClassMetadata $metadata)
$metadata->addIndex(array('username' => 'desc'), array('unique' => true));
$metadata->addIndex(array('email' => 'desc'), array('unique' => true, 'dropDups' => true));
$metadata->addIndex(array('mysqlProfileId' => 'desc'), array('unique' => true, 'dropDups' => true));
$metadata->setQueryFields(array('id', 'name'));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ public function testClassMetadataInstanceSerialization()
$cm->setDiscriminatorField(array('name' => 'disc'));
$cm->mapOneEmbedded(array('fieldName' => 'phonenumbers', 'targetDocument' => 'Bar'));
$cm->setFile('customFileProperty');
$cm->setQueryFields(array('name'));
$this->assertTrue(is_array($cm->getFieldMapping('phonenumbers')));
$this->assertEquals(1, count($cm->fieldMappings));

Expand All @@ -48,6 +49,7 @@ public function testClassMetadataInstanceSerialization()
$this->assertTrue(is_array($cm->getFieldMapping('phonenumbers')));
$this->assertEquals(1, count($cm->fieldMappings));
$this->assertEquals('customFileProperty', $cm->file);
$this->assertEquals(array('name'), $cm->queryFields);
$mapping = $cm->getFieldMapping('phonenumbers');
$this->assertEquals('Documents\Bar', $mapping['targetDocument']);
}
Expand Down Expand Up @@ -182,4 +184,4 @@ public function testDuplicateFieldAndAssocationMapping2_ThrowsException()
$this->setExpectedException('Doctrine\ODM\MongoDB\MongoDBException');
$cm->mapField(array('fieldName' => 'name', 'columnName' => 'name'));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@
<discriminator-map>
<discriminator-mapping value="default" class="Doctrine\ODM\MongoDB\Tests\Mapping\User" />
</discriminator-map>
<query-fields>
<key name="id" />
<key name="name" />
</query-fields>
<field fieldName="id" id="true" />
<field fieldName="name" name="username" type="string" />
<field fieldName="email" type="string" unique="true" drop-dups="true" order="desc" />
Expand Down Expand Up @@ -53,4 +57,4 @@
<lifecycle-callback method="doStuffOnPostPersist" type="postPersist" />
</lifecycle-callbacks>
</document>
</doctrine-mongo-mapping>
</doctrine-mongo-mapping>
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ Doctrine\ODM\MongoDB\Tests\Mapping\User:
fieldName: discr
discriminatorMap:
default: Doctrine\ODM\MongoDB\Tests\Mapping\User
queryFields: [id, name]
fields:
id:
type: id
Expand Down Expand Up @@ -56,4 +57,4 @@ Doctrine\ODM\MongoDB\Tests\Mapping\User:

lifecycleCallbacks:
prePersist: [ doStuffOnPrePersist, doOtherStuffOnPrePersistToo ]
postPersist: [ doStuffOnPostPersist ]
postPersist: [ doStuffOnPostPersist ]
1 change: 1 addition & 0 deletions tests/Documents/User.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
/**
* @ODM\Document(collection="users")
* @ODM\InheritanceType("COLLECTION_PER_CLASS")
* @ODM\QueryFields({"id", "username"})
*/
class User extends BaseDocument
{
Expand Down