fix(sdk): Use the server name from the user id as a fallback URL for fetching the well-known info#5996
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5996 +/- ##
==========================================
+ Coverage 88.59% 88.97% +0.38%
==========================================
Files 364 359 -5
Lines 104341 99318 -5023
Branches 104341 99318 -5023
==========================================
- Hits 92438 88370 -4068
+ Misses 7537 6956 -581
+ Partials 4366 3992 -374 ☔ View full report in Codecov by Sentry. |
CodSpeed Performance ReportMerging this PR will not alter performanceComparing Summary
|
| .to_owned(); | ||
|
|
||
| // First try using the server name or homeserver url | ||
| match self.fetch_client_well_known_with_url(server_url.to_string()).await { |
There was a problem hiding this comment.
Does it make sense to try with the server name first? We know that this can fail, and when this is failing, this is precisely why we need a .well-known file (to discover the homeserver API endpoint).
Maybe this is useful for the first attempt, when login in, and the userId is not known yet? In this case server_url is not resolved yet to the CS endpoint?
The .well-known URL should always be computed from the userId domain part, and this should never fail if the file exists.
There was a problem hiding this comment.
That's the alternative I mentioned in the PR description. I kept the previous structure adding this 2nd attempt, but maybe it's better to always try first with the server name, explicit or implicit, then the homeserver url as the fallback 🤔 .
There was a problem hiding this comment.
That said, if we extract the server name from the user id and we have no other url to check... which scheme should we add for the constructed URL? Should we enforce https, or just default to plain http, just in case the server does not use HTTPS?
There was a problem hiding this comment.
I'd agree with that, but I think I'll try to reuse the scheme in the homeserver url, since otherwise we won't be able to test this behaviour (the network mocking layer in the tests doesn't work with HTTPS if I'm not mistaken).
d64ecfc to
0ad1350
Compare
Hywan
left a comment
There was a problem hiding this comment.
Looks good to me. I thought we were already doing that… anyway. Thanks for the tests too!
Can you explain improve the comments to explain why we are doing this? I believe the explanations must live in the documentation of the method itself rather than an inline comment.
Finally, the CHANGELOG.md expects a new entry 😃.
| // Use the server name, either an explicit one or an implicit one taken from | ||
| // the user id | ||
| let server_url = self | ||
| .server() | ||
| .unwrap_or( | ||
| // Sometimes people configure their well-known directly on the homeserver so use | ||
| // this as a fallback when the server name is unknown. | ||
| &self.homeserver(), | ||
| ) | ||
| .to_string(); | ||
| .map(|server| server.to_string()) | ||
| .or_else(|| self.user_id().map(|id| format!("{}://{}", scheme, id.server_name()))); |
There was a problem hiding this comment.
I believe this part deserves a bit more explanations about why we are doing this, otherwise it looks good!
|
Done, thanks: I added some more inline comments as well as doc comments. I also added the changelog entry, I usually do that once the approach used in the PR has been validated, so I don't have to also change it every time the logic in the PR does 😅 . |
|
For reference, the spec for this behaviour is at https://spec.matrix.org/v1.17/client-server-api/#well-known-uris I think. If you haven't, it would be worth reading through and checking we comply with it. I looked it up because I thought we weren't allowed to guess hostname from Matrix ID, but it looks like in this case we are. |
…er name from the `Client::user_id` as a possible fallback too
5e5700f to
81705e8
Compare

When using
Client::fetch_client_well_known, use the server name in the user id as a fallback value if the server name is missing. If it's not there or this first attempt fails, use the homeserver url instead as we were doing so far.Fixes #5877 (or workarounds it).
Signed-off-by: