Update to Core 12.5.1 #967

edualonso · 2022-08-11T12:30:51Z

Fixes #867

Updated to Core 12.5.1 and fixed the following breaking changes

Queries now receive a realm_query_arg_t instead of a realm_value_t - see Query parser: IN realm-core#4266 for more details.
Anonymous logins now have the option of not reusing the same anonymous user - I've implemented it the same way .NET did, see https://github.com/realm/realm-dotnet/blob/d5b334a1fe5f5d5be1139e2e7f28558e2ebe417d/Realm/Realm/Sync/Credentials.cs#L99 - thoughts @cmelchior?
realm_convert_with_config now receives a boolean that triggers copying over all objects present in the realm specified by the provided configuration, though we default it to false to avoid copying things - thoughs @cmelchior?
Added new recovery modes for client reset (though they are not used yet, they will be added when adding automatic reset with recovery).
onBefore and onAfter client reset callbacks now return a boolean to allow routing errors through onError in Kotlin Native. As a consequence of this SyncClientResetIntegrationJVMTests.kt was no longer needed and was removed.
Sync-related download and upload completion blocks have been unified under realm_sync_wait_for_completion_func_t.

TODO:

update changelog
add missing tests for new anonymous login with reuseExisting = false

rorbech

LGTM - Minor comments ... but haven't evaluated the questions for @cmelchior, so will leave him to do the final approval.

rorbech · 2022-08-12T07:18:49Z

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

