Ensure clearWith lambda is deleting all the list item #6392
Conversation
…et an infinite loop. This specific error will help to figure out what is happening.
bmarty
requested review from
a team,
mnaturel and
fedrunov
and removed request for
a team
| first()?.let { delete.invoke(it) } | ||
| if (previousSize != size + 1) { |
There was a problem hiding this comment.
should this be previousSize == size? (to highlight no deletion has occurred)
There was a problem hiding this comment.
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()
}
}
}
}There was a problem hiding this comment.
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() }
}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Oh i understand your code with map better now, have read too fast. It may work. Let me give it a try.
| /** | ||
| * throw in debug, only log in production. As this method does not always throw, next statement should be a return | ||
| */ | ||
| internal fun fatalError(message: String) { |
There was a problem hiding this comment.
bmarty
force-pushed
the
feature/bma/safe_clearWith
branch
from
e4376bd to
a0025bc
Compare
| lambda.invoke() | ||
| } | ||
|
|
||
| if (!isEmpty()) { |
There was a problem hiding this comment.
Could we use isNotEmpty() to improve readability?
|
SonarCloud Quality Gate failed. |
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:
And got this crash with stacktrace in log:
With this code:
No crash is observed and the DB is properly migrated.