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

Add generic reference as replacement for DBRef #1623

Merged
merged 6 commits into from
Oct 7, 2017
Merged
Show file tree
Hide file tree
Changes from all 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
32 changes: 18 additions & 14 deletions docs/en/reference/annotations-reference.rst
Original file line number Diff line number Diff line change
Expand Up @@ -1218,24 +1218,26 @@ documents.
Optional attributes:

-
targetDocument - A |FQCN| of the target document.
targetDocument - A |FQCN| of the target document. A ``targetDocument`` is
required when using ``storeAs: id``.
-
simple - deprecated (use ``storeAs: id``)
-
storeAs - Indicates how to store the reference. ``id`` uses ``MongoId``,
``dbRef`` uses a `DBRef`_ without ``$db`` value and ``dbRefWithDb`` stores
a full `DBRef`_ (``$ref``, ``$id``, and ``$db``). Note that ``id``
references are not compatible with the discriminators.
storeAs - Indicates how to store the reference. ``id`` stores the identifier,
``ref`` an embedded object containing the ``id`` field and (optionally) a
discriminator. ``dbRef`` and ``dbRefWithDb`` store a `DBRef`_ object and
are deprecated in favor of ``ref``. Note that ``id`` references are not
compatible with the discriminators.
-
cascade - Cascade Option
-
discriminatorField - The field name to store the discriminator value within
the `DBRef`_ object.
the reference object.
Copy link
Member

Choose a reason for hiding this comment

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

Should this specify the logical conflict with id mapping, or is that discussed elsewhere? I assume we throw an exception for such mappings at runtime in any event.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's specified where we explain the different storeAs options:

Note that id references are not compatible with the discriminators.

I can add another note to each discriminator option for clarity.

-
discriminatorMap - Map of discriminator values to class names.
-
defaultDiscriminatorValue - A default value for discriminatorField if no value
has been set in the embedded document.
has been set in the referenced document.
-
inversedBy - The field name of the inverse side. Only allowed on owning side.
-
Expand Down Expand Up @@ -1292,24 +1294,26 @@ Defines an instance variable holds a related document instance.
Optional attributes:

-
targetDocument - A |FQCN| of the target document.
targetDocument - A |FQCN| of the target document. A ``targetDocument`` is
required when using ``storeAs: id``.
-
simple - deprecated (use ``storeAs: id``)
-
storeAs - Indicates how to store the reference. ``id`` uses ``MongoId``,
``dbRef`` uses a `DBRef`_ without ``$db`` value and ``dbRefWithDb`` stores
a full `DBRef`_ (``$ref``, ``$id``, and ``$db``). Note that ``id``
references are not compatible with the discriminators.
storeAs - Indicates how to store the reference. ``id`` stores the identifier,
``ref`` an embedded object containing the ``id`` field and (optionally) a
discriminator. ``dbRef`` and ``dbRefWithDb`` store a `DBRef`_ object and
are deprecated in favor of ``ref``. Note that ``id`` references are not
compatible with the discriminators.
-
cascade - Cascade Option
-
discriminatorField - The field name to store the discriminator value within
the `DBRef`_ object.
the reference object.
-
discriminatorMap - Map of discriminator values to class names.
-
defaultDiscriminatorValue - A default value for discriminatorField if no value
has been set in the embedded document.
has been set in the referenced document.
-
inversedBy - The field name of the inverse side. Only allowed on owning side.
-
Expand Down
5 changes: 3 additions & 2 deletions docs/en/reference/reference-mapping.rst
Original file line number Diff line number Diff line change
Expand Up @@ -356,11 +356,12 @@ fields and as ``MongoId``, it is possible to save references as `DBRef`_ without
the ``$db`` field. This solves problems when the database name changes (and also
reduces the amount of storage used).

The ``storeAs`` option has three possible values:
The ``storeAs`` option has the following possible values:

- **dbRefWithDb**: Uses a `DBRef`_ with ``$ref``, ``$id``, and ``$db`` fields (this is the default)
- **dbRef**: Uses a `DBRef`_ with ``$ref`` and ``$id``
- **id**: Uses a ``MongoId``
- **ref**: Uses a custom embedded object with an ``id`` field
- **id**: Uses the identifier of the referenced object

.. note::

