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

fix: file parsing defaultGraohQLTypes.js #7683

Open
wants to merge 2 commits into
base: alpha
Choose a base branch
from

Conversation

brocklj
Copy link

@brocklj brocklj commented Nov 8, 2021

Problem with executing GraphQL Cloud code.

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

New Pull Request Checklist

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
  • A changelog entry is created automatically using the pull request title (do not manually add a changelog entry)

@parse-github-assistant
Copy link

parse-github-assistant bot commented Nov 8, 2021

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.

@brocklj
Copy link
Author

brocklj commented Nov 8, 2021

Proposed to fix: #7684

@brocklj brocklj mentioned this pull request Nov 8, 2021
4 tasks
@mtrezza
Copy link
Member

mtrezza commented Nov 8, 2021

@brocklj could you please fill out the template?

@brocklj brocklj changed the title Fix: Type in Cloud Code Fix: file type parsing in defaultGraohQLTypes.js Nov 9, 2021
@brocklj brocklj force-pushed the default-graphql-types-fix branch 2 times, most recently from 837a06a to 2074ccd Compare November 9, 2021 10:08
@brocklj brocklj changed the title Fix: file type parsing in defaultGraohQLTypes.js fix: file parsing defaultGraohQLTypes.js Nov 9, 2021
@brocklj
Copy link
Author

brocklj commented Nov 9, 2021

@brocklj could you please fill out the template?

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

@mtrezza
Copy link
Member

mtrezza commented Nov 9, 2021

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.

@brocklj
Copy link
Author

brocklj commented Nov 9, 2021

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.

@mtrezza
Copy link
Member

mtrezza commented Nov 9, 2021

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.

@brocklj
Copy link
Author

brocklj commented Nov 9, 2021

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"

@mtrezza mtrezza changed the base branch from release-4.x.x to alpha November 9, 2021 22:01
@mtrezza
Copy link
Member

mtrezza commented Nov 9, 2021

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 mtrezza requested a review from Moumouls November 9, 2021 22:07
@mtrezza
Copy link
Member

mtrezza commented Nov 10, 2021

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.

@Moumouls
Copy link
Member

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 ?

@brocklj
Copy link
Author

brocklj commented Nov 11, 2021

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

@Moumouls
Copy link
Member

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
Copy link
Member

Moumouls commented Jun 12, 2022

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)

@mtrezza mtrezza removed the request for review from Moumouls July 2, 2022 10:18
@mtrezza
Copy link
Member

mtrezza commented Jul 2, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants