Closed prevostc closed 5 years ago
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.
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
.
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.
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.
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 '0=@a.gif;type=image/gif'
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.
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:
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 :]).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.
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.
Is there a reason why the operations and map parts of the spec do not have a content-type application/json set?
This could help frameworks the content automatically.