Unable to add document to EmbedMany collection with strategy $set #503

Closed
SteveTalbot opened this Issue Feb 19, 2013 · 2 comments

2 participants

@SteveTalbot

Pull request #404 appears to have introduced a new bug.

Using the same example classes in that request:

/** @ODM\Document */
class Parent
{
    /** @ODM\Id */
    private $id;

    /** @ODM\EmbedMany(targetDocument="Child", strategy="set") */
    private $childs;

    ...
}

/** @ODM\EmbeddedDocument */
class Child
{
    /** @ODM\String */
    private $value

   ...
}

If you query a Parent document from the database, add an extra Child, and persist the Parent, all the existing children are deleted from the database and only the new child is stored.

I tried commenting out the change made by #404, and adding the Child works, so it seems the $set strategy is still wrong.

I think the fix is to check the strategy in CollectionPersister::update. If the strategy is "set", execute a single set query to update the entire EmbedMany collection. If not, execute insertRows and deleteRows.

My fix is as follows:

    /**
     * Updates a PersistentCollection instance deleting removed rows and inserting new rows.
     *
     * @param PersistentCollection $coll
     * @param array $options
     */
    public function update(PersistentCollection $coll, array $options)
    {
        $mapping = $coll->getMapping();
        if ($mapping['isInverseSide']) {
            return; // ignore inverse side
        }

        if($mapping['strategy']==='set') {
            list($propertyPath, $parent) = $this->getPathAndParent($coll);
            $collArray = array();
            foreach($coll as $key => $document) {
                if (isset($mapping['reference'])) {
                    $documentUpdates = $this->pb->prepareReferencedDocumentValue($mapping, $document);
                } else {
                    $documentUpdates = $this->pb->prepareEmbeddedDocumentValue($mapping, $document);
                }
                $collArray[$key] = $documentUpdates;
            }
            $query = array($this->cmd.'set' => array($propertyPath => $collArray));
            $this->executeQuery($parent, $query, $options);
        } else {
            $this->deleteRows($coll, $options);
            $this->insertRows($coll, $options);
        }

    }

Since this also guarantees that CollectionPersister::insertRows and CollectionPersister::deleteRows will only be called if the mapping strategy is not "set", these two functions can be simplified.

This works for me but not sure if it will break other use cases. Just tidying up my code now and will submit a pull request as a basis for further discussion.

Cheers,
Steve

@SteveTalbot SteveTalbot pushed a commit that referenced this issue Feb 19, 2013
Steve Talbot See issue #503 8913154
@malarzm
Doctrine member

I believe this ticket can be closed, PR was merged

@malarzm
Doctrine member

Closing issue as PR was merged

@malarzm malarzm closed this Mar 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment