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

Design an API that lets you use graphql-upload with apollo-server-lambda #4951

Closed
glasser opened this issue Feb 23, 2021 · 3 comments
Closed

Comments

@glasser
Copy link
Member

glasser commented Feb 23, 2021

We will remove the direct integration with graphql-upload from Apollo Server 3.

It's easy to plug your favorite version of graphql-upload back in to a middleware-style web library like Express in front of Apollo Server's middleware, but that's not the case for environments like Lambda (the second most popular place to use Apollo Server). We should add some hooks to apollo-server-lambda that allow people to use (among other things!) graphql-upload. The Apollo Server core team is happy to review such a PR but since we are not Lambda users ourselves, we aren't best situated to write that PR.

Ideally we can get this new API out in ASv2 so that folks who want to use graphql-upload with Lambda can migrate to the new API before the old integration is removed in v3.

See #3508 (comment) for some thoughts on how to implement this.

@glasser glasser added this to the Release 3.x milestone Feb 23, 2021
@jaydenseric
Copy link

Probably it's best for apollo-server-lambda not to prescribe an approach for file uploads until there is a reliable serverless API available from graphql-upload, here are the enhancement issues:

Unfortunately serverless environments tend to buffer the whole multipart request into memory, then pass that into the function after. This means the file uploads can't be processed while they stream in, and memory requirements balloon by whatever the size of the files is. Existing hacks using the processRequest function in AWS Lambda are not ideal, for reasons explained here:

jaydenseric/graphql-upload#155 (comment)

Since AWS will gather the whole request in memory before passing it on to your function, any workarounds to try to simulate a request stream for processRequest which then buffers it to "disk" all over again via fs-capacitor would be a performance tragedy.

For propper AWS Lambda support we could publish a function that processes an event into something that can be passed into existing GraphQL handlers, without using fs-capacitor. It would read the GraphQL multipart request form data (Base64?) string provided in the Lambda event, and extract the files and query and variables, putting them together to look like a regular GraphQL POST request for popular GraphQL handlers (e.g. apollo-server-lambda) to consume.

I haven't thought a lot about the details, and don't plan to work on it personally anytime soon as I don't have a need for it for any of my work or personal projects. I prefer to process file uploads as the stream in, for a much faster response time:

https://github.com/jaydenseric/graphql-multipart-request-spec/blob/master/sync-vs-async-graphql-multipart-request-middleware.svg

A function for Lambda should actually be a much simpler implementation that the current processRequest implementation for HTTP requests that stream in, because you can tell up front if the form data is malformed or not and you know all the files are there; you don't have to handle all the problems of expected files never arriving, disconnects part way through, multiple resolvers consuming the same file streams etc.

Ideally the new serverless API would be agnostic to all the serverless environments that exist (and might exist in the future) and instead of supporting specific vendors by name, can easily be adapted to either AWS Lambda or Google Cloud Functions. Maybe by processing a raw request body string?

The Apollo community is welcome to contribute ideas for the above issues and submit a PR.

@glasser
Copy link
Member Author

glasser commented Feb 24, 2021

Thanks @jaydenseric. I do agree that using today's graphql-upload with Lambda isn't optimal. On the other hand, people actually do use it, so my main goal is not completely breaking those folks when the built-in integration is removed in AS3.

I think my ideas in #3508 (comment) are generic enough that they may just generally make apollo-server-lambda more usable, while also solving the graphql-upload integration issue. Hopefully the same hook would also support a non-capacitor graphql-upload implementation!

@glasser
Copy link
Member Author

glasser commented Apr 1, 2021

One other approach here would be to completely replace apollo-server-lambda with something that uses something like https://github.com/vendia/serverless-express to wrap apollo-server-express. So the extension API for apollo-server-lambda would be the same as the extension API for apollo-server-express, and we would make sure that this supports graphql-upload as well.

@glasser glasser added this to To Organize in Apollo Server 3 May 2, 2021
@glasser glasser moved this from To Organize to Potential breaking changes in Apollo Server 3 May 2, 2021
glasser added a commit that referenced this issue May 13, 2021
Before this change, apollo-server-lambda contained a lot of code that tried to
understand the formats of Lambda event (request) and result (response)
formats. It tried to work with APIGatewayProxy messages of payloadFormatVersion
1.0 and 2.0 as well as ALB.

But understanding the intricacies of the various Lambda message formats isn't
really what the Apollo Server project is about. A pretty large fraction of all
maintenance on Apollo Server in 2021 has gone into tweaking details of the
Lambda event parsing and making the Lambda handler support all AS features
without the help of a library supporting composable middleware.

This PR (targeted for AS3) throws away the laboriously constructed bespoke
Lambda parsing and faux-middleware implementation and replaces it with two
packages that solve these problems for us: `@vendia/serverless-express` which
understands a variety of Lambda input and output formats and converts them to
Express format, and `express`, the most popular Node library for defining HTTP
server behavior. (Note that `@vendia/serverless-express` is not related to the
`serverless` CLI/framework.)

Now `apollo-server-lambda` is just a convenience wrapper around
`apollo-server-express`. It has to deal with the difference in startup logic
between serverless and non-serverless environments, but it doesn't have to
reimplement all of the Lambda and HTTP logic from scratch.

As an added advantage, you can now optionally provide your own express app to
`apollo-server-lambda`. Previously, `apollo-server-lambda` gave no real way to
customize any of its behavior past the particular options we define, because
Lambda doesn't have a built-in middleware framework. Since we are removing some
built-in features like `graphql-upload` integration in AS3, it's important that
we continue a way to add custom behavior to your Lambda server. Letting you
define that custom behavior with a standard Express app seems reasonable.

We recognize that Lambda users generally care strongly about bundle size, so
adding two new dependencies may seem problematic. That said, we don't currently
have a principled way of evaluating Lambda bundle sizes when we make choices in
this project, and compared to other dependencies of Apollo Server, these new
dependencies are not very large. For now, the improvement in maintainability and
flexibility seems worth the bundle size increase. If `apollo-server-lambda`
users want to help out with a new project of focusing on Lambda bundle size
optimization, we can work together to define benchmarks based on realistic
build/bundler conditions, and find other ways to reduce bundle size (eg, there
is a fair amount of low hanging fruit inside `apollo-reporting-protobuf`).

Fixes #5078. Fixes #4951 (because that API is just "Express").
glasser added a commit that referenced this issue May 13, 2021
Before this change, apollo-server-lambda contained a lot of code that tried to
understand the formats of Lambda event (request) and result (response)
formats. It tried to work with APIGatewayProxy messages of payloadFormatVersion
1.0 and 2.0 as well as ALB.

But understanding the intricacies of the various Lambda message formats isn't
really what the Apollo Server project is about. A pretty large fraction of all
maintenance on Apollo Server in 2021 has gone into tweaking details of the
Lambda event parsing and making the Lambda handler support all AS features
without the help of a library supporting composable middleware.

This PR (targeted for AS3) throws away the laboriously constructed bespoke
Lambda parsing and faux-middleware implementation and replaces it with two
packages that solve these problems for us: `@vendia/serverless-express` which
understands a variety of Lambda input and output formats and converts them to
Express format, and `express`, the most popular Node library for defining HTTP
server behavior. (Note that `@vendia/serverless-express` is not related to the
`serverless` CLI/framework.)

Now `apollo-server-lambda` is just a convenience wrapper around
`apollo-server-express`. It has to deal with the difference in startup logic
between serverless and non-serverless environments, but it doesn't have to
reimplement all of the Lambda and HTTP logic from scratch.

As an added advantage, you can now optionally provide your own express app to
`apollo-server-lambda`. Previously, `apollo-server-lambda` gave no real way to
customize any of its behavior past the particular options we define, because
Lambda doesn't have a built-in middleware framework. Since we are removing some
built-in features like `graphql-upload` integration in AS3, it's important that
we continue a way to add custom behavior to your Lambda server. Letting you
define that custom behavior with a standard Express app seems reasonable.

We recognize that Lambda users generally care strongly about bundle size, so
adding two new dependencies may seem problematic. That said, we don't currently
have a principled way of evaluating Lambda bundle sizes when we make choices in
this project, and compared to other dependencies of Apollo Server, these new
dependencies are not very large. For now, the improvement in maintainability and
flexibility seems worth the bundle size increase. If `apollo-server-lambda`
users want to help out with a new project of focusing on Lambda bundle size
optimization, we can work together to define benchmarks based on realistic
build/bundler conditions, and find other ways to reduce bundle size (eg, there
is a fair amount of low hanging fruit inside `apollo-reporting-protobuf`).

Fix inconsistency in the content-type returned from health checks (the old
Lambda used `application/json` instead of `application/health+json` like most
other integrations).

This new version passes all of the existing integration tests (plus the
`testApolloServer` suite from
`apollo-server-integration-testsuite/src/ApolloServer.ts` which wasn't being run
previously!) essentially out of the box. (I fleshed out the "mock server"
implementation which converts from Node http requests to Lambda events a bit
more, and changed, but no "core" code or test changes were needed other than
fixing the health check `content-type`.)

