Skip to content

[RFC] Wrap interface for tower::Service<Request> #404

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
nmoutschen opened this issue Jan 17, 2022 · 10 comments
Closed

[RFC] Wrap interface for tower::Service<Request> #404

nmoutschen opened this issue Jan 17, 2022 · 10 comments

Comments

@nmoutschen
Copy link
Contributor

Remark: This is a follow-up to #374 as I'm currently implementing it. See #401 for progress.

Background

Right now, the current implementation in lambda_http::handler takes a handler function that takes a Request and returns an IntoResponse implementation. This allows to pass the wrapped handler function into lambda_runtime::run directly. However, it has a few limitations.

First, Request and IntoResponse aren't Deserialize and Serialize, but lambda_runtime::run require those traits. It means we have to pass through an intermediate Adapter to transform the requests and responses. This is currently handled by lambda_http::handler. As a consequence, the current implementation doesn't allow taking a lambda_runtime::Handler implementation directly. It offers a lambda_http::Handler, which implements it.

This causes some switching away from custom Handler traits towards tower::Service. Ideally, we should allow users to provide tower::Service<Request> implementations, and handle the transformation under the hood on their behalf. However, due to lambda_runtime::run's signature, we cannot do that as-is. We also cannot implement Deserialize and Serialize for Request and Response, as those are all foreign traits and types.

Proposition 1: extension trait for Service<Request>

use tower::Service;
use lambda_http::{Request, ServiceExt};

#[derive(Default)]
struct MyHandler;

impl Service<Request> for MyHandler {
    // skipped
}

#[tokio::main]
async fn main() -> Result<(), Error> {
    // Use .into_runtime() to transform the Service
    lambda_runtime::run(MyHandler::default().into_runtime()).await
}

Proposition 2: create a lambda_http::run wrapper

use tower::Service;
use lambda_http::Request;

#[derive(Default)]
struct MyHandler;

impl Service<Request> for MyHandler {
    // skipped
}

#[tokio::main]
async fn main() -> Result<(), Error> {
    // Use lambda_http::run to wrap the Service
    lambda_http::run(MyHandler::default()).await
}

Proposition 3: your idea here

If you have an idea that gives a cleaner interface, let me know!

@brainstorm
Copy link

I do find Proposition 2 more clean than prop 1, as an end user that is.

@rimutaka
Copy link
Contributor

What are the limitations of Prop 1 vs Prop 2? If the only difference is having .into_runtime() then Prop 2 seems nicer.

@calavera
Copy link
Contributor

Option 2 looks more clear to me. I link that I don't have to remember a different crate name to run the service, it's all in lambda_http.

@greenwoodcm
Copy link
Contributor

I agree that Prop 2 seems cleaner as well

@nmoutschen
Copy link
Contributor Author

Looks like everyone's on agreement on this, and this will simplify the external interface quite a lot. 😄 Thanks, folks!

@LucioFranco
Copy link
Contributor

I think prop 2 looks good. I would probably use service_fn over a manual service implementation. In addition, this supports people to use things like warp or axum which is nice. I think building around tower is a great initiative (I think ive wanted this in here for a while).

cc @davidpdrsn may be interested

@davidpdrsn
Copy link

I think building around tower is a great initiative (I think ive wanted this in here for a while).

I agree! This looks great 😃

@coltonweaver
Copy link
Contributor

+1 to seeing if we can use service_fn, but overall Prop 2 is great. Thanks for putting this together @nmoutschen!

@nmoutschen
Copy link
Contributor Author

We'll be able to support service_fn here, for lambda_http, lambda_runtime, and lambda_extension.

For lambda_runtime, the requirement will be to pass a lambda_runtime::LambdaEvent<T>. I've kept the handler_fn so people can still provide a function with 2 arguments (one for the event, one for the context).

For lambda_http, I'd like to use ServiceExt to hold the context, so people could pass a tower::Service<http::Request>, and thus use service_fn with a function that takes a Request.

@nmoutschen
Copy link
Contributor Author

Closing the ticket as this is now implemented in 84d41c9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants