Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix hydration of proxy objects with lazy public properties #1784

Merged
merged 2 commits into from Apr 13, 2018

Conversation

alcaeus
Copy link
Member

@alcaeus alcaeus commented Apr 12, 2018

This pull request fixes #1775 by ensuring the proxy initializer is unset before hydration for a proxy document begins. As shown in the provided test case, this can lead to data loss if
a) a document contains public properties and is still an uninitialized proxy
b) the document contains a many relationship (reference or embed) which isn't changed during the process.

The data loss occurs because the hydrator sets the value of a lazy public property, triggering proxy initialization. For one, this causes an additional database read which can cause performance problems.
Setting aside any issues due to data being changed in the database in the meantime, the document in the end will be exactly what to expect, without any issues. However, after proxy initialization is complete, the original hydration cycle for the document sets the originalValue for the document in UnitOfWork. This sets a different PersistentCollection (containing the same data internally) as originalValue for any many relationship.

The next time a change set is computed for the (initialized) proxy document, UnitOfWork compares originalValue and actualValue for the relationship. Since the instance changed, it schedules a collection deletion for the originalValue. However, since actualValue already is a PersistentCollection and isn't marked as dirty (because we didn't change anything in the collection), it won't be persisted, leaving only the collection deletion to cause some data loss.

The root issue is fixed by the changes in HydratorFactory, which ensures that a proxy object with public properties is not hydrated twice, removing the cause for the issue. I checked to see whether we need to apply some changes to the logic which deals with collection replacements, but I figured out that the only way we could end up with a non-dirty persistent collection in actualValue is if
a) a non-empty collection was replaced with an empty one; in this case the deletion without insertion is the correct course of action
b) a non-dirty persistent collection containing data was injected into a document; in this case, we won't prevent a user from shooting themselves in the foot.

@alcaeus alcaeus added the Bug label Apr 12, 2018
@alcaeus alcaeus added this to the 1.2.4 milestone Apr 12, 2018
@alcaeus alcaeus self-assigned this Apr 12, 2018
@alcaeus
Copy link
Member Author

alcaeus commented Apr 12, 2018

@pascal-hofmann the fix works on your test, but I'd appreciate it if you could confirm the fix works in your actual setup as well. Unfortunately, there is no more support for ODM 1.1, which means you'll have to upgrade to 1.2 in order to get a bugfix. ODM 1.2 contains no BC breaks, so in theory the upgrade should be relatively straightforward, given your system fulfills the requirements with respect to PHP and MongoDB driver versions.

@pascal-hofmann
Copy link
Contributor

@alcaeus Thanks for looking into this so quickly. I will check if this solves the issue in our application too.

@pascal-hofmann
Copy link
Contributor

@alcaeus I just gave it a try. It looks good: This also fixes the issue in our application! 🎉

@malarzm
Copy link
Member

malarzm commented Apr 13, 2018

Great, merging then :)

@malarzm malarzm merged commit 05cd3a5 into doctrine:1.2.x Apr 13, 2018
@malarzm
Copy link
Member

malarzm commented Apr 13, 2018

@alcaeus will you cherry pick this into master branch or shall I?

@alcaeus
Copy link
Member Author

alcaeus commented Apr 20, 2018

Sorry @malarzm, forgot this one. Created a PR to apply to 2.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants