Add generic reference as replacement for DBRef #1623

alcaeus · 2017-07-26T13:50:25Z

Supersedes #1566.

As discussed in our last hangout, this is what remains of the DBRef replacement. DBRef is not deprecated (yet), athough we might decide to deprecate it at this time. At this time the PR is feature complete, I'd like to write a whole bunch of tests to ensure the whole discriminator handling/omitting target collection name works as I'd expect it to.

The new reference object only stores an ID and optionally a discriminator field. The main difference to DBRef is that it can be used in aggregation pipeline stages that don't handle DBRef objects because of the $ in field names (e.g. $lookup and $graphLookup).

I'll also revisit #1527 to see if we can store additional fields in the reference (which would also apply to DBRef objects).

jmikola · 2017-07-31T16:01:53Z

docs/en/reference/annotations-reference.rst

-    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. They


Should we keep the DBRef documentation link?

Yes, we should. Will update that.

jmikola · 2017-07-31T16:02:49Z

docs/en/reference/annotations-reference.rst

-    ``dbRef`` uses a `DBRef`_ without ``$db`` value and ``dbRefWithDb`` stores
-    a full `DBRef`_ (``$ref``, ``$id``, and ``$db``). Note that ``id``
+    ``ref`` stores fields ``id`` and ``ref`` (similar to DBRef without $ prefix) value and ``refWithDb`` stores
+    a additionally db as parameter. ``dbRef`` and ``dbRefWithDb`` use ``DBRef``, they are deprecated in favor of ref and refWithDb. Note that ``id``


Is there any reason for this to not simply repeat the @ReferenceMany documentation for storeAs?

jmikola · 2017-07-31T16:03:54Z

docs/en/reference/reference-mapping.rst

@@ -360,6 +360,7 @@ The ``storeAs`` option has three possible values:

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


The id documentation should likely be changed to "Uses the identifier" instead of "MongoId" to be consistent with the change you made for @ReferenceMany and storeAs.

jmikola · 2017-07-31T16:06:20Z

lib/Doctrine/ODM/MongoDB/Aggregation/Stage/Lookup.php

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


Since this is repeated for $referenceMapping['storeAs'] above and $mappedByMapping['storeAs'] here, would it make sense to extract this to a helper method?

jmikola · 2017-07-31T16:11:15Z

lib/Doctrine/ODM/MongoDB/DocumentManager.php

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


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.

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.

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.

jmikola · 2017-07-31T16:23:37Z

lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php

