aurae-runtime / auraed

Secure mTLS and gRPC backed runtime daemon. Alternative to systemd. Written in Rust.
https://aurae.io/auraed
Apache License 2.0
60 stars 11 forks source link

Using thiserror for custom error type #32

Open future-highway opened 1 year ago

future-highway commented 1 year ago

The thiserror crate is a common library used for creating custom library errors.

Code similar to the below can be placed in the lib.rs file. I added some comments to help explain the crates usage, but the docs page gives great and more advanced examples. While the docs do say a struct can be used instead of an enum, I prefer an enum.

// Define our own result type where the error is our custom error type
// Usage example: fn i_may_fail() -> crate::Result<DidntFailType> { ... }
type Result<T> = std::result::Result<T, Error>;

// Custom error made using thiserror crate
#[derive(thiserror::Error, Debug)]
pub enum Error {
    // If we want to pass through all io errors the from attribute will make this easier by
    // automatically implementing .into() (impl of from gives impl of into for free)
    #[error("an io related error occurred: {0}")]
    Io(#[from] io::Error),
    // This is our first custom type, where the error is just a String.
    // We can't use from because CustomErrorVariant2 also takes just a String
    #[error("CustomErrorVariant1: {0}")]
    CustomErrorVariant1(String),
    #[error("CustomErrorVariant2: {0}")]
    CustomErrorVariant2(String),
    // I'm not sure how I feel about a catch-all variant, but it is an option too.
    // transparent isn't exclusive to the catch-all variant, we could have used it on Io too, for example
    #[error(transparent)]
    Other(#[from] anyhow::Error)
}
krisnova commented 1 year ago

Can you give implementation examples as well?

future-highway commented 1 year ago

I'll do my best to answer these at least partially answer here, but I would not title them best practices, because I just don't know if they are (this is just what has felt most ergonomic to me). Since you're able to, and this will probably effect a lot of the codebase, I would see if togglebyte can consult. That being said...

The crate should define its own result type and error type at the top level (code above). There is no rule that says individual modules can't have their own Error types as well, with an associated variant on the top level error type (e.g., Observe(#[from] crate::observe::Error), but I wouldn't start with that.

If we have an internal (not pub) function that can only ever return a single error type (e.g., std::io::Error), I would not use the crate's custom error I would stick with the single error type:

// notice this is `pub(crate)`, not pub.
// If it were pub, I would use the custom Error so the lib only ever returns a single error type
pub(crate) fn read_aurae_config(path: String) -> std::result::Result<String, std::io::Error> {
    let mut config_toml = String::new();

   // failure to open the file returns a std::io::Error,
   // and the `?` operator is a shortcut to return Err(e), ending execution of this  function
    let mut file = File::open(&path)?;

   // failure to read the file is the same error type (std::io::Error),
   // and we use the `?` operator again
    file.read_to_string(&mut config_toml)
        .with_context(|| "could not read AuraeConfig toml")?;

    Ok(config_toml)
}

For functions that return multiple error types, then we switch to the custom result/error.

use crate::Result;

// Notice we don't make the return type `Result<AuraeConfig, crate::Error>`,
// our crates result type will only ever return crate::Error as the error;
// you're not allowed to use a different error type due to the type definition above (first issue comment) 
pub(crate) parse_aurae_config(path: &str) -> Result<AuraeConfig> {
    let mut config_toml = String::new();

    // like the first example, this may return a std::io::Error
    let mut file = File::open(&path)?;

    // like the first example, this may return a std::io::Error
    file.read_to_string(&mut config_toml)
        .with_context(|| "could not read AuraeConfig tool")?;

   // this time, the toml parser may return a toml::de::Error
   let parsed: AuraeConfig = toml::from_str(&config_toml)
       .map_err(|_| crate::Error:: CustomErrorVariant1("failed to parse toml for aurae_config"))?; 

   Ok(parsed)
} 

You may have noticed, I did not change the first two uses of the ? operator. Because we used the #[from] attribute in the top level custom error, it should work transparently for the std::io::Error type. However, there is no variant for the toml::de::Error, so a change in approach was needed (map to CustomErrorVariant1). We do have a couple of other options instead of mapping to the not so helpful CustomErrorVariant1:

Option 1: Define another variant similar to the Io variant and avoid the map_err:

#[error("Failed to parse toml: {0}")]
TomlParsing(#[from] toml::de::Error)

Option 2: Define a perhaps more helpful variant and map to it:

#[error("Failed to parse toml '{0}' due to error {1}")]
TomlParsing {
    contents: String,
    source: toml::de::Error,
}
let parsed: AuraeConfig = toml::from_str(&config_toml)
    .map_err(|e| crate::Error::TomlParsing { contents: config_toml, source: e })?;

When an error bubbles up, there are of course 2 possible options: handle every possible error, or more likely, handle what you can and let the others continue to bubble higher.

fn default_config() -> Result<AuraeConfig> {
    let home: String = ...;

    let path = format!("{}/.aurae/config", home);
    let res: Option<AuraeConfig> = match parse_aurae_config(path) {
       Ok(config) => Some(config),
       Err(crate::Error::Io(_)) => {
           // We assume all io errors are due to the file not being found
           // and we will check other paths below
           None
       },
       Err(e) => {
          // We don't bother handling any other error type; let it bubble
          return Err(e);
       }
   };

   if let Some(config) = res {
       return Ok(config);
   }

    // We didn't get the config from the first location, let's check the backup location
    let path = "/etc/aurae/config";
   // This time, we don't have a second backup location if this fails, so just return the result, whether Ok or Err
   // A different higher level function will be responsible for figuring out what to do when no config is returned.
   parse_aurae_config(path)
}

Of course, somewhere along the way, every error needs to be handled, even if it just means logging and crashing at the very top so users can fix the problem.


edited a couple of syntax errors