Fixing case sensitive non latin room name filtering #4352

ouchadam · 2021-10-27T12:25:47Z

Realm does not support non Latin-1 characters in sorting or filtering operations realm/realm-java#1031

In order to workaround this limitation I've added a dedicated normalized room display name field which is a lowercased raw unicode variant of the input string

Adds a normalizedDisplayName field + migration to the SessionStore which includes applying a normalized value straight away
Makes the SessionStoreMigration and QueryStringValueProcess classes so that we can inject the normalizer into them
Introduces a new Case.NORMALIZED in order to reuse the logic in other queries, other places with constraintless freeform text with insensitive search will have the same issue

BEFORE	AFTER

BEFORE	AFTER

…the content based values - allows for handling those cases separately for normalisation

… own dependencies to support normalisation

…iltering rooms by name - fixes non latin-1 character set room names from being ignored when searching with inexact casing

github-actions · 2021-10-27T12:53:34Z

Unit Test Results

  62 files +2   62 suites +2 55s ⏱️ +7s
118 tests +1 118 ✔️ +1 0 💤 ±0 0 ❌ ±0
350 runs +2 350 ✔️ +2 0 💤 ±0 0 ❌ ±0

Results for commit d344be5. ± Comparison against base commit 54daee2.

♻️ This comment has been updated with latest results.

bmarty

Thanks for this PR.
I have one concern about the Realm migration class, else this are just small remarks.

bmarty · 2021-10-27T12:33:29Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/query/QueryStringValue.kt

    }
+
+    fun isNormalized() = this is ContentQueryStringValue && case == Case.NORMALIZED


This should probably be declared as an internal extension.

internal fun QueryStringValue.isNormalized() = this is QueryStringValue.ContentQueryStringValue && case == QueryStringValue.Case.NORMALIZED

