Support RealmValue (aka mixed)

[nielsenko]

Support RealmValue that allows users to store different types in the same field

@RealmModel()
class _AnythingGoes {
  @Indexed()
  late RealmValue any;
  late List<RealmValue> manyAny;
}

void main() {
  final realm = Realm(Configuration.local([AnythingGoes.schema]));

  final something = realm.write(() => realm.add(AnythingGoes(any: RealmValue.string('text'))));

  realm.write(() {
    something.manyAny.addAll([null, true, 'text', 42, 3.14].map(RealmValue.from));
  });
}

Resolves: #684

[coveralls]

Pull Request Test Coverage Report for Build 3713731875

• 58 of 58 (100.0%) changed or added relevant lines in 8 files are covered.
• 3 unchanged lines in 2 files lost coverage.
• Overall coverage increased (+0.4%) to 89.407%

---

Files with Coverage Reduction	New Missed Lines	%
lib/src/realm_class.dart	1	93.73%
lib/src/list.dart	2	85.93%

Totals
Change from base Build 3693854074:	0.4%
Covered Lines:	2819
Relevant Lines:	3153

---

💛 - Coveralls

[blagoev]

Looks good as initial version,
Added some comments.
Also it would be good to check what additional tests .NET has and implement here. I think they have more elaborate tests like using embedded objects etc.

[nirinchev]

We probably need to expose ObjectType similarly to what we're doing in C#.

[nirinchev]

Should we be more descriptive with these error messages? In my experience people don't understand short errors like these and reach out to support to ask them what they mean. We could rephrase that to something like: 'wrong' is declared as 'RealmValue?' which is not allowed. 'RealmValue' can already represent null with 'RealmValue.nullValue()', so you don't need to declare the property itself as nullable.

[nielsenko]

In general I agree that we should have long descriptive error messages, but we need to agree on the team. Previously I was asked to simplify them.

[blagoev]

I would vote for short and clear messages. We could reword this to be more clear, but long error messages stand in the way after a while. So imo, this could be

`RealmValue?` is declared nullable. Use `RealmValue` or `RealmValue myfield = RealmValue.nullValue()` to assign a default null value.

[nielsenko]

I don't think it gets in the way. The short version is at the top, the rest you can read if you are confused.

But I don't think this is specific to this PR. If we want more verbose error messages, I think we should change a lot of them.

[nirinchev]

I don't think this handles the case where you open a Realm with an incomplete schema that contains RealmValue properties linking to tables not in the user schema.

[nielsenko]

I will add a test for this case.

What should be the expected outcome? Do we error out, or use dynamic realm objects?

[nirinchev]

We should return dynamic objects.

[nielsenko]

Hmm .. added a test, but it crashes in

realmCore.getProperty(object, propertyMeta.key);

At least the c-api don't support this

[desistefanova]

Suggested change

	late RealmValue oneAny;
	late List<RealmValue> manyAny;
	late RealmValue oneValue;
	late List<RealmValue> manyValues;

Maybe will be better if you rename all the places where you have members any to value. 'Any' is confusing to me.

[desistefanova]

We are testing only with inMemory Realm. Is that enough?

[nielsenko]

Yes, I don't see a reason test with other configurations.

[nielsenko]

Okay, I had forgotten some things in dart, so I believe there's more work needed to support RealmValue, but that can probably be split into a different PR...

I will just comment that even trying to read a mixed property from core (via the c-api) containing a realm object with unknown schema will crash, and since the property is mixed we really have no way to detect this, so the crash is inevitable 😢

[desistefanova]

Will be good to make more tests for example:

• Query by RealmValue;
• Frozen objects with RealmValue;
• Embedded objects with RealmValue;
• Flexible sync with RealmValue;
• Subscriptions by RealmValue;

[nielsenko]

This would test realm-core more than realm-dart.

[nirinchev]

Some of these have the potential to surface symmetric bugs and may be valuable. Let's say you make a mistake and you store double values as negative the original value (e.g. 4.56 -> RealmValue(-4.56)). This kind of bug will always pass a test like:

final value = 4.56;
realm.write(() {
  myObj.any = value;
});

expect(myObj.any, value);

Ideally, that should be tested by serializing the object to json and comparing the stringified representation as seen by Core, but those other tests may also be valuable. While I agree it's testing Core, historically, SDKs have been the primary source of bug reports for the Core team and it doesn't hurt to have a few extra tests.

[nielsenko]

As the 6th SDK I see less value in tests that essentially test realm-core. Adding them now will delay this PR, and require maintenance down the line.

[nielsenko]

.. an argument can be made that we are only the 2nd SDK to use the c-api though

[nielsenko]

We probably need to expose ObjectType similarly to what we're doing in C#.

I assume this is to catch the error case that you cannot store embedded objects in a RealmValue (or asymmetric objects for that matter, when we eventually support those).

I have reworked the code to catch this misuse in the type system.

[blagoev]

@nirinchev I couldn't get your meaning for this one

We probably need to expose ObjectType similarly to what we're doing in C#.

Could you clarify this. Especially important if it is a reason to block a PR.

[nirinchev]

Okay, I had forgotten some things in dart, so I believe there's more work needed to support RealmValue, but that can probably be split into a different PR. Here's an example of an issue that we can't handle today. Let's say you have a synchronized Realm with the following schema (this is v1 of your app).

class MyDynamic {
  RealmValue value;
}

class Person {
  String name;
}

In v2, you add a new type - Dog. Now, a new client comes in and adds a Dog instance as a value for MyDynamic.value. A client in v1 of your app will have no way to do something meaningful with that object as it won't have the schema for Dog and there's no way to inspect what properties exist. In C# and other SDKs we expose the object schema on RealmObject instances, so you could do something like:

final obj = myDynamic.value.as<RealmObjectBase>();
switch (obj.schema.name) {
  case "Person": // do something
  default:
    final nameProp = obj.schema.properties.firstOrNull(p => p.name == "name");
    if (nameProp != null && nameProp.propertyType == RealmPropertyType.string) {
      // do something with the name
    }
}

My thinking with exposing ObjectType on RealmValue was to support use cases like:

public String getDisplayName(RealmValue value) {
  switch (value.objectType) {
    case null:
      // value is not an object or is null - do nothing
	  return "unknown";
    case "Person":
      final person = value.as<Person>();
      return '${person.firstName} ${person.lastName}';
    case "Company":
      final company = value.as<Company>();
      return company.legalEntity.name;
    default:
      final obj = value.as<RealmObjectBase>();
      final nameProp = obj.schema.properties.firstOrNull(p => p.name == "name");
      if (nameProp != null || nameProp.propertyType == 'string') {
        return obj.dynamic.get<string>('name');
      }

      return "unknown";
  }
}

But again, this depends on the schema being available on the realm object.

[nirinchev]

As discussed, I think it's fine to ship the feature as is while the C API limitation is addressed. I'd still recommend adding a json verification for the roundtrip test to protect us from symmetric bugs.