Better edit (replace handling) #7594

BillCarsonFr · 2022-11-16T09:39:09Z

Type of change

Feature
Bugfix
Technical
Other :

Content

This PR is improving the handling of Edits, and enforcing edit validation rules.

Edit validation: see EventEditValidator ensures the rules as per spec (recently updated)
Added proper unit tests
Persistence layer changes:
- Session data migration needed (v43)
- We now store a reference of the edit event instead of the content to allow more flexibility when validated event; lastEdit instead of LastContent (as sender/types are also needed for validation)
- Late decryption supported with revalidation of edit chain
Fixed a problem in Aggregator with process calls beeing suspend, which is a problem as it's called within a realm transaction and realm instance is locked to a thread. All asyncTransaction helpers have been modified to make the blockparam not suspendable

Handling of server side aggregation is still not handled as there is ongoing discussions regarding that

Motivation and context

#7430

Screenshots / GIFs

N/A

Tests

Try some edits, polls, location sharing

Tested devices

Physical
Emulator
OS version(s):

Checklist

Changes has been tested on an Android device or Android emulator with API 21
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
Pull request includes a new file under ./changelog.d. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#changelog
Pull request includes screenshots or videos if containing UI changes
Pull request includes a sign off
You've made a self review of your PR
If you have modified the screen flow, or added new screens to the application, you have updated the test UiAllScreensSanityTest.allScreensTest()

mnaturel · 2022-11-21T08:57:35Z

...android/src/main/java/org/matrix/android/sdk/api/session/events/model/AggregatedRelations.kt

@@ -50,5 +50,6 @@ import com.squareup.moshi.JsonClass
 data class AggregatedRelations(
        @Json(name = "m.annotation") val annotations: AggregatedAnnotation? = null,
        @Json(name = "m.reference") val references: DefaultUnsignedRelationInfo? = null,
+        @Json(name = "m.replace") val replaces: AggregatedReplace? = null,


I think we may need to update the related Kdoc. And it seems the preview of this doc in the IDE is not working, the <code></code> seems to not be well rendered.

fixed, should use ```

mnaturel · 2022-11-21T09:10:54Z

...android/src/main/java/org/matrix/android/sdk/api/session/events/model/ValidDecryptedEvent.kt

+        val algorithm: String,
+)
+
+fun Event.toValidDecryptedEvent(): ValidDecryptedEvent? {


Maybe this method should be in EventExt file instead?

mnaturel · 2022-11-21T09:11:51Z

...android/src/main/java/org/matrix/android/sdk/api/session/events/model/ValidDecryptedEvent.kt

+        val algorithm: String,
+)
+
+fun Event.toValidDecryptedEvent(): ValidDecryptedEvent? {


I think we should try to add unit tests on this method.

mnaturel · 2022-11-21T09:18:13Z

...-sdk-android/src/main/java/org/matrix/android/sdk/api/session/room/timeline/TimelineEvent.kt

-        in EventType.BEACON_LOCATION_DATA -> (annotations?.editSummary?.latestContent ?: root.getClearContent()).toModel<MessageBeaconLocationDataContent>()
-        else -> (annotations?.editSummary?.latestContent ?: root.getClearContent()).toModel()
+        // XXX
+        // Polls/Beacon are not message contents like others as there is no msgtype subtype to discriminate moshi parsing


Thanks for adding this description comment! Indeed, when adding the implementation for beacon events, I was concerned about that. But I couldn't find any other solution since the timeline mecanism seems to rely a lot on MessageContent... Do you know if there is any example of events like these which are correctly implemented?

I think that they should not be at all MessageContent from the beginning. could use MessageRelationContent though?
Where is it causing problem?

mnaturel · 2022-11-21T09:35:35Z

...c/main/java/org/matrix/android/sdk/internal/database/mapper/EventAnnotationsSummaryMapper.kt

@@ -36,13 +37,22 @@ internal object EventAnnotationsSummaryMapper {
                    )
                },
                editSummary = annotationsSummary.editSummary
-                        ?.let {
-                            val latestEdition = it.editions.maxByOrNull { editionOfEvent -> editionOfEvent.timestamp } ?: return@let null
+                        ?.let { summary ->


I think we should take the occasion to introduce a dedicated EditAggregatedSummaryMapper (as a classical class and not an Object). We could then add unit tests on it.

Extracted as object and added tests

mnaturel · 2022-11-21T09:44:05Z

...c/main/java/org/matrix/android/sdk/internal/database/mapper/EventAnnotationsSummaryMapper.kt

+                             * if two or more replacement events have identical origin_server_ts,
+                             * the event with the lexicographically largest event_id is treated as more recent.
+                             */
+                            val latestEdition = summary.editions.sortedWith(compareBy<EditionOfEvent> { it.timestamp }.thenBy { it.eventId })


I am not sure the implementation is correct. I think the natural order of sorting will be the increasing order and here we would like decreasing order so that we get the most recent first. And I am also not sure about the default sorting for strings.
This is one of the reasons, I strongly think we need unit tests to validate the implementation.

What do you think?

added tests

mnaturel · 2022-11-21T09:45:47Z

...c/main/java/org/matrix/android/sdk/internal/database/mapper/EventAnnotationsSummaryMapper.kt

+                             */
+                            val latestEdition = summary.editions.sortedWith(compareBy<EditionOfEvent> { it.timestamp }.thenBy { it.eventId })
+                                    .lastOrNull() ?: return@let null
+                            // get the event and validate?


Why is there this comment? Is there a validation step missing?

no should be validated before, or for late decryption after

mnaturel · 2022-11-21T12:45:34Z

...roid/src/main/java/org/matrix/android/sdk/internal/database/migration/MigrateSessionTo008.kt

@@ -25,11 +25,11 @@ internal class MigrateSessionTo008(realm: DynamicRealm) : RealmMigrator(realm, 8

    override fun doMigrate(realm: DynamicRealm) {
        val editionOfEventSchema = realm.schema.create("EditionOfEvent")
-                .addField(EditionOfEventFields.CONTENT, String::class.java)
+                .addField("content"/**EditionOfEventFields.CONTENT*/, String::class.java)


Maybe we should add a comment about this commented code? I guess you wanted to keep the previous fields in comments for future reference?

Kept it to show that it was from an added then deleted class, ok to remove though

mnaturel · 2022-11-21T13:05:42Z

vector/src/main/java/im/vector/app/core/extensions/TimelineEvent.kt

@@ -40,7 +41,8 @@ fun TimelineEvent.getVectorLastMessageContent(): MessageContent? {
    // Iterate on event types which are not part of the matrix sdk, otherwise fallback to the sdk method
    return when (root.getClearType()) {
        VoiceBroadcastConstants.STATE_ROOM_VOICE_BROADCAST_INFO -> {
-            (annotations?.editSummary?.latestContent ?: root.getClearContent()).toModel<MessageVoiceBroadcastInfoContent>()
+            (annotations?.editSummary?.latestEdit?.getClearContent()?.toModel<MessageContent>().toContent().toModel<MessageVoiceBroadcastInfoContent>()


I wonder if we could replace this by:
getLastEditNewContent().toModel<MessageVoiceBroadcastInfoContent>()? The difference is that in this case we would use the newContent field instead of toContent() extension method.

mnaturel · 2022-11-21T13:12:35Z

...ava/im/vector/app/features/home/room/detail/timeline/helper/MessageInformationDataFactory.kt

@@ -55,7 +57,7 @@ class MessageInformationDataFactory @Inject constructor(
        private val reactionsSummaryFactory: ReactionsSummaryFactory
 ) {

-    fun create(params: TimelineItemFactoryParams): MessageInformationData {
+    fun create(params: TimelineItemFactoryParams, lastEdit: Event? = null): MessageInformationData {


I am not sure it is needed to add the lastEdit as a new argument. Since it is already contained in the first argument, I think we could retrieve it easily inside this method.

mnaturel · 2022-11-21T13:17:57Z

matrix-sdk-android/src/test/java/org/matrix/android/sdk/test/fakes/FakeRealmConfiguration.kt

-        val transaction = slot<suspend (Realm) -> T>()
-        coEvery { awaitTransaction(instance, capture(transaction)) } coAnswers {
-            secondArg<suspend (Realm) -> T>().invoke(realm)
+        val transaction = slot<(Realm) -> T>()


I realized the capture of the second argument is in fact not necessary when mocking the answer, so we could simplify this by:

fun <T> givenAwaitTransaction(realm: Realm) { coEvery { awaitTransaction(instance, any<(Realm) -> T>()) } answers { secondArg<(Realm) -> T>().invoke(realm) } }

mnaturel · 2022-11-21T13:25:05Z

...n/java/im/vector/app/features/home/room/detail/timeline/factory/TimelineItemFactoryParams.kt

 import org.matrix.android.sdk.api.session.room.timeline.TimelineEvent

 data class TimelineItemFactoryParams(
        val event: TimelineEvent,
+        val lastEdit: Event? = null,


This field is not used: maybe it should be used inside MessageInformationDataFactory.create()?

it's used now

mnaturel · 2022-11-21T13:28:56Z

...main/java/org/matrix/android/sdk/internal/session/room/EventRelationsAggregationProcessor.kt

        private val clock: Clock,
 ) : EventInsertLiveProcessor {

    private val allowedTypes = listOf(
            EventType.MESSAGE,
            EventType.REDACTION,
            EventType.REACTION,
+            // The aggregator handles verification events but just to render tiles in the timeline
+            // It's not participating in verfication it's self, just timeline display
            EventType.KEY_VERIFICATION_DONE,
            EventType.KEY_VERIFICATION_CANCEL,
            EventType.KEY_VERIFICATION_ACCEPT,
            EventType.KEY_VERIFICATION_START,
            EventType.KEY_VERIFICATION_MAC,
            // TODO Add ?


Should we remove the // TODO Add ? since it seems to be done now?

mnaturel · 2022-11-21T13:29:42Z

...main/java/org/matrix/android/sdk/internal/session/room/EventRelationsAggregationProcessor.kt

        private val clock: Clock,
 ) : EventInsertLiveProcessor {

    private val allowedTypes = listOf(
            EventType.MESSAGE,
            EventType.REDACTION,
            EventType.REACTION,
+            // The aggregator handles verification events but just to render tiles in the timeline
+            // It's not participating in verfication it's self, just timeline display


Suggested change

// It's not participating in verfication it's self, just timeline display

// It's not participating in verification itself, just timeline display

mnaturel · 2022-11-21T15:12:15Z

...main/java/org/matrix/android/sdk/internal/session/room/EventRelationsAggregationProcessor.kt

@@ -334,9 +309,8 @@ internal class EventRelationsAggregationProcessor @Inject constructor(
                Timber.v("###REPLACE Computing aggregated edit summary (isLocalEcho:$isLocalEcho)")
                existingSummary.editions.add(
                        EditionOfEvent(
-                                senderId = event.senderId ?: "",
                                eventId = event.eventId,


Suggested change

eventId = event.eventId,

eventId = eventId,

mnaturel · 2022-11-21T15:13:18Z

...main/java/org/matrix/android/sdk/internal/session/room/EventRelationsAggregationProcessor.kt

@@ -369,8 +343,7 @@ internal class EventRelationsAggregationProcessor @Inject constructor(
     * @param editions list of edition of event
     */
    private fun handleThreadSummaryEdition(
-            editedEvent: EventEntity?,
-            replaceEvent: TimelineEventEntity?,
+            editedEvent: EventEntity?, replaceEvent: TimelineEventEntity?,


Maybe it is better to have one argument per line for consistency?

mnaturel

I have posted some comments concerning some implementation details that should be investigated I guess.

After some tests, here the issues I found:

in non encrypted room, the last edited content is not rendered in the timeline for a text message for example. This is always the original content which is rendered
in encrypted room, not really sure but I think we might no more render the local echo of the edited content in the timeline as I have seen some short delay before rendering (it may wait for the remote content before rendering).

I have not tested the update of the app for migration of the DB. If not already done, I think the migration should be tested before merge.

mnaturel · 2022-11-24T08:48:06Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/events/model/EventExt.kt

+ * limitations under the License.
+ */
+
+package org.matrix.android.sdk.api.session.events.model


Were you aware there was an existing EventExt in the org.matrix.android.sdk.internal.session.events package?

Yes but it's internal, and toValidate is API

Okay I see. Thanks for explaining.

mnaturel · 2022-11-24T08:51:10Z

...droid/src/test/java/org/matrix/android/sdk/internal/session/event/ValidDecryptedEventTest.kt

+
+class ValidDecryptedEventTest {
+
+    val fakeEvent = Event(


I think we should declare the variable as private.

mnaturel · 2022-11-24T08:51:27Z

...droid/src/test/java/org/matrix/android/sdk/internal/session/event/ValidDecryptedEventTest.kt

+import org.matrix.android.sdk.api.session.events.model.toValidDecryptedEvent
+import org.matrix.android.sdk.api.session.room.model.message.MessageTextContent
+
+class ValidDecryptedEventTest {


Thanks for adding these tests!

mnaturel · 2022-11-24T08:56:17Z

...est/java/org/matrix/android/sdk/internal/database/mapper/EditAggregationSummaryMapperTest.kt

+import org.matrix.android.sdk.internal.database.model.EditionOfEvent
+import org.matrix.android.sdk.internal.database.model.EventEntity
+
+class EditAggregationSummaryMapperTest {


Could we rename it EditAggregatedSummaryEntityMapperTest? It took me while to find the corresponding tests and IDE does not recognize the associations between the class and the test if naming is not aligned.

mnaturel · 2022-11-24T09:00:50Z

...est/java/org/matrix/android/sdk/internal/database/mapper/EditAggregationSummaryMapperTest.kt

+class EditAggregationSummaryMapperTest {
+
+    @Test
+    fun test() {


Should we add a test when we have same timestamp for 2 EditionOfEvent to check we take the correct latest event in this situation?

good idea, added

mnaturel

Thanks for the updates. I didn't expect android tests (was thinking about manual tests) on database migration but that's awesome we know have tests on this.

I have just added some small new comments. I wonder also you have seen some of my previous comments (and particularly the ones about EventEditValidatorTest)?

bmarty · 2022-11-24T11:04:49Z

I didn't expect android tests (was thinking about manual tests) on database migration but that's awesome we now have tests on this.

Yes, that's nice, @BillCarsonFr can you explain somewhere how to create files like session_42.realm

BillCarsonFr · 2022-11-24T11:52:39Z

I didn't expect android tests (was thinking about manual tests) on database migration but that's awesome we now have tests on this.

Yes, that's nice, @BillCarsonFr can you explain somewhere how to create files like session_42.realm

I launched the app in the emulator (with LOG_PRIVATE_DATA to true).
I added some messages to add the entities needed to test my migration.
Then I used the Device File Explorer to export the database data/data/im.vector.app.debug/files/<hash>/disk_store.realm and save it on my disk
Then I took the key from logs searching for Database key for alias XXX

And I added that to test assets.

We should create a task to do that for crypto and auth (and probably a better one for session)
The idea would be that it has objects for each table in the base to better test the .transform() on dynamic realm

BillCarsonFr · 2022-11-24T11:53:24Z

Thanks for the updates. I didn't expect android tests (was thinking about manual tests) on database migration but that's awesome we know have tests on this.

I have just added some small new comments. I wonder also you have seen some of my previous comments (and particularly the ones about EventEditValidatorTest)?

I want to handle them in another PR, to rationalize test on IMXCryptoStore and have a bunch of fake events in test.fixtures

sonarcloud · 2022-11-24T12:32:55Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

35.9% Coverage
0.0% Duplication

mnaturel

LGTM.

BillCarsonFr requested review from a team and jonnyandrew and removed request for a team November 16, 2022 09:40

BillCarsonFr force-pushed the feature/bca/better_edit_validation branch 2 times, most recently from fe947ca to 6d56967 Compare November 16, 2022 13:34

BillCarsonFr requested review from a team, ganfra and mnaturel and removed request for jonnyandrew, a team and ganfra November 18, 2022 10:09

bmarty changed the title ~~Better edit (replace handnling)~~ Better edit (replace handling) Nov 18, 2022

mnaturel reviewed Nov 21, 2022

View reviewed changes

mnaturel suggested changes Nov 21, 2022

View reviewed changes

BillCarsonFr force-pushed the feature/bca/better_edit_validation branch from 0feb4ed to 7eb595b Compare November 23, 2022 18:08

BillCarsonFr requested a review from mnaturel November 24, 2022 07:22

mnaturel reviewed Nov 24, 2022

View reviewed changes

BillCarsonFr added 10 commits November 24, 2022 12:45

better replace handling

891709e

Add changelog, some cleaning

e66a054

Fix test

3746ede

Fix broken polls states

8b47bf0

fix fake awaitTx

d759f26

code review

e5d3206

fix edit display flicker with local echo

2819957

kdoc fix

ca907df

Migration test and cleaning

c06eca6

code review

bec8b5f

BillCarsonFr force-pushed the feature/bca/better_edit_validation branch from cd497b2 to bec8b5f Compare November 24, 2022 11:45

BillCarsonFr requested a review from mnaturel November 24, 2022 11:53

mnaturel approved these changes Nov 24, 2022

View reviewed changes

BillCarsonFr merged commit 035b1eb into develop Nov 24, 2022

BillCarsonFr deleted the feature/bca/better_edit_validation branch November 24, 2022 16:22

	// It's not participating in verfication it's self, just timeline display
	// It's not participating in verification itself, just timeline display

Better edit (replace handling) #7594

Better edit (replace handling) #7594

Conversation

BillCarsonFr commented Nov 16, 2022 • edited Loading

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

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

mnaturel Nov 21, 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

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

mnaturel Nov 21, 2022 • edited Loading

Choose a reason for hiding this comment

BillCarsonFr Nov 23, 2022 • edited Loading

Choose a reason for hiding this comment

mnaturel left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnaturel left a comment • edited Loading

Choose a reason for hiding this comment

bmarty commented Nov 24, 2022

BillCarsonFr commented Nov 24, 2022

BillCarsonFr commented Nov 24, 2022

sonarcloud bot commented Nov 24, 2022

mnaturel left a comment

Choose a reason for hiding this comment

BillCarsonFr commented Nov 16, 2022 •

edited

Loading

mnaturel Nov 21, 2022 •

edited

Loading

mnaturel Nov 21, 2022 •

edited

Loading

BillCarsonFr Nov 23, 2022 •

edited

Loading

mnaturel left a comment •

edited

Loading

mnaturel left a comment •

edited

Loading