Add keypath filtering for Property/CollectionChanged #3121

JacobOscarGunnarsson · 2022-12-07T12:26:48Z

Partially Fixes #3112. initially we will internally filter the notifications in the following manner:

for objects we won't listen for events happening to collection properties at all. This include backlinks, sets, dictionaries, list and queries/results.
For collections, we will only listen to the shallow modifications that are done on the collection itself. this means that modifications to objects within a collection won't fire a notification

This PR needs to wait until these changes are merged to core

wrappers/src/dictionary_cs.cpp

fealebenpae · 2022-12-07T13:07:18Z

computed_properties contains only backlink/LinkingObjects properties. I think you should also include regular properties where the type is a collection.

Tests/Realm.Tests/Database/NotificationTests.cs

...lm.SourceGenerator/Realms.SourceGenerator.RealmGenerator/TestNotificationObject_generated.cs

papafe

I think the tests make sense! Probably it'll be worth adding also collections of other types, in the same way you did for single link

Realm/Realm/DatabaseTypes/Metadata.cs

Tests/Realm.Tests/Database/TestNotificationObject.cs

Realm/Realm/Handles/ObjectHandle.cs

Realm/Realm/DatabaseTypes/Accessors/ManagedAccessor.cs

papafe · 2022-12-12T10:21:15Z

Realm/Realm/DatabaseTypes/Accessors/ManagedAccessor.cs

@@ -181,12 +181,15 @@ public void SubscribeForNotifications(Action<string> notifyPropertyChangedDelega
                throw new RealmFrozenException("It is not possible to add a change listener to a frozen RealmObjectBase since it never changes.");
            }

+            PropertyType collectionFlag = PropertyType.Array | PropertyType.Dictionary | PropertyType.Set;


Probably we also need to check backlinks? I'm not sure we get notifications for those in any case.

Also, it would make sense to calculate those indices only once, they are not going to change

Backlinks are in a separate vector in the Object Store ObjectSchema so even if they cause notifications they need to be handled separately. But we need to see if they raise notifications in the first place.

Added a test to check this, and they don't in our case

Realm/Realm/Handles/ObjectHandle.cs

wrappers/src/object_cs.cpp

Tests/Realm.Tests/Database/NotificationTests.cs

Realm/Realm/DatabaseTypes/Accessors/ManagedAccessor.cs

fealebenpae · 2022-12-12T12:45:41Z

Realm/Realm/DatabaseTypes/Accessors/ManagedAccessor.cs

@@ -181,12 +181,15 @@ public void SubscribeForNotifications(Action<string> notifyPropertyChangedDelega
                throw new RealmFrozenException("It is not possible to add a change listener to a frozen RealmObjectBase since it never changes.");
            }

+            PropertyType collectionFlag = PropertyType.Array | PropertyType.Dictionary | PropertyType.Set;


Backlinks are in a separate vector in the Object Store ObjectSchema so even if they cause notifications they need to be handled separately. But we need to see if they raise notifications in the first place.

Realm/Realm/DatabaseTypes/Metadata.cs

Realm/Realm/Handles/DictionaryHandle.cs

Realm/Realm/Handles/ListHandle.cs

Realm/Realm/Handles/NotifiableObjectHandleBase.cs

Realm/Realm/Handles/ResultsHandle.cs

Realm/Realm/Handles/SetHandle.cs

Realm/Realm/Handles/ObjectHandle.cs

…fication_callback

wrappers/src/dictionary_cs.cpp

