-
-
Notifications
You must be signed in to change notification settings - Fork 501
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
Commit flow rework and fixed collection logic #1155
Conversation
Something is wrong with this PR but I'm having trouble coming up with a functional test to exhibit the behavior. In my application flow, there is a document with two strategy="set" EmbedMany elements. If I use the mongodb-odm master line, adding to these two EmbedMany elements works normally. When I use this branch, the elements are not appended to. Although I can't figure out how to repro the problem in a functional test, I can show you the queries my app is firing when on the master line versus this branch: mongodb-odm master line queries:
the same code on this branch:
As you can see, the app seems to only update the "surface" of the record when I run my unit tests with this new doctrine-odm branch. In other words, I am missing the queries which should be updating the embedmany elements. |
I'm still trying to figure out how the UnitOfWork works.. but it seems like the chargeAccounts and chargeAccountsHistory elements are not considered to be scheduled collections when using this branch. That's just a guess. |
I'll try to hack a failing test case in few hours, at a glance it seems strange as |
It does not look like my app has any postUpdate events in the failure situation. |
@malarzm I'm out of ideas.. I tried to debug this for 3-4 hours. |
@blockjon could you maybe bisect this PR? Maybe this will give a hint |
Yes I can do this in about 10 hours since I'm about to go to sleep. :) |
@blockjon sure thing is, have a good one ;) |
@blockjon blind guess, but could you comment out |
$oid = spl_object_hash($coll); | ||
$topmostOwner = $this->getOwningDocument($coll->getOwner()); | ||
unset($this->collectionDeletions[$oid]); | ||
unset($this->hasScheduledCollections[spl_object_hash($topmostOwner)][$oid]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@malarzm When I comment this line out, my problem goes away!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh crap I think I know what I did wrong here... I'll push a fix in few minutes, hopefully it'll be that :)
@blockjon can you check now? |
@malarzm works! |
I will now re-run all of my automatic and manual smoke tests... |
@blockjon is no message a good message? :) |
@malarzm I just heard back from QA.. they told me all tests pass. I am therefore interested in rolling this out to my production system. Is this in a fairly stable state now in your opinion? |
@blockjon I do hope it is, with new way of handling atomic collections it should be way more stable than original implementation (and once again thank you for all failing test cases for that stuff) :) For a head up I am going to merge #782 tomorrow (or tonight) and then rebase this PR (but I don't expect tests to start failing because of that) |
@@ -355,42 +363,29 @@ public function update($document, array $options = array()) | |||
$id = $this->uow->getDocumentIdentifier($document); | |||
$update = $this->pb->prepareUpdateData($document); | |||
|
|||
if ( ! empty($update) || $this->uow->hasScheduledCollections($document)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why I didn't ask this earlier, but this seems like we might bump the version of a document even if there is no other update to apply. I realize you want to ensure handleCollections()
is called at the bottom of this method, but those collection modifications will take place in separate write operations AFAIK.
Is the idea here that you want to verify the main document's write so we can throw a LockException before handling collections?
I believe there's still a concern about the collection updates not incorporating versioning/locking in their own write ops; however, that's not a new problem at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is exactly the previous behaviour if only collection was to be updated (IIRC this was the very thing why hasScheduledCollections
was introduced) - bump version and later update collections. From PR description:
This PR doesn't fix #1046 [investigate locking and collection updates] but makes it bloody easy to implement, all that is needed is to pass current document's version to CollectionPersister and take it into account in update query.
How it will be done is yet to be determined, however now we will have both document and its collections update in one place which is way better than before :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -894,12 +883,10 @@ private function computeAssociationChanges($parentDocument, array $assoc, $value | |||
|
|||
if ($value instanceof PersistentCollection && $value->isDirty() && $assoc['isOwningSide']) { | |||
if ($topOrExistingDocument || strncmp($assoc['strategy'], 'set', 3) === 0) { | |||
if ( ! $this->isCollectionScheduledForUpdate($value)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Offhand, should the conditional above be using CollectionHelper::usesSet()
instead of strncmp()
, or is it purposefully ignoring atomic sets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For atomic sets $topOrExistingDocument
will be true
but changing this for consistency is good idea 👍
@jmikola I'll run tomorrow over all comments once again and apply yours, thanks for all the input! |
As per my comment in #1128 (comment) this PR takes care of following things:
PersistenceBuilder
(thing I've mentioned in Fix updating of embedded documents if their atomic collection is not dirty #1154)set
orsetArray
strategies are used (fixes Added failing test case for nested collection update #1104)increase number of tests for atomic strategies by doubling existing tests for collections(which will be done after review to not blur the diff) EDIT: looking at current tests they seem to be enough combined with how atomic strategies are handled nowThis PR doesn't fix #1046 but makes it bloody easy to implement, all that is needed is to pass current document's version to CollectionPersister and take it into account in update query. I can include this here if we decide it's good idea or leave it for another PR.