@@ -722,7 +722,7 @@ private function loadReferenceManyCollectionOwningSide(PersistentCollectionInter
                $mongoId = $reference;
            } else {
                $className = $this->uow->getClassNameForAssociation($mapping, $reference);
-                $mongoId = $reference['$id'];
+                $mongoId = ClassMetadataInfo::getReferenceId($reference, $mapping['storeAs']);


Earlier, I noted that you may want to add the array type hint to getReferenceId()'s first parameter. Alternatively, it may be worth changing getReferenceId() and UnitOfWork::getClassNameForAssociation() to work on simple references, which would let you remove this if/else entirely and simply assign $className and $mongoId the same for all reference types.

jmikola · 2017-07-31T16:24:49Z

lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php

+        if (isset($mappedByMapping['storeAs']) && $mappedByMapping['storeAs'] === ClassMetadataInfo::REFERENCE_STORE_AS_ID) {
+            $mappedByFieldName = $mapping['mappedBy'];
+        } else {
+            $mappedByFieldName = $mapping['mappedBy'] . '.' . ClassMetadataInfo::getReferencePrefix(isset($mappedByMapping['storeAs']) ? $mappedByMapping['storeAs'] : null) . 'id';


Similarly, perhaps we can have a single method here that returns the mappedByFieldName for any reference type. That may allow us to keep getReferencePrefix() private.

jmikola · 2017-07-31T16:32:18Z

lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php

@@ -1424,20 +1429,25 @@ private function getWriteOptions(array $options = array())
    private function prepareDbRefElement($fieldName, $value, array $mapping, $inNewObj)
    {
        $dbRef = $this->dm->createDBRef($value, $mapping);
-        if ($inNewObj) {
+        if ($inNewObj || $mapping['storeAs'] === ClassMetadataInfo::REFERENCE_STORE_AS_ID) {


I don't quite recall what prepareDbRefElement() is used for, but these changes appear to be correct. I assume discriminator values (if applicable) are added later, since this method only deals with basic keys for database references?

jmikola · 2017-07-31T16:33:45Z

lib/Doctrine/ODM/MongoDB/Query/ReferencePrimer.php

@@ -255,6 +255,7 @@ private function addManyReferences(PersistentCollectionInterface $persistentColl
    {
        $mapping = $persistentCollection->getMapping();

+


Unnecessary line break?

jmikola · 2017-07-31T16:35:16Z

tests/Doctrine/ODM/MongoDB/Tests/Aggregation/Stage/LookupTest.php

+    public function testLookupStageReferenceManyStoreAsRef()
+    {
+        $this->requireMongoDB32('$lookup tests require at least MongoDB 3.2.0');
+        $this->insertTestData();


Would it make sense for this to go in the setUp() function for the test class, or is it not needed by each test case?

alcaeus · 2017-09-24T13:05:00Z

@jmikola took some time to work on this and applied most of your feedback. I still need to check for discriminator handling in prepareDBRef (which was renamed to prepareReference in DocumentPersister. I'll also have to revisit the exception mentioned in #1623 (comment) and add some tests. I'll hopefully get to that in a few days, but I believe the pull request is worth another look in the meantime.

alcaeus · 2017-09-25T07:56:37Z

Note: build fails due to new driver version. I'll take a look at that in master branch.

jmikola · 2017-09-26T15:53:48Z

docs/en/reference/annotations-reference.rst

+    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. They
+    are deprecated in favor of ``ref``. Note that ``id`` references are not


For clarity, I'd replace ". They" with "and" to combine these sentences.

jmikola · 2017-09-26T15:54:45Z

docs/en/reference/annotations-reference.rst

 -
    cascade - Cascade Option
 -
    discriminatorField - The field name to store the discriminator value within
-    the `DBRef`_ object.
+    the reference object.


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.

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.

jmikola · 2017-09-26T16:00:29Z

lib/Doctrine/ODM/MongoDB/DocumentManager.php

+        switch ($storeAs) {
+            case ClassMetadataInfo::REFERENCE_STORE_AS_ID:
+                if ($class->inheritanceType === ClassMetadataInfo::INHERITANCE_TYPE_SINGLE_COLLECTION) {
+                    throw MappingException::simpleReferenceMustNotTargetDiscriminatedDocument($referenceMapping['targetDocument']);


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

jmikola · 2017-09-26T16:02:18Z

lib/Doctrine/ODM/MongoDB/DocumentManager.php

+                $dbRef = ['id' => $class->getDatabaseIdentifierValue($id)];
+                break;
+
+            default:


Might be safer to explicitly handle both cases here and leave default: in place to throw an exception for an unsupported strategy. That keeps the code defensive in case we ever add more strategies down the line.

jmikola · 2017-09-26T16:03:46Z

lib/Doctrine/ODM/MongoDB/DocumentManager.php

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


Would it make sense to rename this local variable to $ref, since this was the original name used for two DBRef strategies handled by this function?

jmikola · 2017-09-26T16:10:55Z

lib/Doctrine/ODM/MongoDB/Query/Expr.php

@@ -73,28 +73,34 @@ public function references($document)
    {
        if ($this->currentField) {
            $mapping = $this->getReferenceMapping();
-            $dbRef = $this->dm->createDBRef($document, $mapping);
+            $dbRef = $this->dm->createReference($document, $mapping);


Rename to $ref?

jmikola · 2017-09-26T16:11:09Z

lib/Doctrine/ODM/MongoDB/Query/Expr.php

-                $keys = array('ref' => true, 'id' => true, 'db' => true);
+                if ($storeAs === ClassMetadataInfo::REFERENCE_STORE_AS_REF) {
+                    $keys = ['id' => true];
+                } else {


Another place we might want to be defensive about $storeAs values.

jmikola · 2017-09-26T16:11:30Z

lib/Doctrine/ODM/MongoDB/Query/Expr.php

                }
            }
        } else {
-            $dbRef = $this->dm->createDBRef($document);
+            @trigger_error('Calling ' . __METHOD__ . ' without a current field set will no longer be possible in ODM 2.0.', E_USER_DEPRECATED);


Same question as before about role of @.

jmikola · 2017-09-26T16:11:49Z

lib/Doctrine/ODM/MongoDB/Query/Expr.php

-                $keys = array('ref' => true, 'id' => true, 'db' => true);
+                if ($storeAs === ClassMetadataInfo::REFERENCE_STORE_AS_REF) {
+                    $keys = ['id' => true];
+                } else {


Defensive opportunity.

jmikola · 2017-09-26T16:14:15Z

tests/Doctrine/ODM/MongoDB/Tests/Aggregation/Stage/LookupTest.php

+    /**
+     * @before
+     */
+    public function prepareTest()


Is this an alternative to setUp() and calling parent::setUp()?

Exactly - not sure where the advantages of each are, but I've started using @before and @after instead of setUp and tearDown in test classes and only use the latter in abstract TestCase classes.

This avoids repeatedly checking for the type of reference and duplicating logic. It also allows us to make getReferencePrefix private.

malarzm · 2017-10-06T13:40:14Z

lib/Doctrine/ODM/MongoDB/DocumentManager.php

+    {
+        @trigger_error('The ' . __METHOD__ . ' method has been deprecated and will be removed in ODM 2.0. Use createReference() instead.', E_USER_DEPRECATED);
+
+        if (!isset($referenceMapping['storeAs'])) {


Shouldn't this be fixed on class metadata level?

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.

malarzm · 2017-10-06T13:44:01Z

lib/Doctrine/ODM/MongoDB/Query/Expr.php

-                $this->query[$mapping['name']] = $dbRef;
-            } else {
-                $keys = array('ref' => true, 'id' => true, 'db' => true);
+            switch ($storeAs) {


This seem to be very similar switch to one from DocumentPersister - maybe this should be merged somehow?

I can try and extract the logic to a common method - maybe I can use DocumentManager::createReference for it.

No big deal if it's complicated or anything, just occurred to me while reading :)

Nah, it's worth a shot :)

Turns out the methods are similar to suggest extracting the logic to a separate method, but different enough to not really change anything. Decided to skip it after all.

malarzm · 2017-10-06T13:44:44Z

lib/Doctrine/ODM/MongoDB/Query/Expr.php

-                $this->query[$mapping['name']] = $dbRef;
-            } else {
-                $keys = array('ref' => true, 'id' => true, 'db' => true);
+            switch ($storeAs) {


alcaeus added Feature Needs Test labels Jul 26, 2017

alcaeus added this to the 1.2 milestone Jul 26, 2017

alcaeus mentioned this pull request Jul 26, 2017

[WIP] Feature/ref without dbref #1566

Closed

amcsi mentioned this pull request Jul 27, 2017

Suggestion: New type of reference to deprecate usage of dbref #1560

Closed

jmikola requested changes Jul 31, 2017

View reviewed changes

alcaeus force-pushed the add-new-reference-type branch from 51ad6f0 to c2cc628 Compare September 24, 2017 13:00

jmikola reviewed Sep 26, 2017

View reviewed changes

alcaeus force-pushed the add-new-reference-type branch from fbfc8ce to 761c496 Compare September 27, 2017 17:13

jmikola approved these changes Sep 28, 2017

View reviewed changes

alcaeus force-pushed the add-new-reference-type branch from 761c496 to 40cb88e Compare October 1, 2017 04:22

alcaeus added 6 commits October 4, 2017 19:39

Add generic reference as replacement for DBRef

242211e

Update documentation

23b2c93

Introduce createReference method in DocumentManager

5a8b85c

Add method to generate the reference field name

2084552

This avoids repeatedly checking for the type of reference and duplicating logic. It also allows us to make getReferencePrefix private.

Move server version check to setup method

802d6ab

Apply review feedback

0ac42f6

alcaeus force-pushed the add-new-reference-type branch from 40cb88e to 0ac42f6 Compare October 4, 2017 17:41

alcaeus removed the Needs Test label Oct 4, 2017

malarzm reviewed Oct 6, 2017

View reviewed changes

alcaeus merged commit 845f07a into doctrine:master Oct 7, 2017

alcaeus deleted the add-new-reference-type branch October 7, 2017 13:00

		@@ -255,6 +255,7 @@ private function addManyReferences(PersistentCollectionInterface $persistentColl
		{
		$mapping = $persistentCollection->getMapping();



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

Add generic reference as replacement for DBRef #1623

Add generic reference as replacement for DBRef #1623

Conversation

alcaeus commented Jul 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alcaeus commented Sep 24, 2017

alcaeus commented Sep 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment