http-rs / http-rs/tide

Ergonomics issue with enabling `?` in endpoint

Open
#452 21 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
5.1k
Forks
329
PR merge metrics
No merged PRs in 30d

Description

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:
```rust
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.

```rust
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` 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 {
Ok(get_()?)
}
fn get_() -> crate::Result {
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>` 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?;
```

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.