Check if embedded document is not part of atomic set query #1142
Conversation
… "Cannot update 'chapters.0.status' and 'chapters' at the same time'".
| @@ -485,4 +485,13 @@ private function isScheduledForInsert($document) | |||
| return $this->uow->isScheduledForInsert($document) | |||
| || $this->uow->getDocumentPersister(get_class($document))->isQueuedForInsert($document); | |||
| } | |||
|
|
|||
| private function isPartOfAtomicUpdate($embeddeDoc) | |||
There was a problem hiding this comment.
Since we're also calculating this in the DocumentPersister::getAtomicCollectionUpdateQuery(), perhaps it makes sense to move this to a public UnitOfWork method (annotated with @internal)? Something like isWithinAtomicCollection() with an $embeddedDocument argument seems descriptive enough.
There was a problem hiding this comment.
I was wondering about that but in DocumentPersister::getAtomicCollectionUpdateQuery() we want to get mapping back while here true/false is enough and I don't think bool|array is good idea for is-ser. Maybe method like getTopmostCollectionMapping returning array|null would be better? Sometimes I hate naming things
There was a problem hiding this comment.
Ah, good point about requiring the mapping. Let's skip this for now, and optionally leave a // TODO: ... comment in place to refactor it later.
|
So for the first time, all of my unit and integration tests have passed.That's very exciting as I have been trying for a long time to fix our data corruption problems. I will try this on my canary server this week and see if things actually work correctly but it appears that I'm in really good shape. |
|
@blockjon awesome 👍 |
See logic in UnitOfWork::persistNew().
Restructures the nullable handling and adds an early continue statement. Additionally, limit reference-one handling to owning sides, which is consistent with prepareUpdateData() and prepareUpsertData().
Remove a level of indentation for the embed-many branch by moving the $new check to the main conditional.
Embedded documents were never scheduled for upsert, so they would only be inserted at this point. The embed-many condition for prepareUpsertData() should be identical to what we already do in prepareInsertData(), so we can add an early continue statement and eliminate the later conditionals.
This addresses an earlier hypothesis that embedded documents are never scheduled for upsert (due to logic in UnitOfWork::persistNew()).
Additional fixes PersistenceBuilder handling of embedded docs
|
LGTM 👍 |
|
Merged manually in 10cd916 |
Closes #1141 (@blockjon's test is included in this PR)
For the record it was working for
setstrategy but there were 2 queries issued - first to updatechapters.0.statusfromDocumentPersisterand later to update wholechapterscollection fromCollectionPersister. I think it's worth fixing, but I've ran into some problems and decided it's outside of scope of this PR.