Skip to content
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

introduce lambda-http module based on lando to support apigateway #18

Merged
merged 13 commits into from
Dec 4, 2018

Conversation

softprops
Copy link
Contributor

Issue #, if available:

This attempts to address #13

Description of changes:

Motivation

With this project announced at reinvent, I saw an opportunity to rebase lando a crate providing http crate interfaces for api gateway for deployment on lambda. I started working on that then I stumbled on #13. These crates seem to all serve the same purpose and landos goal from the start was to not to replace tools but instead to work well with them. I saw this work going down the path of fracturing when I'd rather push it forward.

Impl

I just took what I was working on in lando's in migrating towards rebalancing on top of this project and moved into a module in this project.

I took some steps to remove a few dependencies lando had that I think we're actually needed, but my first attempt forced a change to the lambda_runtime::start api which introduced a single allocation in order to support a serde deserializable => http::Request => http::Response => serde serializable transformation where the user space dealt only in terms of http::{Request, Response}'s . I couldn't find a way to avoid it with the lambda_runtime's function pointer interface but I'm open to change/suggestions.

The RequestExt type contains one opinionated method payload which was lando's convenience for transparently deserializing request bodies from form encoded or application/json content. I'm open to pulling this out but I wanted to get the temperature in the room before I did

There's also an opinionated dependency on the failure crate. the failure crate mainly facilitated removing Error definition boilerplate for the payload methods errors. I'm also open to ditching this dependency on impl the boilerplate by hand to keep the dependency tax low

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. ✅

@davidbarsky davidbarsky self-assigned this Nov 30, 2018
@davidbarsky davidbarsky self-requested a review November 30, 2018 16:21
Copy link
Contributor

@sapessi sapessi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, thank you! Few comments in the review. We should clarify that this crate only supports API Gateway's proxy integration events and not events mapped with custom templates.

@davidbarsky
Copy link
Contributor

davidbarsky commented Dec 1, 2018

Small thing @softprops—can you fix the build issues on https://travis-ci.org/awslabs/aws-lambda-rust-runtime/jobs/462147745#L1072? Otherwise, this looks good to me!

@softprops
Copy link
Contributor Author

Will grab this tonight, Bali time :)

@softprops
Copy link
Contributor Author

All set

@danielhstahl
Copy link

I've successfully implemented this PR in my app:

https://github.com/phillyfan1138/option_price_faas

It works great! One suggestion: would it be possible to re-export the crate http? I had to add it to the project as a dependency to get the code to compile (see, for example, https://github.com/phillyfan1138/option_price_faas/blob/master/src/bin/output_constraints.rs)

@softprops
Copy link
Contributor Author

@phillyfan1138 will do. I tried to keep this as keep some corners from lifted lando kept conservative initially. I too found this annoying in practice, much as you typically need to pull in futures crate for the majority of crates that expose futures::{Future,Stream} types in their public API. In lando I ended up re-exporting with DX in mind I'll do that. I'd suggest we start filing things like this as follow ups. I'm a fan of iteration. I'm already thinking there are other things I'd like to add but I want to iterate on it to see if I'll actually need it before I add it

…( http::Response::builder() for example ), re-export the http crate for convenience
@softprops
Copy link
Contributor Author

done.

I backed out of the type alias type alias Response = http::Response<Body>, this was over zealous and didn't come from the lando project but I learned pretty quickly in the example app that you'd need to refer to the http::Response more directly to access its builder() fn as its only defined on Responses with unit types

I re-exported the http crate as requested

I'll likely follow up on this pull with a proposal on one last bit from lando, the IntoResponse trait. Which greatly improved the easy of constructing common types of responses with semantic correctness and decreased verbosity. Again, iteration++ :)

@danielhstahl
Copy link

Awesome! Thanks @softprops !

@softprops
Copy link
Contributor Author

@davidbarsky let me know if there's anything left you'd like to see in this pull. I'd start to.start iterating on top of this in smaller pulls

@davidbarsky
Copy link
Contributor

I think this looks solid rn. I’m happy to merge.

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

Successfully merging this pull request may close these issues.

4 participants