search

jaydenseric / graphql-upload

Middleware and an Upload scalar to add support for GraphQL multipart requests (file uploads via queries and mutations) to various Node.js GraphQL servers.
https://npm.im/graphql-upload
MIT License
1.43k stars 131 forks source link

Support Hapi #239

Closed Pajn closed 2 years ago

Pajn commented 3 years ago

Hi!

As you may know Hapi has built in parsing of multipart requests and there are currently some issues with combining graphql-upload with that. If you want more background, check out this PR against apollo-server-hapi https://github.com/apollographql/apollo-server/pull/4943

The nicest way to integrate Hapi would be to let Hapi parse the request and provide an API from graphql-upload to handle that case, which you also describe in this old comment https://github.com/jaydenseric/graphql-upload/issues/5#issuecomment-309706215

In this comment https://github.com/apollographql/apollo-server/pull/4943#issuecomment-796738593 in the above PR you can see how I do this with Hapi, but with a lot of copy and paste from processRequest. Are you interested to further explore an API that could be exported for Hapi users as well used internally by processRequest to avoid this duplication?

jaydenseric commented 3 years ago

Hello, it's always exciting to see more people working on these problems :)

Before a new integration can be added to the graphql-upload project, we really need to figure out a way to support the async scenario:

Sync vs async GraphQL multipart request middleware

It's important that uploads are processed by resolvers (e.g. forwarded out to cloud storage) as they are streaming in, instead of storing them all up on the server and then processing the result after, for 3 main reasons:

  1. It significantly reduces the time the operation takes for the waiting user.
  2. It reduces the memory or filesystem storage requirements for the server, especially when multiple users are uploading at the same time.
  3. Because the resolvers run as soon as the request starts streaming in (well, after the first operations and map fields have been quickly parsed) it allows the uploads to be cancelled early if something is wrong with the mutation. For example, imagine a mutation that registers a new user with username and profile picture fields. What if that username is already taken? Ideally the user doesn't have to wait for Hapi to finish receiving the entire 20MB profile picture to get a GraphQL error response regarding the username. There are nuances to discuss about sending responses before the request has finished due to browser bugs, but it's a long term design goal for this project and the GraphQL multipart request spec.

The nicest way to integrate Hapi would be to let Hapi parse the request and provide an API from graphql-upload to handle that case

Which scenario (sync vs async) would describe your proposed solution?

Are you interested to further explore an API that could be exported for Hapi users as well used internally by processRequest to avoid this duplication?

I am happy to assist, and eventually accept a high quality integration, but I don't use Hapi myself so I won't be able to help much in the ideation process, and reviewing a PR would probably be a significant time investment from my side. If you could commit to helping to maintain the integration over time it would be a big help.

Pajn commented 3 years ago

Thank you for your answer!

Interesting about the sync and async case. Hapi does have (and I use) and option to stream the files but I get a list of them immediately so when thinking about it it's probably using the sync thing anyway (except for possibly the last file). I'll investigate this further. Now I understand some questions I previously had about the code :)

I absolutely understand your side, please do not feel pressured engage in this. I have solved "my" problem and is enjoying graphql-upload both now with Hapi and, in earlier projects with Express so I'm already grateful, thank you! My first plan is introducing a layer to make external integration easier so that it could be summed up in a Medium or SO post but maybe not provide a fully supported integration. Unfortunately I can not commit to maintaining one at this point as any Node development is up for question in an effort to improve my own mental health. I'll only submit a PR if think it would not be a burden on the project.

Pajn commented 3 years ago

I've now read up more on the async scenario and can confirm that this does not work with Hapi parsing the request. In fact they explicitly say that if you need that you should disable parsing and use a streaming parser. After doing this, processRequest works as is by using the raw request/response objects (like how apollo-server-hapi is already using graphql-upload https://github.com/apollographql/apollo-server/blob/a3282a2d7df0c20d9e10b058defae835120fa5b1/packages/apollo-server-hapi/src/ApolloServer.ts#L18 ).

Unfortunately this also disables parsing of non multipart request and apollo-server-hapi would need to handle JSON parsing themselves for normal requests, which it currently does not support.

If a high quality integration with hapi was added to graphql-upload that parsing would have to be done in graphql-upload which I guess might be out of scope as that would require graphql-upload to also have an opinion on the format of non multipart requests. Without that parsing though I see little reason to use an integration instead of directly using processRequest when handling normal request parsing outside of graphql-upload. Do you think an integration should do parsing of all requests or should we clsoe this issue and instead continue integration work in apollo-server-hapi?

jaydenseric commented 2 years ago

Thanks for the investigation work so far :)

graphql-upload is intended to be a focused tool that various other packages can use as a dependency to implement support for GraphQL multipart requests. To that end we export the function processRequest.

The Koa and Express middleware were also exported because they were low hanging fruit; I understand both of those frameworks well, they are all most people need, and they also serve as examples for how the function processRequest can be used.

Really the burden is on the plethora of frameworks that exist to decide to support GraphQL multipart requests and for their community experts to figure out how that can be implemented on their end. Maybe they can easily plug in processRequest, or maybe they need to do a lot of rejigging of their framework. I'm not the best person to lead those efforts, although I'm happy to answer any questions about how the software I've written here works or is to be used.

If someone is able to figure out an elegantly simple Hapi integration and put together a high-quality PR, we can consider adding it here if it is easy enough for us non Hapi users to understand and maintain.

wzrdtales commented 2 years ago

this was fixed once in here https://github.com/apollographql/apollo-server/pull/3268 but never went upstream. Is it safe to say we can neglect graphql-upload if hapi is used and better just go with identified upload requests instead?