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

AWS Lambda Integration #247

Merged
merged 5 commits into from
Feb 1, 2017
Merged

Conversation

NeoReyad
Copy link
Contributor

@NeoReyad NeoReyad commented Dec 17, 2016

This adds the AWS lambda integration for Apollo Server. This requires a Lambda function that is attached to an API Gateway using the Lambda Proxy Integration. I had to add another parameter to the server to pass in headers. Headers to allow cross origin calls cannot be set when using Lambda Proxy Integration mode and must be passed in when calling the callback function.

To allow for the tests to pass I had to mock out the req and res objects that are required by the test suite. I thought this was easier than copying all the tests. I still get errors printed out, from uncaught exceptions, still looking into why.

I will add an example and some documentation to the README file and complete all the todo tasks below.

Closes #101

TODO:

  • Update CHANGELOG.md with your change (include reference to issue & this PR)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass

@apollo-cla
Copy link

@NeoReyad: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@NeoReyad NeoReyad changed the title Lambda support AWS Lambda Integration Dec 17, 2016
@dfischer
Copy link

dfischer commented Jan 3, 2017

This is awesome.

Copy link
Contributor

@DxCx DxCx left a comment

Choose a reason for hiding this comment

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

Good Job! Thanks!
this can be really awsome :P

"graphql-server-module-graphiql": "^0.4.3"
},
"devDependencies": {
"@types/aws-lambda": "0.0.5",
Copy link
Contributor

Choose a reason for hiding this comment

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

no dep (or peer-dep) for aws-lambda?
i'm not familiar with the node library so i'm not sure ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The typing library aws-lambda only provides type interfaces for the lambda function handler. It used when compiling typescript to ensure proper use of function parameters (event, context, and callback).

throw new Error('Apollo Server requires options.');
}
if (!headers) {
headers = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

can be done with default value on argument

Copy link
Contributor

Choose a reason for hiding this comment

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

also, i'm thinking maybe it will be better to keep the one argument convention like the other implementation, and just extend GraphQLOptions for Lambda, adding the optional headers
then the callback can also decide which headers to send back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, we should eliminate the headers options and stick with one argument. Will make this change.

@NeoReyad NeoReyad force-pushed the lambda-support branch 2 times, most recently from 076f756 to 961275b Compare January 12, 2017 16:31
"node_modules/@types"
],
"types": [
"typed-graphql",
Copy link
Contributor

Choose a reason for hiding this comment

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

please remember to remove (since we moved to @types/graphql) when rebasing this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch thanks.

@DxCx
Copy link
Contributor

DxCx commented Jan 19, 2017

hi @NeoReyad
version 0.5.1 is now out and there are quite few changes in integrations,
can you please rebase and align the integration package so we can merge it in?

@DxCx
Copy link
Contributor

DxCx commented Jan 25, 2017

@helfer please review,
i'll probably rebase and merge tomorrow if we won't hear from @NeoReyad until then..

@NeoReyad
Copy link
Contributor Author

Will Finnish up in a few hours. Just need to test get support and add some more documentation

@DxCx
Copy link
Contributor

DxCx commented Jan 25, 2017

oh cool, thanks @NeoReyad! :)

Copy link
Contributor

@DxCx DxCx left a comment

Choose a reason for hiding this comment

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

Thanks alot, looks amazing!
i can see the commit called WIP what are you still missing?
apart for small changes it looks pretty ready =)

"node_modules/@types"
],
"types": [
"@types/graphql",
Copy link
Contributor

Choose a reason for hiding this comment

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

are you sure you need @types/graphql in here?
it think is requested as dep of core so you get's it from there..

test/tests.js Outdated
@@ -8,3 +8,4 @@ require('../packages/graphql-server-hapi/dist/hapiApollo.test');
require('../packages/graphql-server-koa/dist/koaApollo.test');
require('../packages/graphql-server-restify/dist/restifyApollo.test');
require('../packages/graphql-server-express/dist/apolloServerHttp.test');
require('../packages/graphql-server-lambda/dist/lambdaApollo.test');
Copy link
Contributor

Choose a reason for hiding this comment

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

can you take that 1 line upper? :) apolloServerHttp is not integration test all modules are running but extra testing..

"devDependencies": {
"@types/aws-lambda": "0.0.5",
"@types/body-parser": "0.0.33",
"@types/multer": "0.0.32",
Copy link
Contributor

Choose a reason for hiding this comment

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

are you using multer & body-parser?
if not let's clear

"@types/aws-lambda": "0.0.5",
"@types/body-parser": "0.0.33",
"@types/multer": "0.0.32",
"@types/graphql": "^0.8.5",
Copy link
Contributor

Choose a reason for hiding this comment

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

you can already set to 0.8.6 ;)

@@ -31,6 +31,7 @@ where variant is one of the following:
- express
- koa
- hapi
Copy link
Contributor

Choose a reason for hiding this comment

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

that's not you, but i didn't noted it before.
can you add - restify as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks!

@NeoReyad
Copy link
Contributor Author

Thanks for review @DxCx. I will make the changes. I just want to update the readme of the graphql-server-lambda with basic instructions on how to deploy to lambda and test with get queires.

@DxCx
Copy link
Contributor

DxCx commented Jan 25, 2017

oh cool no problem! 👍

@@ -31,6 +31,7 @@ where variant is one of the following:
- express
- koa
- hapi
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks!

