SDK: Ignore the discovered/custom sliding sync proxy when using SSS. #3743
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3743 +/- ##
==========================================
- Coverage 84.02% 84.00% -0.03%
==========================================
Files 260 260
Lines 26712 26719 +7
==========================================
Hits 22446 22446
- Misses 4266 4273 +7 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Sorry, this fix should not happen only for FFI users, but for all users of the SDK (and then we can even include a test \o/ a very simple one). It should happen around here, where we'd set the proxy to None if the boolean is set. Can you implement that and add a test please?
pixlwave
force-pushed
the
doug/sss-discovered-proxy
branch
2 times, most recently
from
a08b4b8 to
7920ea0
Compare
pixlwave
changed the title
pixlwave
force-pushed
the
doug/sss-discovered-proxy
branch
from
7920ea0 to
86b8b96
Compare
pixlwave
force-pushed
the
doug/sss-discovered-proxy
branch
4 times, most recently
from
5a41c33 to
7023036
Compare
|
Oh yeah good point, updated. |
There was a problem hiding this comment.
Seems we weren't using the SDK logic for the custom proxy, so now we are!
| #[cfg(feature = "experimental-sliding-sync")] | ||
| if self.requires_sliding_sync && sliding_sync_proxy.is_none() { | ||
| // In the future we will need to check for native support on the homeserver too. | ||
| return Err(ClientBuildError::SlidingSyncNotAvailable); | ||
| if self.requires_sliding_sync { | ||
| if self.is_simplified_sliding_sync_enabled { | ||
| // TODO: The server doesn't yet expose the availability of SSS. | ||
| } else if sliding_sync_proxy.is_none() { | ||
| return Err(ClientBuildError::SlidingSyncNotAvailable); | ||
| } |
There was a problem hiding this comment.
I would coalesce all these checks together, since it's not clear that we'd keep requires_sliding_sync for is_simplified_sliding_sync (at least we'd have some method named after it). Instead, we could even throw an error when requires_sliding_sync and is_simplified_sliding_sync are set at the same time, because it sounds like a misconfigure.
There was a problem hiding this comment.
Ah ok, I imagined that requires_sliding_sync would make sure you build a client with support for the type of sliding sync you configured it with but I see the confusion there. For now I'll revert this change and we can think about it when SSS is a bit more mature.
| /// The FFI's build has SSS disabled by default, but the SDK has it enabled | ||
| /// by default. The tests are aligned with the FFI for now (until we remove | ||
| /// regular sliding sync). | ||
| fn make_ffi_builder() -> ClientBuilder { |
There was a problem hiding this comment.
I wouldn't mention FFI here, because it's not clear for a usual reader what FFI we're referring to, and we need to keep a clear separation of concerns. Maybe rename it make_non_sss_client_builder?
There was a problem hiding this comment.
Thank you, my brain wasn't helping me with names when I was doing this 🫠
| let _client = builder.build().await.unwrap(); | ||
|
|
||
| // Then a client should not use the discovered sliding sync proxy. | ||
| #[cfg(feature = "experimental-sliding-sync")] |
There was a problem hiding this comment.
nit: don't need this cfg because the test is guarded against the same cfg guard
Then we could rename _client to client since it'd be always used.
pixlwave
force-pushed
the
doug/sss-discovered-proxy
branch
from
7023036 to
ae2cc31
Compare
Update crates/matrix-sdk/src/client/builder.rs Co-authored-by: Benjamin Bouvier <[email protected]> Signed-off-by: Doug <[email protected]>
pixlwave
force-pushed
the
doug/sss-discovered-proxy
branch
from
b4d6cb9 to
886054e
Compare
Oversight on my part in #3723, this code needed to handle the discovered proxy as well as the custom one.