refactor: Update graphql-yoga to 4.X.X #8718
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| import corsMiddleware from 'cors'; | ||
| import { createYoga, renderGraphiQL } from 'graphql-yoga'; | ||
| import { createFetch } from '@whatwg-node/fetch'; | ||
| import { execute, subscribe } from 'graphql'; | ||
| import { SubscriptionServer } from 'subscriptions-transport-ws'; | ||
| import { handleParseErrors, handleParseHeaders, handleParseSession } from '../middlewares'; | ||
|
|
@@ -31,6 +32,11 @@ class ParseGraphQLServer { | |
|
|
||
| async _getGraphQLOptions() { | ||
| try { | ||
| const formDataLimits = { | ||
| fileSize: this._transformMaxUploadSizeToBytes( | ||
| this.parseServer.config.maxUploadSize || '20mb' | ||
| ), | ||
| }; | ||
| return { | ||
| schema: await this.parseGraphQLSchema.load(), | ||
| context: ({ req: { info, config, auth } }) => ({ | ||
|
|
@@ -39,11 +45,20 @@ class ParseGraphQLServer { | |
| auth, | ||
| }), | ||
| maskedErrors: false, | ||
| // 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: [ | ||
|
There was a problem hiding this comment. This is the recommended way of configuring FormData limits; There was a problem hiding this comment. We're testing it here; There was a problem hiding this comment. 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. There was a problem hiding this comment. 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. There was a problem hiding this comment. 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 :) |
||
| { | ||
| onRequestParse: ({ request }) => { | ||
| request.options.formDataLimits = formDataLimits; | ||
| }, | ||
| }, | ||
| ], | ||
| fetchApi: createFetch({ | ||
| useNodeFetch: true, | ||
| formDataLimits, | ||
| }), | ||
| }; | ||
| } catch (e) { | ||
| this.log.error(e.stack || (typeof e.toString === 'function' && e.toString()) || e); | ||
|
|
@@ -58,7 +73,7 @@ class ParseGraphQLServer { | |
| return this._server; | ||
| } | ||
| const options = await this._getGraphQLOptions(); | ||
| this._server = createYoga(options); | ||
| return this._server; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,10 +5,9 @@ import * as defaultGraphQLTypes from './defaultGraphQLTypes'; | |
| import logger from '../../logger'; | ||
|
|
||
| const handleUpload = async (upload, config) => { | ||
| const data = await upload.buffer(); | ||
|
There was a problem hiding this comment. I am not sure if you should rely on an implementation specific let chunks = [];
for await (const chunk of upload.stream()) {
if (chunk) { chunks.push(chunk); }
}
const buffer = Buffer.concat(chunks);There was a problem hiding this comment. 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. There was a problem hiding this comment. I understand but There was a problem hiding this 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.
Here a example in our test suite |
||
| const fileName = upload.name; | ||
| const type = upload.type; | ||
| if (!data || !data.length) { | ||
| throw new Parse.Error(Parse.Error.FILE_SAVE_ERROR, 'Invalid file upload.'); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -82,6 +82,7 @@ const load = function (parseGraphQLSchema, parseClass, parseClassConfig: ?ParseG | |
| const parseFields = await transformTypes('create', fields, { | ||
| className, | ||
| parseGraphQLSchema, | ||
| originalFields: args.fields || {}, | ||
| req: { config, auth, info }, | ||
| }); | ||
|
|
||
|
|
@@ -190,6 +191,7 @@ const load = function (parseGraphQLSchema, parseClass, parseClassConfig: ?ParseG | |
| const parseFields = await transformTypes('update', fields, { | ||
| className, | ||
| parseGraphQLSchema, | ||
| originalFields: args.fields || {}, | ||
| req: { config, auth, info }, | ||
| }); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,6 +38,7 @@ const load = parseGraphQLSchema => { | |
| const parseFields = await transformTypes('create', fields, { | ||
| className: '_User', | ||
| parseGraphQLSchema, | ||
| originalFields: args.fields || {}, | ||
| req: { config, auth, info }, | ||
| }); | ||
|
|
||
|
|
@@ -114,6 +115,7 @@ const load = parseGraphQLSchema => { | |
| const parseFields = await transformTypes('create', fields, { | ||
| className: '_User', | ||
| parseGraphQLSchema, | ||
| originalFields: args.fields || {}, | ||
| req: { config, auth, info }, | ||
| }); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| import Parse from 'parse/node'; | ||
| import { GraphQLError } from 'graphql'; | ||
|
|
||
| export function enforceMasterKeyAccess(auth) { | ||
| if (!auth.isMaster) { | ||
|
|
@@ -16,7 +16,7 @@ export function toGraphQLError(error) { | |
| code = Parse.Error.INTERNAL_SERVER_ERROR; | ||
| message = 'Internal server error'; | ||
| } | ||
| return new GraphQLError(message, { extensions: { code } }); | ||
| } | ||
|
|
||
| export const extractKeysAndInclude = selectedFields => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,11 +3,12 @@ import { fromGlobalId } from 'graphql-relay'; | |
| import { handleUpload } from '../loaders/filesMutations'; | ||
| import * as defaultGraphQLTypes from '../loaders/defaultGraphQLTypes'; | ||
| import * as objectsMutations from '../helpers/objectsMutations'; | ||
| import deepcopy from 'deepcopy'; | ||
|
|
||
| const transformTypes = async ( | ||
| inputType: 'create' | 'update', | ||
| fields, | ||
| { className, parseGraphQLSchema, req, originalFields } | ||
| ) => { | ||
| const { | ||
| classGraphQLCreateType, | ||
|
|
@@ -44,7 +45,9 @@ const transformTypes = async ( | |
| fields[field] = transformers.polygon(fields[field]); | ||
| break; | ||
| case inputTypeField.type === defaultGraphQLTypes.FILE_INPUT: | ||
| // fields are a deepcopy, but we can't deepcopy a stream so | ||
| // we use the original fields from the graphql request | ||
| fields[field] = await transformers.file(originalFields[field], req); | ||
| break; | ||
| case parseClass.fields[field].type === 'Relation': | ||
| fields[field] = await transformers.relation( | ||
|
|
@@ -152,9 +155,10 @@ const transformers = { | |
| nestedObjectsToAdd = ( | ||
| await Promise.all( | ||
| value.createAndAdd.map(async input => { | ||
| const parseFields = await transformTypes('create', deepcopy(input), { | ||
| className: targetClass, | ||
| parseGraphQLSchema, | ||
| originalFields: input || {}, | ||
| req: { config, auth, info }, | ||
| }); | ||
| return objectsMutations.createObject(targetClass, parseFields, config, auth, info); | ||
|
|
@@ -213,9 +217,10 @@ const transformers = { | |
|
|
||
| let nestedObjectToAdd; | ||
| if (value.createAndLink) { | ||
| const parseFields = await transformTypes('create', deepcopy(value.createAndLink), { | ||
| className: targetClass, | ||
| parseGraphQLSchema, | ||
| originalFields: value.createAndLink || {}, | ||
| req: { config, auth, info }, | ||
| }); | ||
| nestedObjectToAdd = await objectsMutations.createObject( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an internal property of our implementation. It is not part of Fetch spec. I'm not sure if you should test that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@whatwg-node/fetchimplements Fetch API right? But Fetch API doesn't haveBody.options. So that means it is an internal field which is NOT intended to be used by user.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.