Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure clearWith lambda is deleting all the list item #6392

Merged
merged 4 commits into from
Jun 30, 2022
Merged

Ensure clearWith lambda is deleting all the list item #6392

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9866bfe
Ensure clearWith lambda is deleting all the list item, else we will g…
bmarty Jun 28, 2022
98df2d8
Changelog
bmarty Jun 28, 2022
a0025bc
Update after PR review.
bmarty Jun 29, 2022
e53dd1e
Improve readability.
bmarty Jun 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/6392.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Ensure `RealmList<T>.clearWith()` extension is correctly used.
4 changes: 4 additions & 0 deletions ...x-sdk-android/src/main/java/org/matrix/android/sdk/internal/extensions/RealmExtensions.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,11 @@ internal fun RealmObject.assertIsManaged() {
*/
internal fun <T> RealmList<T>.clearWith(delete: (T) -> Unit) {
while (!isEmpty()) {
val previousSize = size
first()?.let { delete.invoke(it) }
if (previousSize != size + 1) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@bmarty bmarty Jun 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()
            }
        }
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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() }
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

error("`clearWith` MUST delete all elements of the RealmList")
}
}
}

Expand Down