Implement IntoResponse for Result

[Licenser]

Implementing IntoResponse for Result allows for Endpoints to use results and by that the ? operator which makes it more ergonomic to use with error handling.

[Licenser]

I don't think the errors here are related to the PR.

[Licenser]

Again this seems to be CI related and not to the code:

[yoshuawuyts]

@Licenser thanks to #438 we now enable handling ? in Endpoints. Does that work for your use case?

Also worth sharing here is that we may want to remove IntoResponse since we now own Response in http_types so there's no need to work around it. If you think this conversion would still be worth adding, it might be nice to add a conversion from http_types::Result to http_types::Response.

[Licenser]

Amusingly enough I think this might be related to #444 I'm currently battling it and would say it doesn't work at all since nothing compiles and error messages are cryptic 😂 but I feel like I'm missing something obvious.

[Licenser]

OKay with that lead I found the issue in #444 but no that doesn't work since it doesn't allow adding a status code to the error which this PR would.

[Licenser]

it might be nice to add a conversion from http_types::Result to http_types::Response

unless the error in http_types::Response carries an HTTP error code that wouldn't solve the problem I think.

[yoshuawuyts]

unless the error in http_types::Response carries an HTTP error code that wouldn't solve the problem I think.

✨ https://docs.rs/http-types/1.2.0/http_types/struct.Error.html#method.status

[Licenser]

Oh, I didn't see that, however, it still misses control over things like content type (returning JSON vs human errors ) but I suppose it'd be more sensible to extend http_types::Error to do that.

I also can not implement From, that on an error that already implements std::error::Error which does make this not really usable without converting every error type in my application to a http_types error manually which is really not very pleasant to use.

I'm not married to IntoResponse but the ergonomics it provides are rather good and with #438 a lot of them get lost which, at least for me, makes using surf a lot more painful then it was before.

(edit)
It also loses some functionality with http_type::Error like setting headers which are somewhat important to me.

[Licenser]

To ilustrate the code now looks like this:

    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()) });

to set up a somewhat more complex endpoint that does still allow setting headers on errors. prior to #438 it was:

    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);

I hope that puts a bit of perspective on the lost ergonomics of the newer version of tide.

[Licenser]

I'll close this as with #438 the improvement on ergonomics I hoped from this is impossible and using From<Result<T,E>> seems more in line going forward. I'll raise the issues w/ #438 in a separate ticket so it can be discussed.

[yoshuawuyts]

I also can not implement From, that on an error that already implements std::error::Error which does make this not really usable without converting every error type in my application to a http_types error manually which is really not very pleasant to use.

We already have an automatic conversion of std::error::Error to http_types::Error, which should be called automatically. I just ran this on playground and I don't think there's a way for endpoint functions to take anything other than tide::Result<Response> as the response type.

I see two ways around this:

• Make tremor endpoints return tide::Result<Response>
• Define a wrapping function or trait that does the conversion for you.

fn convert(f: impl Fn() -> std::io::Result<()>) -> http_types::Result<()> {
    let res = f()?;
    Ok(res)
}

---

Regarding the final part: if I'm understanding it correctly, what you're looking for is a way to convert the text from the error to JSON. If you're going to use http_types::Result as the type to carry your errors and do the conversion later, I'd define a middleware that knows how to serialize http_types::Error to JSON. Maybe there are requirements here that I'm not aware of, but hopefully this will be enough to make progress!

[Licenser]

I made a separate issue for this: #452 as it kind of escapes the context of the PR and is more about the changes in #438