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

Update to use grahpql-upload v8 #1854

Closed
jkosonen opened this issue Oct 24, 2018 · 4 comments
Closed

Update to use grahpql-upload v8 #1854

jkosonen opened this issue Oct 24, 2018 · 4 comments

Comments

@jkosonen
Copy link

jkosonen commented Oct 24, 2018

You can see in the graphql-upload (formerly apollo-upload-server) changelog that a lot of critical improvements have been made between v5 and v7. Some of the problems were very challenging to solve! It should be stable now and we have a lot of tests.

Everyone should migrate from apollo-upload-server v5 to graphql-upload v8+ ASAP.

apollo-upload-server should be updated to use latest graphql-upload

@ghost ghost added the ✋ blocking Prevents production or dev due to perf, bug, build error, etc.. label Oct 24, 2018
@jaydenseric
Copy link

jaydenseric commented Oct 29, 2018

This is a pretty critical issue. Not only are people missing out on a huge amount of fixes and a much more powerful API, but the public can crash Apollo Server v2 infrastructure by sending malformed multipart requests (in theory; I have not gone around trying it). People are even talking about filing CVEs!

Here is a PR that would resolve this issue: #1764.

I ❤️you all at Apollo, event if this particular issue needs work. I don't chase dramas, I'm just trying to make the world a better place and offer some transparency to the community. It's their apps that are affected, after all 🙏

I'll be in San Francisco next week for GraphQL Summit, and am happy to meet anyone that would like to talk about it in person or collaborate.

Context

Before apollo-upload-server (since renamed graphql-upload) was implemented in Apollo Server v2, unsolved challenges and their security implications were transparent to the Apollo and the public via Github issues and Apollo Slack team threads. I also messaged core Apollo team members via Slack about the challenges and gave my best estimates as to when they might be resolved. The response was that as long as they get fixed sometime soonish, not to worry.

It turns out that running resolvers before the request has finished, so that file upload streams can be handled efficiently in resolvers, is super challenging for a lot of reasons. I worked tirelessly on tests for every edge case and solid solutions to each challenge (requests aborted half way through, the same file being used as an argument for multiple mutations, files upload streams being consumed out of order, etc. etc.) with heroic contributions from @mike-marcacci who created fs-capacitor as a big part of the solution. All the issues have been resolved! Today there are 94 closed issues, and only 2 open… A question, and a request for Node.js v6 support:

screen shot 2018-10-29 at 10 46 25 pm

In the process of solving all the complicated challenges across multiple versions of Node.js (with their subtle and not-so-subtle breaking API changes to streams, etc.), Node.js support was lifted from v6 to v8.5. See jaydenseric/graphql-upload#109. I never thought this was a big deal, since absolutely no one I know in 2018 is using Node.js v6, which is only a few months away from end-of-life, when v8 LTS and v10 LTS are available and vastly superior. It turns out that Apollo really doesn't want to drop Node.js v6.

I've said that I'm willing to accept a PR adding Node.js v6 support, even though it means compromising some modern features that I've come to enjoy, such as native async/await. I've even cleared the path by dropping support back to Node.js v6 for a dev dependency.

Not everyone uses Apollo Server. express-graphql gets around 112k downloads per month while apollo-server-core gets around 150k. Personally I've used most of the GraphQL servers available on npm including Apollo Server, and currently use graphql-api-koa. graphql-upload supports most GraphQL servers. My point is, if something like Node.js v6 support is specifically important to Apollo, a for-profit company, it is the perfect opportunity to make a contribution back to the project via a PR. After all, I have contributed to Apollo repos before and have sent hundreds of messages of free support for the community on Slack; I get notifications anytime the word upload is mentioned.

I've spent months of my life dedicated to this problem space at great personal cost, with no contributions from Apollo, despite it powering a major feature of Apollo Server, one of their flagship products. All along, I have been motivated by the promise that the work will help not just myself, but a lot of other people handle uploads intuitively and efficiently in their apps. It breaks my heart every day to see a horribly broken fork (@apollographql/apollo-upload-server) of the last version that happened to support Node.js v6 (that predates fixes) being forced on a trusting community to the tune of 136k installs per week:

