Bring high level room joining logic into GMSL #372
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #372 +/- ##
==========================================
+ Coverage 51.82% 53.41% +1.59%
==========================================
Files 37 38 +1
Lines 5775 5996 +221
==========================================
+ Hits 2993 3203 +210
+ Misses 2491 2473 -18
- Partials 291 320 +29
☔ View full report in Codecov by Sentry. |
fclient/performjoin.go
Outdated
| ) { | ||
| event = remoteEvent | ||
| } | ||
| } |
There was a problem hiding this comment.
We seemingly continue blindly if the event is invalid?
There was a problem hiding this comment.
I don't know what the logic should be if you receive a bad event inside a send_join response.
Synapse just accepts the event as is and doesn't seem to do any checks against it. So we differ from them in that regard.
At this point in the join dance, the resident server has already accepted that event into it's room graph and propagated it out to the other servers in the room. So for us to reject it leaves us in an awkward situation.
|
|
||
| type FederatedJoinClient interface { | ||
| MakeJoin(ctx context.Context, origin, s spec.ServerName, roomID, userID string) (res MakeJoinResponse, err error) | ||
| SendJoin(ctx context.Context, origin, s spec.ServerName, event *Event) (res SendJoinResponse, err error) |
There was a problem hiding this comment.
This is okay for now, but I'm wondering if we can be less specific long term. If we ever decide to change the flow here (removing make_join, adding a third stage) then everything needs to change which is unfortunate. It would be nice to hide this behind the room version PDU bits longer term I guess?
There was a problem hiding this comment.
Agree. That might require some more extensive refactoring. This follows the same pattern as requests to /state. The problem is that Dendrite provides this to GMSL. So for current join flows GMSL requires something that can hit the /make_join and the /send_join endpoints.
If we instead just pass down the something that meets the entire FederationClient interface, then GMSL can more easily adapt it's flows. But by creating this sub-interface it is a little more perscriptive.
| return nil, &FederationError{ | ||
| ServerName: input.ServerName, | ||
| Transient: true, | ||
| Reachable: false, |
There was a problem hiding this comment.
This assumes a network error, but I'm fairly sure we will return an err on non-2xx as well.
There was a problem hiding this comment.
Yup. This keeps the behaviour the same as before the refactor. I tried to limit behaviour changes in this PR
| if err != nil { | ||
| return nil, &FederationError{ | ||
| ServerName: input.ServerName, | ||
| Transient: false, |
There was a problem hiding this comment.
Hard to say if this is transient as it may have failed due to failing to get server keys over the network.
|
Oh and please add tests for the main logic here. It should be easy now as we've interfaced up the fed client! |
No description provided.