RDART-1062: Allow missing values (implicit null)

[nielsenko]

Assuming:

@RealmModel()
class _Player {
  @PrimaryKey()
  late String name;
  _Game? game;
  final scoresByRound = <int?>[]; // null means player didn't finish
}

then the following now works:

Realm(Configuration.inMemory([Player.schema, Game.schema]));
expect(() => fromEJson<Player>({}), throwsA(isA<InvalidEJson>())); // illegal since mandatory name is missing
final p = fromEJson<Player>({'name': 'Ericsen'}); // legal, since game and scoresByRound are optional
expect(p.toEJson(), {'name': 'Ericsen', 'game': null, 'scoresByRound': <int?>[]});

Previously the call to fromEJson<Player>(ejson) would fail unless ejson was a map with shape { 'name': ..., 'game': ..., scoresByRound: ...} despite game being nullable, and scoresByRound being optional.

This PR also rectifies and oversight regarding RealmValue and Decimal128. These types are now supported without explicit calls to register.

  test('RealmValue', () {
    final r = RealmValue.int(42);
    expect(r.toEJson(), {'\$numberInt': '42'});
    expect(fromEJson<RealmValue>({'\$numberInt': '42'}), r);
  });

  test('Decimal128', () {
    final d = Decimal128.fromInt(42);
    expect(toEJson(d), {'\$numberDecimal': '+42E+0'});
    expect(fromEJson<Decimal128>({'\$numberDecimal': '42'}), d);
  });

Also, sets are now supported:

  test('Set on RealmObject', () {
    final realm = Realm(Configuration.inMemory([AllCollections.schema]));
    final o = realm.write(() => realm.add(AllCollections(intSet: {1, 2, 3})));
    expect(o.intSet, {1, 2, 3});
    final deserialized = fromEJsonString<AllCollections>(toEJsonString(o));
    // deserialized is unmanaged, so will never compare equal, but we can test properties
    expect(deserialized.intSet, o.intSet);
  });

and RealValue can be deserialized from DBRef:

    test('RealmObject', () {
      final p = Player('Christian Eriksen');
      final rv = RealmValue.from(p);
      expect(rv.toEJson(), {'\$id': 'Christian Eriksen', '\$ref': 'Player'});
      expect(fromEJson<DBRef<String>>(rv.toEJson()), isA<DBRef<String>>().having((r) => r.id, '\$id', 'Christian Eriksen'));
      expect((fromEJson<RealmValue>(rv.toEJson()).value as Player).name, p.name);
    });

Fixes: #1735
Fixes: #1737
Fixes: #1761
Fixes: #1757

[coveralls-official]

Pull Request Test Coverage Report for Build 9696676877

Details

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

---

Totals
Change from base Build 9695895354:	0.007%
Covered Lines:	5988
Relevant Lines:	6887

---

💛 - Coveralls

[coveralls-official]

Pull Request Test Coverage Report for Build 9697576923

Details

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

---

Totals
Change from base Build 9695895354:	0.007%
Covered Lines:	5988
Relevant Lines:	6887

---

💛 - Coveralls

[coveralls-official]

Pull Request Test Coverage Report for Build 9699703151

Details

• 19 of 19 (100.0%) changed or added relevant lines in 3 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.02%) to 86.954%

---

Totals
Change from base Build 9695895354:	0.02%
Covered Lines:	5992
Relevant Lines:	6891

---

💛 - Coveralls

[coveralls-official]

Pull Request Test Coverage Report for Build 9710791691

Details

• 29 of 30 (96.67%) changed or added relevant lines in 5 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.02%) to 86.957%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/realm_dart/lib/src/handles/native/init.dart	8	9	88.89%

Totals
Change from base Build 9695895354:	0.02%
Covered Lines:	6000
Relevant Lines:	6900

---

💛 - Coveralls

[coveralls-official]

Pull Request Test Coverage Report for Build 9761656848

Details