Fixes #5078. Fixes #4951 (because that API is just "Express").
glasser added a commit that referenced this issue May 13, 2021
Before this change, apollo-server-lambda contained a lot of code that tried to
understand the formats of Lambda event (request) and result (response)
formats. It tried to work with APIGatewayProxy messages of payloadFormatVersion
1.0 and 2.0 as well as ALB.

But understanding the intricacies of the various Lambda message formats isn't
really what the Apollo Server project is about. A pretty large fraction of all
maintenance on Apollo Server in 2021 has gone into tweaking details of the
Lambda event parsing and making the Lambda handler support all AS features
without the help of a library supporting composable middleware.

This PR (targeted for AS3) throws away the laboriously constructed bespoke
Lambda parsing and faux-middleware implementation and replaces it with two
packages that solve these problems for us: `@vendia/serverless-express` which
understands a variety of Lambda input and output formats and converts them to
Express format, and `express`, the most popular Node library for defining HTTP
server behavior. (Note that `@vendia/serverless-express` is not related to the
`serverless` CLI/framework.)

Now `apollo-server-lambda` is just a convenience wrapper around
`apollo-server-express`. It has to deal with the difference in startup logic
between serverless and non-serverless environments, but it doesn't have to
reimplement all of the Lambda and HTTP logic from scratch.

As an added advantage, you can now optionally provide your own express app to
`apollo-server-lambda`. Previously, `apollo-server-lambda` gave no real way to
customize any of its behavior past the particular options we define, because
Lambda doesn't have a built-in middleware framework. Since we are removing some
built-in features like `graphql-upload` integration in AS3, it's important that
we continue a way to add custom behavior to your Lambda server. Letting you
define that custom behavior with a standard Express app seems reasonable.

We recognize that Lambda users generally care strongly about bundle size, so
adding two new dependencies may seem problematic. That said, we don't currently
have a principled way of evaluating Lambda bundle sizes when we make choices in
this project, and compared to other dependencies of Apollo Server, these new
dependencies are not very large. For now, the improvement in maintainability and
flexibility seems worth the bundle size increase. If `apollo-server-lambda`
users want to help out with a new project of focusing on Lambda bundle size
optimization, we can work together to define benchmarks based on realistic
build/bundler conditions, and find other ways to reduce bundle size (eg, there
is a fair amount of low hanging fruit inside `apollo-reporting-protobuf`).

Fix inconsistency in the content-type returned from health checks (the old
Lambda used `application/json` instead of `application/health+json` like most
other integrations).

This new version passes all of the existing integration tests (plus the
`testApolloServer` suite from
`apollo-server-integration-testsuite/src/ApolloServer.ts` which wasn't being run
previously!) essentially out of the box. (I fleshed out the "mock server"
implementation which converts from Node http requests to Lambda events a bit
more, and changed, but no "core" code or test changes were needed other than
fixing the health check `content-type`.)

Fixes #5078. Fixes #4951 (because that API is just "Express").
glasser added a commit that referenced this issue May 21, 2021
Before this change, apollo-server-lambda contained a lot of code that tried to
understand the formats of Lambda event (request) and result (response)
formats. It tried to work with APIGatewayProxy messages of payloadFormatVersion
1.0 and 2.0 as well as ALB.

But understanding the intricacies of the various Lambda message formats isn't
really what the Apollo Server project is about. A pretty large fraction of all
maintenance on Apollo Server in 2021 has gone into tweaking details of the
Lambda event parsing and making the Lambda handler support all AS features
without the help of a library supporting composable middleware.

This PR (targeted for AS3) throws away the laboriously constructed bespoke
Lambda parsing and faux-middleware implementation and replaces it with two
packages that solve these problems for us: `@vendia/serverless-express` which
understands a variety of Lambda input and output formats and converts them to
Express format, and `express`, the most popular Node library for defining HTTP
server behavior. (Note that `@vendia/serverless-express` is not related to the
`serverless` CLI/framework.)

Now `apollo-server-lambda` is just a convenience wrapper around
`apollo-server-express`. It has to deal with the difference in startup logic
between serverless and non-serverless environments, but it doesn't have to
reimplement all of the Lambda and HTTP logic from scratch.

As an added advantage, you can now optionally provide your own express app to
`apollo-server-lambda`. Previously, `apollo-server-lambda` gave no real way to
customize any of its behavior past the particular options we define, because
Lambda doesn't have a built-in middleware framework. Since we are removing some
built-in features like `graphql-upload` integration in AS3, it's important that
we continue a way to add custom behavior to your Lambda server. Letting you
define that custom behavior with a standard Express app seems reasonable.

We recognize that Lambda users generally care strongly about bundle size, so
adding two new dependencies may seem problematic. That said, we don't currently
have a principled way of evaluating Lambda bundle sizes when we make choices in
this project, and compared to other dependencies of Apollo Server, these new
dependencies are not very large. For now, the improvement in maintainability and
flexibility seems worth the bundle size increase. If `apollo-server-lambda`
users want to help out with a new project of focusing on Lambda bundle size
optimization, we can work together to define benchmarks based on realistic
build/bundler conditions, and find other ways to reduce bundle size (eg, there
is a fair amount of low hanging fruit inside `apollo-reporting-protobuf`).

Fix inconsistency in the content-type returned from health checks (the old
Lambda used `application/json` instead of `application/health+json` like most
other integrations).

This new version passes all of the existing integration tests (plus the
`testApolloServer` suite from
`apollo-server-integration-testsuite/src/ApolloServer.ts` which wasn't being run
previously!) essentially out of the box. (I fleshed out the "mock server"
implementation which converts from Node http requests to Lambda events a bit
more, and changed, but no "core" code or test changes were needed other than
fixing the health check `content-type`.)

Fixes #5078. Fixes #4951 (because that API is just "Express").
glasser added a commit that referenced this issue May 21, 2021
Before this change, apollo-server-lambda contained a lot of code that tried to
understand the formats of Lambda event (request) and result (response)
formats. It tried to work with APIGatewayProxy messages of payloadFormatVersion
1.0 and 2.0 as well as ALB.

But understanding the intricacies of the various Lambda message formats isn't
really what the Apollo Server project is about. A pretty large fraction of all
maintenance on Apollo Server in 2021 has gone into tweaking details of the
Lambda event parsing and making the Lambda handler support all AS features
without the help of a library supporting composable middleware.

This PR (targeted for AS3) throws away the laboriously constructed bespoke
Lambda parsing and faux-middleware implementation and replaces it with two
packages that solve these problems for us: `@vendia/serverless-express` which
understands a variety of Lambda input and output formats and converts them to
Express format, and `express`, the most popular Node library for defining HTTP
server behavior. (Note that `@vendia/serverless-express` is not related to the
`serverless` CLI/framework.)

Now `apollo-server-lambda` is just a convenience wrapper around
`apollo-server-express`. It has to deal with the difference in startup logic
between serverless and non-serverless environments, but it doesn't have to
reimplement all of the Lambda and HTTP logic from scratch.

As an added advantage, you can now optionally provide your own express app to
`apollo-server-lambda`. Previously, `apollo-server-lambda` gave no real way to
customize any of its behavior past the particular options we define, because
Lambda doesn't have a built-in middleware framework. Since we are removing some
built-in features like `graphql-upload` integration in AS3, it's important that
we continue a way to add custom behavior to your Lambda server. Letting you
define that custom behavior with a standard Express app seems reasonable.

We recognize that Lambda users generally care strongly about bundle size, so
adding two new dependencies may seem problematic. That said, we don't currently
have a principled way of evaluating Lambda bundle sizes when we make choices in
this project, and compared to other dependencies of Apollo Server, these new
dependencies are not very large. For now, the improvement in maintainability and
flexibility seems worth the bundle size increase. If `apollo-server-lambda`
users want to help out with a new project of focusing on Lambda bundle size
optimization, we can work together to define benchmarks based on realistic
build/bundler conditions, and find other ways to reduce bundle size (eg, there
is a fair amount of low hanging fruit inside `apollo-reporting-protobuf`).

Fix inconsistency in the content-type returned from health checks (the old
Lambda used `application/json` instead of `application/health+json` like most
other integrations).

This new version passes all of the existing integration tests (plus the
`testApolloServer` suite from
`apollo-server-integration-testsuite/src/ApolloServer.ts` which wasn't being run
previously!) essentially out of the box. (I fleshed out the "mock server"
implementation which converts from Node http requests to Lambda events a bit
more, and changed, but no "core" code or test changes were needed other than
fixing the health check `content-type`.)

Fixes #5078. Fixes #4951 (because that API is just "Express").
@glasser glasser closed this as completed May 25, 2021
Apollo Server 3 automation moved this from Potential breaking changes to Done May 25, 2021
@glasser glasser removed this from the Release 3.x milestone Jun 10, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
Development

No branches or pull requests

2 participants