fix: GraphQL uploads fail with HEADER_ALREADY_SEND and/or Invalid JSON

[FransGH]

Pull Request

• Report security issues confidentially.
• Any contribution is under this license.
• Link this pull request to an issue.

Issue

Closes: #8497

Approach

Change mount order of GraphQL and main server

Tasks

[parse-github-assistant]

Thanks for opening this pull request!

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01% ⚠️

Comparison is base (c7fa3b9) 94.12% compared to head (320235a) 94.11%.
Report is 1 commits behind head on release-5.x.x.

❗ Current head 320235a differs from pull request most recent head 5a468c5. Consider uploading reports for the commit 5a468c5 to get more accurate results

Additional details and impacted files

@@                Coverage Diff                @@
##           release-5.x.x    #8720      +/-   ##
=================================================
- Coverage          94.12%   94.11%   -0.01%     
=================================================
  Files                183      183              
  Lines              13803    13803              
=================================================
- Hits               12992    12991       -1     
- Misses               811      812       +1     

Files Changed	Coverage Δ
src/ParseServer.js	90.37% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mtrezza]

Is this an issue that only exists in 5.x or also in current 6.x?

[FransGH]

I can't tell for sure as I haven't yet used 6.x but likely, yes.

[Moumouls]

thanks @FransGH , since it's a bug we need to reproduce the fail in a test before merge. Can you add a test ?

[FransGH]

thanks @FransGH , since it's a bug we need to reproduce the fail in a test before merge. Can you add a test ?

I guess the most critical tests are all existing tests that make sure that changing the mounting order does not affect any other server functionality?

Are there any existing graphql test? Sounds like a basic graphql upload test might just be missing?

[Moumouls]

We have graphql upload tests in parse server test suits. I also use Parse Server in production in my company with many graphql upload usage and we don't have this issue, this is why we should try to reproduce with a failing test.

On you setup moving the app.use fix the issue ? Do you use some serverless env ? It happen just after the startup, or if you try to upload a file with graphql on a cold start instance ? @FransGH

[FransGH]

Just looked at the graphql upload test. It does a http-post with json as payload instead of using the Apollo client that is used with other test. The difference is that Apollo sends a multipart mime which leads to an invalid response from parse-server.

I'll work on adding an additional Apollo upload test but might take some time (have a few other things to complete in the next days..)

[Moumouls]

The difference is that Apollo sends a multipart mime which leads to an invalid response from parse-server.

File upload in graphql is based on multipart, the http post that you see in the tests is actually what appolo graphql upload client plugin do in a browser. If i remember correctly it's not possible to use a upload link of apollo in NodeJS.

I worked recently also on an graphql upload node js client following the official spec: https://github.com/jaydenseric/graphql-multipart-request-spec

And it works correctly on parse-server @FransGH

Technically apollo upload client will not work in node js because it need some API polyfill of browser env. This way in node JS we need to compose the multipart request with form-data package.

I also use Apollo multi part upload system in my company front ends with parse server and it work perfectly :)

[FransGH]

@Moumouls I believe you. Still, there seems to be an issue that in some setups the HEADER_ALREADY_SEND error occurs.

I've been working on a test to reproduce and stumbled over the issue that apollo-upload-client is indeed not compatible with nodejs. It actually throws the "upload.arrayBuffer is not a function" error so that might explain #8497 (seems that the submitter was testing with a nodejs client, not a browser).

I've applied your upload-fix changes too but that seems to be unrelated?

Will spend a bit more time on this but then just have to leave it "as-is" as it's basically working fine now in my setup after changing the mount order.

[Moumouls]

The array buffer issue @FransGH is related to a PR I sent few hours ago. In newer version of parse server, nodejs, grapqhl yoga the upload is broken. I sent a fix PR with all working tests.

The array buffer issue is identified

[FransGH]

Ok. just fyi. I'm testing on the 'release' branch and included the changes from your branch "Moumouls:moumouls/fix-graphql-upload". Existing GraphQL upload test worked fine before and after applying your fix.

In a new test that tried to use the apollo.upload.client I got "upload.arrayBuffer is not a function". It sounds like this and what you have identified are different issues. In my test this is caused by nodejs not supporting arrayBuffer. The "HEADER_ALREADY_SEND / invalid json" is also a different issue.

[Moumouls]

@mtrezza more than one year, and i never encountered this issue with apollo client, and compatible server side mutli part protocol, can we close this ?

[mtrezza]

@Moumouls, yes I think we can close this. If someone can confirm the issue exists for the current release of Parse Server then it would be better to open a new issue if it's unrelated to #8497.