Operations and map content type ? about graphql-multipart-request-spec HOT 8 CLOSED

Interesting idea, I'm not sure sure exactly what the distinction is, if there is any, between file and regular multipart form fields. I've only ever seen file fields specify Content-Type.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Type#Content-Type_in_HTML_forms

I think multipart form parsers like busboy might start treating the fields as files, so it would be a spec breaking change?

For the extra bytes in the request I'm not exactly sure what the benefits would be, since you can not assume that the content is well formed JSON anyway when parsing the form.

from graphql-multipart-request-spec.

I ran into an issue with spring framework not automatically parsing the content of the map and operations fields because the default content-type is text/plain (which, after checking, is in the spec: https://www.ietf.org/rfc/rfc2388.txt).

When I send the proper content-type: application/json with the parts, spring properly parses everything correctly.

It seems that files are identified by either the content-type: application/octet-stream or some file specific attributes like filename.

from graphql-multipart-request-spec.

Frameworks automatically parsing the fields as JSON is a convenience; the field strings can be parsed as JSON manually. In fact, it might be better to not rely on the automatic JSON parsing so you can respond with meaningful errors to the client like this:

https://github.com/jaydenseric/graphql-upload/blob/v8.0.4/src/processRequest.mjs#L163

That said, if it is more "correct" to declare the field content types, I would be in favour of updating the spec to enforce or encourage this.

Would you like to draft a PR that updates relevant wording and all the cURL request and response payload examples to your proposal?

Then we can try it out with existing implementations and see how disruptive it is, decide if it is worth going ahead, and workout what semver the new spec version should be.

from graphql-multipart-request-spec.

I don't think this is going to be viable, as FormData.append() only lets you set a content type for File or Blob fields:

https://stackoverflow.com/a/24535293/1596978

FormData is the primary way clients compose multipart requests, via the fetch API.

from graphql-multipart-request-spec.

I agree that the JS api of FormData is a bit weird but the http content is not that different with or without a content type.

I was able to adapt the curl request to add proper content types:

curl --trace-ascii /dev/stdout localhost:3000/graphql \
  -F 'operations={ "query": "mutation ($file: Upload!) { singleUpload(file: $file) { id } }", "variables": { "file": null } };type=application/json' \
  -F 'map={ "0": ["variables.file"] };type=application/json' \
  -F '[email protected];type=image/gif'

from graphql-multipart-request-spec.

Because of the awkwardness with the FormData API, I don't think we'll mandate the operations and map field content types in the spec.

For now, we are not explicitly forbidding them either. I'm still not sure if setting them would interfere with common multipart request parsing libs such as busboy – if someone is interested in testing it out and sharing the results we could then decide if it's something to recommend or forbid.

Everyone feel free to continue the discussion here.

from graphql-multipart-request-spec.

I don't know busboy that well but It seems like there is no interference when content-type is set. (Demo: https://codesandbox.io/s/5xj2woq0rp). Let me know if you want another lib demo or some other format.

It's true that the FormData API is awkward when you want to set the content type, but I still think that the content-type of these fields should be set:

• I think it's safe to assume that FormData is more often than not used through a lib and not directly (I might be utterly wrong but I wouldn't want to use it directly myself :]).
• Setting the content type does not seem to have an impact on current implementations (needs more testing) and will benefit new implementations (at least spring).
• It's the purpose of this content-type header to tell us what type of data is in the form field and if it's not set, the spec assumes text/plain, which is not right for operations and map fields.

However, I get that you cannot make the content-type header mandatory overnight. Maybe make it optional at first. Once all implementations are updated (server and client), make it mandatory. I don't know how many implementations there is but I might be able to contribute the change to all of them.

from graphql-multipart-request-spec.

I fully agree with @prevostc on this.

Only reason that is needed is this:

It's the purpose of this content-type header to tell us what type of data is in the form field and if it's not set, the spec assumes text/plain, which is not right for operations and map fields.

I disagree with this:

it might be better to not rely on the automatic JSON parsing so you can respond with meaningful errors to the client

Because you could say that about all Content-Type headers, yet they are used everywhere and make everything much easier.

from graphql-multipart-request-spec.