Orm: Wrong commit order in some relation cases

[contentrail]

Issue was found after updating to v.2.6 and associated with improvements in CommitOrderCalculator class.
Here is the example to reproduce.
Objects:

• Book
• PCT with mandatory relation to Book (ManyToOne).
  Book has optional relation to the PCT object. Some Bookes has PCT and some not. But all PCT has relation to Book.
• PCTFee as a child object of PCT (ManyToOne).

/**
 * @ORM\Entity
 * @ORM\Table(name="book")
 */
class Book
{
    /**
     * @var int
     * @ORM\Id
     * @ORM\Column(type="integer")
     * @ORM\GeneratedValue(strategy="NONE")
     */
    private $id;

    /**
     * @var string
     * @ORM\Column(type="string", length=255, nullable=true)
     */
    private $exchangeCode;

    /**
     * @var PCT
     * @ORM\OneToOne(targetEntity="PCT", cascade={"persist", "remove"})
     * @ORM\JoinColumn(name="paymentCardTransactionId", referencedColumnName="id")
     */
    private $paymentCardTransaction;

    public function __construct(int $id)
    {
        $this->id = $id;
    }

    public function getId(): int
    {
        return $this->id;
    }

    public function getExchangeCode(): ?string
    {
        return $this->exchangeCode;
    }

    public function setExchangeCode(string $exchangeCode = null): void
    {
        $this->exchangeCode = $exchangeCode;
    }

    public function getPaymentCardTransaction(): ?PCT
    {
        return $this->paymentCardTransaction;
    }

    public function setPaymentCardTransaction(PCT $paymentCardTransaction = null): void
    {
        $this->paymentCardTransaction = $paymentCardTransaction;
    }
}

/**
 * @ORM\Entity
 * @ORM\Table(name="pct")
 */
class PCT
{
    /**
     * @var int
     * @ORM\Id
     * @ORM\Column(type="integer")
     * @ORM\GeneratedValue(strategy="NONE")
     */
    private $id;

    /**
     * @var Book
     * @ORM\ManyToOne(targetEntity="Book")
     * @ORM\JoinColumn(name="bookingId", referencedColumnName="id", nullable=false)
     */
    private $book;

    /**
     * @var PCTFee[]
     * @ORM\OneToMany(targetEntity="PCTFee", mappedBy="pct", cascade={"persist", "remove"})
     * @ORM\OrderBy({"id" = "ASC"})
     */
    private $fees;

    public function __construct(int $id, Book $book)
    {
        $this->id = $id;
        $this->book = $book;
        $this->fees = new ArrayCollection();
    }

    public function getId(): int
    {
        return $this->id;
    }

    /**
     * @return PCTFee[]|Collection
     */
    public function getFees(): Collection
    {
        return $this->fees;
    }

    public function addFee(PCTFee $fee)
    {
        $this->fees->add($fee);
    }

    public function removeFee(PCTFee $fee)
    {
        $this->fees->removeElement($fee);
    }

    public function getBook(): Book
    {
        return $this->book;
    }
}

/**
 * @ORM\Entity
 * @ORM\Table(name="pct_fee")
 */
class PCTFee
{
    /**
     * @var int
     * @ORM\Id
     * @ORM\Column(type="integer")
     * @ORM\GeneratedValue(strategy="AUTO")
     */
    private $id;

    /**
     * @var PCT
     * @ORM\ManyToOne(targetEntity="PCT", inversedBy="fees")
     * @ORM\JoinColumn(name="paymentCardTransactionId", referencedColumnName="id", nullable=false)
     */
    private $pct;

    public function __construct(PCT $pct)
    {
        $this->pct = $pct;
        $pct->addFee($this);
    }

    public function getId(): ?int
    {
        return $this->id;
    }

    public function getPCT(): PCT
    {
        return $this->pct;
    }
}

How to reproduce:
Find one Book (or create new).
Create new PCT object with even one PCTFee object for this Book object.

Try to flush.

        $booking = $this->em()->getRepository(Book::class)->find(1);
        if (!$booking) {
            $booking = new Book(1);
            $booking->setExchangeCode('1');
            $this->em()->persist($booking);
        }
        $id = (int) $booking->getExchangeCode();
        $id++;
        $booking->setExchangeCode((string) $id); // Change smth.

        $paymentCardTransaction = new PCT($id, $booking);

        $paymentCardTransactionFee = new PCTFee($paymentCardTransaction);

        $this->em()->persist($paymentCardTransaction);

        $this->save();

