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

Documents with inheritance do not $unset null properties #2209

Closed Steveb-p closed 4 years ago

Steveb-p commented 4 years ago

Bug Report

Q A
BC Break no
Version 2.1.2

Summary

When a document class is part of inheritance, fields that are set to null during update are not removed from the database.

Current behavior & How to reproduce

Consider classes declared:

<?php declare(strict_types=1);

use Doctrine\ODM\MongoDB\Mapping\Annotations as ODM;

/**
 * @ODM\Document()
 * @ODM\DiscriminatorMap({"inheriting" = InheritingDocument::class})
 */
class DocumentWithInheritance
{
    /**
     * I'm intentionally leaving @Version as it might have some effect
     *
     * @var int
     * @ODM\Field(type="int")
     * @ODM\Version()
     */
    private $version;

    /**
     * @var int|null
     * @ODM\Field(type="int", name="locked_until")
     */
    private $lockedUntil;

    public function setLocked(int $until): void
    {
        return $this->lockedUntil = $until;
    }

    public function isLocked(): bool
    {
        return isset($this->lockedUntil) && $this->lockedUntil > time();
    }

    public function unlock(): self
    {
        $this->lockedUntil = null;
        return $this;
    }
}

and

<?php declare(strict_types=1);

use Doctrine\ODM\MongoDB\Mapping\Annotations as ODM;

/**
 * @ODM\Document()
 */
class InheritingDocument extends DocumentWithInheritance
{
    // Implementation irrelevant to example
}

When document is updated with null value it will not be $unset.

class IssueTest extends TestCase 
{
    public function testInheritanceNull()
    {
        // Initialization skipped
        $document = new InheritingDocument();
        $document->setLocked(time() + 3600);
        $dm->persist($document);
        $dm->flush();
        $id = $document->getId();
        $dm->clear();

        $document = $dm->find(InheritingDocument::class, $id);
        $document->unlock();
        $dm->flush();

        // Here database will still contain int value, even though document has `lockedUntil = null`
    }
}

While I haven't run the test, I have it confirmed during dev by looking at Doctrine\ODM\MongoDB\Persisters\DocumentPersister::update.

    // ...

    /**
     * Updates the already persisted document if it has any new changesets.
     *
     * @throws LockException
     */
    public function update(object $document, array $options = []) : void
    {
        $update = $this->pb->prepareUpdateData($document);

        $query = $this->getQueryForDocument($document);

        if ($document instanceof InheritingDocument) {
            dd($update, $query);
        }

    // ...

with inheritance:

image

without:

image

I was considering if maybe #2196 might be related to this, but the case seems different enough to warrant a different issue in my opinion.

malarzm commented 4 years ago

Note to myself: check if the same thing is happening on 1.3.x branch after writing a test

Steveb-p commented 4 years ago

It seems I'm at fault for this one (I suspected as much, it would be really strange if an issue like this was not more widespread).

The sample test presented is not what was happening in my code. In reality I was loading the document like so:

$qb = $this->createQueryBuilder();
$qb->findAndUpdate();
$qb->addAnd(
    $qb->expr()->addOr(
        $qb->expr()->field('locked_until')->lt(time()),
        $qb->expr()->field('locked_until')->exists(false),
    )
);
$document = $qb->getQuery()->execute();

What I didn't realize is that I was missing a crucial method call: $qb->returnNew();. Therefore I got a document with null value in locked_until field and this resulted in ODM ignoring it (since as far as it could tell, it wasn't changed).

alcaeus commented 4 years ago

Thanks for updating us. I didn't get around to testing this, so I'm happy you were able to resolve this yourself.