fermyon / spin

Spin is the open source developer tool for building and running serverless applications powered by WebAssembly.
https://developer.fermyon.com/spin
Apache License 2.0
5.17k stars 247 forks source link

Using the Rust SDK router with the new `Request<Json<T>>` type fails when calling `route.handle(req);` #1973

Closed mikkelhegn closed 11 months ago

mikkelhegn commented 11 months ago

Using Spin from main

The following code:

#[http_component]
fn handle_route(req: http::Request<Json<MyType>>) -> Response {
    let mut router = Router::new();
    router.any("/api/*", not_found);
    router.handle(req)
}

produces the following error:

error[E0277]: the trait bound `spin_sdk::http::Request: From<http::Request<Json<MyType>>>` is not satisfied
  --> src/lib.rs:49:19
   |
49 |     router.handle(req)
   |            ------ ^^^ the trait `From<http::Request<Json<MyType>>>` is not implemented for `spin_sdk::http::Request`
   |            |
   |            required by a bound introduced by this call
   |
   = note: required for `http::Request<Json<MyType>>` to implement `Into<spin_sdk::http::Request>`
note: required by a bound in `Router::handle`
  --> /Users/mikkel/.cargo/git/checkouts/spin-91500438ac5656d2/a4da3b7/sdk/rust/src/http/router.rs:42:22
   |
42 |     pub fn handle<R: Into<Request>>(&self, request: R) -> Response {
   |                      ^^^^^^^^^^^^^ required by this bound in `Router::handle`

For more information about this error, try `rustc --explain E0277`.
radu-matei commented 11 months ago

A really convenient option here would also be different bodies per route:

/// A Spin HTTP component that internally routes requests.
#[http_component]
fn handle_route(req: Request) -> Response {
    let mut router = Router::new();
    router.get("/hello/:planet", hello_body);
    router.handle(req)
}

pub fn hello_body(_req: Request<Json<MyType>>, params: Params) -> Result<impl IntoResponse> {
    let planet = params.get("planet").expect("PLANET");

    Ok(Response::new(200, planet.to_string()))
}
mikkelhegn commented 11 months ago

@rylev - I'm still seeing this error with v2.0.0-rc.1 version of Spin and the SDK.

mikkelhegn commented 11 months ago

However, what @radu-matei suggested above works, by using the http::Request<Json<T>> type for a route function, and the spin::http::Request type in the #[http_component]. Not sure if that's the intention of the fix?

rylev commented 11 months ago

@mikkelhegn Sorry for this!

The problem

The reason for this is that the request handed to Router::handle must be convertible to a spin_sdk::http::Request in an infallible way, because we have no way with the Router::handle to handle errors in that conversion. More specifically in this case, the body of the request handed to Router::handle must be convertible to a Vec<u8> (the body type of spin_sdk::http::Request).

A http::Request<Json<T>> cannot be guaranteed to convert to spin_sdk::http::Request without an error because Json<T> is not guaranteed to be serializable to JSON without an error, and there's no way for us to really be able to statically enforce this. There is no type bound on T that we can set to say "T must be serializable to JSON without runtime error". T: Serialize does not guarantee that T is serializable as JSON (hence why serde_json::to_vec returns a Result<Vec<u8>, Error> and not just a Vec<u8>).

Possible solutions

Runtime panics

We could make it an implicit contract that aJson<T> must always be able to successfully serialize to JSON and panic if it does not. This makes things that feel like they should work just work, but naturally having an API that can panic makes me uncomfortable.

Change the signature of Router::handle

We make Router::handle take any type T where T: TryInto<Request>, T::Error: IntoResponse. This would let us handle errors when converting the request types. We could then implement TryInto<Request> for hyperium::Request<Json<T>> where T: Serialize and return an error when serializing T fails.

This is technically a breaking change from what we currently have, but a minor one that we might be able to sneak in before release.

All of this also makes me want to change Router::handle to take T which is bounded by TryIntoRequest trait that we fully control because relying on the std conversion traits is a compatibility hazard for the future. This is why we have custom traits for all other conversions.

I'll open a PR.