Adding atomicSet and atomicSetArray collection strategies

[malarzm]

This is an early implementation of how atomicSet and atomicSetArray could work in my opinion. There are some things that needs to be done

• maybe add atomic=true flag to mapping instead of new strategy (or change existing $set behaviour)
• inserts
• updates
• upserts
• removing collection
• collection within collection with different strategies throw exception if used in embedded documents
• more tests
• docs

cc @jwage @jmikola @alcaeus @lavoiesl @blockjon

If this PR will reach its final form it will resolve #1009, #437, #1095 and #1094

[blockjon]

@malarzm I was thinking that the ODM should always perform all changes to a record in one atomic query. Do you agree, or do you do you feel as though this behavior should be parameterized?

[malarzm]

ODM can't do all changes within one query, for instance (citing @jmikola from #1009 (comment))

Collections are currently handled separately out of necessity for the base case where elements are being added and removed from the same array, since MongoDB doesn't allow that in the same write operation.

but of course this in not the case with $set. For now I have made this parametrized as I wanted to have current tests pass and only new ones I'm writing fail but changing this will not be hard if we come to conclusion that we want to change existing behaviour.

[malarzm]

I'm afraid we won't be able to change existing set behaviour since I'm not able to make atomic update from within DocumentPersister to collection nested in another collection with pushAll strategy (especially not when embedded object containing that collection is yet to be created). Right now I'm thinking about limiting the feature to be available only in top level document (or deeply embedded but only within collections with atomic* strategy).

I've been also thinking that atomic flag in set and setArray could adapt and make atomic updates when possible and fall back to old behaviour but I think this can be at least confusing. That said I think that introducing new strategies is ok and even wanted given said limitations.

[malarzm]

@blockjon could you run your test against this PR?

[blockjon]

Having just quickly read through the code, it appears as if this might do the trick. I will try this today and report back my findings.

[blockjon]

@malarzm Looks like your changes fix my problem as seen here: malarzm#3

[jmikola]

I'd rephrase as "The atomicSet and atomicSetArray strategies may only be used for collections mapped directly in a top-level document." And perhaps we want to duplicate this note for the atomicSetArray section below.

[blockjon]

Friendly question for you all - This branch fixes a critical problem I'm faced with and I would love to use it sooner rather than later.

Since I'm new to this project, I'd like to get a sense of when you feel as though it would be in a state that you feel I should attempt to integrate into my project. Should I want until this gets merged to master, or until you tag a beta 13, for example?

[jmikola]

Would it be safer swap out the else for a separate isCollectionScheduledForDeletion() check here? I realize UnitOfWork::scheduleCollectionOwner(), which populates the array returned by getScheduledCollections(), is currently only called from scheduleCollectionDeletion() and scheduleCollectionUpdate(); however, if that changes in the future, we're looking at a very destructive else condition :)

[malarzm]

The elseif was there before but I'll bring it back ;)

[jmikola]

@jwage: If we make this an elseif again, how do you feel about a final else condition throwing a LogicException?

[malarzm]

@blockjon idea is to have this feature merged and tag BETA-13 then

[jmikola]

LGTM pending test results. @jwage: Do you have a chance to take a look at this within the next day?

Ideally, we'd like to get it included in a new 1.0.0-BETA13 tag this week (to help with doctrine/DoctrineMongoDBBundle#298).

[jwage]

👍

[blockjon]

I AM EXCITED!!!!!!

[smolinari]

Nice work gentlemen. 👍

Scott

[BernardoSilva]

👍 Nice work.

[malarzm]

@blockjon BETA13 is now released :)

[blockjon]

@malarzm This is fantastic news thank you!!!!

[ghost]

@malarzm - I think you need to implement atomicSet as default where possible. I just found a bug in production that happened in only 1/100.000 cases and it was driving me nuts for weeks before I found the problem. Using pushAll and multiple queries as default is really something unexpected for me as a developer.

[blockjon]

@daniphp +1 on your comments there... I had the same feedback here #782 (comment)