#6613 #6614 correct handling of new elements added to an initialized (yet dirty) collection #6616

Ocramius · 2017-08-11T13:19:02Z

This is a very bad bug. The fix introduces some ovehead, but the scenario is usually not that common, so it should be fine.

Ideally, we should keep new and dirty states completely separate, but that's not possible without completely changing also the base class (Doctrine\Common\Collections\AbstractLazyCollection)

…g identifiers to reduce moving parts

…ng`), segregating phone creation away

…iations

…ty status

…are restored in the dirty state of the collection

… `PersistentCollection`

alcaeus · 2017-08-11T13:29:49Z

lib/Doctrine/ORM/PersistentCollection.php

+        $loadedObjects               = $this->collection->toArray();
+        $newObjectsByOid             = array_combine(array_map('spl_object_hash', $newObjects), $newObjects);
+        $loadedObjectsByOid          = array_combine(array_map('spl_object_hash', $loadedObjects), $loadedObjects);
+        $newObjectsThatWereNotLoaded = array_diff_key($newObjectsByOid, $loadedObjectsByOid);


Clever...I'm totally stealing this one.

alcaeus · 2017-08-11T13:30:11Z

tests/Doctrine/Tests/ORM/Functional/Ticket/DDC6613Test.php

+    {
+        $this->id = uniqid('phone', true);
+    }
+}


Needs trailing newline 😛

I'll probably get rid of the test once I convert it to a unit test

alcaeus · 2017-08-11T13:32:23Z

lib/Doctrine/ORM/PersistentCollection.php


-            $this->isDirty = true;
-        }
+        $this->isDirty = true;


The collection shouldn't be marked as dirty if no items were added after initializing.

It actually should: it was loaded, but it was never flushed. I also had a previous assertion for that, see 1fc7f81

Assume you add the following code after 1fc7f81#diff-c32ade803f1349ec4da7da8644879ef6R65:

$this->_em->flush(); self::assertFalse($phones->isInitialized()); self::assertFalse($phones->isDirty()); $phones->add($item2); self::assertFalse($phones->isInitialized()); self::assertTrue($phones->isDirty()); $phones->initialize(); self::assertTrue($phones->isInitialized()); self::assertFalse($phones->isDirty());

After the initialize, the only item that was added to the uninitialized collection was already present after the collection was initialized. Thus, after initializing, the collection is not dirty.

That element does not exist in the DB though, so the collection does indeed need to go through persistence at the next flush() call.

If the collection wasn't dirty after this initialization, the object association would be completely lost (other tests break too if I do that, btw).

If you were to flush after adding $item2, then re-add $item2 to the (now uninitialized) collection. Once the collection is initialized, there will be no element to be added in restoreNewObjectsInDirtyCollection, but the collection will be marked as dirty, even though it wasn't modified.

@alcaeus re-read the test carefully:

add (collection dirty, uninitialized)

flush (collection clean, uninitialized)

add (collection dirty, uninitialized)

initialize (collection dirty, since we added and we didn't flush)

flush (collection clean, initialized)

@alcaeus I converted these tests into more isolated PersistentCollectionTest scenarios: please check them.

Also, should we drop this integration test?

@Ocramius new tests look good. I believe we can drop the integration test.

I've also pushed testWillNotMarkCollectionAsDirtyAfterInitializationIfNoElementsWereAdded to show the issue I was mentioning above: after initializing, there are no new elements in the collection, but it is still marked as dirty. Feel free updating either the test or the collection logic to make it pass.

Ocramius · 2017-08-11T13:36:27Z

tests/Doctrine/Tests/ORM/Functional/Ticket/DDC6613Test.php

@@ -0,0 +1,107 @@
+<?php


This entire test should probably be ported to PersistentCollectionTest and stripped of any UnitOfWork interactions

guilhermeblanco · 2017-08-11T15:29:39Z

lib/Doctrine/ORM/PersistentCollection.php


        if ($this->isDirty) {
-            $newObjects = $this->collection->toArray();
+            $newlyAddedDirtyObjects = $this->collection->toArray();
        }

        $this->collection->clear();


I think the issue could be solved by only not calling clear() here.
IIRC, UnitOfWork already handles dirty elements in the collection and does not replace them when hydrating in BasicEntityPersister::loadCollectionFromStatement (which is where all calls go at the end).

That's not the case: removing the clear() causes this test to fail, as well as another 3:

There were 4 failures: 1) Doctrine\Tests\ORM\Functional\ManyToManyBasicAssociationTest::testRetrieveManyToManyAndAddMore Failed asserting that 4 matches expected 3. /home/ocramius/Documents/doctrine/doctrine2/tests/Doctrine/Tests/ORM/Functional/ManyToManyBasicAssociationTest.php:201 2) Doctrine\Tests\ORM\Functional\SecondLevelCacheOneToManyTest::testCacheInitializeCollectionWithNewObjects Failed asserting that actual size 3 matches expected size 4. /home/ocramius/Documents/doctrine/doctrine2/tests/Doctrine/Tests/ORM/Functional/SecondLevelCacheOneToManyTest.php:377 3) Doctrine\Tests\ORM\Functional\Ticket\DDC1643Test::testNotCloneAndPassAroundFlush Failed asserting that 2 matches expected 3. /home/ocramius/Documents/doctrine/doctrine2/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC1643Test.php:118 4) Doctrine\Tests\ORM\Functional\Ticket\DDC6613Test::testFail Failed asserting that actual size 3 matches expected size 2. /home/ocramius/Documents/doctrine/doctrine2/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC6613Test.php:71

I think the current solution is a decent one, as it keeps the responsibility of re-attaching the NEW records within the PersistentCollection implementation. Distributing it around multiple objects may be better for performance, but is actually a mess to track on the long term

Ocramius · 2017-08-11T15:39:12Z

lib/Doctrine/ORM/PersistentCollection.php

+    private function restoreNewObjectsInDirtyCollection(array $newObjects)
+    {
+        $loadedObjects               = $this->collection->toArray();
+        $newObjectsByOid             = array_combine(array_map('spl_object_hash', $newObjects), $newObjects);


These variables are redundant

…erifies basic dirty collection initialization semantics

…that are also coming from initialization data de-duplicates new and persisted items

… of code

… if all new items are contained in the initialized ones

…it tests

…toreNewObjectsInDirtyCollection` implementation

…erifies basic dirty collection initialization semantics

…that are also coming from initialization data de-duplicates new and persisted items

…s of code

… if all new items are contained in the initialized ones

…it tests

…toreNewObjectsInDirtyCollection` implementation

…y-object-persistence-2.5' into 2.5 Backport #6613 Backport #6614 Backport #6616

…methods

…y-object-persistence' Close #6613 Close #6614 Close #6616

Ocramius · 2017-08-11T19:30:47Z

Merged:

master: 633c846
2.5: 095611c

Sysaninster and others added 16 commits August 11, 2017 13:46

It added the unit test #6613

874d60d

#6613 #6614 simplifying entity definition - using auto-assigned strin…

594e60d

…g identifiers to reduce moving parts

#6613 #6614 better test specification - removing useless assertions

0a1a841

#6613 #6614 smashing entity definitions into the test

112a402

#6613 #6614 removing dedicated DDC6613 model directory

625f792

#6613 #6614 CS - applying @group annotation to the test

c195064

#6613 #6614 rewrote test logic to be less magic-constant-dependent

eca1d6b

#6613 #6614 remove superfluous mappings

a4e547b

#6613 #6614 correcting column mapping (was integer, should be `stri…

80f12ed

…ng`), segregating phone creation away

#6613 #6614 removing phone/user specifics, using ORM naming for assoc…

8c4b5a4

…iations

#6613 #6614 removing IDE-generated header

d2c9b22

#6613 #6614 adding assertions about collection initialization and dir…

5e2257d

…ty status

#6613 #6614 after initialization, the collection should be dirty anyway

1fc7f81

#6613 #6614 ensuring that only newly added items that weren't loaded …

d44e6e1

…are restored in the dirty state of the collection

#6613 #6614 correcting broken test that isn't using objects against a…

031e79e

… `PersistentCollection`

#6613 #6614 correcting broken test that isn't using objects against a…

345cf1a

… `PersistentCollection`

Ocramius added the Bug label Aug 11, 2017

Ocramius added this to the 2.5.7 milestone Aug 11, 2017

Ocramius requested review from lcobucci, guilhermeblanco and alcaeus August 11, 2017 13:19

This was referenced Aug 11, 2017

UNIQUE constraint failed errors when manipulating a LAZY collection and calling toArray() on it #6614

Merged

There is the fail "UNIQUE constraint failed" with a LAZY collection #6613

Closed

alcaeus requested changes Aug 11, 2017

View reviewed changes

Ocramius commented Aug 11, 2017

View reviewed changes

guilhermeblanco reviewed Aug 11, 2017

View reviewed changes

Ocramius commented Aug 11, 2017

View reviewed changes

Ocramius added 2 commits August 11, 2017 18:58

#6613 #6614 #6616 moved integration test basics to a unit test that v…

04a5b12

…erifies basic dirty collection initialization semantics

#6613 #6614 #6616 initializing a dirty collection that has new items …

93c4064

…that are also coming from initialization data de-duplicates new and persisted items

Ocramius self-assigned this Aug 11, 2017

Ocramius and others added 5 commits August 11, 2017 19:08

#6613 #6614 #6616 removing repeated PersistentCollectionTest chunks…

b064fe3

… of code

Add failing test for dirty flag

1174ec6

#6613 #6614 #6616 ensuring that the collection is marked as non-dirty…

a2ca6bb

… if all new items are contained in the initialized ones

#6613 #6614 #6616 removing DDC6613 test, which was fully ported to un…

3579997

…it tests

#6613 #6614 #6616 minor performance optimisations around the new `res…

004ac51

…toreNewObjectsInDirtyCollection` implementation

Ocramius added a commit that referenced this pull request Aug 11, 2017

#6613 #6614 #6616 moved integration test basics to a unit test that v…

bdae362

…erifies basic dirty collection initialization semantics

Ocramius added a commit that referenced this pull request Aug 11, 2017

#6613 #6614 #6616 initializing a dirty collection that has new items …

d6bcb5b

…that are also coming from initialization data de-duplicates new and persisted items

Ocramius added a commit that referenced this pull request Aug 11, 2017

#6613 #6614 #6616 removing repeated PersistentCollectionTest chunk…

61cb03b

…s of code

Ocramius added a commit that referenced this pull request Aug 11, 2017

#6613 #6614 #6616 ensuring that the collection is marked as non-dirty…

15731c7

… if all new items are contained in the initialized ones

Ocramius added a commit that referenced this pull request Aug 11, 2017

#6613 #6614 #6616 removing DDC6613 test, which was fully ported to un…

ab63628

…it tests

Ocramius added a commit that referenced this pull request Aug 11, 2017

#6613 #6614 #6616 minor performance optimisations around the new `res…

5cacb6e

…toreNewObjectsInDirtyCollection` implementation

Ocramius added a commit that referenced this pull request Aug 11, 2017

#6613 #6614 #6616 removed unused import

96c6f4c

Ocramius added a commit that referenced this pull request Aug 11, 2017

Merge branch 'fix/#6614-clean-modified-collection-causing-double-dirt…

095611c

…y-object-persistence-2.5' into 2.5 Backport #6613 Backport #6614 Backport #6616

Ocramius merged commit 004ac51 into master Aug 11, 2017

Ocramius added a commit that referenced this pull request Aug 11, 2017

#6613 #6614 #6616 adding void return type

c2dd274

Ocramius added a commit that referenced this pull request Aug 11, 2017

#6613 #6614 #6616 adding void return type to newly introduced test …

352b3ba

…methods

Ocramius added a commit that referenced this pull request Aug 11, 2017

Merge branch 'fix/#6614-clean-modified-collection-causing-double-dirt…

633c846

…y-object-persistence' Close #6613 Close #6614 Close #6616

Ocramius deleted the fix/#6614-clean-modified-collection-causing-double-dirty-object-persistence branch August 11, 2017 19:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#6613 #6614 correct handling of new elements added to an initialized (yet dirty) collection #6616

#6613 #6614 correct handling of new elements added to an initialized (yet dirty) collection #6616

Ocramius commented Aug 11, 2017

alcaeus Aug 11, 2017

alcaeus Aug 11, 2017

Ocramius Aug 11, 2017

alcaeus Aug 11, 2017

Ocramius Aug 11, 2017

alcaeus Aug 11, 2017

Ocramius Aug 11, 2017

alcaeus Aug 11, 2017

Ocramius Aug 11, 2017 •

edited

Loading

Ocramius Aug 11, 2017

alcaeus Aug 11, 2017

Ocramius Aug 11, 2017

guilhermeblanco Aug 11, 2017

Ocramius Aug 11, 2017

Ocramius Aug 11, 2017

Ocramius Aug 11, 2017

Ocramius commented Aug 11, 2017

#6613 #6614 correct handling of new elements added to an initialized (yet dirty) collection #6616

#6613 #6614 correct handling of new elements added to an initialized (yet dirty) collection #6616

Conversation

Ocramius commented Aug 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ocramius Aug 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ocramius commented Aug 11, 2017

Ocramius Aug 11, 2017 •

edited

Loading