Use ClientBuilder pattern in SDK FFI #739

Anderas · 2022-06-07T15:39:38Z

Change the SDK-ffi architecture to use builder pattern, where the client creates and configures ClientBuilder before creating a client. Further operations, such as login are then methods on the client rather than top-level functions. This is done to closely follow the architecture of the SDK and only provide convenience wrappers for hidden types, concurrency and other iOS-incompatible features.

Additionally expose homeserver_url method to configure custom homeserver, such as localhost.

Define new ClientConfig type which can customize parameters of the client such as homeserver (incl. localhost) or http_proxy.

crates/matrix-sdk/src/client/builder.rs

Anderas · 2022-06-07T15:42:54Z

crates/matrix-sdk-ffi/src/lib.rs

    let builder = new_client_builder(base_path, session.user_id.to_string())?
-        .homeserver_url(&homeurl)
-        .user_id(&session.user_id);
+        .homeserver_url(&homeurl);


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.

crates/matrix-sdk-ffi/src/lib.rs

gnunicorn

I understand the notion - this is mainly for allowing developers to work with a local homeserver, right?

But I'd argue this make the API more confusing as we now have an unclear (and un-intuitive) preference of configuration options. Maybe we can make it more explicit by:
a) make the ClientOption optional itself; and
b) call them config_overwrites or the homeserver-param _overwrite_homeserver

To make clear what has precedence and what is optional...

crates/matrix-sdk-ffi/src/lib.rs

gnunicorn · 2022-06-07T15:52:10Z

crates/matrix-sdk-ffi/src/lib.rs

+    let user = Box::<UserId>::try_from(username.clone())?;
+    let builder = new_client_builder_with_config(base_path, username, config, &user)?;


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.

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)

crates/matrix-sdk/src/client/builder.rs

gnunicorn · 2022-06-07T16:01:22Z

this needs style fixes. You probably want to run cargo xtask fixup and commit the changes, @Anderas

codecov · 2022-06-07T16:16:39Z

Codecov Report

Merging #739 (3f970ad) into main (091fab8) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #739   +/-   ##
=======================================
  Coverage   77.76%   77.76%           
=======================================
  Files          92       92           
  Lines       13675    13675           
=======================================
  Hits        10635    10635           
  Misses       3040     3040

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 091fab8...3f970ad. Read the comment docs.

poljar · 2022-06-07T19:23:04Z

I understand the notion - this is mainly for allowing developers to work with a local homeserver, right?

But I'd argue this make the API more confusing as we now have an unclear (and un-intuitive) preference of configuration options. Maybe we can make it more explicit by: a) make the ClientOption optional itself; and b) call them config_overwrites or the homeserver-param _overwrite_homeserver

To make clear what has precedence and what is optional...

I might be late to the party, but why do we have new_client_builder() instead of a normal constructor? Why aren't we just mimicking the matrix_sdk API 1:1?

gnunicorn · 2022-06-08T13:22:28Z

@poljar this FFI API predates the client-builder pattern and we never updated it to it ... arguably, we might want to do that though. @Anderas what do you think? exposing ClientBuilder and its functions instead and use that to construct the proper client and doing a login on it?

The FFI does still have the handy tooling to serialize the homeserver into the token, too, for convenience. But we could keep that and have that reconstruct the data for the client-builder.

Anderas · 2022-06-09T13:54:06Z

@poljar this FFI API predates the client-builder pattern and we never updated it to it ... arguably, we might want to do that though. @Anderas what do you think? exposing ClientBuilder and its functions instead and use that to construct the proper client and doing a login on it?

The FFI does still have the handy tooling to serialize the homeserver into the token, too, for convenience. But we could keep that and have that reconstruct the data for the client-builder.

That's interesting, I'd like to give this a go. Are you suggesting the client always needs to pass ClientBuilder, but can leave most of its fields as None if there are no overwrites, or would the client builder itself be optional?

Regarding the localhost, maybe I am misunderstanding it, but it seems we cannot extract localhost out of the username unless the localhost runs on https and supports .well-known. And so the only option is to override the servername / url taken out of the username, is this correct?

poljar · 2022-06-09T21:02:05Z

@poljar this FFI API predates the client-builder pattern and we never updated it to it ... arguably, we might want to do that though. @Anderas what do you think? exposing ClientBuilder and its functions instead and use that to construct the proper client and doing a login on it?
The FFI does still have the handy tooling to serialize the homeserver into the token, too, for convenience. But we could keep that and have that reconstruct the data for the client-builder.

That's interesting, I'd like to give this a go. Are you suggesting the client always needs to pass ClientBuilder, but can leave most of its fields as None if there are no overwrites, or would the client builder itself be optional?

The ClientBuilder has a build() method which returns a Client object. Sadly the examples for the ClientBuilder are a bit poor but the function signature of the ClientBuilder::build() method should make things clear:

pub async fn build(self) -> Result<Client, ClientBuildError>

Regarding the localhost, maybe I am misunderstanding it, but it seems we cannot extract localhost out of the username unless the localhost runs on https and supports .well-known. And so the only option is to override the servername / url taken out of the username, is this correct?

Correct, server discovery requires a .well-known to be set up. The server discovery algorithm has been implemented as the spec defines it and I don't think we'll want to add any special handling of localhost. If people want to avoid server discovery, setting the homeserver address manually should always be possible.

gnunicorn · 2022-06-16T10:07:27Z

After having experimented a bit, we've found that the client-builder doesn't really work for FFI as the pattern requires &mut self and consumption of self, which UniFFI doesn't really have any concept of. Thus, for moving forward now, we've decided to stay on the (optional) config-option, which can generate the proper ClientBuilder and uses that on the rust-side.

Anderas · 2022-06-16T10:19:39Z

After having experimented a bit, we've found that the client-builder doesn't really work for FFI as the pattern requires &mut self and consumption of self, which UniFFI doesn't really have any concept of. Thus, for moving forward now, we've decided to stay on the (optional) config-option, which can generate the proper ClientBuilder and uses that on the rust-side.

Thanks for this summary, I was just about to write something along these lines

jplatte · 2022-06-16T10:44:19Z

Is our builder not Clone? I would think we could use ~~Arc::make_mut~~ Arc::try_unwrap to have a reasonably efficient builder pattern that clones in the non-happy path.

gnunicorn · 2022-06-16T11:28:54Z

Is our builder not Clone?

No and there is good reason for it: as it contains the store_config, which may or may not allow to be cloned (for good reason), we can not expect ClientBuilder to be clone either...

jplatte · 2022-06-16T12:57:00Z

I think we can just put it behind an Arc instead of a Box. I can have a closer look tomorrow.

Anderas · 2022-06-27T07:24:35Z

bindings/matrix-sdk-ffi/src/lib.rs

@@ -25,63 +24,6 @@ pub use matrix_sdk::ruma::{api::client::account::register, UserId};

 pub use self::{backward_stream::*, client::*, messages::*, room::*};

-pub fn guest_client(base_path: String, homeurl: String) -> anyhow::Result<Arc<Client>> {


The guest client login is not used by element-x, only in a unit test of sample project that could be recreated in other ways, so I did not port this method yet. If I missed some other reason why this is important, I can bring it back

Anderas · 2022-06-27T07:25:14Z

bindings/matrix-sdk-ffi/src/client.rs

@@ -44,6 +44,25 @@ impl Client {
        }
    }

+    pub fn login(&self, username: String, password: String) -> anyhow::Result<()> {


These are the methods that used to exist as top-level functions, now methods on the Client. Internally they pretty much just call the inner SDK methods of the same name

Anderas · 2022-06-27T07:26:07Z

crates/matrix-sdk/src/client/builder.rs

-    /// 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 {


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())

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

Anderas · 2022-06-27T07:27:37Z

bindings/apple/MatrixRustSDKTests/MatrixRustSDKTests.swift

+                .username(username: "@test:domain")
+                .build()
+
+            try client.login(username: "@test:domain", password: "test")


The use of the new ClientBuilder API highlights that username now has to be passed twice, which is not ideal. The client builder needs username to determine file store path, and potentially homeserver, if not set explicitely. The client needs username to construct user to log in with.

Potentially the client could retrieve the username directly from the builder, but not sure if api like client.login(password) is very good.

Anderas · 2022-06-27T07:30:04Z

After further discussions with @jplatte I refactored the FFI layer to follow the same architecture as the inner SDK, which means exposing a configurable ClientBuilder, later used to build a Client. This means that FFI crate and SDK crate follow the same architecture / patterns.

There are some potentially controversal changes in this PR (removing guest login, removing user_id method) so happy to discuss those further. Also I think this PR has implications for @pixlwave 's work here

bindings/matrix-sdk-ffi/src/client.rs

Co-authored-by: Jonas Platte <jplatte@element.io>

jplatte · 2022-06-27T09:54:46Z

bindings/matrix-sdk-ffi/src/client_builder.rs

+impl Default for ClientBuilder {
+    fn default() -> Self {
+        Self::new()
+    }
+}


What is this Default impl used for?

This was a clippy failure here pointing to this rule

Ah, that's pretty useless in this context but whatever.

jplatte · 2022-06-27T09:55:32Z

crates/matrix-sdk/src/client/builder.rs

-    /// 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 {


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

…rust-sdk into andy/homeserver_login

jplatte · 2022-06-27T11:49:56Z

@poljar @gnunicorn do you want to re-review before this is merged?

gnunicorn

Nice,

Just some minor remarks

gnunicorn · 2022-06-29T07:13:40Z

bindings/matrix-sdk-ffi/src/client_builder.rs

+
+    pub fn username(self: Arc<Self>, username: String) -> Arc<Self> {
+        let mut builder = unwrap_or_clone_arc(self);
+        builder.username = Some(username);


Why hold an inner builder but still have all parameters twice? Why not just

Suggested change

builder.username = Some(username);

builder.inner = builder.inner.username(username);

For the homeserver, too...

We can't currently read those fields from the inner builder, but they're needed in build. I think there's room for improvement here, but it's not entirely clear how this should be improved. In general I think the FFI layer shouldn't add its own logic like deriving a store path from a base path plus username / homeserver, but doing that immediately would mean extra work for using the FFI compared to before this change, which is also not great.

gnunicorn · 2022-06-29T07:15:42Z

bindings/matrix-sdk-ffi/src/client_builder.rs

+            inner: MatrixClient::builder().user_agent("rust-sdk-ios"),
+        }
+    }
+


Docs should probably say that base path must be set. Which let's me wonder if we shouldn't have that as a parameters on new so it is enforced and never Option that could fail at runtime (when calling build) . Wdyt?

I think Andy actually wanted to have it as a new parameter, and I wanted to have it as a builder method instead. My reasoning for that was that it's the usual pattern for builders, and a new ClientBuilder("/some/path") invocation wouldn't be quite clear. (and it would be a very easy to catch programmer error when it isn't set)

However, this point only applies to Kotlin since UniFFI's Swift codegen requires callers to re-state argument names (as in new ClientBuilder(base_path: "/some/path")). So I don't insist on keeping it this way.

Yea I generally favor API that makes invalid states impossible to represent, which would require passing them to new. This sounds like an example of clash between SDK and client-side consistency. Happy to follow up on this in a new PR.

gnunicorn · 2022-06-29T07:18:07Z

bindings/matrix-sdk-ffi/src/client_builder.rs

+        let builder = unwrap_or_clone_arc(self);
+
+        let base_path = builder.base_path.context("Base path was not set")?;
+        let username = builder


This would fail for guest clients... but as this also drops support fir them, I guess that's fine...

feat(ios): Custom homeserver when logging in

a9221c8

Define new ClientConfig type which can customize parameters of the client such as homeserver (incl. localhost) or http_proxy.

Anderas requested a review from a team June 7, 2022 15:40

Anderas commented Jun 7, 2022

View reviewed changes

crates/matrix-sdk/src/client/builder.rs Outdated Show resolved Hide resolved

Anderas commented Jun 7, 2022

View reviewed changes

crates/matrix-sdk-ffi/src/lib.rs Outdated Show resolved Hide resolved

Anderas added 2 commits June 7, 2022 16:48

Missing rename

943ba8d

Rename method and use mutable builder

3cfaab8

gnunicorn suggested changes Jun 7, 2022

View reviewed changes

Make ClientConfig optional

184328a

Anderas requested review from gnunicorn and jplatte June 16, 2022 10:19

Anderas added 2 commits June 16, 2022 11:23

Merge branch 'main' into andy/homeserver_login

f72a93d

Format

6c23a69

jplatte mentioned this pull request Jun 17, 2022

Some builder improvements #770

Merged

Anderas added 4 commits June 17, 2022 14:58

Merge branch 'main' into andy/homeserver_login

f239c9d

Refactor FFI to use the builder pattern

1f6cd44

Remove convenience method

1d39e15

Merge branch 'main' into andy/homeserver_login

c155c61

Anderas changed the title ~~Custom homeserver when logging in~~ Use ClientBuilder pattern in SDK FFI + allow custom homeserver when logging in Jun 27, 2022

Anderas changed the title ~~Use ClientBuilder pattern in SDK FFI + allow custom homeserver when logging in~~ Use ClientBuilder pattern in SDK FFI Jun 27, 2022

Anderas commented Jun 27, 2022

View reviewed changes

Anderas added 2 commits June 27, 2022 09:06

Fix tests and clippy

79be457

Default for ClientBuilder

ed8a69c

jplatte reviewed Jun 27, 2022

View reviewed changes

bindings/matrix-sdk-ffi/src/client.rs Outdated Show resolved Hide resolved

Update bindings/matrix-sdk-ffi/src/client.rs

55b3710

Co-authored-by: Jonas Platte <jplatte@element.io>

jplatte reviewed Jun 27, 2022

View reviewed changes

Anderas added 3 commits June 27, 2022 12:42

Revert removal of user_id method

0aa33df

Merge branch 'andy/homeserver_login' of github.com:matrix-org/matrix-…

a7c9dda

…rust-sdk into andy/homeserver_login

Unused import

3f970ad

jplatte approved these changes Jun 27, 2022

View reviewed changes

gnunicorn approved these changes Jun 29, 2022

View reviewed changes

jplatte merged commit 3c6d159 into main Jun 29, 2022

jplatte deleted the andy/homeserver_login branch June 29, 2022 11:13

Anderas mentioned this pull request Jun 29, 2022

Implement new ClientBuilder pattern for login element-hq/element-x-ios#120

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use ClientBuilder pattern in SDK FFI #739

Use ClientBuilder pattern in SDK FFI #739

Anderas commented Jun 7, 2022 •

edited

Loading

Anderas Jun 7, 2022

gnunicorn left a comment

gnunicorn Jun 7, 2022

Anderas Jun 9, 2022

gnunicorn commented Jun 7, 2022

codecov bot commented Jun 7, 2022 •

edited

Loading

poljar commented Jun 7, 2022

gnunicorn commented Jun 8, 2022

Anderas commented Jun 9, 2022

poljar commented Jun 9, 2022

gnunicorn commented Jun 16, 2022

Anderas commented Jun 16, 2022

jplatte commented Jun 16, 2022 •

edited

Loading

gnunicorn commented Jun 16, 2022

jplatte commented Jun 16, 2022

Anderas Jun 27, 2022

Anderas Jun 27, 2022

pixlwave Jun 27, 2022

Anderas Jun 27, 2022

jplatte Jun 27, 2022

Anderas Jun 27, 2022

Anderas commented Jun 27, 2022

jplatte Jun 27, 2022

Anderas Jun 27, 2022

jplatte Jun 27, 2022

jplatte Jun 27, 2022

jplatte commented Jun 27, 2022

gnunicorn left a comment

gnunicorn Jun 29, 2022

jplatte Jun 29, 2022

gnunicorn Jun 29, 2022

jplatte Jun 29, 2022

Anderas Jun 29, 2022 •

edited

Loading

gnunicorn Jun 29, 2022

		let user = Box::<UserId>::try_from(username.clone())?;
		let builder = new_client_builder_with_config(base_path, username, config, &user)?;

		@@ -25,63 +24,6 @@ pub use matrix_sdk::ruma::{api::client::account::register, UserId};

		pub use self::{backward_stream::, client::, messages::, room::};

		pub fn guest_client(base_path: String, homeurl: String) -> anyhow::Result<Arc<Client>> {

	builder.username = Some(username);
	builder.inner = builder.inner.username(username);

Use ClientBuilder pattern in SDK FFI #739

Use ClientBuilder pattern in SDK FFI #739

Conversation

Anderas commented Jun 7, 2022 • edited Loading

Choose a reason for hiding this comment

gnunicorn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnunicorn commented Jun 7, 2022

codecov bot commented Jun 7, 2022 • edited Loading

Codecov Report

poljar commented Jun 7, 2022

gnunicorn commented Jun 8, 2022

Anderas commented Jun 9, 2022

poljar commented Jun 9, 2022

gnunicorn commented Jun 16, 2022

Anderas commented Jun 16, 2022

jplatte commented Jun 16, 2022 • edited Loading

gnunicorn commented Jun 16, 2022

jplatte commented Jun 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anderas commented Jun 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jplatte commented Jun 27, 2022

gnunicorn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anderas Jun 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anderas commented Jun 7, 2022 •

edited

Loading

codecov bot commented Jun 7, 2022 •

edited

Loading

jplatte commented Jun 16, 2022 •

edited

Loading

Anderas Jun 29, 2022 •

edited

Loading