RDART-969: Keypath filtering on collections #1714

papafe · 2024-06-07T12:54:20Z

This PR adds support for keypath filtering on collections

TODO:

Add support for lists
Add support for sets
Add support for dictionaries
Add basic tests for lists, sets, dictionaries (bulk of the tests will be on results). This includes verifying we can't use keypath filtering on collections that do not contain realm objects
Changelog

papafe · 2024-06-11T09:48:29Z

@nielsenko
I've put the bulk of the tests for the keypath filtering on results, and I've left some basic tests for the other collections (lists, sets, dictionaries), so we don't repeat the same tests.
I have left some todos around about things I am not sure / we need to discuss, but functionally everything should work.

coveralls-official · 2024-06-12T06:28:52Z

Pull Request Test Coverage Report for Build 9477711446

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

87 of 90 (96.67%) changed or added relevant lines in 9 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.2%) to 86.962%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/realm_dart/lib/src/list.dart	12	13	92.31%
packages/realm_dart/lib/src/map.dart	13	14	92.86%
packages/realm_dart/lib/src/set.dart	12	13	92.31%

Totals
Change from base Build 9416962455:	0.2%
Covered Lines:	5996
Relevant Lines:	6895

💛 - Coveralls

…th-new' into fp/collection-keypath-new

packages/realm_dart/test/realm_object_test.dart

nielsenko

Look mostly good to me. Most of my comments are more suggestions, but the the one about import 'dart:ffi'; is a blocker 😄

nielsenko · 2024-06-14T06:24:11Z

CHANGELOG.md