• 64 of 67 (95.52%) changed or added relevant lines in 8 files are covered.
• 2 unchanged lines in 1 file lost coverage.
• Overall coverage increased (+0.003%) to 86.942%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/realm_dart/lib/src/handles/native/init.dart	13	14	92.86%
packages/ejson/lib/src/types.dart	3	5	60.0%

Files with Coverage Reduction	New Missed Lines	%
packages/realm_dart/lib/src/configuration.dart	2	73.11%

Totals
Change from base Build 9695895354:	0.003%
Covered Lines:	6019
Relevant Lines:	6923

---

💛 - Coveralls

[coveralls-official]

Pull Request Test Coverage Report for Build 10217657561

Details

• 83 of 86 (96.51%) changed or added relevant lines in 9 files are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage increased (+0.06%) to 87.177%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/realm_dart/lib/src/handles/native/init.dart	21	22	95.45%
packages/ejson/lib/src/types.dart	3	5	60.0%

Files with Coverage Reduction	New Missed Lines	%
packages/ejson/lib/src/decoding.dart	1	99.19%

Totals
Change from base Build 10212460480:	0.06%
Covered Lines:	6078
Relevant Lines:	6972

---

💛 - Coveralls

[nirinchev]

Need to go more thoroughly through the tests, but here's some initial comments

[nirinchev]

Missing docs for allowCustom.

[nirinchev]

More broadly though, I'm not sure I fully understand the need for allowCustom and the whole saving it, setting it, then restoring it logic seems awkward. Can you give me the tl;dr for what we're trying to achieve here or maybe we can just take it offline; though either way we probably need to add some code comments to make sure that is still understandable in the future.

[nielsenko]

For _allowCustom think thread local storage, but since dart don't have shared heaps, we can just use a static variable. If the allowCustom parameter is set, it will use that for _allowCustom on any recursive calls to fromEJson such as would happen for list etc.

The problem I'm trying to solve is that you could register a type T with all optional values, meaning that {} would deserialize to T (and any other map for that matter, as redundant properties are already allowed). We don't try to search for best matching deserializer, so in the dynamic case, where the type to deserialize to is unknown (such as is the case for RealmValue), the custom deserializers can get in the way.

Hence this flag. I deliberately didn't document it, as it is very internal thing only really meant for RealmValue.

However, I will add, and elaborate the in-code comments.

[nielsenko]

The:

      {'a': 1, 'b': 2}: {
        // Even though Game could be deserialized from this (everything in Game
        // is optional, hence {} and by extension any map can deserialize to Game),
        // we always decode a map as a map inside a RealmValue.
        'a': {'\$numberInt': '1'},
        'b': {'\$numberInt': '2'},
      },

test case illustrate the issue.

[nielsenko]

And the following case in _decodeAny:

    Map<dynamic, dynamic> _ => _tryDecodeCustomIfAllowed(ejson) ?? _decodeDocument<String, dynamic>(ejson), // other maps goes last!!

Obviously any map can decode as a map, so we need to check custom decoders first, but in some cases that is not what you want either, hence the flag.

One could perhaps argue that _decodeAny should never even try the custom decoders, and always return a map for non BSON types. Would yor prefer that?

[nirinchev]

I see, yeah, I guess I can follow the logic now. It does feel to me a bit more awkward than weaving the argument through subsequent calls, but I guess you chose the current implementation as it's more localized and non-breaking change. I'm fine with keeping it as is, just need to add more descriptive comments to make sure we can still follow along in a year from now.

[nielsenko]

I have been thinking about it since yesterday. How do you feel about never trying the custom decoders in _decodeAny. Personally I think I prefer that for a couple of reasons:

1. Custom decoders will only be invoked when the type to deserialize to is known. Calling fromEJson<dynamic>(..) (or implicit fromEJson(..) will never try the custom decoders registered by the user. This removes any ambiguity.
2. It is slightly more performant, as we get rid of _tryDecodeCustomIfAllowed which tries each custom decoder in turn, until the first match.
3. I can get rid of the allowCustom/_allowCustom shenanigans.

[nirinchev]

Sure, that also sounds viable