meabed / graphql-upload-ts

Middleware and an Upload scalar to add support for GraphQL multipart requests (file uploads via queries and mutations) to various Node.js GraphQL servers.
MIT License
44 stars 2 forks source link

Throw error not working #78

Closed jinyang1994 closed 8 months ago

jinyang1994 commented 1 year ago

When an exception is thrown, there is no way to return to the client

import { ApolloError } from 'apollo-server-core'

const resolvers: Resolvers = {
  Upload: GraphQLUpload,
  Mutation: {
    async upload(_, { file }) {
      throw new ApolloError('error') // not working
    }
  }
}

export default resolvers

image

client version

server version

meabed commented 1 year ago

Thank you @jinyang1994 would you be able to open a PR with the proposed fix?

jinyang1994 commented 1 year ago

Thank you @jinyang1994 would you be able to open a PR with the proposed fix?

Sorry I don't know how to fix it

meabed commented 1 year ago

Got it, sure I will have a look

jinyang1994 commented 1 year ago

Thanks

meabed commented 1 year ago

@jinyang1994 Would you be able to make codesanbox with the code to test it?

Thank you

piotr-pawlowski commented 1 year ago

It's a problem with graphql-upload-minimal lib. Throwing error doesn't work there.

meabed commented 1 year ago

@piotr-pawlowski thank you - does it work in this implementation?

piotr-pawlowski commented 1 year ago

@meabed unfortunately your implementation doens't work as well. Throwing error doesn't work - request hangs forever.

meabed commented 1 year ago

Got it, thank you @piotr-pawlowski

Would you be able to share code sandbox with the issue so I could fix.

jantoine1 commented 11 months ago

This is easily reproducible using the official graphql-upload-examples repository as a starting point: https://github.com/jaydenseric/apollo-upload-examples. Uploading a file larger than 10MB outputs an error to the terminal.

Here is a fork with the minimal necessary changes to reproduce the issue: https://github.com/jantoine1/apollo-upload-examples. Uploading a file larger than 10MB hangs.

I've also created a cod esandbox here (note that I've never used code sandbox before): https://codesandbox.io/p/github/jantoine1/apollo-upload-examples/master?workspaceId=5a39add2-0200-4244-877c-f9b17c6d7b7e&file=%2F.editorconfig

meabed commented 11 months ago

Thank you @jantoine1 i will look into it and fix it. Appreciate the effort

meabed commented 9 months ago

Thank you @jantoine1

I have added Apollo example here : https://github.com/meabed/graphql-upload-ts/tree/develop/examples/apollo

Could you give a try and let me know if it works or if you have an issue how to reproduce it with this example?

I have tested with files larger than 20mb and it works fine.

{
  "data": {
    "uploadFile": {
      "uri": "/Users/meabed/workspace/graphql-upload-ts/examples/apollo/dist/examples/apollo/uploaded-1705325159941-2.pdf",
      "filename": "2.pdf",
      "mimetype": "application/pdf",
      "encoding": "7bit",
      "fileSize": 26676280
    }
  }
}
image
solanamonk commented 8 months ago

I encountered this issue when trying to use graph-upload-ts and ValidationPipe in the same app, because ValidationPipe depends on throwing an exception when there's a validation error in the input.

It seems like when an exception is thrown, streams donโ€™t get closed properly. I "fixed" it in my app by forcibly closing the streams in the exceptionFactory function.

Iโ€™m really looking forward to a proper fix.

meabed commented 8 months ago

@solanamonk could share a code sandbox of the issue?

solanamonk commented 8 months ago

@meabed I've created a repo with a sample app https://github.com/solanamonk/validation_pipe_with_graphql_upload

The app allows to upload plain text files and increments the upload counter. If a file of another type is uploaded, it should respond with 400 Bad Request. It only works with my awkward custom pipe. If you use ValidationPipe in main.ts instead, it hangs. ValidationPipe throws an exception here

Here are some one-liners to interact with the API

curl -X POST 'http://localhost:3000/graphql' -H 'Content-Type: multipart/form-data' -H 'x-apollo-operation-name: incrementUploadCounter' -F 'operations={"query":"mutation ($file: Upload!) { incrementUploadCounter(incrementUploadCounter: { file: $file }) { count } }", "variables": { "file": null }}' -F 'map={"0":["variables.file"]}' -F '0=@/path/to/image.jpeg'
curl -X POST -H "Content-Type: application/json" --data '{ "query": "{ uploadCounter { count } }" }' http://localhost:3000/graphql
moroale93 commented 8 months ago

I'm having the same issue on a production app.

meabed commented 8 months ago