-  PR [#1713](https://github.com/realm/realm-dart/pull/1713)). This does **not** imply that realm works on web!
+  for web without breaking compilation. (Issue [#1374](https://github.com/realm/realm-dart/issues/1374))


On purpose?

Not really, sorry, I messed up the conflict it seems 🤦

CHANGELOG.md

packages/realm_dart/lib/src/handles/native/object_handle.dart

nielsenko · 2024-06-14T06:50:04Z

packages/realm_dart/lib/src/list.dart

  /// Allows listening for changes when the contents of this collection changes.
  Stream<RealmListChanges<T>> get changes;
+
+  /// Allows listening for changes when the contents of this collection changes on one of the provided [keyPaths].
+  Stream<RealmListChanges<T>> changesFor([List<String>? keyPaths]);


This is probably a little late in the game... but since changesFor only makes sense for collections of realm objects we could make it an extension method like:

extension ListOfRealmObjectsEx<T extends RealmObject> extends RealmList<T> { Stream<RealmListChanges<T>> changesFor([List<String>? keyPaths]); }

That way it becomes a compile error to try to call changesFor on RealmList<int> etc.

That would actually make sense.
The only "issue" I see is that now changes is defined as changesFor(null) to avoid repetitions.
If we move changesFor to an extension method then we'd need to have some duplication (admittedly not a biggie though). It also seems Dart does not have protected methods, so I don't know if I can go around this with some other language constructs. Any suggestion?

You could make a private _changesFor that both the changesFor extension method, and changes member delegates to?

Yeah, I was thinking something like that. In my mind it sounded uglier than it would probably be, I'll give it a try 😁

I've done it in the last commit

nielsenko · 2024-06-14T06:53:07Z

packages/realm_dart/lib/src/map.dart

@@ -33,6 +33,9 @@ abstract class RealmMap<T extends Object?> with RealmEntity implements MapBase<S

  /// Allows listening for changes when the contents of this collection changes.
  Stream<RealmMapChanges<T>> get changes;
+
+  /// Allows listening for changes when the contents of this collection changes on one of the provided [keyPaths].


Should we spend some time explaining the difference between null and []?

Added more explanation on the extension method

packages/realm_dart/lib/src/map.dart

packages/realm_dart/lib/src/results.dart

packages/realm_dart/test/realm_object_test.dart

Co-authored-by: Kasper Overgård Nielsen <[email protected]>

papafe · 2024-06-14T12:51:23Z

@nielsenko I think I've addressed all the comments.
A couple of other things I've noticed, and that we don't necessarily need to address here, but I thought it would be worth writing down.

We have some inconsistencies with naming:

In the errors for unmanaged sets we call them RealmSets, while we call lists just Lists (same for maps)
Regarding notification controllers, we have ListNotificationController but RealmSetNotificationController.
Regarding tests, we have list_test but we have realm_set_test/realm_map_test
Maybe we should have a different suffix between the native handle implementation and the interface files. just having a look is a little bit confusing sometimes.

Some other considerations (that probably require some work):

Maybe we can move the keypath property to the base NotificationController, instead of having it defined in the various specific classes
Maybe we can have a base class for all collections (results, lists, sets, maps). This way we could move changes and some other common things there. The collection handle themselves could have a common base class.

nielsenko · 2024-06-14T13:03:08Z

In my opinion..

In the errors for unmanaged sets we call them RealmSets, while we call lists just Lists (same for maps)

Going forward we should try to reduce the use of realm prefix to where it is needed. This means only on public facing classes, that are likely to collide with other class names (RealmSet, RealmList, RealmMap, etc.). Internally we should avoid unless it makes code more readable.

Regarding notification controllers, we have ListNotificationController but RealmSetNotificationController.

It should just be SetNotificationController.

Regarding tests, we have list_test but we have realm_set_test/realm_map_test

it should just be set_test.dart.

Maybe we should have a different suffix between the native handle implementation and the interface files. just having a look is a little bit confusing sometimes.

I'm a bit reluctant on this. For each handle, there is the interface, and two implementations.. I would prefer not to suffix the implementations with Native and Web (or whatever)

Some other considerations (that probably require some work):

Maybe we can move the keypath property to the base NotificationController, instead of having it defined in the various specific classes

Maybe we can have a base class for all collections (results, lists, sets, maps). This way we could move changes and some other common things there. The collection handle themselves could have a common base class.

The current C-api don't really give us much benefit, but yes.

However, let us land this PR as is, and get started on the weekend 😄 .. we can always address this later.

nielsenko

LGTM! Great work 👍

papafe · 2024-06-14T13:11:28Z

I'll merge this as soon as the CI agrees

* main: RDART-969: Keypath filtering on collections (#1714)

Added basic tests for collections keypath filtering

36dcaf7

cla-bot bot added the cla: yes label Jun 7, 2024

github-actions bot assigned papafe Jun 7, 2024

papafe added 3 commits June 11, 2024 10:55

Various fixes

1c4a7b7

Basic tests

22727a1

Added basic tests

7f4577b

papafe requested a review from nielsenko June 11, 2024 09:45

Added fix for realm objects collections

634828a

papafe added 2 commits June 12, 2024 14:18

Added changelog and added ticket

a9b00db

Merge branch 'main' into fp/collection-keypath-new

9f97c66

papafe marked this pull request as ready for review June 12, 2024 12:21

papafe added 2 commits June 12, 2024 21:02

Clean up

4c16d7d

Merge remote-tracking branch 'refs/remotes/origin/fp/collection-keypa…

47591e1

…th-new' into fp/collection-keypath-new

papafe commented Jun 12, 2024

View reviewed changes

packages/realm_dart/test/realm_object_test.dart Show resolved Hide resolved

Small correction

ad7f178

nielsenko suggested changes Jun 14, 2024

View reviewed changes

papafe and others added 7 commits June 14, 2024 10:43

Apply suggestions from code review

3e69da1

Co-authored-by: Kasper Overgård Nielsen <[email protected]>

Fixed changelog

8fcc2f6

Avoiding calling native for classkey

7793839

Simplified code

fcf753d

Various fixes

f2461a5

Small fix

49506c3

Added changesFor to extension methods

f81d9ab

nielsenko self-requested a review June 14, 2024 13:04

nielsenko approved these changes Jun 14, 2024

View reviewed changes

Corrected error

e8d4d81

papafe merged commit 16f34e5 into main Jun 14, 2024
53 of 56 checks passed

papafe deleted the fp/collection-keypath-new branch June 14, 2024 13:31

nirinchev added a commit that referenced this pull request Jun 18, 2024

Merge branch 'main' into ni/progress-notifications

f1c44d4

* main: RDART-969: Keypath filtering on collections (#1714)

papafe mentioned this pull request Jun 24, 2024

Add support for keypath filtering on subscriptions for collections #1558

Closed

github-actions bot locked as resolved and limited conversation to collaborators Jul 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RDART-969: Keypath filtering on collections #1714

RDART-969: Keypath filtering on collections #1714

papafe commented Jun 7, 2024 •

edited

Loading

papafe commented Jun 11, 2024 •

edited

Loading

coveralls-official bot commented Jun 12, 2024 •

edited

Loading

nielsenko left a comment

nielsenko Jun 14, 2024

papafe Jun 14, 2024

nielsenko Jun 14, 2024

papafe Jun 14, 2024

nielsenko Jun 14, 2024

papafe Jun 14, 2024

papafe Jun 14, 2024

nielsenko Jun 14, 2024

papafe Jun 14, 2024

papafe commented Jun 14, 2024

nielsenko commented Jun 14, 2024

nielsenko left a comment

papafe commented Jun 14, 2024

		PR [#1713](https://github.com/realm/realm-dart/pull/1713)). This does not imply that realm works on web!
		for web without breaking compilation. (Issue [#1374](https://github.com/realm/realm-dart/issues/1374))

RDART-969: Keypath filtering on collections #1714

RDART-969: Keypath filtering on collections #1714

Conversation

papafe commented Jun 7, 2024 • edited Loading

papafe commented Jun 11, 2024 • edited Loading

coveralls-official bot commented Jun 12, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9477711446

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

nielsenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

papafe commented Jun 14, 2024

nielsenko commented Jun 14, 2024

nielsenko left a comment

Choose a reason for hiding this comment

papafe commented Jun 14, 2024

papafe commented Jun 7, 2024 •

edited

Loading

papafe commented Jun 11, 2024 •

edited

Loading

coveralls-official bot commented Jun 12, 2024 •

edited

Loading