DDC-171: Cascade persist update foreign key for all unchanged entities #2359

Closed
doctrinebot opened this Issue Nov 23, 2009 · 14 comments

1 participant

@doctrinebot

Jira issue originally created by user rickdt:

Using cascade persist, linked entities get updated even if there is no change made.

I get an update on the unchanged foreign key even if I don't modify
the entity at all.

UPDATE email SET client_id = ? WHERE id = ?
$query = $qb->getQuery();
$client = $query->getSingleResult();
$em->flush(); 
@doctrinebot

Comment created by romanb:

Can we see the query? I mean the DQL query! Thanks.

@doctrinebot

Comment created by romanb:

Nevermind. Got it reproduced.

@doctrinebot

Comment created by rickdt:

I tried with revision 6913 and I still reproduce the problem.

Is there anyway I can use your UniTest framework, that way, I could check your test case and maybe suggest improvement.

@doctrinebot

Comment created by rickdt:

I managed to run the UnitTests.

Do you have a test for this specific issue ?

@doctrinebot

Comment created by rickdt:

Here is my result for Doctrine/Tests/ORM/AllTests.php

1) testOptimisticTimestampSetsDefaultValue(Doctrine\Tests\ORM\Functional\Locking\OptimisticTest)
strtotime(): It is not safe to rely on the system's timezone settings. You are **required** to use the date.timezone setting or the date*default_timezone_set() function. In case you used any of those methods and you are still getting this warning, you most likely misspelled the timezone identifier. We selected 'America/New*York' for 'EST/-5.0/no DST' instead
/home/eric/doctrine_trunk/tests/Doctrine/Tests/ORM/Functional/Locking/OptimisticTest.php:126

2) testOptimisticTimestampFailureThrowsException(Doctrine\Tests\ORM\Functional\Locking\OptimisticTest)
DateTime::createFromFormat(): It is not safe to rely on the system's timezone settings. You are **required** to use the date.timezone setting or the date*default_timezone_set() function. In case you used any of those methods and you are still getting this warning, you most likely misspelled the timezone identifier. We selected 'America/New*York' for 'EST/-5.0/no DST' instead
/home/eric/doctrine_trunk/lib/Doctrine/DBAL/Types/DateTimeType.php:33
/home/eric/doctrine_trunk/lib/Doctrine/ORM/Internal/Hydration/AbstractHydrator.php:225
/home/eric/doctrine_trunk/lib/Doctrine/ORM/Internal/Hydration/ObjectHydrator.php:239
/home/eric/doctrine_trunk/lib/Doctrine/ORM/Internal/Hydration/ObjectHydrator.php:112
/home/eric/doctrine_trunk/lib/Doctrine/ORM/Internal/Hydration/AbstractHydrator.php:102
/home/eric/doctrine_trunk/lib/Doctrine/ORM/AbstractQuery.php:511
/home/eric/doctrine_trunk/lib/Doctrine/ORM/AbstractQuery.php:387
/home/eric/doctrine_trunk/tests/Doctrine/Tests/ORM/Functional/Locking/OptimisticTest.php:136

3) testJoinedSubclassPersisterRequiresSpecificOrderOfMetadataReflFieldsArray(Doctrine\Tests\ORM\Functional\Ticket\DDC168Test)
PDOException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'salary' cannot be null
/home/eric/doctrine_trunk/lib/Doctrine/ORM/Persisters/JoinedSubclassPersister.php:190
/home/eric/doctrine_trunk/lib/Doctrine/ORM/UnitOfWork.php:698
/home/eric/doctrine_trunk/lib/Doctrine/ORM/UnitOfWork.php:283
/home/eric/doctrine_trunk/lib/Doctrine/ORM/EntityManager.php:288
/home/eric/doctrine_trunk/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC168Test.php:34
@doctrinebot

Comment created by romanb:

You need to fix your timezone settings but that is unrelated to this problem.

To reproduce it, I added the following test temporarily to Doctrine\Tests\ORM\Functional\BasicFunctionalTest.php:

    public function testFlushDoesNotIssueUnnecessaryUpdates()
    {

        $user = new CmsUser;
        $user->name = 'Guilherme';
        $user->username = 'gblanco';
        $user->status = 'developer';

        $address = new CmsAddress;
        $address->country = 'Germany';
        $address->city = 'Berlin';
        $address->zip = '12345';

        $address->user = $user;
        $user->address = $address;

        $this->_em->persist($user);

        $this->_em->flush();
        $this->_em->clear();

        // show SQL from here on...
        $this->_em->getConnection()->getConfiguration()->setSqlLogger(new \Doctrine\DBAL\Logging\EchoSqlLogger);


        $query = $this->_em->createQuery('select u, a from Doctrine\Tests\Models\CMS\CmsUser u join u.address a');
        echo "QUERY!".PHP_EOL;
        $user2 = $query->getSingleResult();

        echo "FLUSH!".PHP_EOL;
        $this->_em->flush();

    }

However, since I did not really know how to check that, keeping this test in didnt make much sense.

Can you try this test? Before my patch it issued an UPDATE query but not after my changes.

It is pretty difficult to test this in an isolated manner.

@doctrinebot

Comment created by @beberlei:

First two failures are your fault, you havent set the default timezone in php.ini

The last failure is unrelated, its a benchmark for another bug.

@doctrinebot

Comment created by romanb:

OK. Latest trunk contains a test now that verifies that no SQL is executed on the flush.

Can you please tell us how to modify it to reproduce the problem?

Look into: Doctrine/Tests/ORM/Functional/BasicFunctionalTest.php

Last test method.

@doctrinebot

Comment created by rickdt:

Got it! Your fix worked on OneToOne relation but not OneToMany

I did the same test using the CmsArticles entity and here is the result :

SELECT c0*.id AS id0, c0_.status AS status1, c0_.username AS username2, c0_.name AS name3, c1_.id AS id4, c1_.topic AS topic5, c1_.text AS text6, c1_.user_id AS user_id7 FROM cms_users c0_ INNER JOIN cms_articles c1_ ON c0_.id = c1_.user*id
FLUSH!
UPDATE cms_users SET name = ? WHERE id = ?
array(2) {
  [0]=>
  string(6) "FooBar"
  [1]=>
  int(17)
}
UPDATE cms*articles SET user*id = ? WHERE id = ?
array(2) {
  [0]=>
  int(17)
  [1]=>
  int(2)
}
DONE!
DELETE FROM cms*users*groups
DELETE FROM cms_groups
DELETE FROM cms_addresses
DELETE FROM cms_phonenumbers
DELETE FROM cms_articles
DELETE FROM cms_users
@doctrinebot

Comment created by rickdt:

Dont forget to add the cascade persist for the articles relation

public function testFlushDoesNotIssueUnnecessaryUpdates()
    {

        $user = new CmsUser;
        $user->name = 'Guilherme';
        $user->username = 'gblanco';
        $user->status = 'developer';

        $address = new CmsAddress;
        $address->country = 'Germany';
        $address->city = 'Berlin';
        $address->zip = '12345';

        $article = new \Doctrine\Tests\Models\CMS\CmsArticle();
        $article->text = "Lorem ipsum dolor sunt.";
        $article->topic = "A Test Article!";
        $article->setAuthor($user);
        $user->articles[] = $article;


        $address->user = $user;
        $user->address = $address;

        $this->_em->persist($user);

        $this->_em->flush();
        $this->_em->clear();

        // show SQL from here on...
        $this->_em->getConnection()->getConfiguration()->setSqlLogger(new \Doctrine\DBAL\Logging\EchoSqlLogger);


        //$query = $this->_em->createQuery('select u, a from Doctrine\Tests\Models\CMS\CmsUser u join u.address a');
        $query = $this->_em->createQuery('select u, a from Doctrine\Tests\Models\CMS\CmsUser u join u.articles a');
        echo "QUERY!".PHP_EOL;
        $user2 = $query->getSingleResult();

        $user2->name = "FooBar";

        echo "FLUSH!".PHP_EOL;
        $this->_em->flush();

        echo "DONE!".PHP_EOL;

    }
@doctrinebot

Comment created by romanb:

OK. I will look into this later or tomorrow.

Thanks for your help.

@doctrinebot

Comment created by romanb:

Should be fixed now (second attempt) :)

Can you check whether DDC-140 can be closed also?

@doctrinebot

Issue was closed with resolution "Fixed"

@doctrinebot

Comment created by rickdt:

Yes, this is now fixed for me.
I confirm you this also close DDC-140

Thank you for your great work !!

@doctrinebot doctrinebot added this to the 2.0-ALPHA4 milestone Dec 6, 2015
@doctrinebot doctrinebot closed this Dec 6, 2015
@doctrinebot doctrinebot added the Bug label Dec 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment