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

Maybe you should remove the "map" key? #11

Closed
stenin-nikita opened this issue May 10, 2018 · 6 comments
Closed

Maybe you should remove the "map" key? #11

stenin-nikita opened this issue May 10, 2018 · 6 comments
Labels

Comments

@stenin-nikita
Copy link

I think that the "map" in the request is unnecessary. In my opinion it's much easier to do this:

cURL request

curl localhost:3001/graphql \
  -F operations='{ "query": "mutation ($file: Upload!) { singleUpload(file: $file) { id } }", "variables": { "file": "0" } }' \
  -F 0=@a.txt

Request payload

--------------------------cec8e8123c05ba25
Content-Disposition: form-data; name="operations"

{ "query": "mutation ($file: Upload!) { singleUpload(file: $file) { id } }", "variables": { "file": "0" } }
--------------------------cec8e8123c05ba25
Content-Disposition: form-data; name="0"; filename="a.txt"
Content-Type: text/plain

Alpha file content.

--------------------------cec8e8123c05ba25--

Thus, it is easier to organize server-side support:

Example for webonyx/graphql-php

class UploadType extends ScalarType
{
    public $name = 'Upload';

    public function parseValue($value)
    {
        return $_FILES[$value] ?? null;
    }
}

Example for nodejs

import { GraphQLScalarType } from 'graphql'
import requestContext from from 'global-request-context'

export const GraphQLUpload = new GraphQLScalarType({
  name: 'Upload',
  parseValue: value => {
    const { req } = requestContext
    return req.files[value] || null
  }
})

What do you think about this?

@jaydenseric
Copy link
Owner

jaydenseric commented May 11, 2018

In the early evolution of this spec/implementations there was no map field; see https://github.com/jaydenseric/graphql-multipart-request-spec/tree/v1.0.0#multipart-form-field-structure.

Looking at your suggestion, the first observation is that you would lose the ability to manage the file upload streams in resolvers. Middleware would block the resolvers until all the files have be received and temporarily stored somewhere, either in memory or as temp files to populate (in your example) req.files. This would mean you would not be able to get the nice second "Async" waterfall described in this diagram:

Sync vs async GraphQL multipart request middleware

This would mean:

  • Slower response time.
  • More use of system resources.
  • No ability to validate and abort uploads in resolvers.

In theory, if the scalars were able to reliably read the request context and they were able to return a promise, you might be able to remove the map field. I don't think that is really doable though. The suggested global-request-context solution is pretty dubious and requires special setup. Even if scalars had a context argument, people don't consistently attach the request object to GraphQL context and some don't at all.

@jaydenseric
Copy link
Owner

This question has been answered, but feel free to continue the conversation 🙏

@gohelkiran30
Copy link

I am getting error using above curl command

curl localhost:3007/graphql \
  -F operations='{ "query": "mutation ($file: Upload!) { testUpload(file: $file) { id } }", "variables": { "file": "0" } }' \
  -F 0=@a.txt

{"errors":[{"message":"GraphQL Request must include at least one of those two parameters: "query" or "queryId"","category":"request"}]}

I am using webonyx/graphql-php library

@jaydenseric
Copy link
Owner

@gohelkiran30 that request is invalid, as it is missing the map field. Regardless, it would be an issue to take up with your chosen implementation, not the spec itself.

@gohelkiran30
Copy link

@jaydenseric I have tried it with map field also and the issue is same. I don't think the issue is with the implementation as its build on the graphql specifications.
I am using webonyx/graphql-php and Ecodev/graphql-upload library

@jaydenseric
Copy link
Owner

I don't think the issue is with the implementation as its build on the graphql specifications.

That sentence is hard to interpret. You mean to say that you are confident that the GraphQL server faithfully implements this GraphQL multipart request spec?

I can't help you problem solve implementations, you should create issues in the relevant repos if you think they have a bug.

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

No branches or pull requests

3 participants