Handle account data removal (PSG-865, PSG-867) #7740

onurays · 2022-12-07T16:00:34Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Synapse will start to support deleting account data events. If a client removes an account data event then it will be reported in sync response with an empty content {}. This PR adds delete endpoints (will be used in next PR) and also handles them in the sync response.

Motivation and context

Implement MSC-3391

Screenshots / GIFs

Tests

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

ElementBot · 2022-12-07T16:03:12Z

	Warnings
⚠️	Please add a changelog. See instructions here

Generated by 🚫 dangerJS against f4429d4

ElementBot · 2022-12-07T18:44:20Z

	Warnings
⚠️	Please add a changelog. See instructions here

Generated by 🚫 dangerJS against 055bf6d

bmarty

Thanks. Some feedback. OOI in the context of which feature are you implementing this MSC?

bmarty · 2022-12-08T13:20:41Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/RoomAPI.kt

+     * @param roomId the room id
+     * @param type the type
+     */
+    @DELETE(NetworkConstants.URI_API_PREFIX_PATH_MEDIA_PROXY_UNSTABLE + "org.matrix.msc3391/user/{userId}/rooms/{roomId}/account_data/{type}")


Error in the path:

Suggested change

@DELETE(NetworkConstants.URI_API_PREFIX_PATH_MEDIA_PROXY_UNSTABLE + "org.matrix.msc3391/user/{userId}/rooms/{roomId}/account_data/{type}")

@DELETE(NetworkConstants.URI_API_PREFIX_PATH_UNSTABLE + "org.matrix.msc3391/user/{userId}/rooms/{roomId}/account_data/{type}")

Ups, good catch. Done.

bmarty · 2022-12-08T13:24:38Z

...main/java/org/matrix/android/sdk/internal/session/sync/handler/UserAccountDataSyncHandler.kt