bmarty · 2021-10-27T12:33:31Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/query/QueryStringValue.kt


    enum class Case {
        SENSITIVE,
-        INSENSITIVE
+        INSENSITIVE,
+        NORMALIZED


Maybe document here the meaning and the diff of this 3 values?

bmarty · 2021-10-27T12:35:44Z

...android/src/main/java/org/matrix/android/sdk/internal/database/RealmSessionStoreMigration.kt


-internal object RealmSessionStoreMigration : RealmMigration {
+internal class RealmSessionStoreMigration @Inject constructor(


We can have problem with Realm if Migration is not an object. Migration class has to be unique. There is maybe another way to do that (as suggested in the log below).
This observed issue in the past was:

Thread: main, Exception: java.lang.IllegalArgumentException: Configurations cannot be different if used to open the same file. The most likely cause is that equals() and hashCode() are not overridden in the migration class: org.matrix.android.sdk.internal.database.RealmSessionStoreMigration

good to know! also seems like it can be caused by creating the migration class instance multiple times
https://stackoverflow.com/a/36919305

overriding the equals/hashcode seems like the safer approach

9949779

included a simple equality test as well

bmarty · 2021-10-27T12:39:42Z

...dk-android/src/main/java/org/matrix/android/sdk/internal/database/model/RoomSummaryEntity.kt

+     * or filtering by case
+     * See https://github.com/realm/realm-core/issues/777
+     */
+    var normalizedDisplayName: String? = ""


Can this be updated in the set() of displayName above? (but we will need the normalizer so maybe not possible...)
A setter taking the new displayName and the normalizer would avoid mistake in the future maybe.

I'd prefer to avoid calling the normalizer directly in the entity, keeping any logic outside~

what about changing the roomDisplayNameResolver to return a data class with both the resolved name and a normalized version?

data class RoomName(val name: String, val normalizedName: String) val roomName: RoomName = roomDisplayNameResolver.resolve(...) entity.setDisplayName(roomName) ... // entity fun setDisplayName(roomName: RoomName) { if (roomName.name != displayName) { displayName = roomName.name normalizedDisplayName = roomName.normnalizedName } }

it's close to your suggestion, it allows us to enforce the display name is always updated with a normalized name without having to pass the normalizer around everywhere
611bf29

Ok, I added a comment in the commit, not sure where it will appear here 🤷‍♂️

bmarty · 2021-10-27T12:44:51Z

...sdk-android/src/main/java/org/matrix/android/sdk/internal/query/QueryStringValueProcessor.kt

+    fun <T : RealmObject> RealmQuery<T>.process(field: String, queryStringValue: QueryStringValue): RealmQuery<T> {
+        return when (queryStringValue) {
+            is QueryStringValue.NoCondition -> {
+                Timber.v("No condition to process")


This noisy log could probably be removed :)

bmarty · 2021-10-27T12:45:51Z

...sdk-android/src/main/java/org/matrix/android/sdk/internal/query/QueryStringValueProcessor.kt

+    private fun ContentQueryStringValue.toRealmValue(): String {
+        return when (case) {
+            QueryStringValue.Case.NORMALIZED                                   -> normalizer.normalize(string)
+            QueryStringValue.Case.SENSITIVE, QueryStringValue.Case.INSENSITIVE -> string


Nit: I would prefer to have one condition per line

done 👍 7b35648

bmarty · 2021-10-27T12:46:06Z

...sdk-android/src/main/java/org/matrix/android/sdk/internal/query/QueryStringValueProcessor.kt

-        QueryStringValue.Case.INSENSITIVE -> Case.INSENSITIVE
-        QueryStringValue.Case.SENSITIVE   -> Case.SENSITIVE
+        QueryStringValue.Case.INSENSITIVE                                 -> Case.INSENSITIVE
+        QueryStringValue.Case.SENSITIVE, QueryStringValue.Case.NORMALIZED -> Case.SENSITIVE


same remark

bmarty · 2021-10-27T12:49:01Z

.../src/main/java/org/matrix/android/sdk/internal/session/room/summary/RoomSummaryDataSource.kt

-        query.process(RoomSummaryEntityFields.MEMBERSHIP_STR, queryParams.memberships)
-        query.equalTo(RoomSummaryEntityFields.IS_HIDDEN_FROM_USER, false)
+        val query = with(queryStringValueProcessor) {
+            val query = RoomSummaryEntity.where(realm)


Nit: maybe rewrite it to

val query = with(queryStringValueProcessor) { RoomSummaryEntity.where(realm) .process(RoomSummaryEntityFields.ROOM_ID, queryParams.roomId) .let { if (queryParams.displayName.isNormalized()) { it.process(RoomSummaryEntityFields.NORMALIZED_DISPLAY_NAME, queryParams.displayName) } else { it.process(RoomSummaryEntityFields.DISPLAY_NAME, queryParams.displayName) } } .process(RoomSummaryEntityFields.CANONICAL_ALIAS, queryParams.canonicalAlias) .process(RoomSummaryEntityFields.MEMBERSHIP_STR, queryParams.memberships) .equalTo(RoomSummaryEntityFields.IS_HIDDEN_FROM_USER, false) }

to avoid this double val query declaration

bmarty · 2021-10-27T12:49:41Z

...oid/src/main/java/org/matrix/android/sdk/internal/session/room/summary/RoomSummaryUpdater.kt

-        roomSummaryEntity.displayName = roomDisplayNameResolver.resolve(realm, roomId)
+        val roomDisplayName = roomDisplayNameResolver.resolve(realm, roomId)
+        roomSummaryEntity.displayName = roomDisplayName
+        roomSummaryEntity.normalizedDisplayName = normalizer.normalize(roomDisplayName)


This is where using the setter would help

bmarty · 2021-10-27T12:51:12Z

vector/src/main/java/im/vector/app/features/home/room/list/RoomListViewModel.kt

@@ -191,7 +191,7 @@ class RoomListViewModel @AssistedInject constructor(
        }
        updatableQuery?.updateQuery {
            it.copy(
-                    displayName = QueryStringValue.Contains(action.filter, QueryStringValue.Case.INSENSITIVE)
+                    displayName = QueryStringValue.Contains(action.filter, QueryStringValue.Case.NORMALIZED)


All the changes above in the SDK for this simple change in the app, this is nice!

…ing formatting to have a case per line

…odifiers

…ration instances - is needed as realm will throw if multiple migration instances are created and they don't match

… class which contains a normalized version of the room name

bmarty

Thanks for the update!

ouchadam added 4 commits October 27, 2021 12:19

porting QueryStringValue to sealed interface with a sub category for …

72508c6

…the content based values - allows for handling those cases separately for normalisation

making value processing an injectable class, it will need to have its…

2681601

… own dependencies to support normalisation

adding normalisation to the query string cases

d5ed959

adding normalised room display name field and making use of it when f…

4ae04fc

…iltering rooms by name - fixes non latin-1 character set room names from being ignored when searching with inexact casing

bmarty reviewed Oct 27, 2021

View reviewed changes

ouchadam added 8 commits October 27, 2021 14:26

making the isNormalized function an extension and internal to the sdk

dbb4a87

documenting the different query cases

e7a0a4d

removing noisy log which duplicates a type clause and fixing when cas…

7b35648

…ing formatting to have a case per line

removing extra query definition by chaining the query creation with m…

540036f

…odifiers

ensuring the store migration class is always equal to other store mig…

9949779

…ration instances - is needed as realm will throw if multiple migration instances are created and they don't match

extending the room name resolved to create a dedicated room name data…

611bf29

… class which contains a normalized version of the room name

using correct license for matrix-sdk test

63e9e07

adding changelog entry

d344be5

bmarty approved these changes Oct 28, 2021

View reviewed changes

bmarty merged commit 868548d into develop Oct 28, 2021

bmarty deleted the feature/adm/room-filtering branch October 28, 2021 10:27

This was referenced Nov 15, 2021

Searching for rooms is case sensitive with special characters #4465

Closed

Case sensitive searching #3317

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing case sensitive non latin room name filtering #4352

Fixing case sensitive non latin room name filtering #4352

ouchadam commented Oct 27, 2021

github-actions bot commented Oct 27, 2021 •

edited

Loading

bmarty left a comment

bmarty Oct 27, 2021

ouchadam Oct 27, 2021

bmarty Oct 27, 2021

ouchadam Oct 27, 2021

bmarty Oct 27, 2021

ouchadam Oct 27, 2021

ouchadam Oct 27, 2021

bmarty Oct 27, 2021

ouchadam Oct 27, 2021

ouchadam Oct 27, 2021 •

edited

Loading

bmarty Oct 27, 2021

bmarty Oct 27, 2021

bmarty Oct 27, 2021

ouchadam Oct 27, 2021

bmarty Oct 27, 2021

bmarty Oct 27, 2021

bmarty Oct 27, 2021

bmarty Oct 27, 2021

bmarty left a comment

		}

		fun isNormalized() = this is ContentQueryStringValue && case == Case.NORMALIZED


		internal object RealmSessionStoreMigration : RealmMigration {
		internal class RealmSessionStoreMigration @Inject constructor(

Fixing case sensitive non latin room name filtering #4352

Fixing case sensitive non latin room name filtering #4352

Conversation

ouchadam commented Oct 27, 2021

github-actions bot commented Oct 27, 2021 • edited Loading

Unit Test Results

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

Choose a reason for hiding this comment

ouchadam Oct 27, 2021 • 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

bmarty left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 27, 2021 •

edited

Loading

ouchadam Oct 27, 2021 •

edited

Loading