Thank goodness @solanamonk I will check this and see the fix

meabed commented 8 months ago

This is NestJS issue....

@solanamonk could you try this option


  app.use(graphqlUploadExpress({
    overrideSendResponse: false
  }));
meabed commented 8 months ago

@moroale93 can you try the same - if its still exist please send a codesandbox

moroale93 commented 8 months ago

@meabed already tried that ๐Ÿ˜žit doesn't solve the issue.

solanamonk commented 8 months ago

@meabed Thank you! That option worked for my cause! :heart:

Could you please open an issue for Nest.js and provide them with details?

moroale93 commented 8 months ago

@meabed here https://github.com/moroale93/upload-gql I've uploaded an example to see the problem. Here there are 2 packages. The GQL server and the frontend app. On readme you'll find the commands to start both. on frontend app you can select an image file and then press the button to send it. you'll see that the request will stay pending even if the GQL resolver immediatly throws an error.

moroale93 commented 8 months ago

didn't have time to test it @meabed the code is to just give you an idea of the thing ... later this evening I'll give it a check. I'm sorry

meabed commented 8 months ago

@moroale93 I have tried the code it has some bugs - could you fix it and test it to reproduce the problem and let me know

moroale93 commented 8 months ago

@meabed it is still pending also while sending a success response. Uploaded a new version. If you select a file from the app it sends it as multipart so your middleware gets executed. In that case, the resolver returns an object and should make the mutation work, but it's not. if you don't select a file, it's sent as JSON and in that case it all work.

moroale93 commented 8 months ago

I think it's something inside the processRequest function. Because if I mock that with the following function, it's successfully handled.

processRequest: () =>
            Promise.resolve({
              operationName: 'CreateUser',
              variables: {
                input: {
                  field: 'hello',
                  file: {
                    resolve: () => null,
                    reject: () => null,
                    promise: Promise.resolve({
                      fieldName: '1',
                      filename: 'scorecard example.png',
                      mimetype: 'image/png',
                      encoding: '7bit',
                      createReadStream: () => null,
                    }),
                    file: {
                      fieldName: '1',
                      filename: 'scorecard example.png',
                      mimetype: 'image/png',
                      encoding: '7bit',
                      createReadStream: () => null,
                    },
                  },
                },
              },
              query:
                'mutation CreateUser($input: CreateMeInput!) {\n' +
                '  createMe(input: $input) {\n' +
                '    field\n' +
                '    __typename\n' +
                '  }\n' +
                '}',
            }),
moroale93 commented 8 months ago

@meabed is it possible that the response object gets rewritten and so the res.send has no effect? or in case, are some headers uncorrectly set?

meabed commented 8 months ago

@moroale93 the client you have sending application/json on file upload because apollo client doesnt support it - https://www.apollographql.com/docs/react/data/file-uploads/

you would most likely need to try to use this package apollo-upload-client

To get file upload working correctly - you need to have a http client which can send multipart/form-data request correctly which file is detected on the paylaod, usually the browser handle this natively unless the client forces specific content type.

An example of this here: https://github.com/meabed/gqlts/blob/master/runtime/src/fetcher.ts#L59

I have grqphql client with application/json header and when i see files in the request and remove the headers so the browser encodes the request in multipart/form-data natively.

moroale93 commented 8 months ago

you would most likely need to try to use this package apollo-upload-client

I am already using it.

I have grqphql client with application/json header and when i see files in the request and remove the headers so the browser encodes the request in multipart/form-data natively.

And that's exactly how it works on my example.

@meabed

meabed commented 8 months ago

@moroale93 when I tried the code you have there it was sending json, could you make it send multipart form so I could look at it

moroale93 commented 8 months ago

it is sending multipart if you select a file frontend, pull it again ๐Ÿ˜Š @meabed

PS: the graphqlUploadExpress.js file shows an example of usage, but that's not the offical way to use graphql with express.

  const express = require('express');
  const graphqlHTTP = require('express-graphql');
  const { graphqlUploadExpress } = require('graphql-upload-minimal');
  const schema = require('./schema');

  express()
    .use(
      '/graphql',
      graphqlUploadExpress({ maxFileSize: 10000000, maxFiles: 10 }),
      graphqlHTTP({ schema })
    )
    .listen(3000);
meabed commented 8 months ago

@moroale93 i selected a file through the frontend and it was not sending multipart request

Can u reproduce with bash curl as I have in the examples

meabed commented 8 months ago

@moroale93 I dis small changes and tested with curl upload and it works fire โœ…

image image image
#!/bin/bash

