Invalid url should return an error instead of calling panic

[Jonathas-Conceicao]

When building a request, if one provides a invalid url, e.g., http://foo.bar:---, the lib panics. I would expect that the parsing error would be returned at some point.

In my use case the application I have may make requests to links fully provided by the user, and since I can't have the hole app shutting down on a user input error I have to validate the url before passing it to surf's client.

Here's a example of one of these unwrap calls that I'm talking about: https://github.com/http-rs/surf/blob/e9ee6fe54f266e0a099905987ef5c2658d813f91/src/client.rs#L140-L143

One possible way that I think would improve this without needing to change the API would be to have such errors stored in the Request object and returned when the struct is eventually evaluated into a Result<Response, Error>

[DavidBM]

We have been bitten by this too

[yoshuawuyts]

The main blocker for this has been a missing TryFrom<str> impl for Url, but apparently v2.2.0 came out a few days ago which should unblock this from a technical perspective.

This may require a major version bump though, since going from impl AsRef<str> to TryInto<Url> should be a breaking change. Even more so if it's decided to change the return signature to include an error.

[DavidBM]

Would you accept a PR for this? (following what you described, of course)

Edit: I have looked at the code, but definitively the internals and the external api will change, so no idea of how to continue. As I see it, it would require to possibly return an error when creating the request (but that would be a bit unergonomically) or when sending (query, recv_string, recv_json, recv_form, build and send methods) it and just adding the correct error message.

[finnbear]

Fixing this would benefit SurrealDB, specifically by no longer requiring it to parse the Url before surf does.

It may be possible to do this without an API change: Playground

pub fn main() {
    use std::any::Any;

    #[derive(Debug)]
    struct Url(String);

    impl AsRef<str> for Url {
        fn as_ref(&self) -> &str {
            &self.0
        }
    }

    // *Rustc wanted `uri` to be `'static` but there might be a way around that...
    fn get(uri: impl AsRef<str> + 'static) {
        if let Some(url) = (&uri as &dyn Any).downcast_ref::<Url>() {
            println!("Don't have to parse: {:?}", url);
        } else {
            println!("Have to parse: {}", uri.as_ref());
        }
    }

    get("foo");
    get(Url(String::from("foo")));
}

Users can now parse the Url (and handle errors with that) prior to calling surf functions.

Thoughts?