jaydenseric
commented
2 years ago Some side notes about the actual implementation: It's great you guys are going for as few dependencies as possible; I'm not salty that you're not using graphql-upload! But I do worry about the implications of such a simple approach, if my shallow understanding of it is correct.
Is it the first sync scenario explained in this graphic?
It appears you are awaiting the entire multipart request before running any resolvers:
If so, there are a few implications…
- It is much slower, because the file upload streams can't be processed and streamed back out to cloud storage as they are coming in.
- If the app is accepting largish files that means that the memory of the server needs to be able to expand to hold all of the uploads, for all requests being processed simultaneously. If your server has a 2GB RAM limit, and that's how big a file is, a single request could crash the server.
- It means the entire request has to be uploaded before resolvers have a chance to validate the input. What if a user attempts to register an account with a 50mb avatar in the input, but their desired username is taken? Instead of the resolver almost instantly being able to respond with the relevant GraphQL errors about the username, the entire avatar has to be uploaded first just for it to be discarded anyway. This is nuanced because even though we have the ability with
graphql-uploadto respond early before the request is finished, we currently choose not to because it can trigger browser bugs (see https://github.com/jaydenseric/graphql-upload/blob/0cbc9d90a181ace59be279fee4c3b269c47d1cbe/public/graphqlUploadKoa.js#L56). - Developers in resolvers won't be able to inspect or transform the file upload streams as they are coming in, e.g. to determine the true file type based on the file contents stream or to enforce file size limits.
Also, not something problematic with the general strategy but an opportunity for improvement is error handling for when the GraphQL multipart request has weird/invalid form fields. In the early days before we had really robust checks in graphql-upload for everything that could possible be wrong it was possible to bring down Apollo Server just with a maliciously structured request. Hopefully you are already testing for and handling such errors! You can see all the weird things we test for here:
https://github.com/jaydenseric/graphql-upload/blob/v13.0.0/test/public/processRequest.test.mjs
Aside from tightening security, it's nice to have meaningful error messages presented to the developer in the response if they get the structure of the GraphQL multipart request wrong. For example, somehow here would generate an error message if the operations field is missing:
GraphQL Yoga implements GraphQL Multipart Spec in a different way by using WHATWG Fetch API. https://github.com/dotansimha/graphql-yoga/blob/master/packages/common/src/getGraphQLParameters.ts#L50