Expand Down
1 change: 1 addition & 0 deletions doctrine-mongo-mapping.xsd
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@
<xs:simpleType name="reference-store-as">
<xs:restriction base="xs:token">
<xs:enumeration value="id"/>
<xs:enumeration value="ref"/>
<xs:enumeration value="dbRef"/>
<xs:enumeration value="dbRefWithDb"/>
</xs:restriction>
Expand Down
24 changes: 18 additions & 6 deletions lib/Doctrine/ODM/MongoDB/Aggregation/Stage/Lookup.php
Original file line number Diff line number Diff line change
Expand Up @@ -96,26 +96,38 @@ private function fromReference($fieldName)
parent::from($targetMapping->getCollection());

if ($referenceMapping['isOwningSide']) {
if ($referenceMapping['storeAs'] !== ClassMetadataInfo::REFERENCE_STORE_AS_ID) {
throw MappingException::cannotLookupNonIdReference($this->class->name, $fieldName);
switch ($referenceMapping['storeAs']) {
case ClassMetadataInfo::REFERENCE_STORE_AS_ID:
case ClassMetadataInfo::REFERENCE_STORE_AS_REF:
$referencedFieldName = ClassMetadataInfo::getReferenceFieldName($referenceMapping['storeAs'], $referenceMapping['name']);
break;

default:
throw MappingException::cannotLookupNonIdReference($this->class->name, $fieldName);
}

$this
->foreignField('_id')
->localField($referenceMapping['name']);
->localField($referencedFieldName);
} else {
if (isset($referenceMapping['repositoryMethod'])) {
throw MappingException::repositoryMethodLookupNotAllowed($this->class->name, $fieldName);
}

$mappedByMapping = $targetMapping->getFieldMapping($referenceMapping['mappedBy']);
if ($mappedByMapping['storeAs'] !== ClassMetadataInfo::REFERENCE_STORE_AS_ID) {
throw MappingException::cannotLookupNonIdReference($this->class->name, $fieldName);
switch ($mappedByMapping['storeAs']) {
case ClassMetadataInfo::REFERENCE_STORE_AS_ID:
case ClassMetadataInfo::REFERENCE_STORE_AS_REF:
$referencedFieldName = ClassMetadataInfo::getReferenceFieldName($mappedByMapping['storeAs'], $mappedByMapping['name']);
break;

default:
throw MappingException::cannotLookupNonIdReference($this->class->name, $fieldName);
}

$this
->localField('_id')
->foreignField($mappedByMapping['name']);
->foreignField($referencedFieldName);
}

return $this;
Expand Down
83 changes: 61 additions & 22 deletions lib/Doctrine/ODM/MongoDB/DocumentManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -668,15 +668,15 @@ public function getConfiguration()
}

/**
* Returns a DBRef array for the supplied document.
* Returns a reference to the supplied document.
*
* @param mixed $document A document object
* @param object $document A document object
* @param array $referenceMapping Mapping for the field that references the document
*
* @throws \InvalidArgumentException
* @return array A DBRef array
* @return mixed The reference for the document in question, according to the desired mapping
*/
public function createDBRef($document, array $referenceMapping = null)
public function createReference($document, array $referenceMapping)
{
if ( ! is_object($document)) {
throw new \InvalidArgumentException('Cannot create a DBRef, the document is not an object');
Expand All @@ -691,36 +691,54 @@ public function createDBRef($document, array $referenceMapping = null)
);
}

if ($referenceMapping['storeAs'] === ClassMetadataInfo::REFERENCE_STORE_AS_ID) {
if ($class->inheritanceType === ClassMetadataInfo::INHERITANCE_TYPE_SINGLE_COLLECTION) {
throw MappingException::simpleReferenceMustNotTargetDiscriminatedDocument($referenceMapping['targetDocument']);
}
return $class->getDatabaseIdentifierValue($id);
}

$dbRef = array(
'$ref' => $class->getCollection(),
'$id' => $class->getDatabaseIdentifierValue($id),
);
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit rusty, but is DocumentManager::createDBRef() to generic method for creating a reference to another document? I assume it's no longer specific to DBRef objects since the introduction of simple references some time ago.

Copy link
Member Author

Choose a reason for hiding this comment

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

The name would suggest it creates a DBRef object, but it was used to create whatever reference was required according to the mapping. I'd suggest deprecating this and introducing a new createReference method to take its place.

Copy link
Member Author

Choose a reason for hiding this comment

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

Introduced createReference which also makes the $referenceMapping argument mandatory. I've also replaced all occurences of createDBRef with createReference, hoping I didn't create a BC break with that because people expect createDBRef to be called.


if ($referenceMapping['storeAs'] === ClassMetadataInfo::REFERENCE_STORE_AS_DB_REF_WITH_DB) {
$dbRef['$db'] = $this->getDocumentDatabase($class->name)->getName();
$storeAs = isset($referenceMapping['storeAs']) ? $referenceMapping['storeAs'] : null;
switch ($storeAs) {
case ClassMetadataInfo::REFERENCE_STORE_AS_ID:
if ($class->inheritanceType === ClassMetadataInfo::INHERITANCE_TYPE_SINGLE_COLLECTION) {
throw MappingException::simpleReferenceMustNotTargetDiscriminatedDocument($referenceMapping['targetDocument']);
Copy link
Member

Choose a reason for hiding this comment

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

This appears to be the exception I was expecting for the id strategy and discriminated reference conflict.

}

return $class->getDatabaseIdentifierValue($id);
break;


case ClassMetadataInfo::REFERENCE_STORE_AS_REF:
$reference = ['id' => $class->getDatabaseIdentifierValue($id)];
break;

case ClassMetadataInfo::REFERENCE_STORE_AS_DB_REF:
$reference = [
'$ref' => $class->getCollection(),
'$id' => $class->getDatabaseIdentifierValue($id),
];
break;

case ClassMetadataInfo::REFERENCE_STORE_AS_DB_REF_WITH_DB:
$reference = [
'$ref' => $class->getCollection(),
'$id' => $class->getDatabaseIdentifierValue($id),
'$db' => $this->getDocumentDatabase($class->name)->getName(),
];
break;

default:
throw new \InvalidArgumentException("Reference type {$storeAs} is invalid.");
}

/* If the class has a discriminator (field and value), use it. A child
* class that is not defined in the discriminator map may only have a
* discriminator field and no value, so default to the full class name.
*/
if (isset($class->discriminatorField)) {
$dbRef[$class->discriminatorField] = isset($class->discriminatorValue)
$reference[$class->discriminatorField] = isset($class->discriminatorValue)
? $class->discriminatorValue
: $class->name;
}

/* Add a discriminator value if the referenced document is not mapped
* explicitly to a targetDocument class.
*/
if ($referenceMapping !== null && ! isset($referenceMapping['targetDocument'])) {
if (! isset($referenceMapping['targetDocument'])) {
$discriminatorField = $referenceMapping['discriminatorField'];
$discriminatorValue = isset($referenceMapping['discriminatorMap'])
? array_search($class->name, $referenceMapping['discriminatorMap'])
Expand All @@ -736,10 +754,31 @@ public function createDBRef($document, array $referenceMapping = null)
$discriminatorValue = $class->name;
}

$dbRef[$discriminatorField] = $discriminatorValue;
$reference[$discriminatorField] = $discriminatorValue;
}

return $reference;
}

/**
* Returns a DBRef array for the supplied document.
*
* @param mixed $document A document object
* @param array $referenceMapping Mapping for the field that references the document
*
* @throws \InvalidArgumentException
* @return array A DBRef array
* @deprecated Deprecated in favor of createReference; will be removed in 2.0
*/
public function createDBRef($document, array $referenceMapping = null)
{
@trigger_error('The ' . __METHOD__ . ' method has been deprecated and will be removed in ODM 2.0. Use createReference() instead.', E_USER_DEPRECATED);
Copy link
Member

Choose a reason for hiding this comment

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

What role does @ server here? I'd think we'd want to avoid suppression so that a framework's error handler can step in and convert this to an exception if it likes.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure why, but I remember Symfony handling their deprecations like that and we've also done it this way previously.


if (!isset($referenceMapping['storeAs'])) {
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 this be fixed on class metadata level?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since it's a public method I decided to be defensive regarding what we receive. There's no guarantee someone will pass an actual mapping, they could call it with a partial mapping array (e.g. our tests). One more reason to look forward to metadata objects.

$referenceMapping['storeAs'] = ClassMetadataInfo::REFERENCE_STORE_AS_DB_REF;
}

return $dbRef;
return $this->createReference($document, $referenceMapping);
}

/**
Expand Down
11 changes: 3 additions & 8 deletions lib/Doctrine/ODM/MongoDB/Hydrator/HydratorFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -255,13 +255,8 @@ private function generateHydratorClass(ClassMetadata $class, $hydratorClassName,
/** @ReferenceOne */
if (isset(\$data['%1\$s'])) {
\$reference = \$data['%1\$s'];
if (isset(\$this->class->fieldMappings['%2\$s']['storeAs']) && \$this->class->fieldMappings['%2\$s']['storeAs'] === ClassMetadataInfo::REFERENCE_STORE_AS_ID) {
\$className = \$this->class->fieldMappings['%2\$s']['targetDocument'];
\$mongoId = \$reference;
} else {
\$className = \$this->unitOfWork->getClassNameForAssociation(\$this->class->fieldMappings['%2\$s'], \$reference);
\$mongoId = \$reference['\$id'];
}
\$className = \$this->unitOfWork->getClassNameForAssociation(\$this->class->fieldMappings['%2\$s'], \$reference);
\$mongoId = ClassMetadataInfo::getReferenceId(\$reference, \$this->class->fieldMappings['%2\$s']['storeAs']);
\$targetMetadata = \$this->dm->getClassMetadata(\$className);
\$id = \$targetMetadata->getPHPIdentifierValue(\$mongoId);
\$return = \$this->dm->getReference(\$className, \$id);
Expand Down Expand Up @@ -296,7 +291,7 @@ private function generateHydratorClass(ClassMetadata $class, $hydratorClassName,
\$className = \$mapping['targetDocument'];
\$targetClass = \$this->dm->getClassMetadata(\$mapping['targetDocument']);
\$mappedByMapping = \$targetClass->fieldMappings[\$mapping['mappedBy']];
\$mappedByFieldName = isset(\$mappedByMapping['storeAs']) && \$mappedByMapping['storeAs'] === ClassMetadataInfo::REFERENCE_STORE_AS_ID ? \$mapping['mappedBy'] : \$mapping['mappedBy'].'.\$id';
\$mappedByFieldName = ClassMetadataInfo::getReferenceFieldName(\$mappedByMapping['storeAs'], \$mapping['mappedBy']);
\$criteria = array_merge(
array(\$mappedByFieldName => \$data['_id']),
isset(\$this->class->fieldMappings['%2\$s']['criteria']) ? \$this->class->fieldMappings['%2\$s']['criteria'] : array()
Expand Down
43 changes: 43 additions & 0 deletions lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadataInfo.php
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ class ClassMetadataInfo implements \Doctrine\Common\Persistence\Mapping\ClassMet
const REFERENCE_STORE_AS_ID = 'id';
const REFERENCE_STORE_AS_DB_REF = 'dbRef';
const REFERENCE_STORE_AS_DB_REF_WITH_DB = 'dbRefWithDb';
const REFERENCE_STORE_AS_REF = 'ref';

/* The inheritance mapping types */
/**
Expand Down Expand Up @@ -474,6 +475,48 @@ public function __construct($documentName)
$this->rootDocumentName = $documentName;
}

/**
* Helper method to get reference id of ref* type references
* @param mixed $reference
* @param string $storeAs
* @return mixed
* @internal
*/
public static function getReferenceId($reference, $storeAs)
Copy link
Member

Choose a reason for hiding this comment

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

Type hint $reference with array.

Copy link
Member

Choose a reason for hiding this comment

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

Should we still type hint here? Alternatively, should the @param doc above be relaxed to mixed, since we may return $reference directly and we have @return mixed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll relax the doc typehint, we can't typehint in the method since it can either be a reference array (ref or dbRef or an identifier, which again could be a string or int.

{
return $storeAs === ClassMetadataInfo::REFERENCE_STORE_AS_ID ? $reference : $reference[ClassMetadataInfo::getReferencePrefix($storeAs) . 'id'];
}

/**
* Returns the reference prefix used for a reference
* @param string $storeAs
* @return string
*/
private static function getReferencePrefix($storeAs)
{
if (!in_array($storeAs, [ClassMetadataInfo::REFERENCE_STORE_AS_REF, ClassMetadataInfo::REFERENCE_STORE_AS_DB_REF, ClassMetadataInfo::REFERENCE_STORE_AS_DB_REF_WITH_DB])) {
throw new \LogicException('Can only get a reference prefix for DBRef and reference arrays');
}

return $storeAs === ClassMetadataInfo::REFERENCE_STORE_AS_REF ? '' : '$';
}

/**
* Returns a fully qualified field name for a given reference
* @param string $storeAs
* @param string $pathPrefix The field path prefix
* @return string
* @internal
*/
public static function getReferenceFieldName($storeAs, $pathPrefix = '')
{
if ($storeAs === ClassMetadataInfo::REFERENCE_STORE_AS_ID) {
return $pathPrefix;
}

return ($pathPrefix ? $pathPrefix . '.' : '') . static::getReferencePrefix($storeAs) . 'id';
}

/**
* {@inheritDoc}
*/
Expand Down
Loading