Swoorup / dgraph-rs

A dgraph client driver written in rust ⛺
https://crates.io/crates/dgraph
MIT License
98 stars 10 forks source link

Is failure crate necessary? #4

Closed ondrowan closed 5 years ago

ondrowan commented 5 years ago

I found out it's currently not easily possible to match errors produced by gRPC. Let's say there's a wrong alter and I want to catch particular error and present it to user. It's currently very awkward to do so.

// Errorneous alter
let op = dgraph::Operation {
    schema: "name: string @error .",
    ..Default::default()
};

let result = dgraph.alter(&op);

if let Err(err) = result {
    if let Ok(grpc_err) = err.downcast::<grpcio::Error>() {
        if let grpcio::Error::RpcFailure(status) = grpc_err {
            eprintln!("{}", status.details.unwrap())
        }
    }
}

First of all, grpcio::Error should be probably re-exported so users don't need to include grpcio just for error matching.

This also led me to check how failure is used in this library and it seems it could be swapped out for std Error since the more advanced features aren't used anyway. I think it would make error handling a bit easier for users.

Let me know if I've missed something obvious that makes this easier.

Swoorup commented 5 years ago

You are correct. Failure isn't used very much, or perhaps not used at all. I haven't been keeping up to date with the current std Error. Would love to know more about how it can be used.

ondrowan commented 5 years ago

I don't think anything changed regarding std errors. There's RFC2504 that brings changes for std Error trait based on failure's ideas, but I have no idea when it's gonna be implemented.

From what I can see, only bail! macro and conversion to failure::Error is used in this library. So far no custom errors are defined, but that could change in the future. The main feature of failure seems to be backtraces (which are disabled by default if you don't provide a feature flag in cargo), but it's questionable if it's useful for users to trace errors back to particular code in a library. To me, it seems to be mainly useful in application code.

The problem with code I've posted in OP is that I have no idea how to get the original error out of failure::Error. And I haven't figured out how to do it without actually adding failure as dependency, downcasting it to original error and then finally getting the details I wanted. So it seems using failure in library actually forces me to use it also in my application, even if I just want to get the original error. But it's possible there's something I misunderstand about its usage.

Either way, even if failure stays gRPC errors should be either reexported or wrapped into custom Dgraph errors. The latter would be preferable, but I don't know if it's possible since it seems gRPC errors fill details just with error descriptions and no actual error codes (which would help us match particular Dgraph errors). I've checked JS library and it seems there should be some error code provided by gRPC library itself. I'll look into it later. Edit: I just checked and gRPC errors indeed include also status codes, so it should be possible to match particular Dgraph errors.

ondrowan commented 5 years ago

I've reexported grpcio in 10000ec. Hopefully, I'll draft some PR with removal of failure soon. Just to see how it's gonna look like.