"graphql-server-integration-testsuite": "^0.5.1"
},
"peerDependencies": {
"graphql": "^0.6.1 || ^0.7.0 || ^0.8.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

please align with
#278

@@ -0,0 +1,25 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

please align with #281

@DxCx
Copy link
Contributor

DxCx commented Jan 30, 2017

hi @NeoReyad we want to release a new package tomorrow with this PR inside,
can you take care of the last README.md you wanted to add?
or i'll just finish it up and merge, then you will open a new PR for the README?

@NeoReyad
Copy link
Contributor Author

I will finish this tonight. I'm sorry about the delay have been busy.

@DxCx
Copy link
Contributor

DxCx commented Jan 30, 2017

np @NeoReyad :)
thanks alot for the contribution,
i'll close this up, and you are free to open another PR for the README.md
sounds good?

@NeoReyad
Copy link
Contributor Author

Let me just make sure it works. I think I had an issue with parsing the json, not catching an exception on empty string. Give me two hours Ill take a look at it right now.

@NeoReyad
Copy link
Contributor Author

@DxCx If you want to merge it now that is fine and i can send pr for fix if i find one.

@DxCx
Copy link
Contributor

DxCx commented Jan 30, 2017

Thanks,
planing on doing it tomorrow :)

Create an integration that will return a lambda handler for a
graphql or graphiql server. This integration requires an API
Gateway with Lambda Proxy Integration.

The test runner expects req and res objects for testing. We
have to mock these for the tests to execute properly.
Add example lambda handler and some documentation
@NeoReyad
Copy link
Contributor Author

@DxCx Should be good to go. I had the headers as an array instead of an object. Will submit another PR for the documentation. But won't have time till later tonight.

This adds basic instructions to the README for deploying the lambda function. It uses the Serverless Application Model by creating a template that is used by the AWS CLI.
@NeoReyad
Copy link
Contributor Author

Let me know if you want me to change anything else or update the instructions. Sorry about the delay

@DxCx
Copy link
Contributor

DxCx commented Jan 31, 2017

wow @NeoReyad i was about to finish it now and seen you were done.
double thanks, @helfer let's merge and release 👍

@helfer
Copy link
Contributor

helfer commented Feb 1, 2017

This is great, thanks a lot @NeoReyad and @DxCx !

@helfer helfer merged commit 836472b into apollographql:master Feb 1, 2017
@jangerhofer
Copy link

jangerhofer commented Feb 17, 2017

Holy cow, this is killer! Not that I would cast any aspersions on Meteor, but this provides the nail in the coffin of replacing the Meteor stack as far as I am concerned.

Many thanks to you, @NeoReyad!

@nicka
Copy link

nicka commented Mar 3, 2017

Would it be possible to get access to event.headers within the GraphQL context?

@soda0289
Copy link
Contributor

soda0289 commented Mar 3, 2017

@nicka You might want to open an issue for more details. It is possible like this:

exports.graphqlHandler = function(event, context, callback)  {
    const headers = event.headers;
    server.graphqlLambda({ schema: myGraphQLSchema })(event, context, callback);
};

@nicka
Copy link

nicka commented Mar 3, 2017

@soda0289 sorry for my stupidity haha, exactly what I needed! Thanks 👍

@jangerhofer
Copy link

jangerhofer commented Mar 19, 2017

So that covers the request headers, but how does one set response headers? I tried to setup an Apollo client w/ CORS (from my website to the AWS API Gateway URL) but couldn't figure out how to set the "Access-Control-Allow-Origin" header without hard-coding it into the success condition of this library like so: headers['Access-Control-Allow-Origin'] = '*';

Is there an easy option I can pass and I'm missing something? Should I submit a PR to make this less hacky?

@soda0289
Copy link
Contributor

@jangerhofer
This has been discussed in this issue:
#315

I have been meaning to update the documentation or do you think we should allow an option to pass through custom headers. It might be possible to add the headers with the api gateway response mapping but I'm not sure you can do that with the proxy integration.

@Siyfion
Copy link
Contributor

Siyfion commented Jun 16, 2017

@NeoReyad Do you think it might be worth converting the docs (or adding to them) to use Serverless (https://serverless.com/) as it will make the whole process just that much easier and simpler to get started?

@soda0289
Copy link
Contributor

I've been meaning to do this for a while. I will try and get to it this week.

There is a config here if that helps:
#332 (comment)

@Siyfion
Copy link
Contributor

Siyfion commented Jun 16, 2017

Yup, that's pretty much what I ended up with!

@Siyfion
Copy link
Contributor

Siyfion commented Aug 9, 2017

@soda0289 / @NeoReyad / @DxCx are any of you having issues getting this to work on Lambda with Node v6.10? I have a situation whereby the whole application is working perfectly in serverless offline but when I deploy, the authorizer function runs, then the GraphQL code just times-out after my maximum is reached (10secs).

@soda0289
Copy link
Contributor

soda0289 commented Aug 9, 2017

@Siyfion There is an issue open here #456 which suggests that the Lambda function will not respond. If we set callbackWaitsForEmptyEventLoop to true it will respond as soon as the callback is called. You can try setting this in your callback. Let me know if that helps or if you need some sample code

@Siyfion
Copy link
Contributor

Siyfion commented Aug 9, 2017

Wow! 😮 Fastest response ever award, goes to @soda0289! Cheers, and yes, that looks almost certainly like the issue I am facing!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants