jaydenseric / graphql-multipart-request-spec

A spec for GraphQL multipart form requests (file uploads).
1.01k stars 56 forks source link

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

Closed stenin-nikita closed 6 years ago

stenin-nikita commented 6 years ago

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 commented 6 years ago

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:

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 commented 6 years ago

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

gohelkiran30 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

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.