Ergonomics issue with enabling ? in endpoint

[Licenser]

The way ? is now supported in Endpoint introduces some issues for more complex applications that do not use anonymous functions or need some control over how errors are transmitted.

To prefix this, I don't think there is a clear right or wrong in this. Allowing ? in endpoints is a definite benefit for some use cases as the examples shared on twitter and other channels. However, I have the feeling that the impact on more complex API's was considered.

I want to start with a before -> after going from 0.6 to 0.7.

This is w/ tide 0.6 (and a custom fix to implement IntoResponse for Result). It is rather elegant to write:

    app.at("/version").get(api::version::get);
    app.at("/binding")
        .get(api::binding::list_artefact)
        .post(api::binding::publish_artefact);
    app.at("/binding/{aid}")
        .get(api::binding::get_artefact)
        .delete(api::binding::unpublish_artefact);
    app.at("/binding/{aid}/{sid}")
        .get(api::binding::get_servant)
        .post(api::binding::link_servant)
        .delete(api::binding::unlink_servant);
    app.at("/pipeline")
        .get(api::pipeline::list_artefact)
        .post(api::pipeline::publish_artefact);
    app.at("/pipeline/{aid}")
        .get(api::pipeline::get_artefact)
        .delete(api::pipeline::unpublish_artefact);
    app.at("/onramp")
        .get(api::onramp::list_artefact)
        .post(api::onramp::publish_artefact);
    app.at("/onramp/{aid}")
        .get(api::onramp::get_artefact)
        .delete(api::onramp::unpublish_artefact);
    app.at("/offramp")
        .get(api::offramp::list_artefact)
        .post(api::offramp::publish_artefact);
    app.at("/offramp/{aid}")
        .get(api::offramp::get_artefact)
        .delete(api::offramp::unpublish_artefact);

After migrating to 0.7 (with the same custom extension to IntoResponse) it looks like this. Which is rather painful to both read and write it introduces a whole lot of boilerplate that wasn't required before and adds quite a bit of redundancy.

    app.at("/version")
        .get(|r| async { Ok(api::version::get(r).await.into_response()) });
    app.at("/binding")
        .get(|r| async { Ok(api::binding::list_artefact(r).await.into_response()) })
        .post(|r| async { Ok(api::binding::publish_artefact(r).await.into_response()) });
    app.at("/binding/{aid}")
        .get(|r| async { Ok(api::binding::get_artefact(r).await.into_response()) })
        .delete(|r| async { Ok(api::binding::unpublish_artefact(r).await.into_response()) });
    app.at("/binding/{aid}/{sid}")
        .get(|r| async { Ok(api::binding::get_servant(r).await.into_response()) })
        .post(|r| async { Ok(api::binding::link_servant(r).await.into_response()) })
        .delete(|r| async { Ok(api::binding::unlink_servant(r).await.into_response()) });
    app.at("/pipeline")
        .get(|r| async { Ok(api::pipeline::list_artefact(r).await.into_response()) })
        .post(|r| async { Ok(api::pipeline::publish_artefact(r).await.into_response()) });
    app.at("/pipeline/{aid}")
        .get(|r| async { Ok(api::pipeline::get_artefact(r).await.into_response()) })
        .delete(|r| async { Ok(api::pipeline::unpublish_artefact(r).await.into_response()) });
    app.at("/onramp")
        .get(|r| async { Ok(api::onramp::list_artefact(r).await.into_response()) })
        .post(|r| async { Ok(api::onramp::publish_artefact(r).await.into_response()) });
    app.at("/onramp/{aid}")
        .get(|r| async { Ok(api::onramp::get_artefact(r).await.into_response()) })
        .delete(|r| async { Ok(api::onramp::unpublish_artefact(r).await.into_response()) });
    app.at("/offramp")
        .get(|r| async { Ok(api::offramp::list_artefact(r).await.into_response()) })
        .post(|r| async { Ok(api::offramp::publish_artefact(r).await.into_response()) });
    app.at("/offramp/{aid}")
        .get(|r| async { Ok(api::offramp::get_artefact(r).await.into_response()) })
        .delete(|r| async { Ok(api::offramp::unpublish_artefact(r).await.into_response()) });

A few things that do not work and the limitations that we ran into:

• The http_types::Error doesn't allow setting header types so it is not used for any API that requires, header. Either custom error headers or things like Content-Type. (I think this is fixable by extending http_types::Error, but there might be other issues or requirements I didn't think of like the need for async downloads or things that will make using http_types::Error in some cases just not possible).

• Returning http_types::Error into the functions such as api::version::get doesn't solve the problem just moves it. Since it's not possible to implement Into<http_types::Error> for 3rd party Errors using ? for anything that isn't owned by the crate would become a no-go, while intermediating over a custom error type does allow doing this.

• bouncing every call through a 'translation' function. This would work but really breaks ergonomics IMHO example

fn get() -> tide::Result<tide::Response> {
  Ok(get_()?)
}
fn get_() -> crate::Result<tide::Response> {
  let v = serde_json::to_string("0.1.0")?;
  Ok(v.into())
}

I think the tension here is between making something look good in an example and easy to use for simple applications and making something powerful for more complex applications. I honestly don't know what's the "right" answer.

Perhaps returning to not reqiering a Result in Endpoint and instead implementing From<Result<T, E>> for Response would allow a middle ground that is only slightly more painful for simple applications along the lines of this would work?

let mut app = tide::new();
app.at("/").get(|_| -> tide::Result<&'static str> async { Ok("hello world") });
app.listen("localhost:8080").await?;

[Licenser]

A second thought, given http_types::Error gets capabilities to handle headers and other things, how about making Endpoint take and error of type T: Into<http_types::Error> instead of forcing http_types::Error as a type?

[Licenser]

Wait that won't work since it implements From<std::error::Error> so it won't be possible to define that. :(

[danieleades]

i'm sure there's a really good reason why tide::Response can't implement From<Result<T, E>> where T: Into<Response>, E: Into<Response> and then have an endpoint return impl Into<Response> instead of tide::Result<impl Into<Response>>.

what is that (probably excellent) reason?

[Fishrock123]

Ok what I've gathered so far investigating this:

• You can't have the actual middleware or endpoint return type be anything except a Result if you want ? to work, at least not until the Try trait stabilizes.
• impl<T, E> From<Result<T, E>> for Response where T: Into<Response>, E: Into<Response> is doable but unhelpful (?) due to the above.
• You can have impl From<Response> for tide::Result<Response> but ? does not interpret Into<Result> as something it can use (sadly, even though Into<Self> is universal).
• Supporting e.g. .get(|_| async { tide::Response::new(tide::StatusCode::Ok) }) is doable but breaks other things such as directly returning Ok(&str) (did not fully investigate, @jbr did more there)

[danieleades]

@Fishrock
That all makes sense.

So the endpoint has to return a result, and then I guess internally you're taking either the 'Ok' or 'Err' path and converting that to a Response. So I guess what's needed is being able to provide a custom implementation of Into<Response> for your error type. Can that be done without losing ergonomics in the more simple cases (without trait specialisation)?

I would also consider documenting patterns for these more complex cases (returning json body on error, custom status code, etc) to a reasonable "solution". If nothing else it would be part of the design space to consider for future releases

[Licenser]

I have thought quite a bit about it and I understand the reasoning why a result is returned in general, I think the ergonomics break when From<std::Error> is implemented for the tide::Error type.

While the tide error type does allow setting status code and all the funky things there is no way to get this information out of a custom error since it treats all std::Error types the same and rust doesn't allow overwriting this.

The alternative is to return tide::Error from your own functions, now that too is tricky since you can't implement any 'from' for other errors so ? is out of the question without a custom error translation - Also not really ideal from an ergonomics perspective.

The next option is to pass it through a custom function that converts the errors, which is possible but as bad as not returning a result in the first place code wise.

I feel like the way errors are handled right now prioritizes nice looking example code (where error responses mostly don't matter) over real world applications where error handling is a major consideration.

A thought: would it make sense to put the implementation of From<std::Error> behind a feature flag? That way it could be selectively enabled for small/simple code and disabled for bigger code that needs error handling.

[Fishrock123]

@yoshuawuyts Thoughts?

[yoshuawuyts]

If nothing else it would be part of the design space to consider for future releases.

I think this is what well have to do. I'm sympathetic to adding more fields from Response to the Error type as well.

Probably worth explaining also is that the reason why we introduced a custom error type instead of returning Result<Response, Response> is because error chaining, backtraces and downcasting are all important features to have and they don't really make sense to have on Response.

@Licenser we could always add a method to convert from a std error, but we can't add the trait conversion. Coupled with more methods to e.g. set a Body, would that work? I also don't think I quite understand the third paragraph you wrote. Could you elaborate?

[Licenser]

heya sorry I must have written that badly :D.

The trait conversion exists here https://github.com/http-rs/http-types/blob/master/src/error.rs#L122 ;

Since the conversion goes from a generic Error it always is reported as a Internal Server Error that makes using ? for error handling in the code virtually pointless for anything but super simple examples (that is under the assumption that most applications want more than a 500 error code).

Wrapping it in a function works, but now all benefits from returning an error are gone, basically, it turns from

get(|r| my_function_turning_result_into_Response(my_logic(r)))

to

get(|r| my_function_turning_my_error_into_http_error(my_logic(r)))

The alternative is using http_types::Error in our own functions, that too has a issue let me demonstrate:

my_types_function(input) -> crate::Result {
  let decoded = serde_json::from_str(input)?; // I can implement From for crate::Error
  let processed = custom_logic(decoded)?; // this function I got control over anyway
  let encoded = serde_json::to_string(processed)?; // I can implement From for crate::Error
  Ok(encoded);
}

my_types_function(good_stuff) -> http_types::Result {
  let decoded = serde_json::from_str(input).map_err(serde_decode_error_to_http_error)?;
  let processed = custom_logic(decoded)?;
  let encoded = serde_json::to_string(processed).map_err(serde_decode_error_to_http_error)?;
  Ok(encoded);
}

This really makes it very ergonomic.

Of cause this, as much as the current implementation, is base on assumptions how people use it so I want to be clear about the assumptions I make:

1. Error codes (and possibly other things like headers but that is "the same" problem space) are important to applications
2. Most applications will implement their own error type (to be able to use ? and get decent errors)
3. The focus is on applications not examples/experiments (I'll elaborate on this) since I think this is currently a bit of the problem.

to 3) Looking at the docs there is this example:

#[derive(Debug, serde::Deserialize, serde::Serialize)]
struct Counter { count: usize }

let mut app = tide::new();
app.at("/").get(|mut req: Request<()>| async move {
   let mut counter: Counter = req.body_json().await?;
   println!("count is {}", counter.count);
   counter.count += 1;
   Ok(Response::new(tide::http::StatusCode::Ok).body_json(&counter)?)
});
app.listen("127.0.0.1:8080").await?;

This read nice as an example, and it feels like there was the focus when designing the way requests work right now, that's great to get users and make it look slick but it's followed by some disappointment.

I think most of us would not like to put this code in production since, if it gets invalid JSON as an input it will fail with a 500 error instead of a 400/415 indicating the data is wrong.

The downside of dropping the From<StdError>, since it's all tradeoffs, is that now projects need to either return a http_types::Result or implement a custom error type that converts Into<http_types::Error>.

[danieleades]

Surely if you add more methods to the error type to set the body, or status code, without changing the Endpoint signature you're still losing the benefit of the 'try' operator anyway, right?

For me personally it would be ideal if the Endpoint returned a Result<T, E> where T: Into<Response>, E: Into<Response> and the blanket implementation for impl Error was dropped. It could equally return Result<T, E> where T: Into<Response>, E: Into<Error> to hold onto that unique error type.

This would be more powerful and customisable for any real use cases, and really only affect the the ergonomics of the noddy examples in the docs (I'm aware those pretty examples are an important part of this crate's ethos. Those pretty examples and the accompanying blog posts are the reason I'm here after all!)

You'd force users to write Into implementations, but you could have helper methods on the Error/Response type to make this fairly slick.

The end result is I could write my custom conversion and then happily use the try operator directly in the endpoint.

[yoshuawuyts]

Since the conversion goes from a generic Error it always is reported as a Internal Server Error that makes using ? for error handling in the code virtually pointless for anything but super simple examples (that is under the assumption that most applications want more than a 500 error code).

For most applications customizing the error code should be a matter of doing the following:

use tide::prelude::*;

async fn my_endpoint(_: Request<()>) -> tide::Result {
    let file = fs::read("path-to-my-file").status(401)?;
    let res = process(file).await?;
    Ok(res)
}

Status codes can readily be set in most cases.

---

I think most of us would not like to put this code in production since, if it gets invalid JSON as an input it will fail with a 500 error instead of a 400/415 indicating the data is wrong.

You're right this should be a 4xx range error. The fact that it isn't is a bug. We discovered a similar case for query two days ago as well http-rs/http-types#154. If you find a place where we're returning what seems to be a wrong status code, filing a bug would be appreciated!

---

To clarify: we're unlikely to change the signature of the our functions anytime soon. I'm trying to understand better what we can do to improve error handling within our existing framework. What I've gathered so far is:

1. not all methods return the right status codes; we should fix those.
2. it could be useful to add more methods to Errors, so they gain functionality comparable to Response.

[Licenser]

You're right this should be a 4xx range error. The fact that it isn't is a bug.

👍 I'll make PR's when I find them :)

the .status() call (and eventually others) sounds like a nice start. That said I still see problems for anything that is even slightly more complex and accesses functions that potentially have multiple different errors.

i.e. the above example would return 401 if the file can't be read, also if it is missing (that probably should be 404) as well as if the FS is broken (let's hope that doesn't happen but it should be a real 500).

Now since fs returns it's own error and is neither used nor tide/http_types it can't implement it's own From but if it were a custom call all those 4xx/5xx could be part of a error conversion.

To clarify: we're unlikely to change the signature of our functions anytime soon.

While my initial thought was the signature is an issue, during the course of the discussion I am now convinced I was wrong with that. The signature is perfectly fine. Where I see friction is the auto conversion from StdError.

Playing it through, with no StdError conversion:

use tide::prelude::*;

async fn my_endpoint(_: Request<()>) -> tide::Result {
    // This still works since .status() will probably translate 
    let file = fs::read("path-to-my-file").status(401)?;
    // this would either require crate::Error to implement Into<tide::Error> and then get proper 
    // error codes from that
    let res = process(file).await?;
    // or uses the .status() function to just make it 500 as it is now but it also makes it more obvious
    // that errors here are not handled with much detail
    let res = process(file).await.status(500)?;
    Ok(res)
}

EDIT:

Perhaps a good solution would be to put the from for StdErr behind a auto-error feature flag? that way users can pick their poison? and handle ? internally always in a way that uses .status to ensure it doesn't rely on the StdErr conversion? If that's of interest I can make a PR for that.

[danieleades]

adding a body_json method to the Status trait would get me out of trouble (I wasn't even aware I could add a status code this way).

I think putting the auto conversion behind an opt-out feature gate would satisfy all the more complex use cases. It's kind of the nuclear option though. you might only have one endpoint where you need the increased flexibility, and suddenly you're manually handling errors across your whole application.

[Fishrock123]

Hi folks, could you please let us know if http-rs/http-types#174 and/or #570 would help you at all?

tl;dr:

• Make Response store an Error if it was created from one, with methods:
  • error() -> Option<&Error>
  • take_error() -> Option<Error>
• Have Tide always interpret the Result on any endpoint/middleware into a Response.
• Make next.run(request).await always return a Response (with possibly attached error) in middleware rather than a Result.

[Licenser]

I don't think it would, the breaking point of all that is still impl From<Error> for Response which makes it impossible to implement the translation of custom error types and by that makes ? unusable in any scenario with more complex code.

[jbr]

I've been thinking a lot about error handling lately, and my hope is that the solution @Fishrock123 described will address the needs of real-world messy production applications with lots of error handling needs.

Currently, the idea is that every endpoint returns tide::Result. When you use ?, it wraps the custom error type as a tide::Error, which is similar to an Any. This is then attached to a new tide::Response. App code would then register a middleware of some sort (this part is a work in progress) that checks for the presence of that error on the outbound Response and importantly for your use case downcasts the error back to the original error type. A complex application would probably have a bunch of these downcast checks, one per each specific error type, either in one big error handling middleware or one middleware per handled error type (details currently uncertain). Once the tide::Error is downcast to your error type, you have the original struct that you called ? on, allowing you to define a transformation from that to a Response in exactly one location for the entire application (or the subset of the application that the middleware applies to).

The fact that Endpoints return Result should be considered a workaround for the fact that stable rust does not allow us to implement Try as a trait yet. The approach described in this comment always immediately converts the contents of an Err variant into a Response that contains the tide::Error, which in turn contains the original Err content that ? was called on. As a result (no pun intended), tide::Error can be optionally be considered an implementation detail, and as long as you return tide::Result::Err(dyn std::error::Error + Send + Sync + 'static) or use ? to do so, you'll be able to get that error back out if you know its type, and you'll also be able to provide blanket handlers for Errors of types that aren't explicitly handled.

For anyone with concerns about error handling (@Licenser, @danieleades): If you have time for it, a minimal example repo of an app with the sort of custom error types that you're looking to use with Tide would help us ensure that tide's error handling approach allows for an ergonomic, DRY, and concise way of expressing custom error → Response mappings. Hopefully, this shouldn't require any mapping that's repeated for each route handler / endpoint.

[Licenser]

@jbr I put together an example here: https://github.com/Licenser/tide-ergo

Please note this is a synthetic example and not perfect, also quite small so changes are "easy" compared to an impact on a large codebase with thousands of lines of code and more than 4 dependencies :P and 2 functions.

I also add a branch no-std-err in with an 'assumption' that the result would look like: Result<Response, T> where T: Into<Response> and Into<Response> is NOT implemented for std::error::Error.

[jbr]

@Licenser I updated that for the proposed error handling style here https://github.com/Licenser/tide-ergo/blob/10d41ffaf555b596d88910c62d1f8442b10280a0/src/main.rs

[Licenser]

Heya @jbr I see where you are going with this change :) that wasn't obvious me in the beginning, sorry. Thank you for spending the time to demonstrate and explain your idea!

I'd say that would be a nice improvement!

The only concern would be performance if there are many downcast happening, but that's a different topic then ergonomics and at least for me, or my current use-case, not relevant.

[Fishrock123]

Is this still an issue in Tide 0.12? (i.e. with #570)

[Fishrock123]

The only concern would be performance if there are many downcast happening

It's just a TypeId check.