Fix crashes on unmapped error codes #951

clementetb · 2022-08-04T16:19:16Z

The SDK would crash if core throws an exception that is not mapped by the C-API.

This PR fixes this situation by handling these unmapped error codes into proper sync or app exceptions.

cmelchior

Nice, only minor comments + we also need a CHANGELOG update.

cmelchior · 2022-08-05T11:03:17Z

packages/library-base/src/commonMain/kotlin/io/realm/kotlin/exceptions/RealmException.kt

-    public constructor(message: String) : super(message)
-    public constructor(message: String, cause: Throwable) : super(message, cause)
-    public constructor(cause: Throwable) : super(cause)
+    public constructor(message: String?) : super(message)


It was actually on purpose I made these non-null as we should always strive to have some kind of message. I can see you use null in the safeErrorCodeMapping method, but I would consider if we could just return "" there instead since it is kind of an edge case.

cmelchior · 2022-08-05T11:13:00Z

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

    return "[$categoryDesc][$errorDesc] ${error.message}"
 }

+// Prevents exceptions caused by unmapped sync errors from propagating and causing a crash. The exception
+// message describes the missing error code and its category.
+internal fun <T> safeErrorCodeMapping(onException: (message: String?) -> T, block: () -> T): T =


Nice way doing this 👍

cmelchior · 2022-08-05T11:14:02Z

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

-                ProtocolConnectionErrorCode.RLM_SYNC_ERR_CONNECTION_SWITCH_TO_PBS -> { // Connected with wrong wire protocol - should switch to PBS
-                    WrongSyncTypeException(message)
+    return safeErrorCodeMapping(onException = { SyncException(it) }) {
+        when (error.category) {


Probably not worth fixing in this PR, but we would still crash if we suddenly see a category we haven't seen before. It would crash here though, but inside RealmInterop. Maybe just create an issue for it for now?

rorbech

Maybe I just don't get the overall layers of mapping, but the tests looks more like a unittest of the AppErrorCode/SyncErrorCode-constructor than a test verifying that we actually can handle unmapped error codes. Don't know if it possible for Sync errors, but for App-errors we could probably inject a mock HTTP-client that issued unknown error codes. Don't know if it is worth considering for this PR compared to getting the fix out though 🤔 ... has it been verified manually?

# Conflicts: # CHANGELOG.md

edualonso

Only tiny things, otherwise looks good 💯

edualonso · 2022-08-10T11:14:50Z

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

@@ -63,7 +61,7 @@ internal fun convertSyncErrorCode(error: SyncErrorCode): SyncException {
            // See https://github.com/realm/realm-core/blob/master/src/realm/sync/protocol.hpp#L200
            // Use https://docs.google.com/spreadsheets/d/1SmiRxhFpD1XojqCKC-xAjjV-LKa9azeeWHg-zgr07lE/edit
            // as guide for how to categorize Connection type errors.
-            when (ProtocolConnectionErrorCode.fromInt(error.value)) {
+            when (error.error) {


Redundant naming?

Not really, but we can rename the variable error: SyncErrorCode into syncErrorCode: SyncErrorCode for readability.

edualonso · 2022-08-10T11:15:46Z

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

+        SyncErrorCodeCategory.RLM_SYNC_ERROR_CATEGORY_UNKNOWN,
+        -> {


Weird indenting?

Helps add new categories.

edualonso · 2022-08-10T11:17:06Z

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

+        if (errorCodeDesc == null)
+            error.errorCode.toString()
+        else
+            "$errorCodeDesc(${error.errorCode})"


Leave it as a one liner or write braces for the if

cmelchior

Only minor comments 🚀

cmelchior · 2022-08-12T10:02:57Z

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

@@ -16,18 +16,55 @@

 package io.realm.kotlin.internal.interop.sync

+import kotlin.jvm.JvmStatic
+
+interface ErrorCode {


Maybe add a description of what this interface is for? Especially since we also have an errorCode: Int in AppError

cmelchior · 2022-08-12T10:08:11Z

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

-    val category: AppErrorCategory,
+data class AppError internal constructor(
+    val category: AppErrorCategory?,
+    val error: ErrorCode?,


It is a bit confusing that we have both error: ErrorCode and errorCode: Int. Should we either combine them or rename one of them?

Yes, I got the same feeling, but they map core structures, I will try to get better naming.

cmelchior · 2022-08-12T10:10:54Z

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

-                    return value
-                }
+        internal actual fun of(nativeValue: Int): AppErrorCategory? =
+            values().first { value ->


Shouldn't this be firstOrNull otherwise I think it will throw?

cmelchior · 2022-08-12T10:11:49Z

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

-                    return value
-                }
+        internal actual fun of(nativeValue: Int): ClientErrorCode? =
+            values().first { value ->


cmelchior · 2022-08-12T10:16:06Z

...es/cinterop/src/darwin/kotlin/io/realm/kotlin/internal/interop/sync/SyncErrorCodeCategory.kt

-                    return value
-                }
+        internal actual fun of(nativeValue: Int): SyncErrorCodeCategory? =
+            values().first { value ->


cmelchior · 2022-08-12T10:16:56Z

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

-                    return value
-                }
+        internal actual fun of(nativeValue: Int): ProtocolConnectionErrorCode? =
+            values().first { value ->


cmelchior · 2022-08-12T10:17:00Z

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

-                    return value
-                }
+        internal actual fun of(nativeValue: Int): ProtocolSessionErrorCode? =
+            values().first { value ->


cmelchior · 2022-08-12T10:17:05Z

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

-                    return value
-                }
+        internal actual fun of(nativeValue: Int): ServiceErrorCode? =
+            values().first { value ->


cmelchior · 2022-08-12T10:17:10Z

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

-                    return value
-                }
+        internal actual fun of(nativeValue: Int): SyncErrorCodeCategory? =
+            values().first { value ->


cmelchior · 2022-08-12T10:20:05Z

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

+    // server log.
+    val msg = error.message?.let { message: String ->
+        if (message.endsWith(".")) {
+            message


This doesn't add a space before it so I suspect this will make the error a little more unreadable, e.g [Category][ErrorCode]Message. rather than [Category][ErrorCode] Message.

# Conflicts: # CHANGELOG.md

Treat unmapped error codes as app and sync exceptions

de79eb2

clementetb marked this pull request as ready for review August 4, 2022 16:19

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

clementetb self-assigned this Aug 4, 2022

clementetb requested review from cmelchior and rorbech August 4, 2022 16:19

Fix import

36f064b

cmelchior approved these changes Aug 5, 2022

View reviewed changes

rorbech reviewed Aug 5, 2022

View reviewed changes

clementetb added 4 commits August 5, 2022 13:38

PR change requests

cefdc98

Add safe line on categories mapping

fd153eb

Add changelog entry

ba6ea3c

Fix wrong SyncError constructor calls

a92ca9c

clementetb force-pushed the ct/unmapped-error-codes branch from 3ddf15b to a92ca9c Compare August 8, 2022 10:10

clementetb added 10 commits August 8, 2022 12:10

Add accessors to native values

2a3bad2

clean test cases

8b5bc4f

Refactor exception mapping

911c80b

add error instantiation

26a42c4

Clean up

f3d2e12

Update test cases

819fd47

Further clean up

1a7d7fb

Update expected error messages

a7c0558

Merge branch 'releases' into ct/unmapped-error-codes

45d5293

# Conflicts: # CHANGELOG.md

Changelog

b7942ac

clementetb requested review from cmelchior and rorbech August 9, 2022 11:30

edualonso approved these changes Aug 10, 2022

View reviewed changes

PR change requests

ea3e663

cmelchior approved these changes Aug 12, 2022

View reviewed changes

PR change requests

1ce8414

clementetb added 5 commits August 12, 2022 15:48

Migrate app errors to have a common code descriptor

302bc68

Migrate sync errors to use codedescriptor

37039e1

Clean up

63d388f

Use unique CodeDescription

f466f46

Merge branch 'releases' into ct/unmapped-error-codes

8fb6429

# Conflicts: # CHANGELOG.md

clementetb merged commit 2391281 into releases Aug 19, 2022

clementetb deleted the ct/unmapped-error-codes branch August 19, 2022 15:39

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

Fix crashes on unmapped error codes #951

Fix crashes on unmapped error codes #951

clementetb commented Aug 4, 2022

cmelchior left a comment

cmelchior Aug 5, 2022

cmelchior Aug 5, 2022

cmelchior Aug 5, 2022

rorbech left a comment

edualonso left a comment

edualonso Aug 10, 2022

clementetb Aug 10, 2022

edualonso Aug 10, 2022

clementetb Aug 10, 2022

edualonso Aug 10, 2022

cmelchior left a comment

cmelchior Aug 12, 2022

cmelchior Aug 12, 2022

clementetb Aug 12, 2022

cmelchior Aug 12, 2022

clementetb Aug 12, 2022

cmelchior Aug 12, 2022

cmelchior Aug 12, 2022

cmelchior Aug 12, 2022

cmelchior Aug 12, 2022

cmelchior Aug 12, 2022

cmelchior Aug 12, 2022

cmelchior Aug 12, 2022

Fix crashes on unmapped error codes #951

Fix crashes on unmapped error codes #951

Conversation

clementetb commented Aug 4, 2022

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

rorbech left a comment

Choose a reason for hiding this comment

edualonso 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

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

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