parse-server icon indicating copy to clipboard operation
parse-server copied to clipboard

fix: file parsing defaultGraohQLTypes.js

Open brocklj opened this issue 3 years ago • 15 comments

Problem with executing GraphQL Cloud code.

Instead of parsing value object type File object is type of Upload

New Pull Request Checklist

  • [x] I am not disclosing a vulnerability.
  • [x] I am creating this PR in reference to an issue.

Issue Description

This PR solves problems with GraphQL input value of type File. After upgrading from version 4.2.0 to 4.3.0. All migrations calling cloud code functions with inputs of type File ends with:

{ "errors": [ { "message": "Variable \"$input\" got invalid value { resolve: [function], reject: [function], promise: {}, file: { filename: \"ff.jpg\", mimetype: \"image/jpeg\", encoding: \"7bit\", createReadStream: ...Expected type \"File\". [object Object] is not a valid File

Related issue: #7684

Approach

After debbuging the code and searching for where parts throwing exception. Found validation problem in src/GraphQL/loaders/defaultGraphQLTypes.js. Parts expecting an object instance of File receives Upload. Also __Type on object is undefined. So I access type name trough constructor.name. After these changes code works as it was before upgrading to 4.3.0.

TODOs before merging

  • [ ] Add tests
  • [x] A changelog entry is created automatically using the pull request title (do not manually add a changelog entry)

brocklj avatar Nov 08 '21 10:11 brocklj

Thanks for opening this pull request!

  • ❌ Please edit your post and use the provided template when creating a new pull request. This helps everyone to understand your post better and asks for essential information to quicker review the pull request.

Proposed to fix: https://github.com/parse-community/parse-server/issues/7684

brocklj avatar Nov 08 '21 11:11 brocklj

@brocklj could you please fill out the template?

mtrezza avatar Nov 08 '21 15:11 mtrezza

@brocklj could you please fill out the template?

How long could take fix this problem into version 4.3.x?

brocklj avatar Nov 09 '21 10:11 brocklj

I removed the TODOs that don't apply to this PR, only the test is missing. Could you please add a test to make sure this bug can't occur again?

How long could take fix this problem into version 4.3.x?

The process is to first fix a bug in the alpha branch, then backport it to the 4.x branch. So the first step is to determine whether this bug also exists in alpha branch:

  • if it does, then this PR's target branch need to change to alpha, and after merge we may backport the fix into 4.x
  • if it does not, then we need to look how this bug has been fixed in alpha and either backport it the same way into 4.x or consider an individual fix for the 4.x branch.

To answer your question exactly, we won't port this into a 4.3.x release (based on the 4.3 version), but we may port it into a 4.x release (based on the latest 4.x version). Portability depends on the required effort.

mtrezza avatar Nov 09 '21 14:11 mtrezza

if it does, then this PR's target branch need to change to alpha, and after merge we may backport the fix into 4.x

Yes, it does. Tested on release 4.10.4.

brocklj avatar Nov 09 '21 18:11 brocklj

Yes, it does. Tested on release 4.10.4.

The alpha branch is way ahead of 4.10.4. Please try with 5.0.0-alpha.5 which is the latest alpha release.

mtrezza avatar Nov 09 '21 18:11 mtrezza

Tested on 5.0.0-alpha.5. with same error result Variable \"$input\" got invalid value { resolve: [function], reject: [function], promise: {}, file: { filename: \"ff.jpg\", mimetype: \"image/jpeg\", encoding: \"7bit\", createReadStream: [function createReadStream] } } at \"input[0]\"; Expected type \"File\". [object Object] is not a valid File"

brocklj avatar Nov 09 '21 21:11 brocklj

Great, thanks for testing. I changed the target branch of this PR to alpha, so the fix will be for that branch. Could you add a test that fails without the fix and passes with the fix? This will prevent future changes from introducing the same bug again. You can just copy/paste a similar test and modify it.

mtrezza avatar Nov 09 '21 22:11 mtrezza

The tests do not pass. Either the tests need to be adapted, or the fix does not work as intended. I suggest to remove the fix for now and just add a failing test to demonstrate the issue. Then add a fix to make the new test pass.

mtrezza avatar Nov 10 '21 01:11 mtrezza

Hi @brocklj could you provide the GraphQL request that you use in your migrations ? And how you inject your files into the graphQL request ? Feel free to provide your complete request + how you call it ?

Moumouls avatar Nov 11 '21 08:11 Moumouls

Hi @brocklj could you provide the GraphQL request that you use in your migrations ? And how you inject your files into the graphQL request ? Feel free to provide your complete request + how you call it ?

While using apollo-upload-client": "^14.1.3",

`const UPLOAD_MERCHDET_IMAGE_QUERY = gql
  mutation uploadMerchdetImage($input: [File], $mch_id: Int) {
    uploadMerchdetImage(input: $input, mch_id: $mch_id) {
      img_id
      img_url
    }
  }
;


  const [uploadMechdetImage] = useMutation(UPLOAD_MERCHDET_IMAGE_QUERY, {
    onCompleted(data) {
      inputRef.current.value = null;
      setIsUploadInProgress(false);
      setMerchdetState({
        ...merchdetState,
        mch_img_id: data.uploadMerchdetImage.img_id,
        img_url: data.uploadMerchdetImage.img_url
      });
    }
  });`


    if (validity.valid && !imageIsNotValid) {
      setIsUploadInProgress(true);
      await uploadMechdetImage({
        variables: {
          input: [file],
          mch_id
        }
      });
    }
  };`

The request looks like.

operations: {"operationName":"uploadMerchdetImage","variables":{"input":null,"mch_id":881},"query":"mutation uploadMerchdetImage($input: File, $mch_id: Int) {\n  uploadMerchdetImage(input: $input, mch_id: $mch_id) {\n    img_id\n    img_url\n    __typename\n  }\n}\n"}
map: {"1":["variables.input"]}
1: (binary)`

and error message from response:

Variable "$input" got invalid value { resolve: [function], reject: [function], promise: {}, file: { filename: "1634658697301.jpg", mimetype: "image/jpeg", encoding: "7bit", createReadStream: [function createReadStream] } }; Expected type "File". [object Object] is not a valid File

brocklj avatar Nov 11 '21 12:11 brocklj

Hi @brocklj I see that the fix is super simple. Could rebase this PR on alpha and add the failing test at the bottom of the ParseGraphQLServer.spec.js

You have an example in ParseGraphQLServer.spec.js on how to set up a test with custom definitions and upload a file using multipart upload 🙂

Once it's covered, we can merge 👍

Moumouls avatar May 10 '22 12:05 Moumouls

Hi @brocklj Parse Server has switched to GraphQL Yoga with new graphql upload/file handling.

Could you confirm that the issue is still present in Parse Server last alpha version?

Also, could you provide a failing test, it will help to avoid a regression in the future (like the one between 4.2 -> 4.3)

Moumouls avatar Jun 12 '22 19:06 Moumouls

Tests are failing, there seem to be issues that need to be resolved; removed pending review. This PR is stale for some time; if there is no further activity we'll go ahead and close it.

mtrezza avatar Jul 02 '22 10:07 mtrezza