Optimized nested collections deletion in DocumentPersister #1880
Conversation
…letion of collections that is belong to one document.
…sister::deleteAll. Fixed style of code.
…eduled for update - now nested collections for one parent is updated inside one query where it was possible.
There was a problem hiding this comment.
Sorry for the delay, but here's a first code review. @malarzm you've previously spent some time working on collection updates when you implemented the atomic strategies, do you think the test cases are sufficient to guard against mishaps or do we want to add some more?
tests/Doctrine/ODM/MongoDB/Tests/Functional/CollectionPersisterTest.php
Outdated
Show resolved
Hide resolved
|
Just waiting on Feedback by @malarzm on whether we need additional test cases. Please bear with us - both of us currently don't have a whole lot of free time to dedicate to OSS. |
There was a problem hiding this comment.
About review: I've left few remarks about things that stood out while reading. That most probably won't be all, but methods can be simplified and I'd rather take another look then. Also I think the PR is changing too much comparing to original scope.
About tests: from what I remember from implementing atomic strategies the most problematic things were related with my first hacky implementation so nothing from the top of my head (beside subpaths which are already handled). Although the only added tests here are checking deletions while logic for updating collections also changed and that should be thoroughly tested. Or better yet, those changes should be moved to different PR as it's out of scope of original intentions. Also given you're optimizing flow you should add assertions about number of queries fired to actually update the document, otherwise it'll be easy to lose optimizations. For a hint that take a look at this test here: https://github.com/doctrine/mongodb-odm/blob/master/tests/Doctrine/ODM/MongoDB/Tests/Functional/AtomicSetTest.php
Edit: I crossed out saying about PR is too broad as I've just found out it's two merged into one and that was actually asked by us so we can't change this back and forth :)
|
At the current moment I implemented remarks, leave comments and ask some questions. Later on this week, I add required tests. |
…queries where it is required. Added missing tests for checking simultaneous embed-many collections updates under different storage strategies.
… to update/delete methods of CollectionPersister.
|
Thanks @watari! |
Implemented CollectionPersister::deleteAll method for simultaneous deletion of collections that is belong to one document.
Summary
Improved removal of scheduled for removal collections. Now all collections that is belong to one parent is deleted in one query.