Changes room hierarchy endpoint to stable #5443

ericdecanini · 2022-03-07T15:53:12Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Adds stable room hierarchy endpoint with a fallback to the unstable one

Motivation and context

Closes #4462

Screenshots / GIFs

N/A

Tests

Testing Normal Functionality

Login with any account belonging to spaces, preferrably one with subspaces too
Open the home navigation drawer and see that you can see your spaces and subspaces as normal

Testing Fallback

Set debug points in ResolveSpaceInfoTask lines 49 and 51
Force a fail on the stable request (either via code or via a network interceptor) and see that it falls back to the unstable API

Tested devices

Physical
Emulator
OS version(s): Android 10

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

github-actions · 2022-03-07T16:27:59Z

Unit Test Results

  98 files +2   98 suites +2 1m 16s ⏱️ +3s
174 tests +2 174 ✔️ +2 0 💤 ±0 0 ❌ ±0
568 runs +4 568 ✔️ +4 0 💤 ±0 0 ❌ ±0

Results for commit 7226864. ± Comparison against base commit bdc9bc0.

♻️ This comment has been updated with latest results.

ouchadam · 2022-03-09T10:06:30Z

...-android/src/main/java/org/matrix/android/sdk/internal/session/space/ResolveSpaceInfoTask.kt

-    override suspend fun execute(params: ResolveSpaceInfoTask.Params): SpacesResponse {
-        return executeRequest(globalErrorReceiver) {
+
+    private lateinit var params: ResolveSpaceInfoTask.Params


we can avoid the mutable state by chaining the params into the getStableSpaceHierarchy/getUnstableSpaceHierarchy

there's also an argument for avoiding lateinit in places that don't have a strictly documented lifecycle as it can cause unexpected side effects on order changes, eg if the this.params = params was set after getSpaceHierarchy

I considered chaining params as an argument into these private functions but deliberately chose to make params as a lateinit instance variable. My reasoning is as follows:

The class is very cohesive (a.k.a its instance variables are used completely across the class), thus making its intended purpose clearer

It reduces the cognitive effort of reading each function. getSpaceHierarchy() is much easier to read than getSpaceHierarchy(params) where params is repeated across every function

I'm aware that a slight change in ordering would cause this to crash, but the contents and intent of the class is clear enough that a contributor shouldn't be led to making this mistake

It's a step I think is explained quite well in Uncle Bob's Clean Code, but if we still think it would be better to do otherwise then I'm happy to change it

I would lean towards the introduction of mutability and state to the class as cognitively heavier than adding function parameters. As soon as we introduce mutability, threading and ordering become sources of bugs, things that we can safely avoid by keeping our logic stateless (ignoring our potentially stateful class dependencies 😅)

override suspend fun execute(params: ResolveSpaceInfoTask.Params): SpacesResponse { return executeRequest(globalErrorReceiver) { try { spaceApi.getSpaceHierarchy(params) } catch (error: Throwable) { spaceApi.getSpaceHierarchyUnstable(params) } } } private suspend fun SpaceApi.getSpaceHierarchy(params: ResolveSpaceInfoTask.Params) = this.getSpaceHierarchy( spaceId = params.spaceId, suggestedOnly = params.suggestedOnly, limit = params.limit, maxDepth = params.maxDepth, from = params.from ) private suspend fun SpaceApi.getSpaceHierarchyUnstable(params: ResolveSpaceInfoTask.Params) = this.getSpaceHierarchyUnstable( spaceId = params.spaceId, suggestedOnly = params.suggestedOnly, limit = params.limit, maxDepth = params.maxDepth, from = params.from )

with that said, for this class I don't have too strong of an opinion as the scope is restricted and it's just 1 property, I would personally avoid vars and lateinits unless they serve a functional purpose as kotlin has other ways to reduce boilerplate

Fair enough. I pushed the change!

ouchadam · 2022-03-09T10:09:01Z

...c/test/java/org/matrix/android/sdk/internal/session/space/DefaultResolveSpaceInfoTaskTest.kt

+import org.matrix.android.sdk.test.fakes.FakeSpaceApi
+
+@ExperimentalCoroutinesApi
+class DefaultResolveSpaceInfoTaskTest {


thanks for adding these and all the setup 💯

ouchadam · 2022-03-09T10:25:59Z

...c/test/java/org/matrix/android/sdk/internal/session/space/DefaultResolveSpaceInfoTaskTest.kt

+
+    @Test
+    fun `given stable endpoint works, when execute, then return stable api data`() = runBlockingTest {
+        spaceApi.givenStableEndpointWorks()


not a strong opinion just a personal preference, it can be helpful from the test standpoint to be able to relate the inputs and results

// give stable endpoint returns a response, when execute, spaceApi.givenStableEndpointReturns(A_RESPONSE) val result = resolveSpaceInfoTask.execute(spaceApi.params) result shouldBeEqualTo A_RESPONSE

I'm wondering there's a way to relate to get the best of both worlds (with the boilerplate reduction and tightening the relationship between the given and the expected result)

fakeSpaceApi.givenStableEndpointSuccess() val result = resolveSpaceInfoTask.execute(spaceApi.params) result shouldBeEqualTo fakeSpaceApi.successResponse

what do you think?

Tbh I like the first option as it gives the test class more flexibility about the types of responses it can test with but still maintains a very readable front. I'll go with that!

bmarty

My 2 cents about 404.
Thanks for adding the tests!

bmarty · 2022-03-09T11:09:33Z

...-android/src/main/java/org/matrix/android/sdk/internal/session/space/ResolveSpaceInfoTask.kt

+
+    private suspend fun getSpaceHierarchy() = try {
+        getStableSpaceHierarchy()
+    } catch (e: Throwable) {


I would check only 404 here. Other error (such as no network etc.) could be propagated directly

Do we want to check only for 404 specifically? Personally I think it would also be good to fallback even in the case of 500 (internal server error) and such, thus catch a simple HttpException

bmarty · 2022-03-09T11:16:09Z

Also have you checked:

the second bullet of Use the stable hierarchy endpoint from MSC2946 #4462: Ensure that the room_id is not accessed via the list of children_state (this is nested under the room information and should be unnecesary).
the new comment Use the stable hierarchy endpoint from MSC2946 #4462 (comment): The other thing to check is if the join_rules field (plural version) is being used instead of the proper join_rule field.

ericdecanini · 2022-03-09T17:48:45Z

Also have you checked:

the second bullet of Use the stable hierarchy endpoint from MSC2946 #4462: Ensure that the room_id is not accessed via the list of children_state (this is nested under the room information and should be unnecesary).

the new comment Use the stable hierarchy endpoint from MSC2946 #4462 (comment): The other thing to check is if the join_rules field (plural version) is being used instead of the proper join_rule field.

I saw the second comment just now. I'm a bit confused with the wording, but I'm guessing this is saying we want to be using join_rule? In this case we're good

ericdecanini · 2022-03-09T17:50:50Z

...k-android/src/main/java/org/matrix/android/sdk/internal/session/space/DefaultSpaceService.kt

+            order = content.order,
+            viaServers = content.via.orEmpty(),
+            activeMemberCount = summary.numJoinedMembers,
+            parentRoomId = spaceId,


@bmarty please pay more attention to this line. This is where {children_state event}.roomId was being used. I can only assume its intended use was to point to the space id? Please correct me if I'm wrong. ~~I don't have a great understanding of spaces~~

Edit: Spoke to @clokep to get more info on this. I believe this to be correct

ouchadam

I'm not too familiar with the spaces side of things when it comes to the IDs, will delegate to @bmarty for that (or maybe @fedrunov can help out!)

otherwise LGTM 💯

…rvice

ericdecanini · 2022-03-10T13:50:08Z

FYI join_rule is already being used as intended in the app

bmarty

Some remark on the code style, else LGTM (I defer the logic about space to other people)

bmarty · 2022-03-10T19:53:12Z

...-android/src/main/java/org/matrix/android/sdk/internal/session/space/ResolveSpaceInfoTask.kt

+
+    private suspend fun getSpaceHierarchy(params: ResolveSpaceInfoTask.Params) = try {
+        getStableSpaceHierarchy(params)
+    } catch (e: HttpException) {


Why not using HttpException. I would have checked for a rather more expected 404, but OK

I think it would be good to protect ourselves from other server related errors (e.g. 500 - internal server error) and fallback to the unstable api in such cases. Anyway happy to discuss this before merging

bmarty · 2022-03-10T19:56:46Z

...-android/src/main/java/org/matrix/android/sdk/internal/session/space/ResolveSpaceInfoTask.kt

+        getSpaceHierarchy(params)
+    }
+
+    private suspend fun getSpaceHierarchy(params: ResolveSpaceInfoTask.Params) = try {


I found using = try and spilling into 2 methods less readable than a simple

override suspend fun execute(params: ResolveSpaceInfoTask.Params) { executeRequest(globalErrorReceiver) { try { getUnstableSpaceHierarchy(params) } catch (e: HttpException) { getStableSpaceHierarchy(params) } } }

Also I would rename getStableSpaceHierarchy to getSpaceHierarchy (rule: never use stable, just use unstable, intended to be removed after a while)

Good shout!

bmarty

Thanks for the update! I let you merge the PR?
Edit I do it

Changes room hierarchy endpoint

3b0a565

ericdecanini added 11 commits March 8, 2022 11:51

Adds testing for fallback api

bc3b8d0

Adds logic for using stable and unstable hierarchy endpoints

0af6ae6

Fixes legal comments

e5299d7

Changes caught exception type to Throwable

eb46067

Removes unused imports

82b5fc9

Adds changelog file

510206a

Adds debug logs

0892525

Adds slash to v1 prefix path

54828f7

Adds error print stack trace

31f300c

Fixes wrong path parameter in getSpaceHierarchy

fb374b7

Removes debug logs

63cd79d

ericdecanini marked this pull request as ready for review March 9, 2022 09:54

ericdecanini requested review from a team and ouchadam and removed request for a team March 9, 2022 09:55

ouchadam reviewed Mar 9, 2022

View reviewed changes

bmarty reviewed Mar 9, 2022

View reviewed changes

ericdecanini added 4 commits March 9, 2022 12:18

Adds coroutinesTest to matrix sdk gradle

047e767

Replaces caught Exception with HttpException

bbc6e8b

Refactors DefaultSpaceService querySpaceChildren

f76f73f

Replaces children state event room id with space id

2f706d6

ericdecanini commented Mar 9, 2022

View reviewed changes

ericdecanini requested review from ouchadam and bmarty March 9, 2022 17:51

Replaces lateinit var with passing params

a891f59

ouchadam approved these changes Mar 10, 2022

View reviewed changes

Renames mapToSpaceChildInfoList to mapSpaceChildren in DefaultSpaceSe…

a5af478

…rvice

bmarty approved these changes Mar 10, 2022

View reviewed changes

Improves code formatting in ResolveSpaceInfoTask

7226864

bmarty approved these changes Mar 11, 2022

View reviewed changes

bmarty merged commit c89554c into develop Mar 11, 2022

bmarty deleted the task/eric/stable-hierarchy-endpoint branch March 11, 2022 16:05

clokep mentioned this pull request Mar 22, 2022

Stabilize support for MSC2946: spaces summary matrix-org/synapse#10946

Closed

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes room hierarchy endpoint to stable #5443

Changes room hierarchy endpoint to stable #5443

ericdecanini commented Mar 7, 2022 •

edited

Loading

github-actions bot commented Mar 7, 2022 •

edited

Loading

ouchadam Mar 9, 2022

ericdecanini Mar 9, 2022

ouchadam Mar 9, 2022

ericdecanini Mar 10, 2022

ouchadam Mar 9, 2022

ouchadam Mar 9, 2022

ericdecanini Mar 9, 2022

bmarty left a comment

bmarty Mar 9, 2022

ericdecanini Mar 9, 2022

bmarty commented Mar 9, 2022

ericdecanini commented Mar 9, 2022

ericdecanini Mar 9, 2022 •

edited

Loading

bmarty Mar 10, 2022

ouchadam left a comment

ericdecanini commented Mar 10, 2022

bmarty left a comment

bmarty Mar 10, 2022

ericdecanini Mar 10, 2022

bmarty Mar 10, 2022

ericdecanini Mar 10, 2022

bmarty left a comment •

edited

Loading

Changes room hierarchy endpoint to stable #5443

Changes room hierarchy endpoint to stable #5443

Conversation

ericdecanini commented Mar 7, 2022 • edited Loading

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

github-actions bot commented Mar 7, 2022 • edited Loading

Unit Test Results

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty commented Mar 9, 2022

ericdecanini commented Mar 9, 2022

ericdecanini Mar 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ouchadam left a comment

Choose a reason for hiding this comment

ericdecanini commented Mar 10, 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

bmarty left a comment • edited Loading

Choose a reason for hiding this comment

ericdecanini commented Mar 7, 2022 •

edited

Loading

github-actions bot commented Mar 7, 2022 •

edited

Loading

ericdecanini Mar 9, 2022 •

edited

Loading

bmarty left a comment •

edited

Loading