-                UserAccountDataTypes.TYPE_IGNORED_USER_LIST -> handleIgnoredUsers(realm, event)
-                UserAccountDataTypes.TYPE_BREADCRUMBS -> handleBreadcrumbs(realm, event)
+            if (event.content.isEmpty()) {
+                UserAccountDataEntity.delete(realm, event.type)


Should we do some treatment on specific Account data as per the when block in the else block below?

So I would keep the existing code, and move the removal from DB to the handleGenericAccountData() method.

Done. That was also my initial intention. We can trust all when cases check the content.

bmarty · 2022-12-08T13:27:49Z

...oid/src/main/java/org/matrix/android/sdk/internal/session/user/accountdata/AccountDataAPI.kt

@@ -36,4 +37,16 @@ internal interface AccountDataAPI {
            @Path("type") type: String,
            @Body params: Any
    )
+
+    /**
+     * Remove an account_data for the client.


Maybe a bit better, but we may found better.

Suggested change

* Remove an account_data for the client.

* Remove an account_data for the user.

I agree, done.

bmarty · 2022-12-08T13:31:28Z

...main/java/org/matrix/android/sdk/internal/session/sync/parsing/RoomSyncAccountDataHandler.kt

+                } else if (eventType == RoomAccountDataTypes.EVENT_TYPE_FULLY_READ) {
+                    val content = event.getClearContent().toModel<FullyReadContent>()
+                    roomFullyReadHandler.handle(realm, roomId, content)
+                }


Same remark about dealing with specific room account data event.

ElementBot · 2022-12-08T14:13:29Z

	Warnings
⚠️	Please add a reviewer to your PR.

Generated by 🚫 dangerJS against b09a00e

onurays · 2022-12-08T14:28:30Z

Thanks. Some feedback. OOI in the context of which feature are you implementing this MSC?

We started to store device information in account data and we need to delete it when session is logged out / deleted. Otherwise they will live in backend forever.

bmarty · 2022-12-12T09:15:59Z

...main/java/org/matrix/android/sdk/internal/session/sync/handler/UserAccountDataSyncHandler.kt

@@ -256,8 +257,14 @@ internal class UserAccountDataSyncHandler @Inject constructor(
                .equalTo(UserAccountDataEntityFields.TYPE, type)
                .findFirst()
        if (existing != null) {
-            // Update current value
-            existing.contentStr = ContentMapper.map(content)
+            if (content.isNullOrEmpty()) {


Still not good to me. If the account data does not exist yet, it will be added to the DB, and we want to remove it.

Oh, right. Please check my last commit.

bmarty · 2022-12-12T09:16:35Z

...main/java/org/matrix/android/sdk/internal/session/sync/parsing/RoomSyncAccountDataHandler.kt

@@ -58,7 +59,13 @@ internal class RoomSyncAccountDataHandler @Inject constructor(
    private fun handleGeneric(roomEntity: RoomEntity, content: JsonDict?, eventType: String) {
        val existing = roomEntity.accountData.where().equalTo(RoomAccountDataEntityFields.TYPE, eventType).findFirst()
        if (existing != null) {
-            existing.contentStr = ContentMapper.map(content)
+            if (content.isNullOrEmpty()) {


Same remark. Please reorder your if statements.

bmarty

Perfect, thanks. Maybe squash and merge to avoid noisy git history.

sonarcloud · 2022-12-12T11:46:14Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

10.8% Coverage
0.0% Duplication

Onuray Sahin added 3 commits December 7, 2022 18:12

Add api functions to delete account data.

d244f73

Add helper functions to delete user and room account data.

765202e

Handle sync response to delete user and room account data.

f4429d4

Revert unused companion object.

055bf6d

Add changelog.

d6c2022

onurays requested review from a team and bmarty and removed request for a team December 8, 2022 10:47

onurays marked this pull request as ready for review December 8, 2022 10:51

bmarty requested changes Dec 8, 2022

View reviewed changes

Code review fixes.

b09a00e

onurays requested a review from bmarty December 8, 2022 14:14

bmarty requested changes Dec 12, 2022

View reviewed changes

Code review fixes.

746fb77

onurays requested a review from bmarty December 12, 2022 10:41

bmarty approved these changes Dec 12, 2022

View reviewed changes

onurays merged commit 9954045 into develop Dec 12, 2022

onurays deleted the feature/ons/remove_account_data branch December 12, 2022 16:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle account data removal (PSG-865, PSG-867) #7740

Handle account data removal (PSG-865, PSG-867) #7740

onurays commented Dec 7, 2022 •

edited

ElementBot commented Dec 7, 2022

ElementBot commented Dec 7, 2022

bmarty left a comment

bmarty Dec 8, 2022

onurays Dec 8, 2022

bmarty Dec 8, 2022

onurays Dec 8, 2022

bmarty Dec 8, 2022

onurays Dec 8, 2022

bmarty Dec 8, 2022

onurays Dec 8, 2022

ElementBot commented Dec 8, 2022

onurays commented Dec 8, 2022

bmarty Dec 12, 2022

onurays Dec 12, 2022

bmarty Dec 12, 2022 •

edited

onurays Dec 12, 2022

bmarty left a comment

sonarcloud bot commented Dec 12, 2022

	@DELETE(NetworkConstants.URI_API_PREFIX_PATH_MEDIA_PROXY_UNSTABLE + "org.matrix.msc3391/user/{userId}/rooms/{roomId}/account_data/{type}")
	@DELETE(NetworkConstants.URI_API_PREFIX_PATH_UNSTABLE + "org.matrix.msc3391/user/{userId}/rooms/{roomId}/account_data/{type}")

	* Remove an account_data for the client.
	* Remove an account_data for the user.

Handle account data removal (PSG-865, PSG-867) #7740

Handle account data removal (PSG-865, PSG-867) #7740

Conversation

onurays commented Dec 7, 2022 • edited

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

ElementBot commented Dec 7, 2022

ElementBot commented Dec 7, 2022

bmarty 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

ElementBot commented Dec 8, 2022

onurays commented Dec 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty Dec 12, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Dec 12, 2022

onurays commented Dec 7, 2022 •

edited

bmarty Dec 12, 2022 •

edited