@@ -1033,10 +1042,14 @@ actual object RealmInterop {
    ): RealmQueryPointer {
        memScoped {
            val count = args.size
-            val cArgs = allocArray<realm_value_t>(count)
+            val cArgs = allocArray<realm_query_arg_t>(count)


Looks like a code block that are used a number of times. Should probably be abstracted into a method and reused instead of copying it.

I've extracted it to a separate function and added a comment to clarify the usage of realm_query_arg_t as it is a bit quirky.

rorbech · 2022-08-12T07:34:34Z

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

        return memScope {
            args.mapIndexed { i, arg ->
-                realmc.valueArray_setitem(cArgs, i, managedRealmValue(arg))
+                realm_query_arg_t().apply {


Same as for native ... we should probably put this in a separate method

…g test cases for anonymous login without reusing existing user

cmelchior · 2022-08-12T09:24:40Z

CHANGELOG.md

@@ -21,7 +22,7 @@
 * Minimum Android SDK: 16.

 ### Internal
-* None.
+* Updated to Realm Core 12.5.0, commit 4b0a203b202b7a91caf2a387e17e5623d7b37c4a.


cmelchior · 2022-08-12T09:31:29Z

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

+                    beforeCallback.onBeforeReset(beforeDb)
+                    true
+                } catch (e: Throwable) {
+                    false


Should we log the exception here? It seems to be swallowed otherwise?

If this returns false then the execution follows through onError instead, which is the intended behaviour we sought after reporting realm/realm-core#5564 to Yavor.

Yeah, we shouldn't throw it here, but this error is hidden so people wouldn't know they have buggy code. Could we at least do println(e.toStrint) here??

Ah, sorry, I missed you meant log the error. Hm, we can do it easily for native but JVM is a bit more convoluted...

I think we need to address JVM regardless, since exceptions are leaking. I think if we can use Kotlin println...it will work correctly on both JVM and Android...It isn't ideal, but should be sufficient until we can come up with something better

Native is not an issue at all, I can call println with the exception message. For JVM I will add some functions to api_helpers to extract the exception message from JNI and then do the whole JNI ceremony thing to call System.out.println. I'm not sure we can call Kotlin's println since it's a top level function which might be tricky to find by JNI.

cmelchior · 2022-08-12T09:40:53Z

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

+                    afterCallback.onAfterReset(beforeDb, afterDb, didRecover)
+                    true
+                } catch (e: Throwable) {
+                    false


cmelchior · 2022-08-12T09:43:15Z

packages/library-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/AuthenticationProvider.kt

@@ -27,6 +27,7 @@ import io.realm.kotlin.internal.interop.sync.AuthProvider
 */
 public enum class AuthenticationProvider(id: AuthProvider) {
    ANONYMOUS(AuthProvider.RLM_AUTH_PROVIDER_ANONYMOUS),
+    ANONYMOUS_NO_REUSE(AuthProvider.RLM_AUTH_PROVIDER_ANONYMOUS_NO_REUSE),


It is a bit weird that this is its own auth provider, but I. guess we are just exposing the underlying data

Yeah, this is Core's new enum, even though it adds no value having it 🤷 . In fact, its addition caused .NET to have a crash after they updated their Core to 12.5.0: realm/realm-dotnet#2987 so I'd rather add it to avoid problems.

It looks like .NET still only have one auth provider. Is there a way for us to do something similar? 🤔

Hm, it's because they don't have an enum watchdog test but we do 😅

I meant, is there a way for us to only have one auth provider for this in the public API?

Yes, I think so. It would be a matter of changing the testing setup to handle both true and false values while testing ANONYMOUS(AuthProvider.RLM_AUTH_PROVIDER_ANONYMOUS)

cmelchior

After another discussion, I think there is an edge case we are not accounting for on the JVM

cmelchior · 2022-08-12T11:50:32Z

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

@@ -664,19 +664,25 @@ before_client_reset(void* userdata, realm_t* before_realm) {
    auto before_pointer = wrap_pointer(env, reinterpret_cast<jlong>(before_realm), false);
    env->CallVoidMethod(static_cast<jobject>(userdata), java_before_callback_function, before_pointer);
    jni_check_exception(env);
+
+    return true;


I couldn't any changes to code on the JVM side, which means that if the callback throws on the JVM, then jni_check_exception(env); will throw an exception instead of returning false.

Do we have any unit tests of us manually throwing an exception in BeforeClientReset? I

Yes, onBefore here https://github.com/realm/realm-kotlin/pull/967/files#diff-4496cd276f4c265c4a34d2d6835c8a1e290376d9d2d30380bb4dc60ac49a94ebR547
and onAfter here https://github.com/realm/realm-kotlin/pull/967/files#diff-4496cd276f4c265c4a34d2d6835c8a1e290376d9d2d30380bb4dc60ac49a94ebR599

I debugged it when I wrote this for automatic client reset in a separate branch and looks like jni_check_exception does the catching and forwards the error appropriately.

After carefully inspecting the execution it seems it does work because the error is intercepted by the error handler instead, then we detect the error itself has isClientResetRequested so it's routed through the client reset handler. I will change it so that it doesn't take that path and stays within the client reset flow.

Funnily enough the before/after callbacks in the C-API throw an exception too:

if (!callback(userdata.get(), &r1)) { // if we return false it will be caught here throw CallbackFailed(); }

so the execution stays the same regardless. I will return false for consistency with native though.

The exception catch could be done in the cinterop with a lambda.

…er, removed "anonymous no reuse" provider from the public API

… native

# Conflicts: # packages/cinterop/src/darwin/kotlin/io/realm/kotlin/internal/interop/RealmInterop.kt

clementetb

Looking good. I added some comments about exception catching on the client reset callbacks.

clementetb · 2022-08-17T11:01:41Z

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

+                    afterCallback.onAfterReset(beforeDb, afterDb, didRecover)
+                    true
+                } catch (e: Throwable) {
+                    println(e.message)


Shouldn't we use our internal logger so this message could be captured by a custom logger?

Maybe the exception catch could happen in a higher layer, even would be a common logic for all platforms

The internal logger isn't available in cinterop unfortunately. I'm not sure there is good way to get this to work right now, but it is something we should track and hopefully fix when we look into combining our modules.

edualonso · 2022-08-18T14:24:41Z

I'm merging this after discussing options for the logger, having reached the conclusion that may be worth waiting until a potential package refactor.

updated to core 12.5.0 and fixed breaking changes

02b960f

cla-bot bot added the cla: yes label Aug 11, 2022

edualonso marked this pull request as ready for review August 11, 2022 12:30

edualonso marked this pull request as draft August 11, 2022 12:31

edualonso changed the title ~~Updated to core 12.5.0~~ Update to core 12.5.0 Aug 11, 2022

edualonso changed the title ~~Update to core 12.5.0~~ Update to Core 12.5.0 Aug 11, 2022

Eduardo López added 3 commits August 11, 2022 15:40

updated cinterop core version tests

485cc79

updated cinterop tests for queries

88b0cbd

updated missing enums

fea39b4

edualonso requested a review from cmelchior August 11, 2022 16:34

edualonso marked this pull request as ready for review August 11, 2022 16:34

cleanup

99ebb20

rorbech reviewed Aug 12, 2022

View reviewed changes

updated changelog, updated to latest core 12.5.0 commit, added missin…

7a6ed0a

…g test cases for anonymous login without reusing existing user

cmelchior approved these changes Aug 12, 2022

View reviewed changes

cmelchior suggested changes Aug 12, 2022

View reviewed changes

Eduardo López added 2 commits August 13, 2022 20:35

cleanup, added logging of exception messages from onbefore and on aft…

0f4284a

…er, removed "anonymous no reuse" provider from the public API

Removed throwing of exception to align with implementation for kotlin…

83a19fe

… native

edualonso requested a review from cmelchior August 15, 2022 08:15

updated core to 12.5.1

1d8befe

edualonso changed the title ~~Update to Core 12.5.0~~ Update to Core 12.5.1 Aug 16, 2022

Eduardo López added 2 commits August 16, 2022 15:25

fixed core version tests

1d30774

Merge branch 'master' into el/update-core-12-5-0

41a3bc3

# Conflicts: # packages/cinterop/src/darwin/kotlin/io/realm/kotlin/internal/interop/RealmInterop.kt

clementetb approved these changes Aug 17, 2022

View reviewed changes

edualonso merged commit 03f1b9e into master Aug 18, 2022

edualonso deleted the el/update-core-12-5-0 branch August 18, 2022 14:24

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to Core 12.5.1 #967

Update to Core 12.5.1 #967

edualonso commented Aug 11, 2022 •

edited

Loading

rorbech left a comment

rorbech Aug 12, 2022

edualonso Aug 12, 2022

rorbech Aug 12, 2022

cmelchior Aug 12, 2022

cmelchior Aug 12, 2022

edualonso Aug 12, 2022 •

edited

Loading

cmelchior Aug 12, 2022

edualonso Aug 12, 2022

cmelchior Aug 12, 2022

edualonso Aug 13, 2022 •

edited

Loading

cmelchior Aug 12, 2022

cmelchior Aug 12, 2022

edualonso Aug 12, 2022

cmelchior Aug 12, 2022

edualonso Aug 12, 2022

cmelchior Aug 12, 2022

edualonso Aug 12, 2022

cmelchior left a comment

cmelchior Aug 12, 2022

edualonso Aug 12, 2022

edualonso Aug 12, 2022

edualonso Aug 15, 2022

edualonso Aug 15, 2022 •

edited

Loading

clementetb Aug 17, 2022

clementetb left a comment

clementetb Aug 17, 2022 •

edited

Loading

cmelchior Aug 18, 2022

edualonso commented Aug 18, 2022

Update to Core 12.5.1 #967

Update to Core 12.5.1 #967

Conversation

edualonso commented Aug 11, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edualonso Aug 12, 2022 • edited Loading

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

edualonso Aug 13, 2022 • edited Loading

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

cmelchior 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

edualonso Aug 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clementetb left a comment

Choose a reason for hiding this comment

clementetb Aug 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edualonso commented Aug 18, 2022

edualonso commented Aug 11, 2022 •

edited

Loading

edualonso Aug 12, 2022 •

edited

Loading

edualonso Aug 13, 2022 •

edited

Loading

edualonso Aug 15, 2022 •

edited

Loading

clementetb Aug 17, 2022 •

edited

Loading