Appservice virt member test #747
Conversation
Don't assume virtual client membership is join if none is stored, since that leads to a client being told it's joined in rooms it has no membership of. The main appservice client still assumes it's joined every room it receives transaction events about.
agraven
force-pushed
the
appservice-virt-member-test
branch
from
3cf280a
to
d5f14c0
Compare
| "age": 2970366 | ||
| } | ||
| }))? | ||
| .cast::<AnyRoomEvent>(), |
There was a problem hiding this comment.
I wasn't sure where to put the test json, as the needed Vec<Raw> form was slightly different than usual.
There was a problem hiding this comment.
There was a problem hiding this comment.
In most cases I prefer to put the JSON close to the test, even at the cost of some duplication. Otherwise it makes it harder to keep in mind which JSON we're using and what assumptions it carries with it.
| @@ -709,25 +713,28 @@ impl AppService { | |||
| let key = &[USER_MEMBER, room_id.as_bytes(), b".", user_id.as_bytes()].concat(); | |||
| let membership = match client.store().get_custom_value(key).await? { | |||
| Some(value) => String::from_utf8(value).ok().map(MembershipState::from), | |||
| // Assume the appservice is in every known room | |||
| None if user_id == appserv_uid => Some(MembershipState::Join), | |||
There was a problem hiding this comment.
It seemed like a safe assumption that if the appservice receives events for a given room, the main client is already a member of it and we just missed it or haven't processed the relevant membership event yet.
There was a problem hiding this comment.
Yep, seems like a save assumption. A proper fix would be to force a full state sync if we see events for a room the first time, but that's out of scope for this PR.
Codecov Report
@@ Coverage Diff @@
## main #747 +/- ##
==========================================
+ Coverage 71.59% 71.70% +0.10%
==========================================
Files 117 117
Lines 16475 16505 +30
==========================================
+ Hits 11796 11835 +39
+ Misses 4679 4670 -9
Continue to review full report at Codecov.
|
agraven
force-pushed
the
appservice-virt-member-test
branch
from
d5f14c0
to
de0251c
Compare
Test that virtual clients get assigned the correct membership to rooms when processing received transactions.
agraven
force-pushed
the
appservice-virt-member-test
branch
from
de0251c
to
9bd9b46
Compare
|
I think the |
|
That gets covered by assuming the appservice acount is // Assume the appservice is in every known room
None if user_id == appserv_uid => Some(MembershipState::Join),Edit: though it might still make sense to do, since that means we're not using the fallback behavior in every case. |
There was a problem hiding this comment.
This looks good from my point of view, thanks for spending time writing tests for the app-service crate. @johannescpk are your concerns resolved as well?
| "age": 2970366 | ||
| } | ||
| }))? | ||
| .cast::<AnyRoomEvent>(), |
There was a problem hiding this comment.
In most cases I prefer to put the JSON close to the test, even at the cost of some duplication. Otherwise it makes it harder to keep in mind which JSON we're using and what assumptions it carries with it.
|
Fix filed in #764 |
|
Thanks. |
commit 1: Don't assume virtual client membership is join if none is stored, since that leads to a client being told it's joined in rooms it has no membership of. The main appservice client still assumes it's joined every room it receives transaction events about.
commit 2: Test that virtual clients get assigned the correct membership to rooms when processing received transactions.