Homeserver version without patch number parsing #6214

ouchadam · 2022-05-31T16:21:58Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Fixes #6017 Supporting Homeserver versions without a patch number.

Versions without a patch are considered as having a patch of 0 eg 1.5 is parsed as major: 1 minor: 5 patch: 0

Makes the Homeserver version regex support optional patch numbers
Adds unit tests around the parsing

Motivation and context

To support version formats defined by the spec

Screenshots / GIFs

No UI changes

Tests

Handled by unit tests

Tested devices

Physical
Emulator
OS version(s): 28

ouchadam · 2022-05-31T16:22:34Z

...-sdk-android/src/main/java/org/matrix/android/sdk/internal/auth/version/HomeServerVersion.kt

@@ -38,14 +38,14 @@ internal data class HomeServerVersion(
    }

    companion object {
-        internal val pattern = Regex("""[r|v](\d+)\.(\d+)\.(\d+)""")
+        internal val pattern = Regex("""[r|v](\d+)\.(\d+)(?:\.(\d+))?""")


this is the fix, to make the last capture group optional

It should work, but strange that it's not just [r|v](\d+)\.(\d+)(\.(\d+))?. Regex syntax I guess.
EDIT: I see, it's to avoid the group to be captured. TIL

ouchadam · 2022-05-31T16:23:13Z

...-android/src/test/java/org/matrix/android/sdk/internal/auth/version/HomeServerVersionTest.kt

+        (supportedVersions + unsupportedVersions).forEach { (input, expected) ->
+            val result = HomeServerVersion.parse(input)
+
+            assertEquals(expected, result, "Expected $input to be $expected but got $result")


ended up using the assertEquals directly instead of shouldBeEqual in order to provide a custom message

I am always bitten by the parameter ordering. With JUnit the message is first.

bmarty

Thanks. Some remarks.

bmarty · 2022-05-31T16:28:18Z

...-sdk-android/src/main/java/org/matrix/android/sdk/internal/auth/version/HomeServerVersion.kt

@@ -38,14 +38,14 @@ internal data class HomeServerVersion(
    }

    companion object {
-        internal val pattern = Regex("""[r|v](\d+)\.(\d+)\.(\d+)""")
+        internal val pattern = Regex("""[r|v](\d+)\.(\d+)(?:\.(\d+))?""")


It should work, but strange that it's not just [r|v](\d+)\.(\d+)(\.(\d+))?. Regex syntax I guess.
EDIT: I see, it's to avoid the group to be captured. TIL

bmarty · 2022-05-31T16:29:10Z

...-android/src/test/java/org/matrix/android/sdk/internal/auth/version/HomeServerVersionTest.kt

+        (supportedVersions + unsupportedVersions).forEach { (input, expected) ->
+            val result = HomeServerVersion.parse(input)
+
+            assertEquals(expected, result, "Expected $input to be $expected but got $result")


I am always bitten by the parameter ordering. With JUnit the message is first.

bmarty · 2022-05-31T16:33:34Z

...-android/src/test/java/org/matrix/android/sdk/internal/auth/version/HomeServerVersionTest.kt

+    prefixes.map { prefix -> case.copy(input = "$prefix${case.input}") }
+}.flatten()
+
+private data class Case(val input: String, val expected: HomeServerVersion?)


OOI why all that private stuff is not inside class HomeServerVersionTest?

it could be inside the class, although there's nothing instance related in the helpers which is why I opted for having the helpers at the module level (I tend to lift pure functions out of classes)

Very good point, thanks!

bmarty · 2022-05-31T16:38:35Z

...-sdk-android/src/main/java/org/matrix/android/sdk/internal/auth/version/HomeServerVersion.kt

@@ -59,3 +59,5 @@ internal data class HomeServerVersion(
        val v1_3_0 = HomeServerVersion(major = 1, minor = 3, patch = 0)
    }
 }
+
+private fun List<String>.getOptional(index: Int, default: String) = getOrNull(index)?.ifEmpty { default } ?: default


ifEmpty is maybe not necessary. I do not think it can be empty, since we have \d+. Maybe add a test with "v1.4.", expecting null.

the optional block returns an empty string, with ifEmpty removed

java.lang.NumberFormatException: For input string: "" at java.base/java.lang.NumberFormatException.forInputString(NumberFormatException.java:65) at java.base/java.lang.Integer.parseInt(Integer.java:662) at java.base/java.lang.Integer.parseInt(Integer.java:770) at org.matrix.android.sdk.internal.auth.version.HomeServerVersion$Companion.parse$matrix_sdk_android_debug(HomeServerVersion.kt:48)

have updated the test cases to include trailing . 4501c7c

I see, thanks. I would avoid creating this extension and would have written at line 48:

patch = result.groupValues.getOrNull(3)?.ensureNotEmpty()?.toInt() ?: 0

Since, this is not necessary to do "0".toInt().

The extension:

fun String.ensureNotEmpty() = takeIf { it.isNotEmpty() }

could be added to the core ~~module~~ package. I am surprised we do not have created it yet, we have 76 occurrences of .takeIf { it.isNotEmpty() } in the codebase :)

This is obviously not a blocker :).

I can add the extension to the sdk String extensions, currently there are no common modules between the app and the sdk

fa21b6d

bmarty · 2022-05-31T16:39:31Z

...-android/src/test/java/org/matrix/android/sdk/internal/auth/version/HomeServerVersionTest.kt

+                case("1.5", expected = aVersion(1, 5, 0)),
+                case("0.5.1", expected = aVersion(0, 5, 1)),
+                case("1.0.0", expected = aVersion(1, 0, 0)),
+                case("1.10.3", expected = aVersion(1, 10, 3))


Could have a trailing comma.

github-actions · 2022-05-31T17:29:30Z

Unit Test Results

148 files +2 148 suites +2 2m 15s ⏱️ -17s
237 tests +1 237 ✔️ +1 0 💤 ±0 0 ❌ ±0
790 runs +2 790 ✔️ +2 0 💤 ±0 0 ❌ ±0

Results for commit fa21b6d. ± Comparison against base commit e6beb73.

♻️ This comment has been updated with latest results.

bmarty · 2022-06-02T21:29:36Z

...-sdk-android/src/main/java/org/matrix/android/sdk/internal/auth/version/HomeServerVersion.kt

@@ -59,3 +59,5 @@ internal data class HomeServerVersion(
        val v1_3_0 = HomeServerVersion(major = 1, minor = 3, patch = 0)
    }
 }
+
+private fun List<String>.getOptional(index: Int, default: String) = getOrNull(index)?.ifEmpty { default } ?: default


I see, thanks. I would avoid creating this extension and would have written at line 48:

patch = result.groupValues.getOrNull(3)?.ensureNotEmpty()?.toInt() ?: 0

Since, this is not necessary to do "0".toInt().

The extension:

fun String.ensureNotEmpty() = takeIf { it.isNotEmpty() }

could be added to the core ~~module~~ package. I am surprised we do not have created it yet, we have 76 occurrences of .takeIf { it.isNotEmpty() } in the codebase :)

This is obviously not a blocker :).

ganfra

LGTM!

…aking use for finding option regex match

github-actions · 2022-06-06T14:34:49Z

Matrix SDK

Integration Tests Results:

[org.matrix.android.sdk.session]
= passed=19 failures=1 errors=0 skipped=3
[org.matrix.android.sdk.account]
= passed=3 failures=0 errors=0 skipped=2
[org.matrix.android.sdk.internal]
= passed=27 failures=1 errors=0 skipped=1
[org.matrix.android.sdk.ordering]
= passed=16 failures=0 errors=0 skipped=0
[org.matrix.android.sdk.PermalinkParserTest]
= passed=16 failures=0 errors=0 skipped=0

bmarty · 2022-06-07T08:58:21Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/extensions/Strings.kt

+/**
+ * Returns null if the string is empty.
+ */
+fun String.ensureNotEmpty() = ifEmpty { null }


thanks, better than my proposal!

ouchadam added matrix-sdk PR-Small PR with less than 20 updated lines labels May 31, 2022

ouchadam commented May 31, 2022

View reviewed changes

bmarty reviewed May 31, 2022

View reviewed changes

ouchadam added 5 commits June 1, 2022 10:41

adding test cases around parsing homeserver versions

ea151b3

supporting homeserver versions without a patch value

289ce74

adding changelog entry

b3841c9

adding test case for trailing . on the version

4501c7c

adding trailing commas to lists

3756b2d

ouchadam force-pushed the feature/adm/homeserver-version-parsing branch from 30145c5 to 3756b2d Compare June 1, 2022 09:45

using correct license for matrix sdk class

10016fc

bmarty approved these changes Jun 2, 2022

View reviewed changes

ouchadam requested review from a team, ganfra and Claire1817 and removed request for a team and Claire1817 June 6, 2022 07:59

ganfra approved these changes Jun 6, 2022

View reviewed changes

creating generic extension for converting empty strings to null and m…

fa21b6d

…aking use for finding option regex match

ouchadam enabled auto-merge June 6, 2022 13:51

ouchadam merged commit 0ef67b6 into develop Jun 6, 2022

ouchadam deleted the feature/adm/homeserver-version-parsing branch June 6, 2022 13:54

bmarty reviewed Jun 7, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Homeserver version without patch number parsing #6214

Homeserver version without patch number parsing #6214

ouchadam commented May 31, 2022 •

edited

Loading

ouchadam May 31, 2022

bmarty May 31, 2022 •

edited

Loading

ouchadam May 31, 2022

bmarty May 31, 2022

bmarty left a comment

bmarty May 31, 2022 •

edited

Loading

bmarty May 31, 2022

bmarty May 31, 2022

ouchadam Jun 1, 2022

bmarty Jun 2, 2022

bmarty May 31, 2022

ouchadam Jun 1, 2022

bmarty Jun 2, 2022 •

edited

Loading

ouchadam Jun 6, 2022

bmarty May 31, 2022

github-actions bot commented May 31, 2022 •

edited

Loading

bmarty Jun 2, 2022 •

edited

Loading

ganfra left a comment

github-actions bot commented Jun 6, 2022

bmarty Jun 7, 2022

Homeserver version without patch number parsing #6214

Homeserver version without patch number parsing #6214

Conversation

ouchadam commented May 31, 2022 • edited Loading

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Choose a reason for hiding this comment

bmarty May 31, 2022 • edited Loading

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

bmarty May 31, 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

bmarty Jun 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 31, 2022 • edited Loading

Unit Test Results

bmarty Jun 2, 2022 • edited Loading

Choose a reason for hiding this comment

ganfra left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 6, 2022

Matrix SDK

Integration Tests Results:

Choose a reason for hiding this comment

ouchadam commented May 31, 2022 •

edited

Loading

bmarty May 31, 2022 •

edited

Loading

bmarty May 31, 2022 •

edited

Loading

bmarty Jun 2, 2022 •

edited

Loading

github-actions bot commented May 31, 2022 •

edited

Loading

bmarty Jun 2, 2022 •

edited

Loading