Error:

An exception occurred while executing 'INSERT INTO pct_fee (paymentCardTransactionId) VALUES (?)' with params [null]:

SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'paymentCardTransactionId' cannot be null

I've try to figure out why and found that CommitOrderCalculator produce wrong commit order: PCTFee, Book, PCT.
It's wrong because PCTFee depends on PCT - PCT must be saved earlier.

New code in CommitOrderCalculator uses weights of relations. But weights is wrong.
We have 2 relations with different weights (nullable and not nullable):

• PCT to Book with weight 0 (not nullable)
• Book to PCT with weight 1 (nullable).

Before version 2.6 CommitOrderCalculator has checked both relations. But now it checks only relation with maximum weight!

I've removed code using weights from CommitOrderCalculator:

case self::IN_PROGRESS:
                    if (isset($adjacentVertex->dependencyList[$vertex->hash]) &&
                        $adjacentVertex->dependencyList[$vertex->hash]->weight < $edge->weight) {
                        $adjacentVertex->state = self::VISITED;

                        $this->sortedNodeList[] = $adjacentVertex->value;
                    }
                    break;

and bug has disappeared.

[Ocramius]

This requires a reproducible/minimal test case before the CommitOrderCalculator can be adapted though. It seems that you already solved the issue - do you mind sending a patch with the files and examples above?

Also, do you think that a unit test case can be made around CommitOrderCalculator? I am fairly sure that the change is simply colliding with some other test scenario.

[contentrail]

It seems that you already solved the issue - do you mind sending a patch with the files and examples above?

I didn't fix it. In fact I've just rollback CommitOrderCalculator class to its previous version by functionality.

[bendavies]

I'm also getting this error but really struggling to produce a minimal test case. Still trying

[paali]

I have this too. I debugged the flush call and commit order calculation seems to be wrong (for whatever reason). Went back to 2.5.14 and the problem disappeared. I was using nelmio/alice to populate a test database and \Doctrine\ORM\UnitOfWork::getCommitOrder seems to return wrong order. I don't believe alice has anything to do with the issue though.

My case shortly:
User has one-to-many relation to UserData
UserData has many-to-one, non-nullable relation to User (as user)
UserData has many-to-one, non-nullable relation to User (as dataOwnerr)
Other relations might be related. There are quite a lot of them.
UserData inserstions try to occur first which causes the cannot be null error seen above.

Sorry I cannot help more at he the moment. Just "me too" really.

[contentrail]

<?php

include '../vendor/doctrine/orm/lib/Doctrine/ORM/Internal/CommitOrderCalculator.php';
$calc = new \Doctrine\ORM\Internal\CommitOrderCalculator();

$classPCT = json_decode('{"name":"PCT","namespace":"","rootEntityName":"PCT","customGeneratorDefinition":null,"customRepositoryClassName":null,"isMappedSuperclass":false,"isEmbeddedClass":false,"parentClasses":[],"subClasses":[],"embeddedClasses":[],"namedQueries":[],"namedNativeQueries":[],"sqlResultSetMappings":[],"identifier":["id"],"inheritanceType":1,"generatorType":5,"fieldMappings":{"id":{"fieldName":"id","type":"integer","scale":0,"length":null,"unique":false,"nullable":false,"precision":0,"id":true,"columnName":"id"}},"fieldNames":{"id":"id"},"columnNames":{"id":"id"},"discriminatorValue":null,"discriminatorMap":[],"discriminatorColumn":null,"table":{"name":"pct"},"lifecycleCallbacks":[],"entityListeners":[],"associationMappings":{"book":{"fieldName":"book","joinColumns":[{"name":"bookingId","unique":false,"nullable":false,"onDelete":null,"columnDefinition":null,"referencedColumnName":"id"}],"cascade":[],"inversedBy":null,"targetEntity":"Book","fetch":2,"type":2,"mappedBy":null,"isOwningSide":true,"sourceEntity":"PCT","isCascadeRemove":false,"isCascadePersist":false,"isCascadeRefresh":false,"isCascadeMerge":false,"isCascadeDetach":false,"sourceToTargetKeyColumns":{"bookingId":"id"},"joinColumnFieldNames":{"bookingId":"bookingId"},"targetToSourceKeyColumns":{"id":"bookingId"},"orphanRemoval":false},"fees":{"fieldName":"fees","mappedBy":"pct","targetEntity":"PCTFee","cascade":["persist","remove"],"orphanRemoval":false,"fetch":2,"orderBy":{"id":"ASC"},"type":4,"inversedBy":null,"isOwningSide":false,"sourceEntity":"PCT","isCascadeRemove":true,"isCascadePersist":true,"isCascadeRefresh":false,"isCascadeMerge":false,"isCascadeDetach":false}},"isIdentifierComposite":false,"containsForeignIdentifier":false,"idGenerator":{},"sequenceGeneratorDefinition":null,"tableGeneratorDefinition":null,"changeTrackingPolicy":1,"isVersioned":null,"versionField":null,"cache":null,"reflClass":{"name":"PCT"},"isReadOnly":false,"reflFields":{"id":{"name":"id","class":"PCT"},"book":{"name":"book","class":"PCT"},"fees":{"name":"fees","class":"PCT"}}}');
$calc->addNode($classPCT->name, $classPCT);