# upload.sh
# Usage: ./upload.sh http://localhost:4000/graphql test.png
# Usage: ./upload.sh http://localhost:4000/graphql test.jpg

curl -v -L $1 \
  -H 'x-apollo-operation-name: CreateUser' \
  -H 'Apollo-Require-Preflight: true' \
  -F operations='{ "query": "mutation ($file: Upload!) { createMe(file: $file) { field  } }", "variables": { "file": null } }' \
  -F map='{ "0": ["variables.file"] }' \
  -F 0=@$2

There is something wrong in the frontend client it doesnt send multipart form upload

solanamonk commented 8 months ago

@meabed I'm afraid you might have closed it too early, it turns out it hangs on two binary uploads.

I added change to my repository to reproduce https://github.com/solanamonk/validation_pipe_with_graphql_upload

2 text uploads, ok:

curl -X POST 'http://localhost:3000/graphql' -H 'Content-Type: multipart/form-data' -H 'x-apollo-operation-name: incrementUploadCounter' -F 'operations={"query":"mutation ($file: Upload, $file2: Upload) { incrementUploadCounter(incrementUploadCounter: { file: $file, file2: $file2 }) { count } }", "variables": { "file": null, "file2": null }}' -F 'map={"0":["variables.file"],"1":["variables.file2"]}' -F '0=@/path/to/file.txt' -F '1=@/path/to/file2.txt'

1 binary upload, ok (responds with a validation error):

curl -X POST 'http://localhost:3000/graphql' -H 'Content-Type: multipart/form-data' -H 'x-apollo-operation-name: incrementUploadCounter' -F 'operations={"query":"mutation ($file: Upload) { incrementUploadCounter(incrementUploadCounter: { file: $file }) { count } }", "variables": { "file": null }}' -F 'map={"0":["variables.file"]}' -F '0=@/path/to/file.jpeg'

2 binary uploads, hangs:

curl -X POST 'http://localhost:3000/graphql' -H 'Content-Type: multipart/form-data' -H 'x-apollo-operation-name: incrementUploadCounter' -F 'operations={"query":"mutation ($file: Upload, $file2: Upload) { incrementUploadCounter(incrementUploadCounter: { file: $file, file2: $file2 }) { count } }", "variables": { "file": null, "file2": null }}' -F 'map={"0":["variables.file"],"1":["variables.file2"]}' -F '0=@/path/to/file.jpeg' -F '1=@/path/to/file2.jpeg'
codeholic commented 8 months ago

Oh, wait. It seems it doesn't have anything to do whether the file is binary or not, but with the size of files. I just took two JPEG files of 125 bytes, and it worked.

meabed commented 8 months ago

I will have a look, but I tried with large file too and it works fine.

meabed commented 8 months ago

@codeholic can u make repo with the exact error and file size? So I could have a look

meabed commented 8 months ago

@solanamonk i will check this example

meabed commented 8 months ago

@solanamonk could you organise the repo in way to reproduce the issue directly please

solanamonk commented 8 months ago

@meabed it is right here https://github.com/solanamonk/validation_pipe_with_graphql_upload

npm install
npm run start:dev

then run

curl -X POST 'http://localhost:3000/graphql' -H 'Content-Type: multipart/form-data' -H 'x-apollo-operation-name: incrementUploadCounter' -F 'operations={"query":"mutation ($file: Upload, $file2: Upload) { incrementUploadCounter(incrementUploadCounter: { file: $file, file2: $file2 }) { count } }", "variables": { "file": null, "file2": null }}' -F 'map={"0":["variables.file"],"1":["variables.file2"]}' -F '0=@2814.png' -F '1=@6127.png'
meabed commented 8 months ago

@solanamonk I got the issue - and working to fix it.

meabed commented 8 months ago

@solanamonk could you try the best this version 2.1.1-beta.42 - I have made changes and tested and looks good ๐Ÿ‘

meabed commented 8 months ago

@moroale93 could you try the best this version 2.1.1-beta.42

solanamonk commented 8 months ago

@meabed Thank you for a prompt fix, that worked like a charm for me! :heart: Would you mind to explain shortly what the issue was so I can learn something today? :smile:

meabed commented 8 months ago

The upload file stream was hanging or stuck because its multipart upload ( first part or chunk ) was not consumed with stream.on('data'=>{}) - so it won't progress to upload second chunk of the upload!

So the stream had to be reworked a bit to allow all the multipart upload to be sent and then pipeline new streams to graphql resolver.

I will release this fixes shortly today.

meabed commented 8 months ago

Fixes has been released in 2.1.1

https://github.com/meabed/graphql-upload-ts/releases/tag/v2.1.1