Revamp Tide, dropping Extractors and simplifying the framework #156
Conversation
There was a problem hiding this comment.
I quickly looked through the diff and this is huge! And much cleaner. 👏
I don't get the idea where we can drop the nested routing though. I feel like we should make App, Server or something implement Endpoint, so that we can effectively nest an App into another App with Route. Does it make sense to you?
|
I really like how this change makes the framework extremely approachable. The mental model for someone using the framework now is simple, as all they have to concern themselves with is how to get data from either the request or the context, and how to return a response. I think this makes for an excellent base for third party extensions, I can already see how a generic session interface could look like. I agree with @tirr-c , nesting routes is a very useful form of encapsulation. A possible idea for a future time would be a way to specify extension traits on a nested |
|
@tirr-c agreed re: nesting! To clarify a bit: before we were using nesting for a few different purposes, including customizing configuration and middleware for specific subroutes. I was wanting to simplify that story, but totally agree that we need some way to nest full apps. I'll get that added to this PR 👍 |
|
Update:
|
src/context.rs
Outdated
| struct ContextData { | ||
| meta: http::request::Parts, | ||
| body: Mutex<http_service::Body>, | ||
| extensions: Mutex<Extensions>, |
There was a problem hiding this comment.
I think Context isn't required to be Send nor Sync because only one thread will be accessing the struct during the lifetime (as each request is processed in its own thread.) Did I get it wrong?
There was a problem hiding this comment.
I would not agree. It's certainly rare, but also not unusual to spawn further threads during a request execution that might have access to the request data.
|
@tirr-c I'm also uneasy about the mutexes, but one note: this is currently in place to work around the compiler limitations mentioned in #3. In the long run, we'll be able to pass the endpoint |
|
Update: the PR is now at feature parity with master, including form extraction and response generation. Next up:
After that, I think we will be ready to fully review and land, based on stakeholder feedback so far! |
src/app.rs
Outdated
| pub fn middleware(&mut self, middleware: impl Middleware<Data> + 'static) -> &mut Self { | ||
| self.router.middleware(middleware); | ||
| self | ||
| } |
There was a problem hiding this comment.
Silly q: I was poking around trying to build an app with middleware against this branch (I'm excited for these changes!), but noticed that app.middleware() no longer works – was this method removed intentionally / is it coming back in a different form? (Or have I just jumped the gun entirely? 😅)
There was a problem hiding this comment.
@chrisdickinson Good question! -- I hadn't realized this was missing until now. Middleware is staying as a concept, so it should be coming back in some form. Probably more on this this week!
|
I feel a bit uneasy commenting on this PR since I haven't taken an interest in For example, I have an extractor which looks at the request for an It seems like with this PR equivalent would be extend Just my opinion, of course - you could argue my objection simply boils down to a matter of taste, and I appreciate that this framework is taking a different path from |
|
@jesskfullwood I actually had the same thought in mind. However, I have two signatures in mind which would read much nicer.
I haven't thought about the implementation details yet since I am still building up enough knowledge, but I think this approach would be much cleaner then relying on the right parameter in |
|
The thing is, the extraction of value from the I'm sure @aturon can clarify this more as I might be mis-remembering somethings. |
|
We might need to think about how we can pass "subpath" information to nested apps, if we make |
|
Love these changes!
Currently with Hyper, we can pull it out with |
|
Yes, @bIgBV, that's exactly how I'm thinking about it! Right now the focus is to settle on the "core" APIs, and we'll have to continue to experiment to see whether we can make those ergonomic enough to use directly, or whether we want some (macro or trait-based) layer on top. Update: I've pushed up code that gets the test suite and examples working, and adds a bunch of helpful error infrastructure (which makes endpoints quite a bit nicer to write). At this point the code is ready for a final review, while I work on rewriting the docs :-) |
|
@aturon Awesome! Btw, did you happen to give any thought to #156 (comment)? I was thinking - one way would be to expose it in the |
|
@aturon I was wondering how can we use context in middleware. If in routes/middleware some changes are done, how we can get the changes on to response. e.g. for the following middleware handler function we need a way to get some details on to response. fn handle<'a>(&'a self, cx: Context<Data>, next: Next<'a, Data>) -> FutureObj<'a, Response> {
box_async! {
// Some changes to context here
let mut res = await!(next.run(cx)); //Context is moved here
// How to extract some bits from context to response here?
}
} |
|
There's an unfortunate name clash with this |
|
@prasannavl could you perhaps open an issue about that? I agree we should have a way to do this, and like the approach you're proposing. However it probably works best to make it a follow-up to this PR, as it can be added without any breaking changes to Tide (: |
|
@yoshuawuyts - Sure, makes sense. Will do that in a bit. Just wanted to make sure it was in @aturon's radar, in case he had something else in mind through the redesign. @aturon - Just went through the PR -- just thinking out loud here - could I haven't given much thought to it - it could potentially make the ergonomics much worse too and the complexity might just not be worth it as well. But just wanted to spend a tiny bit exploring it. |
|
Should The method that calls This moves the responsibility of error handling to the app rather than the IntoResponse provider. And think the app has to have control on most occasions. Currently (in this PR): pub fn json<T: serde::Serialize>(t: T) -> Response {
// TODO: remove the `unwrap`
http::Response::builder()
.status(http::status::StatusCode::OK)
.header("Content-Type", "application/json")
.body(Body::from(serde_json::to_vec(&t).unwrap()))
.unwrap()
}This is actually problem today, since assuming this unwrap is removed with a 500 response, during an error. This becomes problematic if an app were to say abstract this like this: fn api_response(code: StatusCode, context: &Context) -> Response {
let mut response = json_from_context_infallible(&context);
*response.status_mut() = code;
response
}Now whether or not the status code has to be mutated if the Even before this current PR, the previous version suffers this as well: fn json<T: Send + serde::Serialize>(code: StatusCode, body: T) -> Response {
let mut response = Json(body).into_response();
*response.status_mut() = code;
response
}(Note, currently, there is not handled at all, so this will just panic). Assuming this is handled properly, I have to actually examine the response to provide the correct handling. I think In short, I think its a far superior approach to not look at this as a naive And the final handler exactly has that one purpose, and a dedicated handler conversion of unhandled or unexpected errors to produce a graceful response. I don't think |
| pub type Response = http::Response<Body>; | ||
| pub type Response = http_service::Response; | ||
|
|
||
| pub fn json<T: serde::Serialize>(t: T) -> Response { |
There was a problem hiding this comment.
Perhaps return a Result to be more rust idiomatic? #156 (comment)
| .header("Content-Type", "application/json") | ||
| .body(Body::from(serde_json::to_vec(&t).unwrap())) | ||
| .unwrap() | ||
| } | ||
|
|
||
| /// A value that is synchronously convertable into a `Response`. | ||
| pub trait IntoResponse: Send + Sized { |
There was a problem hiding this comment.
Please consider TryIntoResponse instead. #156 (comment)
|
We already have an issue around error handling which I think overlaps with whether we should return cc/ @prasannavl |
|
OK, we're ready to merge! @tirr-c @yoshuawuyts @bIgBV, any concerns before we do so? |
aturon
changed the title
| } | ||
|
|
||
| /// Mutably access the request body | ||
| pub fn body(&mut self) -> &mut http_service::Body { |
There was a problem hiding this comment.
I've generally seen two methods for mutable and immutable access. Any particular reason why we have a single function returning a mutable reference? I ask because I figure the body will only be read more often than not.
There was a problem hiding this comment.
The reason this one is mutable is because, since the body is a stream, to read it you must mutate it.
But more generally you're right that this API design doesn't quite smell right. And in practice, you end up using take_body anyway! So I think I'll drop this method for now.
| /// # Panics | ||
| /// | ||
| /// Panic if `key` is not a parameter for the route. | ||
| pub fn param<T: FromStr>(&self, key: &str) -> Result<T, T::Err> { |
| .unwrap_or_else(|| CookieData { | ||
| content: self | ||
| .headers() | ||
| .get("Cookie") |
There was a problem hiding this comment.
Maybe a more descriptive name like tide-cookie? Just a thought.
| end_point_impl_raw!([Head] $($X),*); | ||
| end_point_impl_raw!($($X),*); | ||
| fn poll(self: Pin<&mut Self>, waker: &std::task::Waker) -> std::task::Poll<Response> { | ||
| let inner = unsafe { self.map_unchecked_mut(|wrapper| &mut wrapper.fut) }; |
There was a problem hiding this comment.
Maybe a comment explaining why we need unsafe here?
There was a problem hiding this comment.
Better yet -- nixed the unsafe :)
This commit reworks Tide, with the goal of reducing the total number of concepts
in the framework. The key new idea is to remove the notion of `Extractor`s, which
in turn allows us to remove or simplify several other concepts in Tide.
We'll first lay out the new design, and then discuss the tradeoffs made in this
simplification.
Here's a full list of the concepts in Tide after this commit:
| Concept | Description |
| ----- | ----------- |
| `App` | Builder for Tide applications |
| `Route` | Builder for an individual route |
| `Endpoint` | Trait for actual endpoints |
| `Context` | The request context for an endpoint |
| `IntoResponse` | A trait for converting into a `Response` |
| `Middleware` | A trait for Tide middleware |
Previously, the `Endpoint` trait was treated as a somewhat magical internal
abstraction, and we used a macro to provide `Endpoint` implementations for
actual endpoints (with varying numbers of extractor arguments).
In this commit, an `Endpoint` is just an asynchronous function from a `Context`
to a `Response`:
```rust
pub trait Endpoint<AppData>: Send + Sync + 'static {
/// The async result of `call`.
type Fut: Future<Output = Response> + Send + 'static;
/// Invoke the endpoint.
fn call(&self, cx: Context<AppData>) -> Self::Fut;
}
```
For convenience, this trait is implemented for async functions that return any
value that implements `IntoResponse`:
```rust
impl<AppData, F, Fut> Endpoint<AppData> for F
where
F: Fn(Context<AppData>) -> Fut,
Fut: Future
Fut::Output: IntoResponse,
// ...
```
This implementation is in contrast to the macro-generated implementations we
previously had, which allowed endpoints with varying numbers of `Extractor`
arguments. The intent is for endpoints to perform their own extraction directly
on the `Context`, as we'll see next.
The `Context` type contains all of the request and middleware context an
endpoint operates on. You can think of it as wrapping an `http_service::Request`
with some additional data.
It's easiest to understand `Context` through the APIs it provides. First, we have
methods for getting basic http request information, mirroring the `http` APIs:
```rust
impl<AppData> Context<AppData> {
pub fn method(&self) -> &Method;
pub fn uri(&self) -> &Uri;
pub fn version(&self) -> Version;
pub fn headers(&self) -> &HeaderMap;
}
```
The context also has a handle to application data, which typically would store
database connection pools and other "application-global" state. This API
replaces the old `AppData` extractor:
```rust
impl<AppData> Context<AppData> {
pub fn app_data(&self) -> &AppData {
&self.app_data
}
}
```
Similarly, we provide a *direct* API for extracting any "route parameters"
(i.e. placeholders in the route URL), replacing the need for `NamedSegment` and
the like:
```rust
impl<AppData> Context<AppData> {
pub fn route_param(&self, key: &str) -> Option<&str>;
}
```
Basic body extraction is likewise built in via `Context` methods, replacing the
`Str`, `Bytes`, and `Json` extractors:
```rust
impl<AppData> Context<AppData> {
pub async fn body_bytes(&mut self) -> std::io::Result<Vec<u8>>;
pub async fn body_string(&mut self) -> std::io::Result<String>;
pub async fn body_json<T: serde::de::DeserializeOwned>(&mut self) -> std::io::Result<T>;
}
```
Looking at the [message database example](https://github.com/rustasync/tide/blob/master/examples/messages.rs#L44),
we previously had endpoints like this:
```rust
async fn new_message(mut db: AppData<Database>, msg: body::Json<Message>) -> String {
db.insert(msg.clone()).to_string()
}
async fn set_message(
mut db: AppData<Database>,
id: head::Path<usize>,
msg: body::Json<Message>,
) -> Result<(), StatusCode> {
if db.set(*id, msg.clone()) {
Ok(())
} else {
Err(StatusCode::NOT_FOUND)
}
}
```
These endpoints would now be written something like this (where `Error` is
intended as a general error type, convertible into a response):
```rust
async fn new_message(cx: Context<Database>) -> Result<String, Error> {
let msg = await!(cx.body_json())?;
cx.app_data().insert(msg).to_string()
}
async fn set_message(cx: Context<Database>) -> Result<(), Error> {
let msg = await!(cx.body_json())?;
if cx.app_data().set(cx.route_param("id"), msg) {
Ok(())
} else {
Err(StatusCode::NOT_FOUND)
}
}
```
The endpoint code is a bit more verbose, but also arguably easier to follow,
since the extraction (and error handling) is more clear.
In addition, the basic extraction approach is *more discoverable*, since it
operates via normal methods on `Context`.
Part of the idea of the old `Extractor` trait was that Tide would provide an
*extensible* system of extractors; you could always introduce new types that
implement `Extractor`. But now most of the existing extractors are built-in
`Context` methods. How do we recover extensibility?
Easy: we use Rust's ability to extend existing types with new methods, via
traits! (Note: this is directly inspired by the Gotham framework).
Let's say we want to provide cookie extraction. Previously, we'd have a `Cookies`
type that you could use as an endpoint argument for extraction. Now, instead, we
can introduce a `Cookies` *trait* that's used to extend `Context` with new APIs:
```rust
trait Cookies {
fn cookies(&self) -> CookieJar;
}
impl<AppData> Cookies for Context<AppData> { ... }
```
This pattern is called an "extension trait" -- a trait whose sole purpose is to
extend an existing type with new methods. There are several nice properties of
this approach:
- The resulting extraction API is just a direct and natural as the built-in
ones: just a method call on the `Context` object.
- The methods that are available on `Context` are controlled by what traits are
in scope. In other words, if you want to use a custom extractor from the
ecosystem, you just bring its trait into scope, and then the method is
available. That makes it easy to build a robust ecosystem around a small set
of core Tide APIs.
One of the major benefits of moving extraction into the endpoint body, rather
than via `Extractor` arguments, is that it's much simpler to provide
configuration. For example, we could easily provide a customized json body
extractor that limited the maximum size of the body or other such options:
```rust
impl<AppData> Context<AppData> {
pub async fn body_json_cfg<T: serde::de::DeserializeOwned>(&mut self, cfg: JsonConfig) -> std::io::Result<T>;
}
```
As a result, we can drop much of the complexity in `App` around configuration.
Following the spirit of the changes to extractors, response generation for
non-standard Rust types is now just done via a free function:
```rust
mod response {
pub fn json<T: serde::Serialize>(t: T) -> Response { ... }
}
```
As before, there's a top-level `App` type for building up a Tide application.
However, the API has been drastically simplified:
- It no longer provides a configuration system, since extractors can now be
configured directly.
- It no longer allows for the middleware list to be customized per route;
instead, middleware is set up only at the top level.
These simplifications make the programming model much easier to understand;
previously, there were inconsistencies between the way that middleware nesting
and configuration nesting worked. The hope is that we can get away with this
much simpler, top-level model.
When actually adding routes via `at`, you get a `Route` object (which used to be
`Resource`). This object now provides a *builder-style* API for adding
endpoints, allowing you to chain several endpoints. Altogether, this means we
can drop nested routing as well.
The middleware trait is more or less as it was before, adjusted to use `Context`
objects and otherwise slightly cleaned up.
This commit also switches to using the route-recognizer crate, rather than the
path-table crate, as the underlying routing mechanism. In addition to being more
efficient, route-recognizer provides a more intuitive semantics for "ambiguous"
routing situations. See issue http-rs#12 and issue http-rs#141 for more details.
|
All comments addressed, merging! Thanks everybody for your patience on this one! |
| // Tide, and as executor context in Juniper. | ||
| #[derive(Clone, Default)] | ||
| struct Context(Arc<atomic::AtomicIsize>); | ||
| struct Data(Arc<atomic::AtomicIsize>); |
There was a problem hiding this comment.
app_data is already Arc<AppData>, do we still need Arc here?
This commit reworks Tide, with the goal of reducing the total number of concepts
in the framework. The key new idea is to remove the notion of
Extractors, whichin turn allows us to remove or simplify several other concepts in Tide.
We'll first lay out the new design, and then discuss the tradeoffs made in this
simplification.
Here's a full list of the concepts in Tide after this commit:
AppRouteEndpointContextIntoResponseResponseMiddlewarePreviously, the
Endpointtrait was treated as a somewhat magical internalabstraction, and we used a macro to provide
Endpointimplementations foractual endpoints (with varying numbers of extractor arguments).
In this commit, an
Endpointis just an asynchronous function from aContextto a
Response:For convenience, this trait is implemented for async functions that return any
value that implements
IntoResponse:This implementation is in contrast to the macro-generated implementations we
previously had, which allowed endpoints with varying numbers of
Extractorarguments. The intent is for endpoints to perform their own extraction directly
on the
Context, as we'll see next.The
Contexttype contains all of the request and middleware context anendpoint operates on. You can think of it as wrapping an
http_service::Requestwith some additional data.
It's easiest to understand
Contextthrough the APIs it provides. First, we havemethods for getting basic http request information, mirroring the
httpAPIs:The context also has a handle to application data, which typically would store
database connection pools and other "application-global" state. This API
replaces the old
AppDataextractor:Similarly, we provide a direct API for extracting any "route parameters"
(i.e. placeholders in the route URL), replacing the need for
NamedSegmentandthe like:
Basic body extraction is likewise built in via
Contextmethods, replacing theStr,Bytes, andJsonextractors:Looking at the message database example,
we previously had endpoints like this:
These endpoints would now be written something like this (where
Errorisintended as a general error type, convertible into a response):
The endpoint code is a bit more verbose, but also arguably easier to follow,
since the extraction (and error handling) is more clear.
In addition, the basic extraction approach is more discoverable, since it
operates via normal methods on
Context.Part of the idea of the old
Extractortrait was that Tide would provide anextensible system of extractors; you could always introduce new types that
implement
Extractor. But now most of the existing extractors are built-inContextmethods. How do we recover extensibility?Easy: we use Rust's ability to extend existing types with new methods, via
traits! (Note: this is directly inspired by the Gotham framework).
Let's say we want to provide cookie extraction. Previously, we'd have a
Cookiestype that you could use as an endpoint argument for extraction. Now, instead, we
can introduce a
Cookiestrait that's used to extendContextwith new APIs:This pattern is called an "extension trait" -- a trait whose sole purpose is to
extend an existing type with new methods. There are several nice properties of
this approach:
The resulting extraction API is just a direct and natural as the built-in
ones: just a method call on the
Contextobject.The methods that are available on
Contextare controlled by what traits arein scope. In other words, if you want to use a custom extractor from the
ecosystem, you just bring its trait into scope, and then the method is
available. That makes it easy to build a robust ecosystem around a small set
of core Tide APIs.
One of the major benefits of moving extraction into the endpoint body, rather
than via
Extractorarguments, is that it's much simpler to provideconfiguration. For example, we could easily provide a customized json body
extractor that limited the maximum size of the body or other such options:
As a result, we can drop much of the complexity in
Apparound configuration.Following the spirit of the changes to extractors, response generation for
non-standard Rust types is now just done via a free function:
As before, there's a top-level
Apptype for building up a Tide application.However, the API has been drastically simplified:
configured directly.
instead, middleware is set up only at the top level.
These simplifications make the programming model much easier to understand;
previously, there were inconsistencies between the way that middleware nesting
and configuration nesting worked. The hope is that we can get away with this
much simpler, top-level model.
When actually adding routes via
at, you get aRouteobject (which used to beResource). This object now provides a builder-style API for addingendpoints, allowing you to chain several endpoints. Altogether, this means we
can drop nested routing as well.
The middleware trait is more or less as it was before, adjusted to use
Contextobjects and otherwise slightly cleaned up.
This commit also switches to using the route-recognizer crate, rather than the
path-table crate, as the underlying routing mechanism. In addition to being more
efficient, route-recognizer provides a more intuitive semantics for "ambiguous"
routing situations. See issue #12 and issue #141 for more details.
TODO
Appnesting