$classPCTFee = json_decode('{"name":"PCTFee","namespace":"","rootEntityName":"PCTFee","customGeneratorDefinition":null,"customRepositoryClassName":null,"isMappedSuperclass":false,"isEmbeddedClass":false,"parentClasses":[],"subClasses":[],"embeddedClasses":[],"namedQueries":[],"namedNativeQueries":[],"sqlResultSetMappings":[],"identifier":["id"],"inheritanceType":1,"generatorType":4,"fieldMappings":{"id":{"fieldName":"id","type":"integer","scale":0,"length":null,"unique":false,"nullable":false,"precision":0,"id":true,"columnName":"id"}},"fieldNames":{"id":"id"},"columnNames":{"id":"id"},"discriminatorValue":null,"discriminatorMap":[],"discriminatorColumn":null,"table":{"name":"pct_fee"},"lifecycleCallbacks":[],"entityListeners":[],"associationMappings":{"pct":{"fieldName":"pct","joinColumns":[{"name":"paymentCardTransactionId","unique":false,"nullable":false,"onDelete":null,"columnDefinition":null,"referencedColumnName":"id"}],"cascade":[],"inversedBy":"fees","targetEntity":"PCT","fetch":2,"type":2,"mappedBy":null,"isOwningSide":true,"sourceEntity":"PCTFee","isCascadeRemove":false,"isCascadePersist":false,"isCascadeRefresh":false,"isCascadeMerge":false,"isCascadeDetach":false,"sourceToTargetKeyColumns":{"paymentCardTransactionId":"id"},"joinColumnFieldNames":{"paymentCardTransactionId":"paymentCardTransactionId"},"targetToSourceKeyColumns":{"id":"paymentCardTransactionId"},"orphanRemoval":false}},"isIdentifierComposite":false,"containsForeignIdentifier":false,"idGenerator":{},"sequenceGeneratorDefinition":null,"tableGeneratorDefinition":null,"changeTrackingPolicy":1,"isVersioned":null,"versionField":null,"cache":null,"reflClass":{"name":"PCTFee"},"isReadOnly":false,"reflFields":{"id":{"name":"id","class":"PCTFee"},"pct":{"name":"pct","class":"PCTFee"}}}');
$calc->addNode($classPCTFee->name, $classPCTFee);

