Make WpRequestBuilder constructor a non-throwing function

[crazytonyli]

Problem

At the moment, in Swift, we have to use the try keyword to call WordPressAPI(siteURL: ..., ...), because internally the initializer calls WpRequestBuilder::new which may throw an url parsing error. However, the try keyword is not necessary, because the site url argument is a URL type, which means url parsing error would never happen.

Also, in practice, WpRequestBuilder::new is called after the app goes through authentication, which means the site url is guaranteed to be a valid url. The native wrapper should use appropriate URL type (OkHttp3.HttpUrl or Foundation.URL) instead of string type for the "site url" argument.

Solution

Two main changes in this PR:

1. The site_url argument in WpRequestBuilder::new is changed from String to ApiBaseUrl, which means the constructor now no longer throws.
2. ApiBaseUrl is now exposed as a uniffi record, which has a string property, instead of a url::Url property.

Since we can't add constructor nor a static function to a uniffi record type, I added a api_base_url_from_str(str: &str) -> Option<ApiBaseUrl> function to convert a string to a valid url type. This method is used in native wrappers to implement error-free conversion of HttpUrl / URL -> ApiBaseUrl, because HttpUrl and URL are guarenteed to be valid url strings.

A note about Kotlin implementation. I couldn't get Kotlin tests to compile locally, so the Kotlin code is kind of half done. I'll see how the tests run on CI first.

[oguzkocer]

Thanks for the PR @crazytonyli! Having only a throwing constructor in WpRequestBuilder was an issue on my radar, but I was planning to address it once request/endpoint generation and login work was completed. Now that this PR is here, let's try to get it merged to address the issue in the current state.

In my opinion, there are a few issues with the proposed approach:

ApiBaseUrl can be constructed from a random string which breaks all the assumptions from both ApiBaseUrl & WpRequestBuilder. For example, we have helper functions such as by_appending & by_extending which both unwraps the parser result because it trusts that the url it's working with is valid.

This assumption can be satisfied by doing the right thing externally from ApiBaseUrl, such as the approach you've taken with api_base_url_from_str - although that admittedly should return a Result and not an Option. However, that wouldn't be idiomatic Rust because proper Rust code will only trust what it can verify itself. The reason the previous implementation is free to work with those assumptions is that the only way to get an instance of ApiBaseUrl is to go through the necessary error handling. Once that promise is broken, the rest of the implementation will need to be updated as well.

Unfortunately, that promise can not be upheld while using uniffi::Record because the fields of a Record is directly exposed - and can not be hidden. Note, for example, in the previous Rust implementation the url field is not pub making it impossible to build the instance without using the constructor outside of this module.

The solution to this is actually a pretty simple one. We can use uniffi::Object because contrary to uniffi::Record, Object types can only be built by using constructors and their fields are not directly exposed to bindings. If we make this change, WpNetworkRequest 's exported constructor will result in Lift<UniFfiTag> is not implemented for ApiBaseUrl error. I am a little ashamed to admit that I've gotten stuck with that problem a few times myself and didn't realize there was a very simple solution to it. We just need to change the constructor parameter from ApiBaseUrl to Arc<ApiBaseUrl>. In retrospect, this makes a lot of sense. I was reviewing your PR from my mobile this morning and one thing I got a little worried was that we'd be taking the ApiBaseUrl directly and then putting into an Arc to share with all the request builders. I thought the constructor parameter should be Arc<T> instead. I think the reason your implementation was OK was because with Record types, we are given full ownership of the object by the bindings - whether it's from a copy or move. With a uniffi::Object type, we don't get a full ownership and have to work with a reference counted type.

---

There is another seemingly minor, but in my opinion, important problem in the proposed changes. The ApiBaseUrl will now need to parse the String url into a Url type every time it has to work with it. This is very inefficient considering how many times it has to do this operation.

There is no extra action necessary to address this problem, because applying the previous suggestions will solve this as we'll go back to the original implementation. I still wanted to bring this up, because it's something we should keep in mind. For example, if we had to make your suggested changes, I think we'd implement an internal ApiBaseUrl type that works with Url instead of String so that we can do the parsing once to eliminate the extra cost.

---

I hope I did a decent enough job in explaining the issues and how they could be solved. Assuming we are in agreement, my suggestion would be to revert all the changes to several files. It's easier to show this with a commit then to list all the files, so here it is: 5805bad

After that you can change the ApiBaseUrl to be exported as a uniffi::Object.

