Open dylanowen opened 3 years ago
I'd like to add that the spec currently defines that the graphql query itself as well as the mapping for the files are json encoded. Since the GraphQL spec does explicitly not define an encoding, this spec shouldn't either. An example would be that a server uses CBOR instead of json for the "normal" GraphQL queries but then has to decide if it should require the mapping for the files to be cbor too or json.
This is not great and would cause different behaviour across implementations where one expects json and the other the same encoding as everywhere else.
Also why not expand the spec for responses? There are probably some cases where a external cdn is too much but just putting the stuff directly in a field in the response isn't great either.
@Erik1000 regarding https://github.com/jaydenseric/graphql-multipart-request-spec/issues/55#issuecomment-911800652
the [GraphQL multipart request] spec currently defines that the graphql query itself as well as the mapping for the files are json encoded. Since the GraphQL spec does explicitly not define an encoding, this spec shouldn't either.
The official GraphQL Foundation GraphQL over HTTP spec (still a draft) does in fact require servers and clients to support JSON:
Servers and clients over HTTP MUST support JSON and MAY support other, additional serialization formats. — https://github.com/graphql/graphql-over-http/blob/main/spec/GraphQLOverHTTP.md#serialization-format
The point of the GraphQL multipart request spec is to allow all sorts of clients to send file uploads to all sorts of GraphQL APIs; if we have different versions of a spec for serialization formats other than JSON then it undermines this goal. JSON is by far the easiest to work with in browser code, where the size of your code and dependencies really matters for performance. I think it's safe to assume almost all server environments have the means to process JSON, but JSON is one of the only serialization formats clients can elegantly work with (without introducing third party library bloat).
Regarding https://github.com/jaydenseric/graphql-multipart-request-spec/issues/55#issuecomment-913834280
Also why not expand the spec for responses?
This was one of the first things considered years ago during early experimentation, but I haven't met anyone that wants or needs this yet. If it's something you want to experiment with, keep in mind that there is a GraphQL over HTTP RFC for incremental delivery that specifies multipart/mixed
GraphQL responses. It doesn't interfere with multipart GraphQL requests because it's for responses:
https://github.com/graphql/graphql-over-http/pull/124#issuecomment-658260163
But if you plan to experiment with multipart GraphQL responses for file downloads within the response, it might need to be accounted for as a user might want both file downloads as well as an incremental delivery?
The official GraphQL Foundation GraphQL over HTTP spec (still a draft) does in fact require servers and clients to support JSON:
I see, didn't know about this spec. Why not write it like the spec itself? E.g. json MUST be supported but a server MAY also accept everything serialized in something else (like CBOR).
As for incremental delivery, I'll look into that, seems interesting!
I'd like to reopen the discussion on @enjoylife's suggested simplification of the spec #50 . I believe their suggestion can greatly simplify this specification and ease the implementation for typesafe frameworks while still achieving the original goals for JS GraphQL libraries.
Just for clarity, this is a rough draft of how I imagine the new version of the spec could look building off of what @enjoylife has defined:
ex:
I've implemented a prototype of this in my own Scala server and I have a very rough implementation of this for Apollo:
Prototype Apollo Attachments Gist
Regarding the comments in #11 (describing how the
map
field is necessary for performance), this is an implementation detail of the server and something we can solve for Apollo. If our Apollo plugin finds anUpload(file_id)
we grab a promise from our sharedUploads
object which we will resolve once we parse our file or reject after we finish parsing the entire request. This lets us execute our GraphQL request as soon as we find it in our form fields.This is a trace from running my gist:
You can see that we've achieved the same async waterfall where our GraphQL request execution starts immediately.
Yes, each file is always referenced by its
uid
so your server can choose to arrange its json however it desires without any issues.An added benefit of this proposal over the current spec is the ability to define file references outside of variables. Right now you're required to always have a
"variables"
section to reference via yourmap
form-field. It's not possible to send something like:This doesn't really change between the current spec and this proposal. You're always looking up in your context for the file based on its uid. There's no reason you can't repeatedly query the same file based on its uid.
ex:
Although this is true of the new spec change, we'll always be parsing GraphQL requests in GraphQL servers, it's a matter of leveraging the server libraries to facilitate this. This is something that could maybe be handled by an Apollo
validationRule
or definitely by an Apollo plugin. We're writing a spec for GraphQL, we should be using the tools our GraphQL servers provide to us.Even if we're writing an implementation of this spec for a framework that gives 0 options to validate our GraphQL request, the current JS spec implementation has already defined code that would catch maxFiles as they were streaming through via
Busboy
: https://github.com/jaydenseric/graphql-upload/blob/2ee7685bd990260ee0981378496a8a5b90347fff/public/processRequest.js#L67Exactly, this spec appears to be designed in order to run as a JS Server middleware. There is a good amount of indirection, implementation specific solutions, and dependencies on the implementing language/framework. This all creates more work for server implementers.
I did an audit of the various server implementations and all of the ones I looked at either depend on:
Object
type and casting (ignoring type safety)There doesn't seem to be a good way to add this specification to a typesafe language/framework without it devolving into the proposed spec change.
For Example
async-graphql
in Rusthttps://github.com/async-graphql/async-graphql/blob/f62843cbd34ef9bf28f70f8df07d4f61f8038e0a/src/request.rs#L115
we can see that internally, after the
map
is parsed, we replace thenull
inside the variables definition with auid
to reference the specific file.https://github.com/async-graphql/async-graphql/blob/f62843cbd34ef9bf28f70f8df07d4f61f8038e0a/src/types/upload.rs#L99
When we get the value out of the Scalar, we pull the actual file stream out of the context via that same
uid
.For Example
caliban
in Scalahttps://github.com/ghostdogpr/caliban/blob/660beeae538768d817a73cb4535e0e3bd1a8cb82/adapters/play/src/main/scala/caliban/uploads/Upload.scala#L89
We're setting our variable value to the filename in order to pull it out of the context later.
For Example
sangria
in Scala(This isn't actually in the library but this gist describes how to implement it).
https://gist.github.com/dashared/474dc77beb67e00ed9da82ec653a6b05#file-graphqlaction-scala-L54
we store our uploaded file separately from our GraphQL request.
https://gist.github.com/dashared/474dc77beb67e00ed9da82ec653a6b05#file-controller-scala-L68
we pass our file through via the context.
https://gist.github.com/dashared/474dc77beb67e00ed9da82ec653a6b05#file-exampleschema-scala-L15
Inside of our resolver we lookup the file in the context to actually use it.
All of these examples have implemented @enjoylife's proposal under the covers in order to preserve some form of type safety.
Note:
We can use these libraries as a guide to show us how to implement supporting both the current version of the spec and the proposed change in the same server with plenty of code sharing.
For Reference
graphql-upload
in JavaScriptThis JavaScript implementation depends on the language being dynamic so that we can overwrite our variables with an
Upload
instance.https://github.com/jaydenseric/graphql-upload/blob/60f428bafd85b93bc36524d1893aa39501c50da1/public/processRequest.js#L232
We assign the
Upload
instance to the specified location in our GraphQL Json Requesthttps://github.com/jaydenseric/graphql-upload/blob/60f428bafd85b93bc36524d1893aa39501c50da1/public/GraphQLUpload.js#L81
When parsing our Scalar value, we check and cast to make sure we found an
Upload
instance.For Reference
graphql-java-servlet
in JavaThis Java implementation depends on the top level
Object
type so that we can check and cast our variables on the fly.https://github.com/graphql-java-kickstart/graphql-java-servlet/blob/eb4dfdb5c0198adc1b4d4466c3b4ea4a77def5d1/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/GraphQLMultipartInvocationInputParser.java#L138
We set each
http.Part
in our variable maphttps://github.com/graphql-java-kickstart/graphql-java-servlet/blob/eb4dfdb5c0198adc1b4d4466c3b4ea4a77def5d1/graphql-java-servlet/src/main/java/graphql/kickstart/servlet/apollo/ApolloScalars.java#L28
When parsing our Scalar, we check and cast to make sure we found a
http.Part
instance.I can't speak for @enjoylife but I don't believe the proposed changes to this spec are implying the code for
graphql-upload
is heavy.graphql-upload
is quite elegant in its implementation. In fact, for my Apollo prototype I borrowed heavily fromgraphql-upload
. The heavy parts are that:In Summary
"The point of this spec is to create a standard for interoperability between all GraphQL clients and servers, regardless of the languages or ecosystems" and the current iteration of this specification constrains non-dynamic languages in order to be written inside of a JS Server Middleware. Evolving this specification will better fit the growing GraphQL ecosystem and make this specification future proof so that everybody can benefit from the work you've done here.