Add support for Keypath filtering in notifications #1547

cmelchior · 2023-10-13T14:41:22Z

Closes #661

This PR adds support for keypath filtering which makes it possible to select which fields should trigger the notifications, and enable notifications more than 4 levels deep.

TODO

Add tests for query flows
Add tests for single object query flows
Figure out how to do our internal interface, I.e. some do not support keypaths like Realm and aggregate flows. Either add two different interfaces or have 1 but just ignore the keypath for some notifications (internally)
Add support for passing the keypath to the C-API.
Should the public API use List<String>(), List<String>? or vararg String. Right now I'm leaning towards List<String>? but should be discussed.
Merge Rework KeyPathArray filters for notifications in the C-API. realm-core#7087 in Core

Added handling of char** to String[]

cmelchior · 2023-10-27T13:48:54Z

This PR depends on realm/realm-core#7087 being merged first.

# Conflicts: # packages/external/core # packages/jni-swig-stub/realm.i # packages/jni-swig-stub/src/main/jni/realm_api_helpers.cpp # packages/jni-swig-stub/src/main/jni/realm_api_helpers.h

# Conflicts: # packages/external/core

packages/cinterop/src/jvm/kotlin/io/realm/kotlin/internal/interop/RealmInterop.kt

clementetb · 2023-11-14T09:57:55Z

packages/jni-swig-stub/src/main/jni/realm_api_helpers.cpp

@@ -143,7 +143,9 @@ bool migration_callback(void *userdata, realm_t *old_realm, realm_t *new_realm,
 // TODO OPTIMIZE Abstract pattern for all notification registrations for collections that receives
 //  changes as realm_collection_changes_t.
 realm_notification_token_t *
-register_results_notification_cb(realm_results_t *results, jobject callback) {
+register_results_notification_cb(realm_results_t *results,
+                                 int64_t key_path_array_ptr,


Shouldn't we use void* on pointers?

We cannot in this case, as this is methods being called from the JVM which doesn't have that concept. We are using 0 in that case

clementetb · 2023-11-14T10:01:22Z

packages/jni-swig-stub/src/main/jni/realm_api_helpers.cpp

                    reinterpret_cast<realm_object_t*>(collection_ptr),
                    user_data, // Use the callback as user data
                    user_data_free,
-                    NULL, // See https://github.com/realm/realm-kotlin/issues/661
+                    (key_path_array_ptr == 0) ? NULL : reinterpret_cast<realm_key_path_array_t*>(key_path_array_ptr),


Is this nullability check required?

Strictly no, but it makes it easier to understand what is going on IMO

packages/library-base/src/commonMain/kotlin/io/realm/kotlin/internal/ObjectBoundRealmResults.kt

# Conflicts: # CHANGELOG.md

rorbech

Really nice. I have a number of comments around internals but most notably around tests.

packages/cinterop/src/commonMain/kotlin/io/realm/kotlin/internal/interop/RealmInterop.kt

packages/external/core

...e/src/commonTest/kotlin/io/realm/kotlin/test/common/notifications/RealmNotificationsTests.kt

packages/test-base/src/commonTest/kotlin/io/realm/kotlin/test/common/QueryTests.kt

packages/library-base/src/commonMain/kotlin/io/realm/kotlin/types/RealmMap.kt

...onTest/kotlin/io/realm/kotlin/test/common/notifications/RealmDictionaryNotificationsTests.kt

...c/commonTest/kotlin/io/realm/kotlin/test/common/notifications/RealmListNotificationsTests.kt

packages/test-base/src/commonTest/kotlin/io/realm/kotlin/test/common/utils/FlowableTests.kt

packages/library-base/src/commonMain/kotlin/io/realm/kotlin/query/RealmElementQuery.kt

packages/library-base/src/commonMain/kotlin/io/realm/kotlin/query/RealmSingleQuery.kt

packages/library-base/src/commonMain/kotlin/io/realm/kotlin/types/RealmList.kt

clementetb · 2023-11-21T14:56:31Z

packages/library-base/src/commonMain/kotlin/io/realm/kotlin/internal/KeyPathFlowable.kt

 */
+
+internal interface KeyPathFlowable<T> {
+    fun asFlow(keyPaths: List<String>? = null): Flow<T>


Is there any reason for using List and not vararg?

This is the internal interface that just mirrors the public API which uses List. I was considering whether to use vararg or List in the public API but ended up with List as it makes it easier to extend the API later. I.e. there is the potential that projections will end up adding arguments to the asFlow method as well.

nhachicha

Nice 🎉 minor comments 👍

packages/test-base/src/commonTest/kotlin/io/realm/kotlin/test/common/QueryTests.kt

...c/commonTest/kotlin/io/realm/kotlin/test/common/notifications/BacklinksNotificationsTests.kt

nhachicha · 2023-11-23T10:16:50Z

...c/commonTest/kotlin/io/realm/kotlin/test/common/notifications/BacklinksNotificationsTests.kt

+            findLatest(results.first())!!.booleanField = true
+        }
+        realm.write {
+            // Update field that should trigger a notification


I'm not following here, you're navigating to the top parent and updating it, whilst the query is on the leaf element? this should not trigger the notification while the previous update (booleanField = true) should ?

Yes, because I used the keypath listOf("objectBacklinks.objectBacklinks.objectBacklinks.objectBacklinks.objectBacklinks.stringField") which only consider changes through the backlinks as things that should trigger a notification

...rc/commonTest/kotlin/io/realm/kotlin/test/common/notifications/RealmSetNotificationsTests.kt

nhachicha · 2023-11-23T11:15:53Z

packages/library-base/src/commonMain/kotlin/io/realm/kotlin/types/RealmSet.kt

+     * objects inside the set will result in a change being emitted. Nested properties can be
+     * defined using a dotted syntax, e.g. `parent.child.name`. If no keypaths are provided, changes
+     * to all top-level properties and nested properties up to 4 levels down will trigger a change.
+     * Keypaths are only supported for sets of objects.


why only supported for sets of objects? it's also supported for List, RealmResults etc...

It is, but this doc is on the RealmSet. They key thing I wanted to spell out was that keyPaths only works on RealmSet<RealmObject> but maybe it could be clarified better?

packages/library-base/src/commonMain/kotlin/io/realm/kotlin/internal/query/ScalarQuery.kt

nhachicha · 2023-11-23T11:36:11Z

packages/library-base/src/commonMain/kotlin/io/realm/kotlin/internal/RealmMapInternal.kt

@@ -61,7 +63,7 @@ internal abstract class ManagedRealmMap<K, V> constructor(
    internal val parent: RealmObjectReference<*>,
    internal val nativePointer: RealmMapPointer,
    val operator: MapOperator<K, V>
-) : AbstractMutableMap<K, V>(), RealmMap<K, V>, CoreNotifiable<ManagedRealmMap<K, V>, MapChange<K, V>>, Flowable<MapChange<K, V>> {


why removing Flowable interface?

It was an internal interface that matched our public API. But with the KeyPath changes that is no longer the case and is actually harmful instead now as Notifier cannot use it to handle all flows the same.

nhachicha · 2023-11-23T11:40:01Z

packages/jni-swig-stub/realm.i

+    const jclass clazz = jenv->FindClass("java/lang/String");
+
+    while ($1[len]) len++;
+    jresult = jenv->NewObjectArray(len, clazz, NULL);


check for OOM

rorbech

Some minor comments about nested runBlocking, etc.. Just wanted to highlight it, but don't think we need to bother fixing it and rerunning CI ... unless there is other reasons for doing so.

packages/library-base/src/commonMain/kotlin/io/realm/kotlin/internal/RealmListInternal.kt

packages/library-base/src/commonMain/kotlin/io/realm/kotlin/internal/RealmMapInternal.kt

rorbech · 2023-12-01T12:47:21Z

packages/library-base/src/commonMain/kotlin/io/realm/kotlin/internal/util/Validation.kt

+     * to that type, otherwise an IllegalArgumentException is thrown with the provided error message.
+     */
+    @OptIn(ExperimentalContracts::class)
+    public inline fun <reified T : Any?> isType(arg: Any?, errorMessage: String) {


packages/test-base/src/commonTest/kotlin/io/realm/kotlin/test/common/QueryTests.kt

Christian Melchior added 3 commits August 14, 2023 09:52

Backup

b5c3a06

Merge branch 'main' into cm/keypath-notifications

0c22cdb

Backup

6885aa3

github-actions bot assigned cmelchior Oct 13, 2023

Christian Melchior added 2 commits October 26, 2023 14:16

Merge branch 'main' into cm/keypath-notifications

d10e5ec

Actually getting something to work.

3a880cd

Added handling of char** to String[]

Christian Melchior added 11 commits November 5, 2023 20:47

Add first step of tests

81bd939

Results tests

0d48749

Add tests for sets and lists

813aa34

Add map tests

8dd8c3c

Formatting

94577bd

Update core submodule

caa4f38

Merge branch 'main' into cm/keypath-notifications

d73ed88

# Conflicts: # packages/external/core # packages/jni-swig-stub/realm.i # packages/jni-swig-stub/src/main/jni/realm_api_helpers.cpp # packages/jni-swig-stub/src/main/jni/realm_api_helpers.h

More cleanup

1ec7bd1

Merge branch 'main' into cm/keypath-notifications

e097810

# Conflicts: # packages/external/core

Fix macOS implementation

4c6f151

Add changelog

078535e

clementetb reviewed Nov 14, 2023

View reviewed changes

Christian Melchior added 2 commits November 16, 2023 08:56

Add support for keypaths on queries

cdfd2de

Merge branch 'main' into cm/keypath-notifications

a0015be

# Conflicts: # CHANGELOG.md

cmelchior marked this pull request as ready for review November 16, 2023 07:58

cmelchior requested review from nhachicha and rorbech November 16, 2023 07:58

Christian Melchior added 3 commits November 17, 2023 17:37

Merge branch 'main' into cm/keypath-notifications

8abe065

Cleanup

4f43141

Fix static analysis

9ec94d0

rorbech reviewed Nov 21, 2023

View reviewed changes

clementetb reviewed Nov 21, 2023

View reviewed changes

PR feedback

4a25622

nhachicha approved these changes Nov 23, 2023

View reviewed changes

Christian Melchior added 3 commits November 23, 2023 14:45

Use correct commit from master.

9314d70

PR feedback + add mention of wildcards to docs

1422d2b

Fix detekt

438f72f

rorbech approved these changes Dec 1, 2023

View reviewed changes

Christian Melchior added 2 commits December 1, 2023 14:21

PR feedback

45b909c

Fix test

fa1de3c

cmelchior merged commit 3ec94a4 into main Dec 1, 2023
1 of 2 checks passed

cmelchior deleted the cm/keypath-notifications branch December 1, 2023 13:54

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Keypath filtering in notifications #1547

Add support for Keypath filtering in notifications #1547

cmelchior commented Oct 13, 2023 •

edited

Loading

cmelchior commented Oct 27, 2023

clementetb Nov 14, 2023

cmelchior Nov 22, 2023

clementetb Nov 14, 2023

cmelchior Nov 22, 2023

rorbech left a comment

clementetb Nov 21, 2023

cmelchior Nov 22, 2023

nhachicha left a comment

nhachicha Nov 23, 2023

cmelchior Dec 1, 2023

nhachicha Nov 23, 2023

cmelchior Dec 1, 2023

nhachicha Nov 23, 2023

cmelchior Dec 1, 2023

nhachicha Nov 23, 2023

rorbech left a comment

rorbech Dec 1, 2023

Add support for Keypath filtering in notifications #1547

Add support for Keypath filtering in notifications #1547

Conversation

cmelchior commented Oct 13, 2023 • edited Loading

cmelchior commented Oct 27, 2023

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

rorbech left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhachicha 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

rorbech left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmelchior commented Oct 13, 2023 •

edited

Loading