doctrine / mongodb-odm

The Official PHP MongoDB ORM/ODM
https://www.doctrine-project.org/projects/doctrine-mongodb-odm/en/latest/
MIT License
1.09k stars 504 forks source link

Using push() - with each() - and pushAll() in DocumentRepository doesn't prepare embedded documents #1165

Closed net02 closed 2 years ago

net02 commented 9 years ago

When updating an EmbedMany field in a DocumentRepository, explicitly using QueryBuilder pushAll or push (with Expr::each, i.e. to leverage Expr::slice), embedded documents are not prepared.

A MongoException zero-length keys are not allowed, did you use $ with double quotes? ensues due to the string representation of the PHP Object being used.

Examples:

/**
 * @ODM\Document(repositoryClass="FooRepository")
 */
class Foo
{
    ...
    /** 
     * @ODM\EmbedMany(targetDocument="Bar") 
     */
    protected $bars = array();
    ...
}

class FooRepository extends DocumentRepository
{
    public function pushBar(Foo $foo, Bar $bar) {
        $qb = $this->createQueryBuilder();
        $innerExpr = $qb->expr();
        $innerExpr
            ->sort('barField', 1)
            ->slice(3)
            ->each([$bar])
        ;
        $qb
            ->update()
            ->field('bars')->push($innerExpr)
            ->field('id')->equals($foo->getId())
            ->getQuery()
            ->execute()
        ;
        // or
        $qb
            ->update()
            ->field('bars')->pushAll([$bar])
            ->field('id')->equals($foo->getId())
            ->getQuery()
            ->execute()
        ;
    }
}

The second example doesn't really make sense since using Foo::addBar($bar) with the default strategy works, but I stumbled upon the first then verified it affected explicitly calling pushAll too.

As a workaround, I tried manually preparing values and stumbled upon another bug with $each. Here's the hack:

class FooRepository extends DocumentRepository
{
    public function pushBar(Foo $foo, Bar $bar) {
        $pb = new \Doctrine\ODM\MongoDB\Persisters\PersistenceBuilder($this->getDocumentManager(), $this->getDocumentManager()->getUnitOfWork());
        $metadata  = $this->getClassMetadata();
        $value = $pb->prepareEmbeddedDocumentValue($metadata->fieldMappings['bars'], $bar);

        $qb = $this->createQueryBuilder();
        $innerExpr = $qb->expr();
        $innerExpr
            ->sort('barField', 1)
            ->slice(3)
            ->each([$value])
        ;
        ...
    }
}

This raises an error in DocumentPersister::prepareQueryOrNewObj (which I guess should be revised to fix $each and $pushAll embedded documents preparation in first place). It's quite convoluted but it breaks down to

in_array(0, array('$and', '$or', '$nor')

asserting true due to in_array not being strict, and it's easy fixed adding the strict flag.

I could set up a pull request with this small fix, and maybe a shortcut to PersistenceBuilder || prepareValue in DocumentRepository (it's not accessible from the DocumentPersister right now). Tried my hand at fixing the grand schema of things in DocumentPersister::prepareQueryOrNewObj but I think it's too much for me at the moment :)

malarzm commented 9 years ago

@net02 thanks for the report! I think so far everybody (including me) was using plain values with QueryBuilder but indeed it makes sense to consider such usage. As for the feature itself I don't think PersistenceBuilder should be exposed anywhere for preparing embedded documents' value and all action should be done behind the scenes, maybe somewhere nearby these lines? Just a quick idea :)

I have added this to 1.x milestone but if somebody would hack an implementation by the time we want to release 1.0 then most probably it will be included ;)

malarzm commented 9 years ago

However there's one thing that need to be tackled: nested embedded documents. Given that we may not know exact mapping all the way up we may insert wrong discriminator value to newly created embedded document.

dev-ish commented 8 years ago

Found this thread after a google search and unfortunately it doesn't looks like this feature is implemented in doctrine/mongodb-odm so far. It would be really nice if this behaviour was implemented.

For those that need this, i found a not so nice way to do this. As a quick solution

$pb = new \Doctrine\ODM\MongoDB\Persisters\PersistenceBuilder($this->getDocumentManager(), $this->getDocumentManager()->getUnitOfWork());
$metadata  = $this->getClassMetadata();

$value = $pb->prepareEmbeddedDocumentValue($metadata->fieldMappings['bar'], $history);

$this->getCollection()->findAndUpdate(
    [$metadata->fieldMappings['id']['name'] => new \MongoId($product->getId())],
    ['$push' => [
        $metadata->fieldMappings['bar']['name'] => [
            '$each' => [$value],
            '$slice' => 3,
            '$sort' => [
                'barField' => -1
            ]
        ]
    ]]
);
stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in a week if no further activity occurs. Thank you for your contributions.