DiscriminatorMap field values injected too often

[AD7six]

Using a document with the following annotations:

/**
* @MongoDB\Document(collection="users")
* @MongoDB\InheritanceType("SINGLE_COLLECTION")
* @MongoDB\DiscriminatorField(fieldName="type")
* @MongoDB\DiscriminatorMap({"user"="User", "brand"="Brand", "merchant"="Merchant"})
*/
class User {

And code like so:

    $return = $repository->createQueryBuider() 
        ->update()
        ->field('_id')->equals(new \MongoId($userId))
        ->field('type')->set($type)
        ->getQuery()
        ->execute();

Executes a query like so (profiler output):

       {
         "ts" : ISODate("2012-02-08T10:18:29.076Z"), 
         "op" : "update", "ns" : "mydb.users", 
         "query" : {
             "_id" : ObjectId("4efa6a7a411e208429000007"), 
             "type" : { "$in" : [ "user", "brand", "merchant" ] }
         }, 
         "updateobj" : {
             "$set" : { "type" : "brand" }, 
             **"type" : { "$in" : [ "user", "brand", "merchant" ] }**
         }, 
         "exception" : "assertion db/ops/update.h:315", 
         "millis" : 1, 
         "client" : "127.0.0.1", 
         "user" : ""
       }

expected results would be a query ala:

db.users.update(
    {_id: ObjectId("4efa6a7a411e208429000007"), <optional autoinjected type condition>},
    {$set: {type: "brand"}}
) 

The problem isn't restricted to updates, any query which does /not/ have the discriminatory field as a top-level array key will have it auto-injected generating invalid syntax, for example:

    $qb = $repository->createQueryBuider()
    $return = $qb
            ->addOr($qb->expr()->field('email')->equals($input))   
            ->addOr($qb->expr()->field('username')->equals($input))   
            ->addOr($qb->expr()->field('name')->equals($input))   
            ->getQuery()
            ->getSingleResult();

Will execute a query like so (profiler output):

{   
    "ts" : ISODate("2012-02-08T15:09:31.452Z"),
    "op" : "query",
    "ns" : "mydb.users",
    "query" : {
        "$query" : {
            "$or" : {
                "0" : {
                    "email" : "foo@bar.com",
                    "type" : { "$in" : [ "user", "brand", "merchant" ] }
                },
                "1" : {
                    "username" : "foo@bar.com",
                     "type" : { "$in" : [ "user", "brand", "merchant" ] }
                },
                "2" : {
                    "name" : "foo@bar.com",
                     "type" : { "$in" : [ "user", "brand", "merchant" ] }
                },
                **"type" : { "$in" : [ "user", "brand", "merchant" ] }**
            },
             "type" : { "$in" : [ "user", "brand", "merchant" ] }
        },
        "$orderby" : [ ]
    },
    "ntoreturn" : 1,
    "exception" : "$or requires nonempty array",
    "exceptionCode" : 13262,
    "responseLength" : 73,
    "millis" : 0,
    "client" : "127.0.0.1",
    "user" : ""
}

expected query conditions would be:

{
    $or: [{email: "foo@bar.com"}, {username: "foo@bar.com"}, {name: "foo@bar.com"}],
    <autoinjected type condition>
}

[jmikola]

Is the type field above both a mapped field and a discriminator? IIRC, an exception is thrown if you attempt to do so, so I assume not.

When working through a recent issue with discriminator fields, I spoke to @jwage about mapping a discriminator field as a PHP property in my document model (in order to have a getter for it). I believe he said it's purposely not hydrated and cannot be used as a property-mapped field.

I'm not sure you can refer to the discriminator field in a query, though. From your example:

   $return = $repository->createQueryBuider() 
        ->update()
        ->field('_id')->equals(new \MongoId($userId))
        ->field('type')->set($type)
        ->getQuery()
        ->execute();

I believe it's only to be set after inferring the class name against the discriminator map. That could explain the odd behavior here. If it's not to be supported, perhaps ODM needs to throw an exception to be explicit.

[AD7six]

I'm not clear what your question regarding mapped fields means - there is no "map" except DiscriminatorMap in any of the classes related to the User document being used (User, Brand, Merchant).

Granted, updating a discriminatory field is probably an edge case. The first query in the ticket is purely because the User object deliberately doesn't have a setter for this property (but I need to update it).

However, it doesn't matter what field is in the update query, e.g. attempting to update name with the same syntax as above yields:

 {   
     "ts" : ISODate("2012-02-08T21:22:28.415Z"), 
     "op" : "update", 
     "ns" : "mydb.users", 
     "query" : { 
         "_id" : ObjectId("4efacf06411e206706000004"), 
         "type" : { "$in" : [ "user", "brand", "merchant" ] } 
     },  
     "updateobj" : { 
         "$set" : { 
             "name" : "foo", 
             "type" : { "$in" : [ "user", "brand", "merchant" ] } 
         },  
         "type" : { "$in" : [ "user", "brand", "merchant" ] } 
     },  
     "exception" : "assertion db/ops/update.h:315",
     "millis" : 1,
     "client" : "127.0.0.1",
     "user" : ""
 }

And in any event, criteria which contain an $operator should not IMO be modified at all (e.g. $not: { ... <autoinjected type condition>} - totally undesirable), and obviously not update objects either.

I did take a look at what changes would be required but it looks like a none trivial change to prevent this as and I don't want to waste your time providing a patch that is useless (since I'm not familiar with the doctrine code).

[jmikola]

By mapped field I meant something like this:

/** @MongoDB\String */
protected $type;

That was something I was playing around with two weeks ago, and it resulted in an exception because type was already registered (i.e. mapped) in the class metadata as a discriminator field. And even without the annotation, Doctrine will not hydrate a property corresponding to the discriminator field. Sorry for straying off-topic with that.

In your last example, it's clear that "type" : { "$in" : [ "user", "brand", "merchant" ] } does not belong in a $set command. The behavior you're seeing could certainly be the result of some recent changes that fixed discriminator bugs. I believe one bug @jwage fixed was discriminators not being set at all on some update/upsert queries. I'll wait for him to chime in on this, unless you'd like to back-pedal through his recent commits over the last two weeks and see if anything stands out.

[jwage]

Hi, can you provide a test case for the expected behavior? and I will work on a fix.

[AD7six]

Possibly, though this is a good reason not to:

[10:18][andy@work:~/www/apps/myapp/vendor/doctrine-mongodb-odm(master)]$ phpunit 
PHPUnit 3.6.7 by Sebastian Bergmann.

Configuration read from /home/andy/www/apps/myapp/vendor/doctrine-mongodb-odm/phpunit.xml.dist

.......................................................Segmentation fault
[10:18][andy@work:~/www/apps/myapp/vendor/doctrine-mongodb-odm(master)]$ 

[kusmierz]

Any news/progress?

[jwage]

Hi, I don't have any test case or anything to work from. Can you help provide something for us to work against?

[kusmierz]

<?php
/**
 * @MongoDB\Document(collection="articles", repositoryClass="Acme\AcmeBundle\Document\ArticleRepository")
 * @MongoDB\DiscriminatorField(fieldName="type")
 * @MongoDB\DiscriminatorMap({"article"="Article", "gallery"="\Acme\AcmeBundle\Document\Gallery"})
 */
class Article

now I have in <Acme\AcmeBundle\Document\ArticleRepository>:

<?php

namespace Acme\AcmeBundle\Document;

use Doctrine\ODM\MongoDB\DocumentRepository;

/**
 * ArticleRepository
 *
 * This class was generated by the Doctrine ORM. Add your own custom
 * repository methods below.
 */
class ArticleRepository extends DocumentRepository
{
    /**
     * Find tag for ajax action
     *
     * @param string $value
     * @return array
     */
    public function findAjaxValue($value)
    {
        // simple validation
        if (empty($value) || !is_string($value)) {
            return array();
        }

        $value = (string) $value;
        return $this->findBy(array(
            'public' => true,
            '$or' => array(
                array('_id' => $value), // można wkleić ID
                array('title' => array('$regex' => '^' . preg_quote($value) . '.*')),
            )), array('guid' => true), 20);
    }
}

And as result I've got exception "$and/$or/$nor must be a nonempty array/500 Internal Server Error - MongoCursorException".

The query:

db.articles.find({
    "public": true,
    "$or": {
        "0": { "_id": "p" },
        "1": { "title": { "$regex": "^p.*" } },
        "type": { "$in": [ "article" ] } // !!!!!????
    },
    "type": { "$in": [ "article" ] }
}).sort({ "guid": true });

I think that lines are ambiguous. You can't inject "type": { "$in": [ "article" ] } in $or statement!

[jwage]

Can you make a phpunit test case so I can run it?

[kusmierz]

I don't know if it's correct test case, but it works (I mean it doesn't ;)

<?php

// (...)
use Documents\Functional\SameCollection1;

class PersistenceBuilderTest extends BaseTest
{
    // (...)
    public function testFindWithOrOnCollectionWithDiscriminatorMap()
    {
        $id = '4f28aa84acee41388900000a';
        $ids = array($id);

        /**
         * @var \Doctrine\ODM\MongoDB\Query\Builder $qb
         */
        $qb = $this->dm->createQueryBuilder('Documents\Functional\SameCollection1');
        $qb
            ->addOr($qb->expr()->field('id')->in($ids))
            ->select('id')->hydrate(false);
        /**
         * @var \Doctrine\ODM\MongoDB\Query\Query $query
         * @var \Doctrine\ODM\MongoDB\Cursor $results
         */
        $query = $qb->getQuery();
        $results = $query->execute();
        $debug = $query->debug();

        $this->assertEquals($id, (string) current($debug['$or'][0]['_id']['$in']));

        $this->assertInstanceOf('Doctrine\MongoDB\Cursor', $results);
        $this->assertEquals(0, $query->count());
    }
    // (...)
}

[kusmierz]

Is it fixable? :)

[jwage]

Most likely, I just haven't had a chance to look at it yet. I will try and make time to work on the odm tonight after work.

[kusmierz]

Thanks in advance :)

[wedgybo]

A quick update on what causes this. It looks like this part here gets run on every pass and adds in the discriminator values to all commands.

if ($this->class->hasDiscriminator() && ! isset($query[$this->class->discriminatorField['name']])) {
    $discriminatorValues = $this->getClassDiscriminatorValues($this->class);
    $query[$this->class->discriminatorField['name']] = array('$in' => $discriminatorValues);
}

I've created a quick hack which involves only running that section of code when the query is being generated for the final time.

library/Doctrine/ODM/MongoDB/Query/Builder.php
233

•    $query['query'] = $this->expr->getQuery();
  

•   $query['query'] = $this->expr->getQuery($this->class->hasDiscriminator());
  

And then look for that being true when preparing the query. I'll paste the full patch if anyone is interested, but I think this is a pretty hacky way to do it. I'm sure there's a better way of detecting when that discriminator code should be added.

[kusmierz]

I think this would be better:

diff --git a/lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php b/lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php
index 907d8df..a8aa512 100644
--- a/lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php
+++ b/lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php
@@ -780,11 +780,13 @@ class DocumentPersister
         if (is_scalar($query) || $query instanceof \MongoId) {
             $query = array('_id' => $query);
         }
-        if ($this->class->hasDiscriminator() && ! isset($query[$this->class->discriminatorField['name']])) {
+        static $queryNestLevel = 0;
+        if (!$queryNestLevel && $this->class->hasDiscriminator() && ! isset($query[$this->class->discriminatorField['name']])) {
             $discriminatorValues = $this->getClassDiscriminatorValues($this->class);
             $query[$this->class->discriminatorField['name']] = array('$in' => $discriminatorValues);
         }
         $newQuery = array();
+        ++ $queryNestLevel;
         if ($query) {
             foreach ($query as $key => $value) {
                 if (isset($key[0]) && $key[0] === $this->cmd && is_array($value)) {
@@ -795,6 +797,7 @@ class DocumentPersister
             }
             $newQuery = $this->convertTypes($newQuery);
         }
+        -- $queryNestLevel;
         return $newQuery;
     }

It will add discriminator thing only once to the most top of the query (and don't inject in values). What do you think? It's also connected with #283 issue.

[kusmierz]

@jwage we are waiting for your patch ;)

[jwage]

I was hoping someone would send a pull request with the failing test since we use this awesome system with that feature :) I guess I will resort to copying and pasting the test and committing it up myself :(

[kusmierz]

see the test commited in 03d3bd7, I have injected value even in $newObj object in update method, could you look?

[kusmierz]

Any progress? :)

[jwage]

I haven't had time to look yet.