Skip to content
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.

Sign up for GitHub

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

Jump to bottom

RDART-941: Clear collections in mixed when reassigned #1554

Merged
merged 2 commits into from
Mar 14, 2024
Merged

RDART-941: Clear collections in mixed when reassigned #1554

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0fb22ed
Clear collections in mixed when reassigned
nirinchev Mar 14, 2024
5491459
Add a few more assertions
nirinchev Mar 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions packages/realm_dart/lib/src/native/realm_core.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3021,6 +3021,10 @@ class _RealmCore {
collectionHandle = listHandle;

final list = realm.createList<RealmValue>(listHandle, null);

// Necessary since Core will not clear the collection if the value was already a collection
list.clear();

for (final item in value.value as List<RealmValue>) {
list.add(item);
}
Expand All @@ -3030,6 +3034,10 @@ class _RealmCore {
collectionHandle = mapHandle;

final map = realm.createMap<RealmValue>(mapHandle, null);

// Necessary since Core will not clear the collection if the value was already a collection
Copy link
Contributor

@nielsenko nielsenko Mar 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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));

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

map.clear();

for (final kvp in (value.value as Map<String, RealmValue>).entries) {
map[kvp.key] = kvp.value;
}
Expand Down
183 changes: 183 additions & 0 deletions packages/realm_dart/test/realm_value_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,8 @@ void main() {
expect(obj.oneAny.type, RealmValueType.list);
expectMatches(obj.oneAny, [true, 5.3]);

writeIfNecessary(realm, () => obj.oneAny = RealmValue.from(['foo']));

writeIfNecessary(realm, () => obj.oneAny = RealmValue.from(999));
expectMatches(obj.oneAny, 999);

Expand Down Expand Up @@ -566,13 +568,194 @@ void main() {
expect(obj.oneAny.type, RealmValueType.map);
expectMatches(obj.oneAny, {'bool': true, 'double': 5.3});

writeIfNecessary(realm, () => obj.oneAny = RealmValue.from({'foo': 'bar'}));
expectMatches(obj.oneAny, {'foo': 'bar'});

writeIfNecessary(realm, () => obj.oneAny = RealmValue.from(999));
expectMatches(obj.oneAny, 999);

writeIfNecessary(realm, () => obj.oneAny = RealmValue.from([1.23456789]));
expectMatches(obj.oneAny, [1.23456789]);
});

test('Map inside list when $managedString can be reassigned', () {
final realm = getMixedRealm();
final obj = AnythingGoes(
oneAny: RealmValue.from([
true,
{'foo': 'bar'},
5.3
]));
if (isManaged) {
realm.write(() => realm.add(obj));
}

final list = obj.oneAny.asList();

expect(list[1].type, RealmValueType.map);

writeIfNecessary(realm, () => list[1] = RealmValue.from({'new': 5}));
nirinchev marked this conversation as resolved.
Show resolved Hide resolved
expectMatches(obj.oneAny, [
true,
{'new': 5},
5.3
]);

writeIfNecessary(realm, () {
list.add(list[1]);
});

expectMatches(obj.oneAny, [
true,
{'new': 5},
5.3,
{'new': 5}
]);

// TODO: Self-assignment - this doesn't work due to https://github.com/realm/realm-core/issues/7422
// writeIfNecessary(realm, () {
// list[1] = list[1];
// });
// expectMatches(obj.oneAny, [
// true,
// {'new': 5},
// 5.3,
// {'new': 5}
// ]);
});

test('Map inside map when $managedString can be reassigned', () {
final realm = getMixedRealm();
final obj = AnythingGoes(
oneAny: RealmValue.from({
'a': 5,
'b': {'foo': 'bar'}
}));

if (isManaged) {
realm.write(() => realm.add(obj));
}

final map = obj.oneAny.asMap();

expect(map['b']!.type, RealmValueType.map);

writeIfNecessary(realm, () => map['b'] = RealmValue.from({'new': 5}));
expectMatches(obj.oneAny, {
'a': 5,
'b': {'new': 5}
});

writeIfNecessary(realm, () {
map['c'] = map['b']!;
});

expectMatches(obj.oneAny, {
'a': 5,
'b': {'new': 5},
'c': {'new': 5},
});

// TODO: Self-assignment - this doesn't work due to https://github.com/realm/realm-core/issues/7422
// writeIfNecessary(realm, () {
// map['a'] = map['a'];
// });
// expectMatches(obj.oneAny, {
// 'a': 5,
// 'b': {'new': 5},
// 'c': {'new': 5},
// });
});

test('List inside list when $managedString can be reassigned', () {
final realm = getMixedRealm();
final obj = AnythingGoes(
oneAny: RealmValue.from([
true,
['foo'],
5.3
]));
if (isManaged) {
realm.write(() => realm.add(obj));
}

final list = obj.oneAny.asList();

expect(list[1].type, RealmValueType.list);

writeIfNecessary(realm, () => list[1] = RealmValue.from([5, true]));
expectMatches(obj.oneAny, [
true,
[5, true],
5.3
]);

writeIfNecessary(realm, () {
list.add(list[1]);
});

expectMatches(obj.oneAny, [
true,
[5, true],
5.3,
[5, true]
]);

// TODO: Self-assignment - this doesn't work due to https://github.com/realm/realm-core/issues/7422
// writeIfNecessary(realm, () {
// list[1] = list[1];
// });
// expectMatches(obj.oneAny, [
// true,
// [5, true],
// 5.3,
// [5, true]
// ]);
});

test('List inside map when $managedString can be reassigned', () {
final realm = getMixedRealm();
final obj = AnythingGoes(
oneAny: RealmValue.from({
'a': 5,
'b': ['foo']
}));

if (isManaged) {
realm.write(() => realm.add(obj));
}

final map = obj.oneAny.asMap();

expect(map['b']!.type, RealmValueType.list);

writeIfNecessary(realm, () => map['b'] = RealmValue.from([999, true]));
expectMatches(obj.oneAny, {
'a': 5,
'b': [999, true]
});

writeIfNecessary(realm, () {
map['c'] = map['b']!;
});

expectMatches(obj.oneAny, {
'a': 5,
'b': [999, true],
'c': [999, true]
});

// TODO: Self-assignment - this doesn't work due to https://github.com/realm/realm-core/issues/7422
// writeIfNecessary(realm, () {
// map['a'] = map['a'];
// });
// expectMatches(obj.oneAny, {
// 'a': 5,
// 'b': [999, true],
// 'c': [999, true]
// });
});

test('RealmValue when $managedString can store complex struct', () {
final realm = getMixedRealm();
final rv = persistIfNecessary(
Expand Down
Loading