Add support for lambda to upload file

[charleswong28]

Fix #1419, #1703 file upload using lambda.

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

Graphql and apollo-server are still quite new to me. It would be great if anyone here could point me to the right direction.

1. uploadsConfig is now passed to createHandler but I believe it's better to be passed to apollo server instead. Not sure how to access apollo server config in handler.
2. Unit test was using apollo-server-integration-testsuite but I am not sure if other package is able to test with file upload or not.

[apollo-cla]

@charleswong28: 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/

[charleswong28]

Encountered a problem with uploading image in my project.

Error: Part terminated early due to unexpected end of multipart data
 at ...node_modules/apollo-server-lambda/node_modules/@apollographql/apollo-upload-server/node_modules/dicer/lib/Dicer.js:65:36
    at _combinedTickCallback (internal/process/next_tick.js:131:7)
    at process._tickDomainCallback (internal/process/next_tick.js:218:9)

Changing request.push(event.body); to request.push(Buffer.from(event.body, 'ascii')); fixed the problem. From my understanding, there is null character in event.body, the stream is closed earlier and the multipart data is ended unexpectedly. A new commit (6f28f8b) is pushed.

[charleswong28]

According to this, event.isBase64Encoded will be true after adding multipart/form-data to Binary Media Types. In order to encode the stream correctly, I changed to

Buffer.from(event.body, event.isBase64Encoded ? 'base64' : 'ascii')

in this commit.

[yornaath]

So close! In need of this fix! :D Please

[amwill04]

@charleswong28 have been using this in development for the past week and havent had any issues so far FWIW

[lora-reames]

Any movement on reviewing this PR? my work progress is blocked by the issues this fixes

[amwill04]

Can someone advise if I can take over this PR? As far as I am aware it has passed all test and simply requires to be rebased with master - is this correct?

Im obviously not want to take any credit from @charleswong28 - I just simply need this fix ASAP

[charleswong28]

Can someone advise if I can take over this PR? As far as I am aware it has passed all test and simply requires to be rebased with master - is this correct?

Im obviously not want to take any credit from @charleswong28 - I just simply need this fix ASAP

I am not sure uploadsConfig and unit test are the best way to do it. Hope there are some suggestions from the team. Anyway, I've squashed and rebased to master.

My production branch also needs this fix ASAP. It's quite troublesome to use forked local branch currently when deploying this on lambda.

[amwill04]

@charleswong28 it is troublesome. MY work around was the following:

• updatepacakge.json in fork to:

// Basically rewrite the dependencies
{
  "name": "apollo-server-lambda",
  "description": "Production-ready Node.js GraphQL server for AWS Lambda",
  "keywords": [
    "GraphQL",
    "Apollo",
    "Server",
    "Lambda",
    "Javascript"
  ],
  "author": {
    "name": "[email protected]"
  },
  "bugs": {
    "url": "https://github.com/apollographql/apollo-server/issues"
  },
  "bundleDependencies": false,
  "dependencies": {
    "@apollographql/graphql-playground-html": "^1.6.0",
    "apollo-server-core": "^2.1.0",
    "apollo-server-env": "^2.0.3",
    "graphql-tools": "^3.0.4"
  },
  "deprecated": false,
  "description": "Production-ready Node.js GraphQL server for AWS Lambda",
  "devDependencies": {
    "apollo-server-integration-testsuite": "^2.1.0"
  },
  "engines": {
    "node": ">=6"
  },
  "gitHead": "86dd95e223460f8e1768141a8cefa58bcd409b03",
  "homepage": "https://github.com/apollographql/apollo-server#readme",
  "keywords": [
    "GraphQL",
    "Apollo",
    "Server",
    "Lambda",
    "Javascript"
  ],
  "license": "MIT",
  "main": "dist/index.js",
  "name": "apollo-server-lambda",
  "peerDependencies": {
    "graphql": "^0.12.0 || ^0.13.0 || ^14.0.0"
  },
  "repository": {
    "type": "git",
    "url": "https://github.com/apollographql/apollo-server/tree/master/packages/apollo-server-lambda"
  },
  "scripts": {
    "clean": "rm -rf dist",
    "compile": "tsc",
    "prepare": "npm run clean && npm run compile"
  },
  "types": "dist/index.d.ts",
  "version": "2.1.0"
}

• yarn prepare
• tar.gz apollo-server-lambda and move to my project folder.
• edit project pacakge.json

"dependencies": {
    "apollo-server-lambda": "file:./apollo-server-lambda.tar.gz",
}

[ariariel]

Any updates on this PR being merged?

[k-komarov]

Any updates?

[kaamoss]

Any updates on this? Like everyone else here I have a few projects that could benefit a lot from this.

[NHebrard]

Like everyone else, any updates on this PR being merged?

[abernix]

Thanks for opening this PR!

It looks like this has adopted many of the practices Apollo uses on non-"handler" based implementations (that is to say, those that instead use applyMiddleware), which is the correct inspiration to work off. That said, I think we also need to hold it up to another createHandler implementation (the first for Apollo Server which uses uploads) to make sure it shares as many common patterns as possible, and particularly making sure to guard against the possibility of processFileUploads being undefined (which was introduced in #2054).

I think the first step now will be rebasing this current master, taking care to implement new functionality which was introduced in #2054 to support graphql-upload@8 (this PR is currently using graphql-upload@5). I think this portion of that changeset gives a good indication, but it's worth looking at the totality of the change and making sure things align.

When that is done, I think we can get this into a release.

[charleswong28]

@jalie @bluedusk I think we need to create an example repository for this but I am not available today. Will try it this weekend.

Thanks mate @charleswong28 , I could use some time to build a example repo. Any idea how could I install your version of apollo-server-lambda with npm rather than copy your code to my project ?

That would be very helpful. Sorry that I still couldn't squeeze time out for this yet.
The comment from amwill04 would also work by compressing the repository but the package.json has typo when copying and pasting. You will see when you copy it. The idea is basically build it and then use "apollo-server-lambda": "file:./apollo-server-lambda.tar.gz", to refer to it.

Another way would be cloning it next to your example repository and use "apollo-server-lambda": "file:./apollo-server/packages/apollo-server-lambda", to refer to it. This will be easier when you change code inside apollo-server-lambda repository.

[bluedusk]

Create a demo with your PR Code:
A apollo server fileupload demo using apollo-server-lambda:
https://github.com/bluedusk/apollo-server-lambda-fileupload

[charleswong28]

Create a demo with your PR Code:
A apollo server fileupload demo using apollo-server-lambda:
https://github.com/bluedusk/apollo-server-lambda-fileupload

It shows internal server error due to service timeout and "src" is missing in your postman.json. I've created a PR in your repository.
bluedusk/apollo-server-lambda-fileupload#1

In my branch, I am able to see the

[ Promise {
    { filename: 'package.json',
    mimetype: 'application/json',
    encoding: '7bit',
    createReadStream: [Function: createReadStream] } } ]

in uploadFiles resolver files param.

Hope this could help!

[JorgeCeja]

Any updates on when this PR could be merged to master?

[bluedusk]

Hey guys, Anyone from the team following this PR ?

[eliliam]

Is this PR going to get merged or has it fallen stale?

[intellix]

this was replaced with https://github.com/jaydenseric/graphql-upload I suppose

[eliliam]

I'm all down for using graphql-upload for now, but don't you think it would be better to merge this into master for sake of continuity between implementations?

[abernix]

I've rebased this off the recent master and taken care of the merge conflicts to bring it up to date, but I need to ask that this code be re-structured into a more natural and intuitive work-flow.

I've left some notes below, but as an example, kicking off the chain with a function which returns a Promise called fileUploadProcess (not to be confused with the existing processFileUploads!) doesn't seem like the right ordering of concern. File uploads are an optional feature, so this is counter intuitive for someone reading the code.

Also, the critical promiseWillStart is now buried inside a then-able, rather than being the very first thing that happens (which it should be).

I'm pretty sure the implementation you've offered here would work as is, but in terms of maintainability, we need to consider another approach. I'll help how I can, but I hope you can understand my hesitation here.

[abernix]

The promiseWillStart must be awaited prior to entering the fileUploadProcess. We can't wait until after processing the file upload to fire this callback.

[abernix]

The code here would seem to work, but this is a confusing arrangement of logic. The fileUploadProcess shouldn't be the thing that kicks off the chain, it should be an optional chain within the path of request handling.

To put it another way, let's maintain that graphqlLambda is the thing that gets kicked off, and the callback passed to graphqlLambda be the part of the code that brings the upload processing, rather than having graphqlLambdatriggered withinfileUploadProcess` — that's not an intuitive place for it.

[abernix]

This name is too close to processFileUploads. This might be better named as a processRequest, but that's not even quite right. I realize that this code left some to be desired before, but I'm afraid that this makes it quite difficult to understand. There are several other frameworks to use as a basis, but this is pretty far from all of them right now.

[jasonlimantoro]

Any updates for this PR?
Just for anyone who stumbles around this issue, I have tested that this PR works with apollo-upload-client package

[mrfoster]

I would love to see this get finished off, it's so close yet such slow progress..

I have had a real headache getting this working:

1. Ran npm i in root folder followed by npm link in the apollo-server-lambda folder. This resulted in a weird error trying to rename a non existent folder (.staging). After a bit of searching, I overcame it by removing the package.lock file. This time node ran out of memory and after increasing the limit to 8000mb, I discovered that it was just endlessly looping around dependencies. I am using the latest LTS node and npm and have no idea how to overcome it so gave up.

2. I copied the folder into my repo and got it working after switching a couple of imports to import * as.

3. I then discovered that CORS was broken but, after a bit of digging around, I applied the following commit and CORS was fixed. 7e78a23#diff-16d4d9e1275e5b29399d54c22f79316f

4. I re-enabled the upload config and got the following error This implementation of ApolloServer does not support file uploads because the environment cannot accept multi-part forms. This was a little easier to track down and was fixed by adding

  supportsUploads(): boolean {
    return true;
  }

5. It now appears to work but the files seem to be corrupt. I have checked that multipart/form-data has been added to BinaryMediaTypes so I'm a little lost but will hopefully track it down today and post back.

Hopefully this will help someone if they are desperate enough to use it now.

[mrfoster]

Turned out that I was handling base64 encoded files elsewhere in my code. I can confirm that it is now working by applying steps 3 and 4 from #1739 (comment)

[zakirbaytar]

Any updates on this PR?

[mikemeerschaert]

What's left on this preventing a merge? I need to add a feature to handle file uploads in my project very soon and I'd like to do it the officially supported way so it doesn't have to be changed later. Are there specific action items that need to be addressed before this gets merged?

[aymericbouzy]

@charleswong28 I see the last commit was on Feb 14th 2019 : are you still planning to work on this PR? is it ok if I open a new PR with commits to address the requested changes by @abernix?

[jurienhamaker]

Would love to take over this PR if @charleswong28 is not planning on fixing it further.

[charleswong28]

Would love to take over this PR if @charleswong28 is not planning on fixing it further.

@jurienhamaker @aymericbouzy Sorry for not having enough time to work on this. It would be great if you guys could help.

[aymericbouzy]

@charleswong28 no worries, we totally understand 😄
@jurienhamaker you seem highly motivated, please proceed 😊 I'm new to the repo, but if you want any help, please ask!

[k00k]

I've created a new PR #3676 that uses some of @charleswong28's work from this PR and attempts to incorporate the comments by @abernix on this PR. All checks have passed, so hoping to get it merged pretty quickly as we actually need to use it :)

Feel free to chime in over there.

[abernix]

Superseded by #3926, which has merged into release-2.13.0. An alpha release has been published on the alpha npm tag which includes #3926, and can be installed with npm install [email protected]. Please try it out and report any issues on a new issue which references #3926.