Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use ClientBuilder pattern in SDK FFI #739

Merged
merged 16 commits into from
Jun 29, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ class MatrixRustSDKTests: XCTestCase {

func testReadOnlyFileSystemError() {
do {
let _ = try loginNewClient(basePath: "", username: "test", password: "test")
let _ = try loginNewClient(basePath: "", username: "test", password: "test", config: ClientConfig(homeserver: nil, httpProxy: nil))
} catch ClientError.Generic(let message) {
XCTAssertNotNil(message.range(of: "Read-only file system"))
} catch {
Expand Down
7 changes: 6 additions & 1 deletion crates/matrix-sdk-ffi/src/api.udl
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
namespace sdk {
[Throws=ClientError]
Client login_new_client(string base_path, string username, string password);
Client login_new_client(string base_path, string username, string password, ClientConfig config);

[Throws=ClientError]
Client guest_client(string base_path, string homeserver);
Expand All @@ -13,6 +13,11 @@ namespace sdk {
string gen_transaction_id();
};

dictionary ClientConfig {
string? homeserver;
string? http_proxy;
};

[Error]
interface ClientError {
Generic(string msg);
Expand Down
36 changes: 31 additions & 5 deletions crates/matrix-sdk-ffi/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,10 @@ pub fn guest_client(base_path: String, homeurl: String) -> anyhow::Result<Arc<Cl

pub fn login_with_token(base_path: String, restore_token: String) -> anyhow::Result<Arc<Client>> {
let RestoreToken { session, homeurl, is_guest } = serde_json::from_str(&restore_token)?;

let builder = new_client_builder(base_path, session.user_id.to_string())?
.homeserver_url(&homeurl)
.user_id(&session.user_id);
.homeserver_url(&homeurl);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in another comment user_id was actually setting server name, and that method is mutually exclusive with homeserver_url (as the docs state, only the last will be used). In effect the homeurl is always overriden by user_id_server, and for example localhost cannot be used properly.


// First we need to log in.
RUNTIME.block_on(async move {
let client = builder.build().await?;
Expand All @@ -61,12 +62,15 @@ pub fn login_new_client(
base_path: String,
username: String,
password: String,
config: ClientConfig
) -> anyhow::Result<Arc<Client>> {
let builder = new_client_builder(base_path, username.clone())?;
let user = Box::<UserId>::try_from(username)?;
let user = Box::<UserId>::try_from(username.clone())?;
let builder = new_client_builder_with_config(base_path, username, config, &user)?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this ... isn't really making sense to me. UserId must contain a homeserver-address, which we then look up on the rust-side by doing the spec-jumps of .well-known. Given that we have to pass both config and username, I'd argue that username should have preference as it is more specific.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea I can see this is not ideal. Preferring username if both are available would not work for localhost (without the support of https), but an alternative could be to always set the server from username and potentially override by homeserver if passed from the client (as you suggest in the other comment)


// First we need to log in.
RUNTIME.block_on(async move {
let client = builder.user_id(&user).build().await?;
let client = builder.build().await?;

client.login(user, &password, None, None).await?;
let c = Client::new(client, ClientState { is_guest: false, ..ClientState::default() });
Ok(Arc::new(c))
Expand All @@ -82,6 +86,28 @@ fn new_client_builder(base_path: String, home: String) -> anyhow::Result<ClientB
Ok(MatrixClient::builder().user_agent("rust-sdk-ios").store_config(store_config))
}

fn new_client_builder_with_config(base_path: String, home: String, config: ClientConfig, user: &UserId) -> anyhow::Result<ClientBuilder> {
let builder = new_client_builder(base_path, home)?;

let builder = match config.homeserver {
Anderas marked this conversation as resolved.
Show resolved Hide resolved
Some(homeserver) => builder.homeserver_url(&homeserver),
None => builder.user_id_server(&user)
};
Anderas marked this conversation as resolved.
Show resolved Hide resolved

let builder = match config.http_proxy {
Some(proxy) => builder.proxy(proxy),
None => builder
};

Ok(builder)
}

#[derive(Default, Debug)]
pub struct ClientConfig {
homeserver: Option<String>,
http_proxy: Option<String>,
}

#[derive(Default, Debug)]
pub struct ClientState {
is_guest: bool,
Expand Down
2 changes: 1 addition & 1 deletion crates/matrix-sdk/src/client/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ impl ClientBuilder {
/// This method is mutually exclusive with
/// [`homeserver_url()`][Self::homeserver_url], if you set both whatever was
/// set last will be used.
pub fn user_id(self, user_id: &UserId) -> Self {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that user_id naming was confusing (sets server, not user), and server_name_from_user_id was too long, it seems we could simply just rely on server_name(user_id.server_name())

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please not do this in this PR? I'd rather deprecate it for now, not remove it entirely.

pub fn user_id_server(self, user_id: &UserId) -> Self {
gnunicorn marked this conversation as resolved.
Show resolved Hide resolved
self.server_name(user_id.server_name())
}

Expand Down