Fix query preparation when querying for referenced object #1481

Merged
merged 1 commit into from Aug 5, 2016

Projects

None yet

2 participants

@alcaeus
Member
alcaeus commented Aug 1, 2016

This PR fixes an issue introduced with #1363 when running queries like $dm->getRepository(User::class)->findOneBy(['account' => $account]);, with $account being a referenced document. When using the references method of the query builder, the query array looks like this:

[
    'account.$id' => MongoId(...)
]

When using includesReferenceTo which is made for @ReferenceMany mappings, the result looks like this:

[
    'account' => [
        '$elemMatch' => [
            '$id' => MongoId(...)
        ]
    ]
]

However, the implementation in the DocumentPersister always yielded a full DBRef object:

[
    'account' => [
        '$ref' => 'accounts',
        '$id' => MongoId(...)
        '$db' => 'mydatabasename'
    ]
]

This caused issues when forcing queries to be covered by indexes (through the notablescan option in MongoDB), since it would require two different kinds of indexes on a reference: one on the entire DBRef object to cover queries like created through the DocumentPersister and another index on the $id field of the reference to cover queries created by the query builder in combination with references.

This solution tries to apply the same logic applied by the query builder to the documents in question. The only exception are @ReferenceOne fields without a target document set. These would produce the following query array:

[
    'account.$ref' => 'accounts',
    'account.$id' => MongoId(...),
    'account.$db' => 'mydatabasename'
]

This is not possible at this time since the DocumentPersister always assumes that a single field has exactly one value. For now, the PR includes a failing test case for this, as the DocumentPersister excludes @ReferenceOne fields without targetDocument set.

@alcaeus alcaeus added the bug label Aug 1, 2016
@alcaeus alcaeus added this to the 1.1.2 milestone Aug 1, 2016
@malarzm malarzm commented on an outdated diff Aug 1, 2016
...Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php
@@ -1036,7 +1036,20 @@ private function prepareQueryElement($fieldName, $value = null, $class = null, $
if (! empty($mapping['reference']) && is_object($value) && ! ($value instanceof \MongoId)) {
try {
- return array($fieldName, $this->dm->createDBRef($value, $mapping));
+ $dbRef = $this->dm->createDBRef($value, $mapping);
+
+ if (! is_array($dbRef)) {
@malarzm
malarzm Aug 1, 2016 Member

I had to stop for a moment and think about why such a check is here, the original check on strategy is way more readable. Also it will prevent us from hitting an edge case when id might be an array

@malarzm malarzm commented on an outdated diff Aug 1, 2016
...Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php
@@ -1036,7 +1036,20 @@ private function prepareQueryElement($fieldName, $value = null, $class = null, $
if (! empty($mapping['reference']) && is_object($value) && ! ($value instanceof \MongoId)) {
try {
- return array($fieldName, $this->dm->createDBRef($value, $mapping));
+ $dbRef = $this->dm->createDBRef($value, $mapping);
+
+ if (! is_array($dbRef)) {
+ return array($fieldName, $dbRef);
+ } elseif ($mapping['type'] == 'many') {
+ $keys = ['$ref' => true, '$id' => true, '$db' => true];
+ if (isset($mapping['targetDocument'])) {
@malarzm
malarzm Aug 1, 2016 Member

Don't we need this check too?

@malarzm
Member
malarzm commented Aug 1, 2016

As a starter I'm sorry for introducing the bug, I admit that I haven't thought about indexes at all back then :)

This is not possible at this time since the DocumentPersister always assumes that a single field has exactly one value.

If this is only issue of what's prepareQueryElement is returning (and how) it could be fairly easy (found 3 places using the method) to rework it so it could return an array of tuples (or ["field.$id" => abc, "field.$ref" => xyz] array) - what do you think?

@malarzm malarzm commented on an outdated diff Aug 1, 2016
...Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php
@@ -1036,7 +1036,20 @@ private function prepareQueryElement($fieldName, $value = null, $class = null, $
if (! empty($mapping['reference']) && is_object($value) && ! ($value instanceof \MongoId)) {
try {
- return array($fieldName, $this->dm->createDBRef($value, $mapping));
+ $dbRef = $this->dm->createDBRef($value, $mapping);
+
+ if (! is_array($dbRef)) {
+ return array($fieldName, $dbRef);
+ } elseif ($mapping['type'] == 'many') {
@malarzm
malarzm Aug 1, 2016 Member

This should be ===

@alcaeus
Member
alcaeus commented Aug 1, 2016 edited

it could be fairly easy to rework it so it could return an array of tuples

I thought about that as well - just wasn't sure which route we want to take. I'll have to see how we can accomodate that without creating a weird return type, but if at all possible I'll do exactly that. Thanks for the input! 👍

@alcaeus
Member
alcaeus commented Aug 2, 2016

@malarzm udpated PR as per your suggestion. prepareQueryElement now returns an array of key-value tupels so that more than one query element can be returned.

@malarzm malarzm and 1 other commented on an outdated diff Aug 3, 2016
...Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php
@@ -1189,12 +1190,16 @@ private function prepareQueryElement($fieldName, $value = null, $class = null, $
? $this->dm->getClassMetadata($targetMapping['targetDocument'])
: null;
- list($key, $value) = $this->prepareQueryElement($nextObjectProperty, $value, $nextTargetClass, $prepareValue);
+ $fieldNames = $this->prepareQueryElement($nextObjectProperty, $value, $nextTargetClass, $prepareValue);
+
+ return array_map(function ($preparedTupel) use ($fieldName) {
@malarzm
malarzm Aug 3, 2016 Member

Nitpicking, but you have a typo in var name ;)

@alcaeus
alcaeus Aug 3, 2016 Member

It's not a typo, it's German! 😁

@malarzm
malarzm Aug 3, 2016 Member

So the germanization of Doctrine begins, one variable at a time? :P

@alcaeus alcaeus Fix queries when querying for a referenced object afe12fa
@malarzm
Member
malarzm commented Aug 5, 2016

Thanks \o/

@malarzm malarzm merged commit d3dddc0 into doctrine:1.1.x Aug 5, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alcaeus alcaeus deleted the alcaeus:fix-dbref-queries branch Oct 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment