Use cursor for counting documents in inversed collections

[alcaeus]

Revisiting this once again. This change was previously introduced in #649 and subsequently reverted in #692 because it can return incorrect results in certain use cases.

Instead of ignoring elements added to the collection like before, the code now counts the elements stored in the collection. If the collection is inversed and has not been initialized, it uses a count command on the cursor to get the number of documents in the database and adds that to the number of elements in the collection. This should return accurate results as well as improve time and memory consumption considerably. Note that all variants have edge cases where the result of count() will be incorrect - this can not be avoided as long as documents are manually added to the collection.

@spantaleev I would appreciate your feedback on this. I use inverse references a lot, but wherever I use them I never manually add documents to them. It would help a lot if you could test this against the code where you previously ran into the issue of wrong counts.

[jmikola]

I use inverse references a lot, but wherever I use them I never manually add documents to them. It would help a lot if you could test this against the code where you previously ran into the issue of wrong counts.

@jwage can confirm, but I don't recall ever using inverse references like this (i.e. manually adding to them) at OpenSky or Exercise.com. I believe we operated under the assumption that those collections were read-only (or at least treated them as such), since the actual relationship was owned on the "many" side.

Should we consider documenting that inverse collections are read-only, or perhaps enforce that with a special PersistentCollection sub-class? AFAIK, changes to such a collection would not be reflected in the changeset (so they are already read-only from ODM's perspective). If users actually want to modify the collection (so their data model doesn't have to care), I suppose they can manually unwrap it in a lifecycle event and work with an ArrayCollection.

[alcaeus]

@jmikola That's exactly how I understood them so far: they are meant for reading but any changes you do will leave you in an inconsistent state since there's no tracking of anything.

I agree with having an ImmutablePersistentCollection class which forbids any write operations (add(), clear(), remove(), removeKey(), set()) in order to preserve the intended behavior. Only thing is, while it might be that we're just strictly enforcing something that was an unwritten rule before, we're technically breaking BC here.

[jmikola]

Only thing is, while it might be that we're just strictly enforcing something that was an unwritten rule before, we're technically breaking BC here.

Indeed, but we do have a chance to address this before 1.0.0 is tagged :)

[jmikola]

In any event, I think this change is OK.

I'll create a separate ticket where we can discuss making inverse-side PersistentCollections immutable.

[malarzm]

👍 merged manually in f3ea0fc