screen shot 2018-10-29 at 10 58 16 pm

Users keep coming to me for support about issues we already worked hard to solve months ago. This poor developer experience needlessly tarnishes both my own and Apollo's reputation and really hits morale.

Due to a lack of progress I've tried my best to reach out in the #apollo-maintainers Slack channel multiple times…

screen shot 2018-10-30 at 12 01 42 am

The problem and solution gets acknowledged…

screen shot 2018-10-30 at 12 03 18 am

Then ignored again…

screen shot 2018-10-29 at 10 18 55 pm

What Apollo Server users can do in the meantime

I have updated the apollo-upload-examples API to disable the outdated and dangerous Apollo Server upload handling and use graphql-upload instead; here is the commit.

To migrate your existing Apollo Server v2 project:

  1. npm install graphql-upload.

  2. Disable the Apollo Server upload handling by using uploads: false in the ApolloServer options.

  3. Setup the graphql-upload middleware.

  4. Setup the Upload scalar resolver.

  5. Migrate to the new API in your mutation resolvers:

    - const { stream, filename, mimetype } = await upload
    + const { createReadStream, filename, mimetype } = await upload
    + const stream = createReadStream()

    If you don't, it will still work for now but you will see this deprecation warning:

    screen shot 2018-10-29 at 10 03 43 pm

    This API change happened in v7 and was nessesary to fix a whole bunch of issues, the most obvious being that you can now use a single Upload as a variable that can be used as input for multiple mutations in one request. Previously, consuming the stream in one resolver would interfere with consuming the same stream in another.

@MANTENN
Copy link

MANTENN commented Nov 4, 2018

File returns a promise and inside an array. This is a solution for those who encounter the same issue as I do

uploadMaterialImage: async (p, a, { models, user }) => {
   const { env: { PUBLICDIR } } = process;
   const uploadDir = "." + PUBLICDIR + "/images";
     let { file: [file] } = await a;
     return file.then(
       async ({
         filename, mimetype, encoding, createReadStream
       }) => {
         const stream = createReadStream();
         const { ok, errors, payload } =  await (async () => {
             const id = shortid.generate();
             let filepath = `${uploadDir}/${id}-${filename}`;
             return new Promise((resolve, reject) => {
               stream.on('error', err => {
                 if (stream.truncated) {
                   // Delete the truncated file
                   fs.unlinkSync(filepath);
                 }
                 reject({ok: false, errors: err});
               });
   
               stream
                 .pipe(fs.createWriteStream(filepath))
                 .on('error', err => reject({ok: false, errors: err}))
                 .on('finish', () => {
                   filepath = filepath.split(".")
                   filepath.shift();
                   filepath = filepath.join(".")
                   return resolve({ok: true, payload: {
                   path: filepath
                  }})});
             });
           })();
           return {
             ok,
             errors,
             payload
           }
       }
     )
 }

@abernix
Copy link
Member

abernix commented Dec 5, 2018

This should be addressed by #2054. As I've requested in that PR, I'd really appreciate anyone who is utilizing file uploads to try upgrading to the published alpha which updates graphql-upload to v8. I've detailed the progress on this matter extensively in #2054, but the high-bit is that this should be ready to try out now in apollo-server@2.3.0-alpha.0. Please detail any problems (or successes!) you encounter with the upgrade, as the feedback will guide its final release.

Ref: #2054 (comment)

@abernix abernix closed this as completed Dec 5, 2018
@abernix
Copy link
Member

abernix commented Dec 13, 2018

The alpha releases didn't identify any problems so I've graduated this to the official apollo-server-*@2.3.0 releases.

@abernix abernix removed the ✋ blocking Prevents production or dev due to perf, bug, build error, etc.. label Sep 3, 2019
@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
None yet
Development

No branches or pull requests

4 participants