Realm/Realm/Handles/DictionaryHandle.cs

        {
            EnsureIsOpen();

-            var result = NativeMethods.add_notification_callback(this, managedObjectHandle, out var nativeException);
+            var propertyIndicesLength = propertyIndices == null ? (IntPtr)0 : (IntPtr)propertyIndices.Length;
+            var result = NativeMethods.add_notification_callback(this, managedObjectHandle, propertyIndices, propertyIndicesLength, out var nativeException);


Realm/Realm/Handles/ListHandle.cs

        {
            EnsureIsOpen();

-            var result = NativeMethods.add_notification_callback(this, managedObjectHandle, out var nativeException);
+            var propertyIndicesLength = propertyIndices == null ? (IntPtr)0 : (IntPtr)propertyIndices.Length;
+            var result = NativeMethods.add_notification_callback(this, managedObjectHandle, propertyIndices, propertyIndicesLength, out var nativeException);


Realm/Realm/Handles/ObjectHandle.cs

-
-            var result = NativeMethods.add_notification_callback(this, managedObjectHandle, out var nativeException);
+            var propertyIndicesLength = propertyIndices == null ? (IntPtr)0 : (IntPtr)propertyIndices.Length;
+            var result = NativeMethods.add_notification_callback(this, managedObjectHandle, propertyIndices, propertyIndicesLength, out var nativeException);


Realm/Realm/Handles/ResultsHandle.cs

        {
            EnsureIsOpen();

-            var result = NativeMethods.add_notification_callback(this, managedObjectHandle, out var nativeException);
+            var propertyIndicesLength = propertyIndices == null ? (IntPtr)0 : (IntPtr)propertyIndices.Length;
+            var result = NativeMethods.add_notification_callback(this, managedObjectHandle, propertyIndices, propertyIndicesLength, out var nativeException);


Realm/Realm/Handles/SetHandle.cs

        {
            EnsureIsOpen();

-            var result = NativeMethods.add_notification_callback(this, managedObjectHandle, out var nativeException);
+            var propertyIndicesLength = propertyIndices == null ? (IntPtr)0 : (IntPtr)propertyIndices.Length;
+            var result = NativeMethods.add_notification_callback(this, managedObjectHandle, propertyIndices, propertyIndicesLength, out var nativeException);


Realm/Realm/Handles/ListHandle.cs

@@ -63,7 +64,7 @@
            public static extern void destroy(IntPtr listInternalHandle);

            [DllImport(InteropConfig.DLL_NAME, EntryPoint = "list_add_notification_callback", CallingConvention = CallingConvention.Cdecl)]
-            public static extern IntPtr add_notification_callback(ListHandle listHandle, IntPtr managedListHandle, out NativeException ex);
+            public static extern IntPtr add_notification_callback(ListHandle listHandle, IntPtr managedListHandle, [MarshalAs(UnmanagedType.LPArray), In] IntPtr[] propjerty_indices, IntPtr property_count, out NativeException ex);


Realm/Realm/Handles/ResultsHandle.cs

@@ -43,7 +44,7 @@
            public static extern void clear(ResultsHandle results, SharedRealmHandle realmHandle, out NativeException ex);

            [DllImport(InteropConfig.DLL_NAME, EntryPoint = "results_add_notification_callback", CallingConvention = CallingConvention.Cdecl)]
-            public static extern IntPtr add_notification_callback(ResultsHandle results, IntPtr managedResultsHandle, out NativeException ex);
+            public static extern IntPtr add_notification_callback(ResultsHandle results, IntPtr managedResultsHandle, [MarshalAs(UnmanagedType.LPArray), In] IntPtr[] propjerty_indices, IntPtr property_count, out NativeException ex);


Realm/Realm/Handles/SetHandle.cs

@@ -36,7 +37,7 @@
            public static extern void destroy(IntPtr handle);

            [DllImport(InteropConfig.DLL_NAME, EntryPoint = "realm_set_add_notification_callback", CallingConvention = CallingConvention.Cdecl)]
-            public static extern IntPtr add_notification_callback(SetHandle handle, IntPtr managedSetHandle, out NativeException ex);
+            public static extern IntPtr add_notification_callback(SetHandle handle, IntPtr managedSetHandle, [MarshalAs(UnmanagedType.LPArray), In] IntPtr[] propjerty_indices, IntPtr property_count, out NativeException ex);


fealebenpae

A few small comments. I'd also like to see the PR updated with the Core submodule pointing to the Core branch so I can see the related wrappers changes in full as well.

Tests/Realm.Tests/Database/NotificationTests.cs

Realm/Realm/DatabaseTypes/Accessors/ManagedAccessor.cs

…allow or not.

…fication callback

wrappers/src/dictionary_cs.cpp

JacobOscarGunnarsson requested review from fealebenpae, nirinchev, LaPeste and papafe December 7, 2022 12:26

cla-bot bot added the cla: yes label Dec 7, 2022

github-actions bot assigned JacobOscarGunnarsson Dec 7, 2022

JacobOscarGunnarsson commented Dec 7, 2022

View reviewed changes

wrappers/src/dictionary_cs.cpp Show resolved Hide resolved

papafe reviewed Dec 7, 2022

View reviewed changes

Tests/Realm.Tests/Database/NotificationTests.cs Outdated Show resolved Hide resolved

JacobOscarGunnarsson force-pushed the jg/keypath_filtering_internal branch from 41e1de8 to 0c846c4 Compare December 8, 2022 15:08

JacobOscarGunnarsson commented Dec 8, 2022

View reviewed changes

...lm.SourceGenerator/Realms.SourceGenerator.RealmGenerator/TestNotificationObject_generated.cs Show resolved Hide resolved

JacobOscarGunnarsson force-pushed the jg/keypath_filtering_internal branch 3 times, most recently from a1ed80f to b51e8b8 Compare December 8, 2022 15:43

papafe reviewed Dec 9, 2022

View reviewed changes

Realm/Realm/DatabaseTypes/Metadata.cs Outdated Show resolved Hide resolved

papafe reviewed Dec 9, 2022

View reviewed changes

Tests/Realm.Tests/Database/TestNotificationObject.cs Outdated Show resolved Hide resolved

github-advanced-security bot found potential problems Dec 12, 2022

View reviewed changes

Realm/Realm/Handles/ObjectHandle.cs Fixed Show fixed Hide fixed

Realm/Realm/Handles/ObjectHandle.cs Fixed Show resolved Hide resolved

Realm/Realm/DatabaseTypes/Accessors/ManagedAccessor.cs Fixed Show fixed Hide fixed

JacobOscarGunnarsson requested a review from papafe December 12, 2022 09:45

papafe reviewed Dec 12, 2022

View reviewed changes

fealebenpae requested changes Dec 12, 2022

View reviewed changes

Jacob Gunnarsson added 10 commits December 13, 2022 12:48

add tests to outline how we want notifications to work for objects

6a8d8a1

move GetPropertyIndex to Metadata

ef78f98

add propertyIndices to managedAccessor

c470a09

Add logic in wrapper to deal with propertyindices for object_add_noti…

e995770

…fication_callback

renaming, removed unecessary comment

d65e755

make tests more elaborate

7803682

add empty-check for array

7a6b9e8

use linq expression instead of loop

30ca8b7

make propertyIndices an array rather than list

b27e492

make getpropertyIndex an instance method of Metadata

9af03ce

Jacob Gunnarsson added 2 commits December 14, 2022 14:18

fix warnings

e63ddb3

disambiguate using, collectionchanged += or SubscribeForNotifications

6a7093f

JacobOscarGunnarsson commented Dec 14, 2022

View reviewed changes

wrappers/src/dictionary_cs.cpp Show resolved Hide resolved

github-advanced-security bot found potential problems Dec 14, 2022

View reviewed changes

JacobOscarGunnarsson requested review from papafe and fealebenpae December 14, 2022 14:38

fealebenpae requested changes Dec 14, 2022

View reviewed changes

Tests/Realm.Tests/Database/NotificationTests.cs Outdated Show resolved Hide resolved

Tests/Realm.Tests/Database/NotificationTests.cs Outdated Show resolved Hide resolved

Realm/Realm/DatabaseTypes/Accessors/ManagedAccessor.cs Outdated Show resolved Hide resolved

JacobOscarGunnarsson mentioned this pull request Dec 14, 2022

add support for shallow notifications realm/realm-core#6122

Merged

update submodule

21890f7

papafe reviewed Dec 14, 2022

View reviewed changes

Realm/Realm/DatabaseTypes/Accessors/ManagedAccessor.cs Outdated Show resolved Hide resolved

Realm/Realm/DatabaseTypes/Accessors/ManagedAccessor.cs Outdated Show resolved Hide resolved

Realm/Realm/DatabaseTypes/Accessors/ManagedAccessor.cs Outdated Show resolved Hide resolved

Jacob Gunnarsson added 7 commits December 16, 2022 08:36

fix some pr comments

2996f3d

update core submodule

bd973ae

share common logic in wrappers for add_notification_callback

a28aa5b

fixed bug, now disambiguates if collection-subscribtions should be sh…

112405a

…allow or not.

add test for backlink

ccce9ba

rename property indices and make it lazy

0da4cc7

update submodule

c30f3fc

JacobOscarGunnarsson requested review from fealebenpae and papafe December 19, 2022 15:08

Jacob Gunnarsson added 3 commits December 19, 2022 16:18

add changelog

54263d4

make shallow an optional parameter

1437d0f

check if collection contains primitives or objects when adding a noti…

e68cca6

…fication callback

JacobOscarGunnarsson commented Dec 20, 2022

View reviewed changes

wrappers/src/dictionary_cs.cpp Show resolved Hide resolved

JacobOscarGunnarsson marked this pull request as ready for review December 20, 2022 09:36

nirinchev mentioned this pull request Jan 13, 2023

Keypath filtering #3179

Merged

2 tasks

nirinchev closed this Jan 20, 2023

nirinchev deleted the jg/keypath_filtering_internal branch January 20, 2023 13:52

nirinchev mentioned this pull request Mar 24, 2023

Add keypath filtering for Property/CollectionChanged #3112

Closed

peppy mentioned this pull request Apr 17, 2023

Show count of beatmaps in collections in manage dialog ppy/osu#22932

Merged

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 keypath filtering for Property/CollectionChanged #3121

Add keypath filtering for Property/CollectionChanged #3121

JacobOscarGunnarsson commented Dec 7, 2022 •

edited

Loading

fealebenpae commented Dec 7, 2022

papafe left a comment

papafe Dec 12, 2022

fealebenpae Dec 12, 2022

JacobOscarGunnarsson Dec 19, 2022

fealebenpae Dec 12, 2022

fealebenpae left a comment

Add keypath filtering for Property/CollectionChanged #3121

Add keypath filtering for Property/CollectionChanged #3121

Conversation

JacobOscarGunnarsson commented Dec 7, 2022 • edited Loading

fealebenpae commented Dec 7, 2022

papafe left a comment

Choose a reason for hiding this comment

papafe Dec 12, 2022

Choose a reason for hiding this comment

fealebenpae Dec 12, 2022

Choose a reason for hiding this comment

JacobOscarGunnarsson Dec 19, 2022

Choose a reason for hiding this comment

fealebenpae Dec 12, 2022

Choose a reason for hiding this comment

fealebenpae left a comment

Choose a reason for hiding this comment

JacobOscarGunnarsson commented Dec 7, 2022 •

edited

Loading