refactor: Update graphql-yoga to 4.X.X #8718

Moumouls · 2023-08-27T10:58:43Z

Pull Request

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

Issue

Closes: #8717

Approach

Follow Graphql Yoga migration guide

Tasks

Add tests
Add changes to documentation (guides, repository pages, code comments)
Add security check
Add new Parse Error codes to Parse JS SDK

parse-github-assistant · 2023-08-27T10:58:45Z

I will reformat the title to use the proper commit message syntax.

parse-github-assistant · 2023-08-27T10:58:48Z

Thanks for opening this pull request!

Moumouls · 2023-08-27T11:02:05Z

@dotansimha we will be happy to get your feedback here or someone of the team, everything seems good to me, but you will have maybe some idea of improvement to get full advantage of the v4.

@mtrezza an easy quick win :)

codecov · 2023-08-27T11:06:06Z

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (42929e0) 94.32% compared to head (3dec0e0) 94.32%.

❗ Current head 3dec0e0 differs from pull request most recent head ad477f3. Consider uploading reports for the commit ad477f3 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##            alpha    #8718   +/-   ##
=======================================
  Coverage   94.32%   94.32%           
=======================================
  Files         185      185           
  Lines       14762    14766    +4     
=======================================
+ Hits        13924    13928    +4     
  Misses        838      838

Files Changed	Coverage Δ
src/GraphQL/loaders/parseClassMutations.js	`100.00% <ø> (ø)`
src/GraphQL/loaders/usersMutations.js	`92.59% <ø> (ø)`
src/GraphQL/ParseGraphQLServer.js	`94.23% <100.00%> (+0.35%)`	⬆️
src/GraphQL/loaders/filesMutations.js	`80.64% <100.00%> (ø)`
src/GraphQL/parseGraphQLUtils.js	`92.00% <100.00%> (ø)`
src/GraphQL/transformers/mutation.js	`97.22% <100.00%> (+0.02%)`	⬆️

... and 2 files with indirect coverage changes

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

Moumouls · 2023-08-27T11:06:26Z

oh interesting @mtrezza we still support Node 14, Yoga need Node 16 at least, any chance to bump a major version of parse-server to drop node 14 on move forward with 16/18/20. 14 sounds quite old since it's 2 version before the current LTS

mtrezza · 2023-08-27T11:14:31Z

Node 14 will be dropped in alpha branch in Nov, in beta in Dez, in release in Jan with Parse Server 7.

Moumouls · 2023-08-27T13:21:30Z

okay so this one will wait November @mtrezza, useless to migrate to v3 temporarily, since this one with v4 is pretty much ready to merge

Moumouls · 2023-08-28T11:17:58Z

Hi @ardatan , @dotansimha, i migrated parse to v4. I took me sometimes because for some reasons the Boby ponyfill in our current setup seems to loose formData limits config.

I needed to add onParseRequest to enforce the formDataLimits option. If you want to investigate. You can use the "should support file" test in ParseGraphQLServer spec and removing the onParseRequest to reproduce

ardatan · 2023-08-28T11:30:40Z

spec/ParseGraphQLServer.spec.js

@@ -9450,7 +9452,8 @@ describe('ParseGraphQLServer', () => {
              body: body2,
            });
            expect(res.status).toEqual(200);
-            const result2 = JSON.parse(await res.text());
+            const resText = await res.text();


Suggested change

const resText = await res.text();

const result2= await res.json();

res.json already parses it as JSON.

yes good catch useless JSON.parse

ardatan · 2023-08-28T11:31:19Z

spec/ParseGraphQLServer.spec.js

