RDART-941: Clear collections in mixed when reassigned

[nirinchev]

Works except when reassigning collections to the same managed value, which depends on realm/realm-core#7422.

[nielsenko]

Will core change their behaviour, and hence this is something we need to get back to, or is this by design on cores part?

[coveralls-official]

Pull Request Test Coverage Report for Build 8286821755

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.08%) to 86.581%

---

Totals
Change from base Build 8190485741:	0.08%
Covered Lines:	5749
Relevant Lines:	6640

---

💛 - Coveralls

[nielsenko]

I wonder if it would be better to track what keys are not updated, and then remove only those, instead of removing all followed by setting them again.

I believe those to operations look quite different in the oplog.

Something similar holds for list, ie. only clear the elements in the old-list from length of the new-list and forward (if any), and update the rest.

[elle-j]

I guess it also depends on what operations are expected when you e.g. set a field to a new collection. In this case, it may be preferable/expected to have the items cleared, then set new ones, rather than removing some items and updating some items.

[nielsenko]

It is definitely a choice, but it would avoid realm/realm-core#7422.

[elle-j]

I don't think we want to avoid that though (that issue) as it could make it easier for the SDKs, and the SDKs wouldn't have to call into Core for every item insert. It's definitely an interesting discussion to have, also for Core if we do delegate to them.

[nielsenko]

Eventually the behavior should move to core, as to avoid the repeated calls. I'm more interested in what behavior we want.

Even if we go with clear, then assign, I think this should be handled by core, not the individual SDKs.

Also, to be consistent we should allow setting all collections, not just those in a mixed.

[nirinchev]

Clearing the collection is the correct behavior here rather than replacing some of the elements because of the merge rules for CLEAR vs SET. Here's an example of when this could produce unwanted results:

// Point 1
// All clients have the following state
obj.mixed ==  RealmValue.from([1]);

// Point 2, Client 1 updates the mixed value
obj.mixed = RealmValue.from(true);  // Changes the type
obj.mixed =  RealmValue.from([5]);    // Reverts back to a list
obj.mixed.clear(); // Clears the list

// Point 3 > 2, Client 2 updates the mixed value
obj.mixed =  RealmValue.from(['foo']);

If we didn't clear at point 3, the CLEAR instruction at point 2 would conflict with the SET instruction at point 3 and win over it, so we would lose the changes by Client 2, even though they were made at a later point. With the clear we're doing now, the point 2 CLEAR will be discarded and the SET-s in point 3 will be preserved.

Regarding setting non-mixed collections - the challenge there is that it breaks certain expectations compared to regular objects - e.g.:

final myList = [1];
myPodo.list = myList;

myList.add(2);

// With a plain dart object, you'd get
expect(myPodo.list, hasLength(2));

So there is a strong expectation that what you set is what you get when working with properties. With collections, however, it wouldn't be the case, as we'd need to copy the collection contents over to Core, but wouldn't be able to manage it in-place. This is somewhat mitigated with collections in mixed due to the indirection that Mixed/RealmValue provides. When you pass in a list to RealmValue.from, the API implies that you're initializing a RealmValue with the contents of a collection and not just using the collection as is, so it's less clear whether users should expect this code to work:

final myList = [1];
myPodo.mixed = RealmValue.from(myList);

myList.add(2);

expect(myPodo.mixed.asList(), hasLength(2));

[elle-j]

Thanks for the clarification! Regarding the last example, for the SDKs having the Mixed wrapped (e.g. the RealmValue class), I don't think the expectation necessarily is that myList.add(2); also should be acting on myPodo.mixed due to the intermediate API that they'd have to go through RealmValue.from().

In this case, I'd say that a user's expectations may instead be based on what the API docs of RealmValue.from() says that it returns (e.g. the same reference as myList or something else (the latter in this case)).

I think the ambiguity of what users would expect becomes more ambiguous for the JS SDK where the syntax is not differentiated as we don't wrap the Mixed value.

[elle-j]

LGTM, nice test updates 🙂