$classBook = json_decode('{"name":"Book","namespace":"","rootEntityName":"Book","customGeneratorDefinition":null,"customRepositoryClassName":null,"isMappedSuperclass":false,"isEmbeddedClass":false,"parentClasses":[],"subClasses":[],"embeddedClasses":[],"namedQueries":[],"namedNativeQueries":[],"sqlResultSetMappings":[],"identifier":["id"],"inheritanceType":1,"generatorType":5,"fieldMappings":{"id":{"fieldName":"id","type":"integer","scale":0,"length":null,"unique":false,"nullable":false,"precision":0,"id":true,"columnName":"id"},"exchangeCode":{"fieldName":"exchangeCode","type":"string","scale":0,"length":255,"unique":false,"nullable":true,"precision":0,"columnName":"exchangeCode"}},"fieldNames":{"id":"id","exchangeCode":"exchangeCode"},"columnNames":{"id":"id","exchangeCode":"exchangeCode"},"discriminatorValue":null,"discriminatorMap":[],"discriminatorColumn":null,"table":{"name":"book"},"lifecycleCallbacks":[],"entityListeners":[],"associationMappings":{"paymentCardTransaction":{"fieldName":"paymentCardTransaction","targetEntity":"PCT","joinColumns":[{"name":"paymentCardTransactionId","unique":true,"nullable":true,"onDelete":null,"columnDefinition":null,"referencedColumnName":"id"}],"mappedBy":null,"inversedBy":null,"cascade":["persist","remove"],"orphanRemoval":false,"fetch":2,"type":1,"isOwningSide":true,"sourceEntity":"Book","isCascadeRemove":true,"isCascadePersist":true,"isCascadeRefresh":false,"isCascadeMerge":false,"isCascadeDetach":false,"sourceToTargetKeyColumns":{"paymentCardTransactionId":"id"},"joinColumnFieldNames":{"paymentCardTransactionId":"paymentCardTransactionId"},"targetToSourceKeyColumns":{"id":"paymentCardTransactionId"}}},"isIdentifierComposite":false,"containsForeignIdentifier":false,"idGenerator":{},"sequenceGeneratorDefinition":null,"tableGeneratorDefinition":null,"changeTrackingPolicy":1,"isVersioned":null,"versionField":null,"cache":null,"reflClass":{"name":"Book"},"isReadOnly":false,"reflFields":{"id":{"name":"id","class":"Book"},"exchangeCode":{"name":"exchangeCode","class":"Book"},"paymentCardTransaction":{"name":"paymentCardTransaction","class":"Book"}}}');
$calc->addNode($classBook->name, $classBook);

$calc->addDependency($classPCT->name, $classBook->name, 0);
$calc->addDependency($classPCT->name, $classPCTFee->name, 1);
$calc->addDependency($classBook->name, $classPCT->name, 1);

$result = $calc->sort();

$okSortedClasses = ['PCT', 'PCTFee', 'Book'];
$rightOrder = implode(',', $okSortedClasses);
foreach ($result as $class) {
    $okClassName = array_shift($okSortedClasses);
    if ($class->name !== $okClassName) {
        echo 'Wrong class commit order! Should be: '.$rightOrder.'. Got: '.implode(',', array_map(function ($item) {return $item->name;}, $result));
        break;
    }
}

[lcobucci]

@contentrail awesome, could you turn that into a unit test and send a PR?

[bendavies]

@lcobucci there is a testcase here?
https://github.com/doctrine/doctrine2/pull/6533

[lcobucci]

@bendavies that's a functional test, I've asked for a unit test (since @contentrail isolated the cause of the bug)

[someother1]

After a upgrade, this affects us too - was a pain to find it!

I just have to say:
Its a shame that this has not been resolved - open more then 3 months ..
Its a shame that this project is not maintained as it should be (900 Issues, growing and growing...)

[Ocramius]

@someother1 the project is active and maintained: if you need this to be fixed then remembered that this is open source, and fixes come from contributors first.

[Majkl578]

This looks similar to https://github.com/doctrine/doctrine2/issues/7259 which is fixed in 2.6.x-dev, can you confirm @contentrail @paali @someother1? Thanks!

[bendavies]

@Majkl578 or it might be this one:
https://github.com/doctrine/doctrine2/pull/7180

[holantomas]

@Majkl578 I can confirm that 2.6.x-dev(2.6.x-dev c10433e) didn't fix that. Updated from 2.6.3 right now, clean cache, and it's still in wrong order so it failing on cannot be null, my configuration is.
Person::$address - oneToOne to Address
Person::$household - manyToOne to HouseHold
HouseHold::$members - oneToMany to Person

After setup new instance of Address and HouseHold, I put them over setters to new instance of Person(of course instance of Person is added to HouseHold::$members collection). Then manually persist all entities.

$em->persist($address);
$em->persist($household);
$em->persist($person);

When I try to flush, the commit order is

• Address
• Person
• HouseHold

Exception by Person.household_id cannot be null. What is interesting that when I change persist order to:

$em->persist($household);
$em->persist($address);
$em->persist($person);

Then it fails on Person.address_id cannot be null ... so commit order depend on persist order?

Do you guys have any more info about? There're many issues about that, should I discuss somewhere else?