Modular Diplonat (with the option to disable useless modules) #8
|
@ -62,10 +62,14 @@ impl RuntimeConfig {
|
|||
|
||||
impl RuntimeConfigConsul {
|
||||
pub(super) fn new(opts: ConfigOptsConsul) -> Result<Self> {
|
||||
let node_name = opts
|
||||
.node_name
|
||||
.expect("'DIPLONAT_CONSUL_NODE_NAME' is required");
|
||||
let url = opts.url.unwrap_or(super::CONSUL_URL.to_string());
|
||||
let node_name = match opts.node_name {
|
||||
Some(n) => n,
|
||||
_ => return Err(anyhow!("'DIPLONAT_CONSUL_NODE_NAME' is required")),
|
||||
|
||||
};
|
||||
let url = match opts.url {
|
||||
Some(url) => url,
|
||||
_ => super::CONSUL_URL.to_string(),
|
||||
};
|
||||
|
||||
Ok(Self { node_name, url })
|
||||
}
|
||||
|
@ -75,11 +79,16 @@ impl RuntimeConfigAcme {
|
|||
pub fn new(opts: ConfigOptsAcme) -> Result<Option<Self>> {
|
||||
if !opts.enable {
|
||||
return Ok(None)
|
||||
}
|
||||
};
|
||||
quentin
commented
I know this is not linked to this specific pull request but when your function returns a Result, there is no reason to panic with expect. Instead, replace it with:
Please do it for the whole file :) I know this is not linked to this specific pull request but when your function returns a Result, there is no reason to panic with expect.
Instead, replace it with:
```rust
let email = opts
.email
.context("'DIPLONAT_ACME_EMAIL' is required if ACME is enabled")?;
```
Please do it for the whole file :)
adrien
commented
There's sense in doing so: when a parameter is invalid (like here), the method should panic or return an Err(..). I'm still modifying the calls to return errors instead of panicking straight away. Using `.context(...)` is not a method of `Option`.
There's sense in doing so: when a parameter is invalid (like here), the method should panic or return an Err(..).
I'm still modifying the calls to return errors instead of panicking straight away. Using `match` for better clarity ;)
quentin
commented
You can convert an Option to an Error with But we need an Error to map to the None. It is a bit challenging as it will be our first error in Diplonat, previously we were relaying only on errors generated by our dependencies. For now, let's keep it simple, our error library provides us a simple macro to create an error from a string. Just a simple example:
So you can write:
For your information, You made the following point:
First, you're right, when a parameter is invalid, we must handle this as an error. It was what I was suggesting to you in my first comment but I thought the return type of Second, we are writing Rust and we are in a library: in this situation, it is not acceptable to panic. As there is no clean way to recover from a panic, we terminate the program from a library without giving the program a chance to recover from the error. Even if we are not a "standalone library" and are writing the main program, we might change our mind the future. We should Third, I am not sure we discussed it but
So in fact, what I suggest is that we return from the function with an Error if this parameter has not be provided. You can convert an Option to an Error with `ok_or`: https://doc.rust-lang.org/std/option/enum.Option.html#method.ok_or
But we need an Error to map to the None. It is a bit challenging as it will be our first error in Diplonat, previously we were relaying only on errors generated by our dependencies.
For now, let's keep it simple, our error library provides us a simple macro to create an error from a string. Just a simple example:
```rust
return Err(anyhow!("Missing attribute: {}", missing))
```
So you can write:
```rust
let email = opts
.email
.ok_or(anyhow!("'DIPLONAT_ACME_EMAIL' is required if ACME is enabled"))?;
```
For your information, `context` allows to enrich an existing error with some context. Indeed, here it is not appropriate as we have no error at all. But in the case where your library throw a cryptic error, you can enrich this error with some context in your program and your users will thank you many times later.
You made the following point:
> There's sense in doing so: when a parameter is invalid (like here), the method should panic or return an Err(..).
First, you're right, when a parameter is invalid, we must handle this as an error. It was what I was suggesting to you in my first comment but I thought the return type of `opts.email` would be a `Result` and not an `Option`.
Second, we are writing Rust and we are in a library: in this situation, it is not acceptable to panic. As there is no clean way to recover from a panic, we terminate the program from a library without giving the program a chance to recover from the error. Even if we are not a "standalone library" and are writing the main program, we might change our mind the future. We should `panic!` only when there is no option. To illustrate my point, we could think that later, we would allow our main program to reload its configuration and retry the initialization if one actor fails.
Third, I am not sure we discussed it but `?` is syntaxic sugar for:
```rust
let err = anyhow!("'DIPLONAT_ACME_EMAIL' is required if ACME is enabled")
match opts.email.ok_or(err) {
Ok(email) => {
// our logic
}
Err(e) => return Err(e)
}
```
So in fact, what I suggest is that we return from the function with an Error if this parameter has not be provided.
adrien
commented
Here's my new RuntimeConfigAcme implementation in full (in commit
We do return an error instead of panicking. But I may have overdone it with using match, but I didn't find any nice shorthand in PS: Quite puzzled by the fact that Rust allows putting a Here's my new RuntimeConfigAcme implementation in full (in commit `f5ac36e`). Do you find it better?
```rust
impl RuntimeConfigAcme {
pub fn new(opts: ConfigOptsAcme) -> Result<Option<Self>> {
if !opts.enable {
return Ok(None)
};
let email = match opts.email {
Some(email) => email,
_ => {
return Err(anyhow!(
"'DIPLONAT_ACME_EMAIL' is required if ACME is enabled"
))
}
};
Ok(Some(Self { email }))
}
}
```
We do return an error instead of panicking. But I may have overdone it with using match, but I didn't find any nice shorthand in `Option` methods (I checked `ok_or`). At least the intent is clear!
PS: Quite puzzled by the fact that Rust allows putting a `return` inside a `match` arm, which should be an expression returning a String (in this case). Welp, gotta get used to it!
quentin
commented
Sorry, I was not clear enough >< About handling error, I want to avoid But So, I don't think you should replace your I have written a small example on how to use See the code without leaving gitea
So, you may ask why I was advocating for To sum up, I think we can define the following rules:
I hope that this time I will be more clear. Sorry, I was not clear enough ><
About handling error, I want to avoid `expect` and `unwrap` because they make our program panic, ie. the program is killed.
But `unwrap_or` is totally fine as it does not make the program panic.
And when the case is simple enough we can use one of these keywords: `unwrap_or` on `Result` or `ok_or` on Options instead of a `match`, it is totally ok.
So, I don't think you should replace your `unwrap_or` with a `match` in `f5ac36e`.
I have written a small example on how to use `ok_or` to convert an `Option` into a `Result` and checked that it compiles here: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=5bc6046485bab0cd4ae694b94d2ba1db
<details>
<summary>See the code without leaving gitea</summary>
```rust
use std::{
fmt,
error::Error
};
#[derive(Debug)]
struct RequiredParameterError;
impl Error for RequiredParameterError {}
impl fmt::Display for RequiredParameterError {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(f, "A required parameter is not defined")
}
}
fn foo(param: Option<String>) -> Result<String, RequiredParameterError> {
let email = param.ok_or(RequiredParameterError{})?;
return Ok("maito:".to_string() + &email)
}
fn main() {
let user_param1 = Some("hello@example.org".to_string());
let user_param2 = None;
assert!(foo(user_param1).is_ok());
assert!(foo(user_param2).is_err());
}
```
</details>
<br/>
So, you may ask why I was advocating for `match`?
Because in `fw_actors.rs` you were using again `unwrap()` that can cause a panic in the program and we can't replace it by these simple keywords that are `ok_or`, `unwrap_or`, etc.
To sum up, I think we can define the following rules:
1. Never write `unwrap()` or `expect()` (yes, never is too brutal, but it should be an exception)
2. Replace them, when possible, by an appropriate small function if it exists
3. Otherwise, if your case is too specific, use a match
I hope that this time I will be more clear.
I know that when we start coding together, it takes some times to align our minds but once it will be done, it will be easier :)
|
||||
|
||||
let email = opts
|
||||
.email
|
||||
.expect("'DIPLONAT_ACME_EMAIL' is required if ACME is enabled");
|
||||
let email = match opts.email {
|
||||
Some(email) => email,
|
||||
_ => {
|
||||
return Err(anyhow!(
|
||||
"'DIPLONAT_ACME_EMAIL' is required if ACME is enabled"
|
||||
))
|
||||
}
|
||||
};
|
||||
|
||||
Ok(Some(Self { email }))
|
||||
}
|
||||
|
@ -91,7 +100,13 @@ impl RuntimeConfigFirewall {
|
|||
return Ok(None)
|
||||
}
|
||||
|
||||
let refresh_time = Duration::from_secs(opts.refresh_time.unwrap_or(super::REFRESH_TIME).into());
|
||||
let refresh_time = Duration::from_secs(
|
||||
match opts.refresh_time {
|
||||
Some(t) => t,
|
||||
_ => super::REFRESH_TIME,
|
||||
}
|
||||
.into(),
|
||||
);
|
||||
|
||||
Ok(Some(Self { refresh_time }))
|
||||
}
|
||||
|
@ -103,16 +118,29 @@ impl RuntimeConfigIgd {
|
|||
return Ok(None)
|
||||
}
|
||||
|
||||
let private_ip = opts
|
||||
.private_ip
|
||||
.expect("'DIPLONAT_IGD_PRIVATE_IP' is required if IGD is enabled");
|
||||
let private_ip = match opts.private_ip {
|
||||
Some(ip) => ip,
|
||||
_ => {
|
||||
return Err(anyhow!(
|
||||
"'DIPLONAT_IGD_PRIVATE_IP' is required if IGD is enabled"
|
||||
))
|
||||
}
|
||||
};
|
||||
|
||||
let expiration_time = Duration::from_secs(
|
||||
opts
|
||||
.expiration_time
|
||||
.unwrap_or(super::EXPIRATION_TIME)
|
||||
.into(),
|
||||
match opts.expiration_time {
|
||||
Some(t) => t.into(),
|
||||
_ => super::EXPIRATION_TIME,
|
||||
}
|
||||
.into(),
|
||||
);
|
||||
let refresh_time = Duration::from_secs(
|
||||
match opts.refresh_time {
|
||||
Some(t) => t,
|
||||
_ => super::REFRESH_TIME,
|
||||
}
|
||||
.into(),
|
||||
);
|
||||
let refresh_time = Duration::from_secs(opts.refresh_time.unwrap_or(super::REFRESH_TIME).into());
|
||||
|
||||
if refresh_time.as_secs() * 2 > expiration_time.as_secs() {
|
||||
return Err(anyhow!(
|
||||
|
|
cf my following comment on
DIPLONAT_ACME_EMAIL