Instead of modifying the original WpNetworkRequest constructor, my suggestion is to add a second non-throwing constructor named with_api_base_url. I have been using from_foo for the names of these types of constructors, but I think we are supposed to use with_foo instead as suggested in the Rust API guidelines. (Not sure how I personally feel about it, but I don't have a strong enough opinion to go against it)

Kotlin can continue using the constructor from the implementation in trunk. It doesn't suffer from the problem you have explained in the PR description and in fact the suggested solution is a bit more awkward for Kotlin in my opinion. So, let's revert that back.

In order to have a tidy implementation, I think TryFrom<&str> for ApiBaseUrl should return a WpApiError type so we can easily expose that from its constructor and use it directly in WpNetworkRequest constructor. We can consider adding a separate error type for this in the future, but I think this is the simplest approach for now.

That should address everything - except you now will need to update Swift to use the ApiBaseUrl constructor and handle the error appropriately. I think that's the right thing to do anyway, so I don't see that as an issue.

I've made all these changes in a branch, so if you want to directly apply it you can: da5ff92. Alternatively, you can make the changes yourself and then use that as a reference, or.. completely ignore it 😅 I just wanted to provide that because the code is always easier to understand then a wall of text, so I hope you find it useful.

[crazytonyli]

Thanks for the review, Oguz.

ApiBaseUrl can be constructed from a random string which breaks all the assumptions from both ApiBaseUrl & WpRequestBuilder.

Using a random string is not possible in Rust, but is possible in uniffi API. I'm surprised that uniff-rs would generate constructor to assign value to private properties though. I guess we just need to deal with that.

As I mentioned in the PR description, even though we can use a random string to create ApiBaseUrl via uniffi API, we don't do that in practice, because all URLs are valid by the point we create a WpRequestBuilder instance. And ApiBaseUrl is an internal type, which consumers should not use.

api_base_url_from_str - although that admittedly should return a Result and not an Option.

I took a shortcut here, because that's an private Rust function, but an public uniffi API. The ApiBaseUrl constructor and the TryFrom<&str> implementation are the only two options for Rust to create ApiBaseUrl instances.

Since the private Rust API is only used by the wrapper, I don't think we need the errors. Because again the error would never happen in practice. Even if the error happens, the native wrapper doesn't actually "handle" (like error recovering or showing error message) it, other than crashing.

The solution to this is actually a pretty simple one. We can use uniffi::Object

Agreed. However, given that solution caused serious issues in #135, I didn't go with that here. If that's not an issue here, I'm more than happy to change it to an Object type.

The ApiBaseUrl will now need to parse the String url into a Url type every time it has to work with it. This is very inefficient considering how many times it has to do this operation.

I thought about that. Yes, in theory it's less inefficient. But in practice, in most cases, we only convert String to Url once, which is the case before and after this PR changes. But still, I agree with you it's not ideal we have to do this conversion in a getter function.

---

Another thing I forgot to mention is I want to remove WpApiError::SiteUrlParsingError. Because again it's not an error that'll happen, but the apps would have to write code to "handle" it. Whatever their error handling code is, it'll just be dead code. Please let me know your thoughts regarding removing WpApiError::SiteUrlParsingError.

[oguzkocer]

As I mentioned in the PR description, even though we can use a random string to create ApiBaseUrl via uniffi API, we don't do that in practice, because all URLs are valid by the point we create a WpRequestBuilder instance. And ApiBaseUrl is an internal type, which consumers should not use.

This type is exposed through uniffi, which means any client that uses the bindings might use it incorrectly. Let's treat the projects in this repo as proper open source projects and not take into account whether we are consuming it correctly or not. That's how projects become client centric and I'd like to avoid it as much as possible. We might even be the ones who trip up when we are implementing this for other languages.

Thinking about how we are currently using it is also a very non-Rust like approach. As I mentioned in the PR review, idiomatic Rust will only trust things it can verify at call site and handle it accordingly. If it can't verify that something can only be called with valid data, it has to handle the invalid data case.

Since the private Rust API is only used by the wrapper, I don't think we need the errors. Because again the error would never happen in practice. Even if the error happens, the native wrapper doesn't actually "handle" (like error recovering or showing error message) it, other than crashing.

This is not correct. Kotlin reports this as a recoverable error. Even if that weren't the case, I'd strongly disagree with the argument. The current clients are not the only clients we should take into account.

When we started this project we set out to do it only once and do it the right way. We picked Rust partially because it helps us achieve that. It forces you to think about this kind of thing and we want to take advantage of that. As library developers we need to be defensive in our implementation and the current native wrappers not actually utilizing an error (which again is not true) can't be our reasoning.

I thought about that. Yes, in theory it's less inefficient. But in practice, in most cases, we only convert String to Url once, which is the case before and after this PR changes. But still, I agree with you it's not ideal we have to do this conversion in a getter function.

This is not correct either. As mentioned in my review, every time by_appending or by_extending is called, we'd re-parse the string into Url after the proposed changes from this PR. These are extensively used in endpoint types. In practice, it means we'll re-parse the string at least once for every request and multiple times for some of them. Before the change, we'd parse it once at the initialization and never again.

[crazytonyli]

Thinking about how we are currently using it is also a very non-Rust like approach. As I mentioned in the PR review, idiomatic Rust will only trust things it can verify at call site and handle it accordingly.

Can you help me understand this by giving an example of how the updated ApiBaseUrl type can be used in a non-idiomatic way?

This is not correct. Kotlin reports this as a recoverable error. Even if that weren't the case, I'd strongly disagree with the argument. The current clients are not the only clients we should take into account.

I should have been more clear. By "recovering", I mean it from an application perspective, not programming language perspective. Like automatic retry.

I want to talk about handling the URL parsing error specifically. Please bear in mind that fn api_base_url_from_str(str: &str) -> Option<ApiBaseUrl> is an uniffi API that can only be used in Kotlin/Swift/Python.

If we work on the url crate, I don't think returning an Option is a good API at all. It's a way better API to return a concrete error with as much information about how the library fails to parse the given string.

However, we are working on a WordPress REST client library, where parsing url is a tiny part of this library, there is nothing wrong with returning a specific error that says "failing to parse this string as url because ...". But I don't think that's absolute necessary. IMHO, it's totally okay for this library to return a Option to indicate that url parsing failed for whatever reason.

As mentioned in my review, every time by_appending or by_extending is called, we'd re-parse the string into Url after the proposed changes from this PR.

Sorry for repeating myself. I get that and I agree with. What I was saying was, I agree that this conversion is not ideal, but in practice, all the existing code only call it once per request. Again, I agree that it's not ideal and we should avoid it, like using a lazy variable or something.

[oguzkocer]

Can you help me understand this by giving an example of how the updated ApiBaseUrl type can be used in a non-idiomatic way?

Sure. The code below wouldn't be considered idiomatic because it unwraps a Result when it can't sufficiently prove that standard_absolute_url is a parsed url as it's possible to create an instance of ApiBaseUrl with invalid data due to the generated code by uniffi::Record. As a result, by_appending would need to be updated to return a Result<T, E>.

Once uniffi::Record is removed, it can be considered it's "correct" code, but I don't think it's a good one. So, whether it's idiomatic would depend on where you draw the line.

#[derive(Debug, Clone, uniffi::Record)]
pub struct ApiBaseUrl {
    standard_absolute_url: String,
}

impl ApiBaseUrl {
    fn by_appending(&self, segment: &str) -> Url {
        self.url()
            .clone()
            .append(segment)
            .expect("ApiBaseUrl is already parsed, so this can't result in an error")
    }

---

Just to provide further context about how this stuff is handled in Rust, let's say you wanted to expose all the fields of a type as public, but you still want to control how your type is constructed, you can do the following:

pub struct Foo {
    pub bar: u32,
    // Force consumers to use the constructor
    _baz: (),
}

and now the following code will not compile:

mod showcase {
    pub struct Foo {
        pub bar: u32,
        // Force consumers to use the constructor
        _baz: (),
    }

    impl Foo {
        fn new(bar: u32) -> Self {
            Self { bar, _baz: () }
        }
    }
}

fn test() {
    // won't compile
    let _ = showcase::Foo { bar: 2, _baz: () };

    // does compile
    let _ = showcase::Foo::new(2);
}

A similar, but still possibly instructive example would be how you handle Send and Sync for your types. These are marker traits that get automatically implemented by compiler as long as all fields in a type is Send or Sync and they represent whether it's safe to send a type to another thread and whether it's safe to share a type between threads respectively. Most of the time this is exactly what you want, but if for some reason you need to disallow it, there is a PhantomData marker type you can use for this like so:

pub struct Foo {
    pub bar: u32,
    // disable `Send` & `Sync` auto traits
    _phantom: std::marker::PhantomData<std::rc::Rc<()>>,
}

fn send_function<T: Send>(bar: T) {}

fn test() {
    let f = Foo {
        bar: 2,
        _phantom: std::marker::PhantomData,
    };
    // won't compile with:
    // `Rc<()>` cannot be sent between threads safely
    send_function(f);
}

That's all to say that, in idiomatic Rust, you are expected to go out of your way (if necessary) to ensure correctness of your code.

[oguzkocer]

Superseded by #164.