@@ -126,8 +126,10 @@ describe('ParseGraphQLServer', () => {

    it("should return schema and context with req's info, config and auth", async () => {
      const options = await parseGraphQLServer._getGraphQLOptions();
-      expect(options.multipart).toEqual({
-        fileSize: 20971520,
+      expect(new options.fetchApi.Body().options).toEqual({


This is an internal property of our implementation. It is not part of Fetch spec. I'm not sure if you should test that.

It's covered now, it seems that you fetch system rely on this option. We need to ensure on our side that the option is correctly setup. And in case of a fail, it's just a test we will be able to update it easily :) @ardatan

@whatwg-node/fetch implements Fetch API right? But Fetch API doesn't have Body.options. So that means it is an internal field which is NOT intended to be used by user.

And in case of a fail, it's just a test we will be able to update it easily :)

After the long process in my previous PRs, I don't think it should be that easy :) So I'd not recommend to keep this test like that.

ardatan · 2023-08-28T11:32:20Z

src/GraphQL/ParseGraphQLServer.js

+        // Needed to ensure formDataLimits since it seems to not working
+        // this is a temporary fix until the issue is resolved
+        // we need to ask graphql-yoga team
+        plugins: [


This is the recommended way of configuring FormData limits;
https://the-guild.dev/graphql/yoga-server/docs/features/file-uploads#configuring-multipart-request-processing-only-for-nodejs

We're testing it here;
Test -> https://github.com/dotansimha/graphql-yoga/blob/main/packages/graphql-yoga/__integration-tests__/incremental-delivery.spec.ts#L302
Configuration -> https://github.com/dotansimha/graphql-yoga/blob/main/packages/graphql-yoga/__integration-tests__/incremental-delivery.spec.ts#L192

i tried without the plugin and just the config on the Yoga docs, but i encountered a bug. The format data options seems to be lost during the request processing so the multipart limit don't work. This is why i needed to add the plugin to enforce the option.

Like I said above, it is not part of Fetch API spec. It is VERY specific to our implementation. I'd strongly recommend to avoid this kind of testing. If something is not working as expected, you can help us to reproduce it on our repo.

I understand that the issue is quite specific, @ardatan. I'm not particularly fond of relying on this workaround myself. I invested a considerable amount of time investigating into this issue. I've managed to identify a reproducible case within the Parse Server test suite. However, I'll not be able to dedicate time to reproduce the bug on Yoga repo.

Given that Parse is designed to be a user-friendly Backend as a Service (BaaS), it's crucial to ensure the seamless functionality of all its features. Therefore, we conduct integration tests with our dependencies to guarantee the production readiness and stability of the software.

This is precisely why I reached out to the Guild for assistance. It would be greatly appreciated if you could investigate the bugs within our test suite, as I suspect they might all be linked to the 'fetch ponyfill' library also maintenained by the guild team.

If you find that investigating into Yoga-related bugs within the Parse Server test suite isn't within your scope of interest, please don't hesitate to let me know. I completely understand and respect that. Additionally, I intend to experiment with the new Apollo Server v4. Perhaps within the context of the Parse Server, we'll encounter fewer issues.

It's not an ultimatum; perhaps Yoga is well-suited for certain use cases, but in the context of Parse Server, there appear to be some issues.

I also admire all the work that the guild has done on tools like GraphQL codegen :)

ardatan · 2023-08-28T11:33:06Z

src/GraphQL/loaders/filesMutations.js

@@ -5,10 +5,9 @@ import * as defaultGraphQLTypes from './defaultGraphQLTypes';
 import logger from '../../logger';

 const handleUpload = async (upload, config) => {
-  const data = Buffer.from(await upload.arrayBuffer());
+  const data = await upload.buffer();


I am not sure if you should rely on an implementation specific buffer method which is not part of Fetch spec. You can create a Node.js buffer with the size information like Buffer.from(arrayBuffer, undefined, upload.size) (it is still weird how arrayBuffer doesn't have the correct size) or consume the stream;

let chunks = []; for await (const chunk of upload.stream()) { if (chunk) { chunks.push(chunk); } } const buffer = Buffer.concat(chunks);

using arrayBuffer i got some weird behavior and corrupted file. using buffer worked. the buffer() method seems implemented by @whatwg-node if i remember correctly during my investigation @ardatan.

I got some huge issue migrating ot Yoga V4, it didn't worked easily out of the box. I landed with the current implementation to get file upload working with multi part limits.

I understand but .buffer is not intended to be used like that. We cannot guarantee it will be always there because it is not part of Fetch spec. I'd not recommend to use that, and prefer the alternatives I mentioned in my previous comment.

HI @ardatan , i tried the arrayBuffer method. But we got actually have something strange. Some content of the request seems to leak into the file content. It do not happen with .buffer() file.
I'm not sure why.

Here a example in our test suite

Moumouls · 2023-10-29T08:19:21Z

@mtrezza i close this in favor of #8727

refactor: update graphql-yoga to 4.X.X

9fe024e

parse-github-assistant bot changed the title ~~refactor: update graphql-yoga to 4.X.X~~ refactor: Update graphql-yoga to 4.X.X Aug 27, 2023

Moumouls requested a review from a team August 27, 2023 11:02

Moumouls added 2 commits August 27, 2023 17:02

fix: add compat for other fetch polyfill

019faf4

fix: graphql upload for all versions

c22d38b

Moumouls mentioned this pull request Aug 28, 2023

bug: schema option does not accept GraphQLSchema type dotansimha/graphql-yoga#2973

Open

cleanup

ad477f3

ardatan reviewed Aug 28, 2023

View reviewed changes

Moumouls closed this Oct 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Update graphql-yoga to 4.X.X #8718

refactor: Update graphql-yoga to 4.X.X #8718

Moumouls commented Aug 27, 2023 •

edited

Loading

parse-github-assistant bot commented Aug 27, 2023

parse-github-assistant bot commented Aug 27, 2023

Moumouls commented Aug 27, 2023

codecov bot commented Aug 27, 2023 •

edited

Loading

Moumouls commented Aug 27, 2023

mtrezza commented Aug 27, 2023

Moumouls commented Aug 27, 2023

Moumouls commented Aug 28, 2023

ardatan Aug 28, 2023

Moumouls Aug 28, 2023

ardatan Aug 28, 2023

Moumouls Aug 28, 2023

ardatan Aug 28, 2023

ardatan Aug 28, 2023

ardatan Aug 28, 2023

Moumouls Aug 28, 2023

ardatan Aug 28, 2023

Moumouls Aug 29, 2023

ardatan Aug 28, 2023 •

edited

Loading

Moumouls Aug 28, 2023

ardatan Aug 28, 2023 •

edited

Loading

Moumouls Aug 29, 2023

Moumouls commented Oct 29, 2023

	const resText = await res.text();
	const result2= await res.json();

refactor: Update graphql-yoga to 4.X.X #8718

refactor: Update graphql-yoga to 4.X.X #8718

Conversation

Moumouls commented Aug 27, 2023 • edited Loading

Pull Request

Issue

Approach

Tasks

parse-github-assistant bot commented Aug 27, 2023

parse-github-assistant bot commented Aug 27, 2023

Thanks for opening this pull request!

Moumouls commented Aug 27, 2023

codecov bot commented Aug 27, 2023 • edited Loading

Codecov Report

Moumouls commented Aug 27, 2023

mtrezza commented Aug 27, 2023

Moumouls commented Aug 27, 2023

Moumouls commented Aug 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ardatan Aug 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ardatan Aug 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Moumouls commented Oct 29, 2023

Moumouls commented Aug 27, 2023 •

edited

Loading

codecov bot commented Aug 27, 2023 •

edited

Loading

ardatan Aug 28, 2023 •

edited

Loading

ardatan Aug 28, 2023 •

edited

Loading