anterofit icon indicating copy to clipboard operation
anterofit copied to clipboard

error handling

Open little-dude opened this issue 8 years ago • 7 comments

I started to build a client for the github api: https://github.com/little-dude/ghrs

One thing I could not figure out is how to handle cases where the server returns an error (403 or 404 for example). Ideally, when this happens, I'd like to return an error that contains:

  • the status code
  • the response headers
  • the response body

That way the user can take an appropriate action depending on the error.

Currently, I only get a deserialization error, which is not very helpful. Is there a way to do this?

little-dude avatar Mar 14 '17 18:03 little-dude

I intended this use-case to be handled via wrapping the return type of the service method with net::response::TryWithRaw which will give the raw Hyper response including status code and headers. The body is accessible too but it may have been partially read by the deserialization process. But this does necessitate leaking this information to the user, or concealing your service trait, which is not what I'd like.

I think to support this properly, there needs to be a way to map the result from within the service method body, and set a different error type for Request, so you can get the raw response and then package it up into your own error. I'll have to mull over this.

abonander avatar Mar 14 '17 23:03 abonander

Thank you for the suggestion, I'll give a try to TryWithRaw for now. That also solves the problem of accessing the header of the response (wether there is an error or not), which is necessary to handle paging in the github api.

little-dude avatar Mar 15 '17 01:03 little-dude

I tried something like this:

#[macro_use]
extern crate anterofit;
extern crate serde;
#[macro_use]
extern crate serde_derive;
extern crate serde_json;

use anterofit::{Request, JsonAdapter};
use anterofit::net::response::{TryWithRaw};
use serde::{Deserialize};

#[derive(Debug, Deserialize)]
pub struct Foo;

#[derive(Debug)]
pub struct User {
    adapter: JsonAdapter,
}

service! {
    pub trait MyService {
        fn try_get_foo(&self) -> TryWithRaw<Foo> {
            GET("")
        }
    }

    impl for User{
        |this| &this.adapter
    }

    impl[T] for T [where T: AsRef<User>] {
        |this| &this.as_ref().adapter
    }
}

impl User {
    pub fn get_foo(&self) -> Request<Foo> {
        self.try_get_foo().on_result(|res| {
            match res {
                Ok(TryWithRaw { raw: response, result: Ok(res) }) => {
                    Ok(res)
                },
                Ok(TryWithRaw { raw: response, result: Err(e) }) => {
                    Err("Something went wrong while getting foo".to_string())
                },
                Err(e) => {
                    Err("Something went really wrong while getting foo".to_string())
                },
            }
        })
    }
}

With this, I was trying to get a Result<Foo, String>, but this does not work because on_result must return an anterofit::Result<T> which is an alias (do we say alias in Rust?) for Result<T, anterofit::error::Error>. So I cannot really use my own errors with this.

Is this what you meant by "wrapping the return type of the service method with net::response::TryWithRaw" ?

I tried to wrap Request into my own type, something like:

use anterofit;
use anterofit::net::Call;

pub struct Request<'a, T=()>(anterofit::Request<'a, T>);

impl<'a, T> Request<'a, T> {

    pub fn new(req: anterofit::Request) -> Self {
        Request(req)
    }
    pub fn immediate(res: anterofit::Result<T>) -> Request<'static, T> {
        Request(anterofit::Request::immediate(res))
    }
    pub fn exec_here(self) -> Result<T, String> {
        self.0.exec_here().map_err(|e| "my error".to_string()
    }
    pub fn is_immediate(&self) -> bool {
        self.0.is_immediate()
    }

}

impl<'a, T> Request<'a, T> where T: Send + 'static {
    pub fn exec(self) -> Call<T> {
        self.0.exec()
    }

    pub fn on_complete<F, R>(self, on_complete: F) -> Request<'a, R>
        where F: FnOnce(T) -> R + Send + 'static, R: Send + 'static
    {
        self.on_result(|res| res.map(on_complete))
    }

    // this does not work, since self.0.on_result signature does not match
    pub fn on_result<F, R>(self, on_result: F) -> Request<'a, R>
        where F: FnOnce(anterofit::Result<T>) -> Result<R, String> + Send + 'static, R: Send + 'staticV
        Request(self.0.on_result(on_result))
    }
}

But:

  • don't I need to wrap anterfit::net::Call too ?
  • I could not figure out how to handle on_result

little-dude avatar Mar 27 '17 01:03 little-dude

I've got kind-of a plan to support this. On the parse-generics branch, I'm experimenting with a syntax for forcing a full return type using -!> instead of ->, and then I have an idea to add a couple macros that let you override some of the logic in the generated service method, but the concept is far from complete. Feedback and input are most definitely welcome.

abonander avatar Mar 27 '17 02:03 abonander

What do you mean by "full return type" exactly ?

Since you're taking suggestions, I have one although it's unrelated to error handling.

Currently, for requests that take query parameters, I do something like this:


#[derive(Default, Debug)]
pub struct GetNotificationsParams {
    pub all: Option<bool>,
    pub participating: Option<bool>,
    pub since: Option<DateTime<UTC>>,
    pub before: Option<DateTime<UTC>>,
    pub page: Option<u32>,
    pub per_page: Option<u32>,
}
service! {
    pub trait RootService {
        fn get_notifications(&self, opts: Option<GetNotificationsParams>) -> TryWithRaw<Vec<Thread>> {
            GET("notifications");
            |mut builder| {
                if let Some(ref opts) = opts {
                    if let Some(all) = opts.all {
                        builder.head_mut().query(&[("all", &all)]);
                    }
                    if let Some(participating) = opts.participating {
                        builder.head_mut().query(&[("participating", &participating)]);
                    }
                    if let Some(since) = opts.since {
                        builder.head_mut().query(&[("since", since.to_rfc3339())]);
                    }
                    if let Some(before) = opts.before {
                        builder.head_mut().query(&[("before", before.to_rfc3339())]);
                    }
                    if let Some(page) = opts.page {
                        builder.head_mut().query(&[("page", &page)]);
                    }
                    if let Some(per_page) = opts.per_page {
                        builder.head_mut().query(&[("per_page", &per_page)]);
                    }
                }
                Ok(builder)
            }
        }
    }
}

This works well, but the caller always has to pass the extra argument:

// if no query parameter
root.get_notifications(None)
// or with some parameters
root.get_notification(Some(GetNotificationsParams {
    //
}))

It would be nice if we coud just write:

// if not query parameter
root.get_notifications()
// or if we want to set a few query parameters
root.get_notifications().set_page(2).set_participating(true)

That would require the service methods to return some kind of RequestBuilder though, which we would need to build into Request.

I don't know how realistic this is, but since you asked for suggestions here is one :)

little-dude avatar Mar 27 '17 15:03 little-dude

So currently, when you define a service method and declare some return type T, it gets rewritten as Request<T>:

service! {
    pub trait MyService {
        fn get_api_version(&self) -> String {
            GET!("/version")
        }
    }
}
// becomes
pub trait MyService {
    fn get_api_version(&self) -> ::anterofit::Request<String> {
        // ...
    }
}

Using -!> to declare a return type would override this, e.g.:

pub type MyRequest<T> = ::anterofit::Request<T>;

service! {
    pub trait MyService {
        fn get_api_version(&self) -!> MyRequest<String> {
            GET!("/version")
        }
    }
}
// becomes
pub trait MyService {
    fn get_api_version(&self) -> MyRequest<String> {
        // ...
    }
}

I'm still not sure on the exact token I want to use; I wanted something that draws attention to itself but my current choice could be confused with -> !, which is used to designate a diverging function (one that will never return, e.g. panics or aborts or loops forever). Maybe => or -=> but they seem too subtle.

I hadn't considered the use case you mentioned but I can see its utility. You could possibly do something like this by overriding the return type as shown above and short-circuiting the conversion from RequestBuilder to Request somehow. I've got a similar use-case in mind for allowing mapping the request result within a service method definition (as opposed to calliing Request::on_result() later) but the concept isn't fleshed out yet.

abonander avatar Mar 27 '17 21:03 abonander

Fwiw I like the => version, even though it's subtle.

little-dude avatar Mar 28 '17 16:03 little-dude