Skip to content

Proper error propagation? #7

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

Closed
maxcountryman opened this issue Apr 14, 2019 · 4 comments
Closed

Proper error propagation? #7

maxcountryman opened this issue Apr 14, 2019 · 4 comments

Comments

@maxcountryman
Copy link

I'm wondering what the proper way to propagate errors in a handler function is?

For instance, I have a Reqwest client that talks to an external API. The Reqwest client returns a Result. My handler function has this return signature: Result<impl IntoResponse, HandlerError>. Ideally I'd be able to unwrap the Reqwest Result with the question-mark notation, e.g. client.get(...)?. However that won't work and fails with an error like this:

the trait `std::convert::From<reqwest::error::Error>` is not implemented for `lambda_runtime_errors::HandlerError`

Of course I can call .unwrap() on the Result but I'm curious if there's a better way?

@softprops
Copy link
Owner

@maxcountryman the error representation story for aws lambda rust is an evolving one I'd suggest bring this discussion up over here -> awslabs/aws-lambda-rust-runtime#94

HandlerError can be derived for a handful of types with From implementations for strs, failure crate types, and serde json errors.

I would say what you want should be possible with something like

let response = send_reqwest().map_err(|reqwest_error| {
  // convert to str of Fail type here
})?;

I would have to dig though reqwest docs a bit to see what that transformation would look like.

@maxcountryman
Copy link
Author

@softprops thank you very much for pointing out that thread and the suggested pattern. Using that as a framework, this is what I've landed on for now:

fn handler(_: Request, _: Context) -> Result<impl IntoResponse, HandlerError> {
    return match envy::from_env::<Config>() {
        Ok(config) => {
            let client = CloudflareClient::new(&config.auth_email, &config.auth_key);
            client
                .purge_cache(config.zone_identifier)
                .map_err(|reqwest_error| format_err!("{:#?}", reqwest_error))?;

            Ok(json!({}))
        }
        Err(envy_error) => Err(HandlerError::from(format_err!("{:#?}", envy_error))),
    };
}

It's perhaps not the cleanest (and separately I wish there were a better way of handling the config parameters) but I think it's an improvement over just swallowing the error whole.

@softprops
Copy link
Owner

softprops commented Apr 21, 2019

I might try something like this to clean example up.

fn handler(_: Request, _: Context) -> Result<impl IntoResponse, HandlerError> {
    let Config { auth_email, auth_key, zone_identifier } = envy::from_env::<Config>()
        .map_err(|e| format_err!("{}", e))?;
   CloudflareClient::new(&auth_email, &auth_key)
       .purge_cache(zone_identifier)
       .map_err(|e| format_err!("{}", e))?;
    Ok(())
}

full disclosure I haven't compiled this example.

Some approaches I typical take are to

  • leverage ? where I can to make sure errors are propagated earlier in code some the code that follows retains focus and clarity on a business operation.
  • when possible destructure an envy deserialized type with a let binding let Foo { bar, baz, boom } = . The reason being that the code that uses the values depends less on the whole config and more on just the parts they need. I usually weigh this with the number of fields I'm unpacking in scope.
  • this seemed like a fire and forget operation. so returning an empty json object {} probably isn't useful to the client so I'd just return an empty 200 response.

I think once the error representation with the rustlang runtime gets sorted out ? will become a bit easier to access without the extra transform. For instance if it only depended on rusts std::error::Error type, the type most if not all crates implement for their custom errors. Converting to the concrete types the rustlang supports would no longer be necessary.

@maxcountryman
Copy link
Author

This is fantastic! Thank you for taking the time to walk me through your thinking--it's very helpful to a Rust newcomer like myself.

It sounds like the error handling situation should make things even cleaner in the future, but even so this is many times better than my first pass at it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants