Tide: Why panic if key is not a parameter for the route?

[k-nasa]

Question

Why panic if key is not a parameter for the route?
I read code.
It seems to be intentionally panic when there is no key.
This seems unfriendly, but why is this behavior?

/// Extract and parse a route parameter by name.
///
/// Returns the results of parsing the parameter according to the inferred
/// output type `T`.
///
/// The name should *not* include the leading `:` or the trailing `*` (if
/// any).
///
/// # Errors
///
/// Yields an `Err` if the parameter was found but failed to parse as an
/// instance of type `T`.
///
/// # Panics
///
/// Panic if `key` is not a parameter for the route.
pub fn param<T: FromStr>(&self, key: &str) -> Result<T, T::Err> {
    self.route_params.find(key).unwrap().parse()
}

Additional context

[jbr]

I just ran into this. I expected the Result to be an Err if key is not a parameter for the route, instead of a panic. @yoshuawuyts would a PR be welcome? We could add an error enum to distinguish between "unable to parse" and "key is not a parameter." My use case was reusing a handler function between two different routes, expecting that I could safely test if the route param was available.

Alternatively, instead of a Result with an error type, there could be an additional pub fn try_param<T: FromStr>(&self, key: &str) -> Option<Result<T, T::Err>>, where None is returned when the key is not a parameter?

[emmanuelantony2000]

@jbr if a PR is required, can I take up this??

[prabirshrestha]

Just ran into the same issue.

https://github.com/http-rs/tide/blob/353f9c8e9355e6c53d02a2d99d671bbb54aa6496/src/request.rs#L171-L180

I would prefer param() to return option of result instead of having a new method try_param.

[vladan]

I handle it like this
https://github.com/vladan/tide/commit/e0d835abd828ad7bbed9ca518c491df61c60f4ed#diff-2873e3b87f87572c686867a1c112e3fdR32

Is it an option worth considering?

[yoshuawuyts]

The decision to panic rather than return an error was a deliberate one. There are two aspects to it:

1. What would the ideal way be to handle this?
2. What are possible alternatives now?

Ideally we'd be able to detect param mismatches during compilation and fail to compile if the parameters aren't used correctly. However with Tide's current design this is not possible; maybe we could do something here eventually using const generics-- but that seems quite far out.

On the other side there's what's been proposed in this thread: make the API return a Result and let users handle errors. But if we consider panics to be for programmer errors, and errors for runtime errors, this squarely falls in the category of "programmer errors". There are indeed ways to encode this as a runtime error, including by creating a new enum, or returning Result<Result<T>>, but I think that would be the wrong approach.

But that doesn't mean we can't do better! Looking at the snippet @prabirshrestha linked we can see that we're calling unwrap without further explanation, which likely is the source of the confusion and frustration folks have experienced. If we could instead call expect with a helpful error message we might solve the problem as well. Ideally we could say something like this:

fn main() {
    let param = "foo";
    let route = "/landing";
    panic!("param \":{}\" is not part of route \"{}\"", param, route);
}

thread 'main' panicked at 'param ":foo" is not part of route "/landing"', src/main.rs:4:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

[vladan]

Why would this be considered a programmer error?

async fn greet(req: Request<()>) -> Result<String> {
    let name = req.param("name").unwrap_or("world".to_owned());
    Ok(format!("Hello, {}!", name))
}

fn main() -> Result<()> {
    task::block_on(async {
        let mut app = tide::new();
        app.at("/hello").get(greet);
        app.at("/hello/:name").get(greet);
        app.listen("127.0.0.1:8080").await?;
        Ok(())
    })
}

And it wouldn't necessarily need to be Result<Result<T>>, it can easily be flatten to only one Result<T, ParamError>.

[jbr]

This was exactly the example I was going to type out when at a full keyboard. Until/unless tide’s router supports optional params, reusing a route handler for two routes is the way to handle both /some_path and /some_path/:param

[vladan]

I will update my code and submit a PR then :) if it doesn't fit future plans / design, then it can be closed.

[prabirshrestha]

Here is an app that II was working on and completely blocked on this bug unless I create a new func.. https://github.com/prabirshrestha/rblog/blob/ffea293bc09eae3e6be695badfb053ef65f45737/src/main.rs#L20

app.at("/").get(routes::posts::get_posts);
app.at("/page/:page").get(routes::posts::get_posts);

I would definitely say it is a valid scenario.

[jbr]

@prabirshrestha That was exactly the scenario I had when I ran into this

[peterhuene]

I have also run into this issue from a different direction: I have "external" handlers and configuration my server uses for dynamically creating routes at runtime. I don't want to panic my server when one of these handlers requests a route parameter that wasn't present in the configuration used to build the route.

I would also like to :+1: the idea of changing the error type from T::Err into a ParamError enum encapsulating both a "param not found error" and a "conversion error".