Skip to content
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

Add support for anyhow::Error #1241

Closed
wants to merge 1 commit into from
Closed

Conversation

nivekuil
Copy link

anyhow is an error handling crate like failure. It seems to be better supported and more popular now as it uses std::error::Error and not a custom trait. This PR adds the same sort of support failure has, also behind a feature flag.

@fafhrd91
Copy link
Member

I am not planing to support it. and failure will be removed in near future

@fafhrd91 fafhrd91 closed this Dec 28, 2019
@codecov
Copy link

codecov bot commented Dec 28, 2019

Codecov Report

Merging #1241 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1241   +/-   ##
=======================================
  Coverage   80.45%   80.45%           
=======================================
  Files         159      159           
  Lines       18518    18518           
=======================================
  Hits        14899    14899           
  Misses       3619     3619
Impacted Files Coverage Δ
actix-http/src/error.rs 79.82% <ø> (ø) ⬆️

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 7bd2270...267a3f3. Read the comment docs.

@tmladek
Copy link

tmladek commented Sep 13, 2020

Hello, may I ask why is anyhow unsupported - especially seeing as the docs still encourage using the now deprecated and unmaintained failure crate?

@popzxc
Copy link
Member

popzxc commented Sep 13, 2020

Hmm. I could not find any mentions of the failure in the actix-web docs. Could you please provide an example? It seems that it's a documentation issue.

@tmladek
Copy link

tmladek commented Sep 13, 2020

Apologies! I couldn't find it now, though it must have been there quite recently, as I haven't been working with actix for more than a few weeks, and I clearly remember learning about failure (and through it, about anyhow) via the link in the docs.

In that case, may I instead ask why was automatic error compatibility abandoned completely?

A generic, simple way to return a 500 on the off-chance a rare error occurs is something I'd wager to be common in most projects utilizing actix, but right now it requires implementing and quite awkward conversions from/to a custom error response struct. Or is there a strategy I am not seeing?

@popzxc
Copy link
Member

popzxc commented Sep 13, 2020

Well, actix provides a bunch of helper functions to generate required errors from any type that implements Display.
For 500 it will be ErrorInternalServerError.

Isn't it flexible enough for your needs?

@tmladek
Copy link

tmladek commented Sep 13, 2020

Well, it's flexible enough, but it turns every value? into a value.map_err(ErrorInternalServerError)?, which adds up to a lot of ErrorInternalServerErrors in the end, even in rather straightforward code:

#[get("/get/{object_id}")]
pub async fn get_object(
    state: web::Data<State>,
    address_str: web::Path<String>,
) -> Result<HttpResponse, actix_web::Error> {
    let response: Result<Vec<Entry>> = state
        .db
        .send(crate::database::RetrieveObject {
            target: Address::decode(
                &decode(address_str.into_inner()).map_err(ErrorInternalServerError)?,
            )
            .map_err(ErrorInternalServerError)?,
        })
        .await.map_err(ErrorInternalServerError)?;

    debug!("{:?}", response);

    let entries = response.map_err(error::ErrorInternalServerError)?;
    let mut result: HashMap<String, serde_json::Value> = HashMap::new();
    for entry in entries {
        result.insert(
            encode(entry.identity).map_err(ErrorInternalServerError)?,
            json!({
                "target": encode(entry.target.encode().map_err(ErrorInternalServerError)?).map_err(ErrorInternalServerError)?,
                "key": entry.key,
                "value": entry.value.as_string().map_err(ErrorInternalServerError)?,
            }),
        );
    }
    Ok(HttpResponse::Ok().json(result))
}

But once again, I might be overlooking something?

edit: For instance, my code being overly reliant on Results - however, all of these directly flow from the underlying operations, like in the encode and decode calls, which may fail for instance due to memory allocation failures, malformed data, or a failure to retrieve a connection from the connection pool - something I do not expect to happen quite often in normal operation, but still would prefer the code not to panic in that instance.

@robjtede
Copy link
Member

robjtede commented Sep 13, 2020

Apologies! I couldn't find it now, though it must have been there quite recently

Don't worry, you're not going mad. It was updated as part of this PR which was merged yesterday: actix/actix-website#188

We'd like to encourage use of the standard Error trait rather than tying the project into specific error handling crates. A review of error handling might be something we can look at soon though.

In my own apps, I feel the easiest and cleanest method is a global error enum that implements ResponseError (with custom status code and responses for each variant) and has From impls for each error type you want to just use ? on.

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

Successfully merging this pull request may close these issues.

5 participants