fix(sdk): correctly import e2ee history in join_room_by_id
#5284
Conversation
It turns out that downstream clients can and do call `Client::join_room_by_id()` rather than `Room::join`, so we need to do the room key history import in the lower-level method.
crates/matrix-sdk/src/client/mod.rs
Outdated
| /// Information about the state of a room before we joined it. | ||
| #[derive(Debug, Clone, Default)] | ||
| struct PreJoinRoomInfo { | ||
| /// The user that invited us to the room, if any | ||
| pub inviter: Option<RoomMember>, | ||
| } |
There was a problem hiding this comment.
Can you include the room state prior to accepting the join, or even better, a mark_as_dm bool that follows the logic that's currently in finish_join_room? Then, we can fix the race you've spotted with respect to sync vs join (and include a test for it 🙃).
There was a problem hiding this comment.
I'd quite like to make that a separate problem, rather than scope-creeping the current fix into fixing other things.
| /// # Arguments | ||
| /// | ||
| /// * `room_id` - The `RoomId` of the room that was joined. | ||
| /// * `pre_join_room_info` - Information about the room before we joined. |
There was a problem hiding this comment.
Do you find that it brings extra usefulness, here? I don't, but would be open to keeping this if you find it useful.
There was a problem hiding this comment.
well to be honest no, it's not useful in its own right. You can make the same argument for the docs on join_room_by_id, though.
Generally I favour adding documentation even where it's "obvious", because (a) it saves having to think about the decision about where the line is between "obvious" and not, and (b) it means we have a better starting point for documentation as the api evolves: for example in this case, if we add a third argument which does need documentation, that's easier because the documentation is already there for the other two.
Personally then, I think having the arguments listed is neutral-to-marginally-positive. But not a strong opinion.
There was a problem hiding this comment.
Yeah, I'm generally against this list of arguments, unless they bring extra value (and agreed it's not any useful in join_room_by_id either). I've seen so many instances where those argument lists were desynchronized from the actual code, or just repeating the type with more words, that I find that we should avoid them, unless they provide some real value. But it's fine to keep as well, no strong opinions here, once again.
There was a problem hiding this comment.
I've seen so many instances where those argument lists were desynchronized from the actual code,
Counterpoint: having the argument list in the documentation as a matter of course means that you start to remember that you can't change arguments without changing documentation. If they're only listed occasionally when they bring extra value, then the chances are much higher that people forget to maintain them.
There was a problem hiding this comment.
Since nobody has strong opinions here, I'm just going to leave it.
There was a problem hiding this comment.
I like the argument list, if they don't contain useful information to me I can ignore them. If they are missing I can't manifest them back.
I'd like to see them everywhere in the public API as well.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5284 +/- ##
==========================================
- Coverage 90.19% 88.72% -1.48%
==========================================
Files 334 330 -4
Lines 105205 88770 -16435
Branches 105205 88770 -16435
==========================================
- Hits 94889 78758 -16131
+ Misses 6254 6205 -49
+ Partials 4062 3807 -255 ☔ View full report in Codecov by Sentry. |
Co-authored-by: Benjamin Bouvier <[email protected]> Signed-off-by: Richard van der Hoff <[email protected]>
It's difficult to think of what a new test would look like, other than an exact repeat of the test I've changed. We talked about this on the crypto team, and concluded that the fundamental problem here was that there is no functional test at the application level to check that the functionality actually works (i.e. that the app is holding matrix-sdk in the way that matrix-sdk expects to be held). But, testing at that level brings its own set of problems. |
|
Ignoring the coverage metric because it seems to be bogus |
|
One thing that comes to mind about the API here is that you can join a room using the This means that we might not have the bundle available if people just don't sync. I guess that this isn't much different from the bundle arriving late. Still, I wish that the clients using this over the FFI were more normal and would just use the correct API. 😞 |
It turns out that downstream clients can and do call
Client::join_room_by_id()rather thanRoom::join, so we need to do the room key history import in the lower-level method.