Ensure clearWith lambda is deleting all the list item

[bmarty]

Else we will get an infinite loop. This specific error will help to figure out what is happening.

Tested locally with a Realm schema upgrade:

internal class MigrateSessionTo032(realm: DynamicRealm) : RealmMigrator(realm, 32) {

    override fun doMigrate(realm: DynamicRealm) {
        val chunks = realm.where("ChunkEntity")
                .findAll()

        chunks.forEach { chunk ->
            chunk.getList(ChunkEntityFields.TIMELINE_EVENTS.`$`).clearWith {
                // NO OP, let the app crash!
            }
        }
    }
}

And got this crash with stacktrace in log:

java.lang.RuntimeException: Unable to create application im.vector.app.VectorApplication: java.lang.IllegalStateException: `clearWith` MUST delete all elements of the RealmList
        at android.app.ActivityThread.handleBindApplication(ActivityThread.java:6465)
        at android.app.ActivityThread.access$1300(ActivityThread.java:219)
        at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1859)
        at android.os.Handler.dispatchMessage(Handler.java:107)
        at android.os.Looper.loop(Looper.java:214)
        at android.app.ActivityThread.main(ActivityThread.java:7356)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:492)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:930)
     Caused by: java.lang.IllegalStateException: `clearWith` MUST delete all elements of the RealmList
        at org.matrix.android.sdk.internal.extensions.RealmExtensionsKt.clearWith(RealmExtensions.kt:36)
        at org.matrix.android.sdk.internal.database.migration.MigrateSessionTo032.doMigrate(MigrateSessionTo032.kt:31)
        ...

With this code:

            chunk.getList(ChunkEntityFields.TIMELINE_EVENTS.`$`).clearWith {
                it.deleteFromRealm()
            }

No crash is observed and the DB is properly migrated.

[ouchadam]

should this be previousSize == size? (to highlight no deletion has occurred)

[bmarty]

I see your point, but I expect previousSize == size + 1 to be the only valid condition, so I think it's better like that.

We may have lambda which add object(s) to the list, or delete multiple objects, we live in a crazy world!

This will be possible to delete another item than the provide one in the lambda, for instance the last one, but at the end this will have the same result.

Another solution to avoid that would have to change the Extension to:

internal fun <T : RealmObject /* <- Needed contract to be able to use `isValid` */> RealmList<T>.clearWith(delete: (T) -> Unit) {
    while (!isEmpty()) {
        first()?.let {
            delete.invoke(it)
            if (it.isValid) {
                // Object has not been deleted
                error("`clearWith` MUST delete all elements of the RealmList")
            }
        }
    }
}

or even just delete it with a warning:

internal fun <T : RealmObject /* <- Needed contract to be able to use `isValid` */> RealmList<T>.clearWith(delete: (T) -> Unit) {
    while (!isEmpty()) {
        first()?.let {
            delete.invoke(it)
            if (it.isValid) {
                // Object has not been deleted
                Timber.w("`clearWith` MUST delete all elements of the RealmList")
                it.deleteFromRealm()
            }
        }
    }
}

[ouchadam]

thanks for explaining 👍 if possible I'd prefer for us to avoid the while entirely as that seems to be the root of our problem

fun RealmList<T>.clearWith(delete: (T) -> Unit) {
  map { item ->
    { delete(item) }
  }.forEach { it.invoke() }
}

[mnaturel]

I would add that just with the signature of the method, we are not sure the provided lambda will be a deleting action, we could pass any action on a type T that just do nothing.

[bmarty]

I do not think we can use map, because the RealmList is getting empty while we call deleteFromRealm() on its items. This is not really Kotlin logic.
I will add some Kdoc on the fun.

[bmarty]

Oh i understand your code with map better now, have read too fast. It may work. Let me give it a try.

[bmarty]

@ouchadam @mnaturel , I have updated the PR, and tested again dummy migration, it behaves at expected.

[bmarty]

inspired from https://github.com/vector-im/element-android/blob/develop/vector/src/main/java/im/vector/app/core/error/fatal.kt#L24

[mnaturel]

Could we use isNotEmpty() to improve readability?

[bmarty]

e53dd1e

[mnaturel]

Thanks!

[ouchadam]

thanks for the update!

[sonarcloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication