[RDART-968] Add support for keypath filtering on object #1601

papafe · 2024-03-22T10:37:25Z

This PR adds the changeFor([List<String>? keyPaths] method to the generated realm classes to support keypath filtering on object notifications

TODO

* main: (31 commits) Add vNext Changelog header (#1594) [Release 2.0.0] (#1593) Cleanup the changelog for 2.0.0 (#1592) Wrong test for local development (#1591) Add vNext Changelog header (#1589) [Release 2.0.0-beta.2] (#1588) Update melos to 5.2.0 (#1587) RDART-977: Hierarchical logger (#1574) Fix release workflow (#1582) RDART-978: Add support RealmValue.fromJson (#1581) Fix oversight in install command Add vNext Changelog header (#1579) [Release 2.0.0-beta.1] (#1577) RDART-976: Fix install command (#1576) Update build instructions and switching Flutter/Dart versions (#1571) gitignore release folder Remove incorrectly committed files Cleanup changelog, bring back melos publishing (#1573) Add vNext Changelog header (#1572) [Release 2.0.0-alpha.5] (#1570) ...

papafe · 2024-03-26T08:05:34Z

@nirinchev @nielsenko It's still a little rough, but I thought it would be a good idea if you could give me some initial feedback, so I know I'm going in the right direction

papafe · 2024-03-26T08:11:12Z

packages/realm_dart/test/realm_object_test.dart

+    });
+
+    // TODO Need to decide what we expect here....
+    test('mappedProperty', () async {


This test at the moment passes, but we should decide what kind of behaviour we want to have with mapped properties.
At the moment we need to pass the "managed" name when we subscribe, but we get the mapped name in changes

* main: RDART-989: Update realm-core to v14.4.1 (#1603)

papafe · 2024-04-05T09:26:50Z

packages/realm_dart/lib/src/realm_object.dart

+    return getChangesFor<T>(object, null);
+  }
+
+  //TODO we need to decide if we want to have keyPaths be nullable here of we force developers to use getChanges in that case


What do you think?

As a user, you will not be using these, but the property changes and the method changesFor, so I don't think it matter that much.

I think I put this comment in the wrong place, but I was actually referring to changesFor, that at the moment can accept a null argument. Should we keep it like this? Ot should we make the keypath argument non-nullable and force developers to use changes for the default notifications?

I think we should allow null as well, so changes and changesFor(null) is essentially the same.

papafe · 2024-04-05T09:29:45Z

This is more or less reviewable. I'm still trying to understand why the backlink tests are failing (it seems that no backlink notifications are raised).
Apart from that I left some notes in the PR for things I'm not sure about, and I'll keep the list of TODOs updated in the PR description.
I will do housekeeping (fixing the changelog, generator tests, documentation, ...) once I get a first pass at the PR, in case I'm doing something completely wrong and I need to change my approach 😉

nielsenko

So far so good 👍 . I'll take another look once you set it ready-for-review.

For other reviewers. The double creation of the keypathNative array is a small blemish we already discussed and will address in a later PR

nielsenko · 2024-04-08T08:39:10Z

packages/realm_dart/lib/src/native/realm_core.dart

+      [List<String>? keyPaths, int? classKey]) {
+    return using((Arena arena) {
+      Pointer<realm_key_path_array> kpNative = nullptr;
+
+      if (keyPaths != null && classKey != null) {
+        final length = keyPaths.length;
+        final keypathsNative = arena<Pointer<Char>>(length);
+
+        for (int i = 0; i < length; i++) {
+          keypathsNative[i] = keyPaths[i].toCharPtr(arena);
+        }
+
+        kpNative = _realmLib.invokeGetPointer(() => _realmLib.realm_create_key_path_array(object.realm.handle._pointer, classKey, length, keypathsNative));
+      }


LGTM

Perhaps get the classKey from the realmObject instead of passing it as an argument?

packages/realm_dart/lib/src/native/realm_core.dart

nielsenko · 2024-04-08T08:47:17Z

packages/realm_dart/lib/src/realm_object.dart

+    return getChangesFor<T>(object, null);
+  }
+
+  //TODO we need to decide if we want to have keyPaths be nullable here of we force developers to use getChanges in that case


As a user, you will not be using these, but the property changes and the method changesFor, so I don't think it matter that much.

packages/realm_dart/lib/src/realm_object.dart

* main: RDART-996: Don't serialize backlinks (#1617) Update README.md (#1608) RDART-983: Refactor how we open dynamic library to give better error message (#1614) RDART-991: Rename Key enum (#1613) RDART-992: Handle Identifer expression as well (#1612) Add workflow_dispatch for development Common setup script (#1610) RDART-992: Const initializer evaluation is too simple (#1607) # Conflicts: # CHANGELOG.md

papafe · 2024-04-08T12:46:22Z

@nielsenko fixed the comments on the PR, added documentation and changelog entry, fixed the generated code tests
The only thing missing is fixing the backlinks and mapped property tests, for which I'm waiting for some inputs from core

packages/realm_dart/lib/src/realm_object.dart

nielsenko

LGTM

* main: New documentation URL (#1621) RDART-980: Add tests for collections in `Mixed` with synced realms (#1585) RDART-987: Schema migration support follow-up (#1622) Update changelog for collections in mixed and sync RDART-987: Schema migration support (#1586) # Conflicts: # packages/realm_dart/test/realm_value_test.realm.dart

coveralls-official · 2024-04-16T10:30:17Z

Pull Request Test Coverage Report for Build 8703959258

Details

33 of 35 (94.29%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.05%) to 86.398%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/realm_dart/lib/src/realm_object.dart	13	15	86.67%

Totals
Change from base Build 8659161761:	0.05%
Covered Lines:	5882
Relevant Lines:	6808

💛 - Coveralls

papafe added 4 commits March 21, 2024 16:34

Stub for keypath filtering

e3eee1d

Added correction to generation

5e62ecc

Corrected missing file

b73813a

cla-bot bot added the cla: yes label Mar 22, 2024

github-actions bot assigned papafe Mar 22, 2024

papafe added 3 commits March 22, 2024 13:33

Corrected core version

84d30d7

Moved tests

cba4d51

Added basic tests for object keypath filtering

96a309b

papafe commented Mar 26, 2024

View reviewed changes

papafe added 5 commits March 27, 2024 10:19

Added tests

b7b2466

Merge branch 'main' into fp/keypath-filtering

928ec55

* main: RDART-989: Update realm-core to v14.4.1 (#1603)

Test fix

2525c88

Fixed keypath verification

50a5641

Added more tests for backlinks

ad3fe6e

papafe requested review from nielsenko and nirinchev April 5, 2024 09:25

papafe commented Apr 5, 2024

View reviewed changes

nielsenko reviewed Apr 8, 2024

View reviewed changes

papafe added 8 commits April 8, 2024 11:34

Fixes according to PR

a433602

Modified expected files

670693a

Another fix to generator

79cb3d1

Added documentation and changelog

6d1ae46

Small fix

adcea6d

Fixed test

94b80af

Added fix

d22507a

papafe marked this pull request as ready for review April 8, 2024 12:44

papafe changed the title ~~[RDART-968] [WIP] Add support for keypath filtering on object~~ [RDART-968] Add support for keypath filtering on object Apr 8, 2024

Fixed mapTo

fbe87cf

papafe commented Apr 8, 2024

View reviewed changes

packages/realm_dart/lib/src/realm_object.dart Show resolved Hide resolved

papafe requested a review from nielsenko April 11, 2024 14:24

nielsenko approved these changes Apr 16, 2024

View reviewed changes

papafe added 4 commits April 16, 2024 11:26

Added skip to backlink test

792824b

Fixed core version

fad8c0f

Restored missing file

8981b00

papafe merged commit 905874c into main Apr 18, 2024
57 checks passed

papafe deleted the fp/keypath-filtering branch April 18, 2024 08:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RDART-968] Add support for keypath filtering on object #1601

[RDART-968] Add support for keypath filtering on object #1601

papafe commented Mar 22, 2024 •

edited

papafe commented Mar 26, 2024

papafe Mar 26, 2024

papafe Apr 5, 2024

nielsenko Apr 8, 2024

papafe Apr 8, 2024

nielsenko Apr 16, 2024

papafe commented Apr 5, 2024 •

edited

nielsenko left a comment

nielsenko Apr 8, 2024

nielsenko Apr 8, 2024

papafe commented Apr 8, 2024

nielsenko left a comment

coveralls-official bot commented Apr 16, 2024

[RDART-968] Add support for keypath filtering on object #1601

[RDART-968] Add support for keypath filtering on object #1601

Conversation

papafe commented Mar 22, 2024 • edited

papafe commented Mar 26, 2024

papafe Mar 26, 2024

Choose a reason for hiding this comment

papafe Apr 5, 2024

Choose a reason for hiding this comment

nielsenko Apr 8, 2024

Choose a reason for hiding this comment

papafe Apr 8, 2024

Choose a reason for hiding this comment

nielsenko Apr 16, 2024

Choose a reason for hiding this comment

papafe commented Apr 5, 2024 • edited

nielsenko left a comment

Choose a reason for hiding this comment

nielsenko Apr 8, 2024

Choose a reason for hiding this comment

nielsenko Apr 8, 2024

Choose a reason for hiding this comment

papafe commented Apr 8, 2024

nielsenko left a comment

Choose a reason for hiding this comment

coveralls-official bot commented Apr 16, 2024

Pull Request Test Coverage Report for Build 8703959258

Details

💛 - Coveralls

papafe commented Mar 22, 2024 •

edited

papafe commented Apr 5, 2024 •

edited