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

Implement Upload spec #139

Open
aaahrens opened this issue Oct 18, 2021 · 3 comments
Open

Implement Upload spec #139

aaahrens opened this issue Oct 18, 2021 · 3 comments
Labels
enhancement New feature or request help wanted Issues that anyone could pick up and implement if useful to them needs design Issues where we don't know exactly what we need yet; discuss on the issue before implementing
Milestone

Comments

@aaahrens
Copy link

It would be really nice to implement the graph multipart spec in order to be able to test it.

@aaahrens aaahrens added the enhancement New feature or request label Oct 18, 2021
@StevenACoffman
Copy link
Member

I think this was implemented in the gqlgen test client in 99designs/gqlgen#1661 and shouldn't be too difficult to accomplish here.

@benjaminjkraft benjaminjkraft added help wanted Issues that anyone could pick up and implement if useful to them needs design Issues where we don't know exactly what we need yet; discuss on the issue before implementing labels Oct 21, 2021
@benjaminjkraft
Copy link
Collaborator

Neat idea! We'll need to figure out what the API should look like: is a file just an io.Reader or is it some custom type? (Can you rebind it to be os.File or fs.File to avoid having to loop through a []os.File to make it an []io.Reader?) Do we need any other metadata beyond that specified in the query? In general I'm having a little trouble parsing out exactly what is necessary or would make sense as an API, but if someone more familiar can do that I think it would make great sense to add!

@Sonna
Copy link

Sonna commented Feb 23, 2022

The gqlgen file upload example README breaks down what the expected Request payload should look like and how to construct it using curl.

The WithFiles() option for he gqlgen test client 99designs/gqlgen#1661 is a simple helper option to not only transform the GraphQL request into a 'multipart/form-data' Content Type that should have been supported previously, but it also loops though the provided variables and looks for os.File types to then transform them into the appropriate part within the 'multipart/form-data', with the help of http.DetectContentType() from the temporary in-memory file (so it should be able handle images, plain text files or binary files all the same). It also relies on the File type to help bind the name of the file to name of the variable within the GraphQL request in the 'Content-Disposition' for each file part.

Then there is some extra stuff to dedupe file uploads in the variables & request, so a duplicate file can be referenced multiple times within the variables, but exist once in the 'multipart/form-data' of the request.

The WithFiles() option is mostly a refactor of what was already there within the example/fileupload/fileupload_test.go, but slightly retooled for use outside its own tests and relies on the os.File from core Go to fill in the blanks when building the 'multipart/form-data' body of the request rather than a custom file struct type.

could it use io.Reader [instead]?

Yes, but only for the content of the File. Some other custom type would be needed to let the developer fill in the blanks or you set them to be unique/random assuming they do not care about the filename within the body of the request.

Can you rebind it to be os.File or fs.File to avoid having to loop through a []os.File to make it an []io.Reader?

Then you may end up dropping support for an array of files in your request variables; e.g. https://github.com/99designs/gqlgen/blob/master/_examples/fileupload/readme.md#file-list

{
  query: `
    mutation($files: [Upload!]!) {
      multipleUpload(files: $files) {
        id
      }
    }
  `,
  variables: {
    files: [
      File, // b.txt
      File  // c.txt
    ]
  }
}

It depends on how you construct your request and provide variables. If the developer provide the whole 'multipart/form-data' body of the request up front you can avoid os.File, but it puts onus on them rather than the test client to build it out.

An io.Reader might be possible where it does read in the file;

b, _ := ioutil.ReadFile(fileData[0].file.Name()) // Bravo file content.

https://github.com/99designs/gqlgen/pull/1661/files#diff-c89143397afbd44160aa66cf858dff15d64c0f0dfba958cf1450c97912980df9R111

Which is used to fill out this example part of the 'multipart/form-data' body of the request

--------------------------d7aca2a93c3655e0
Content-Disposition: form-data; name="0"; filename="b.txt"
Content-Type: text/plain

Bravo file content.

The Content-Type could be determine from the read in content of io.Reader value, but filename="?" would likely need to be made up. This would be the filling in of the blanks I mentioned earlier.

Also, name="?" references the key in the map used to reference files within the GraphQL request variables; e.g.

--------------------------d7aca2a93c3655e0
Content-Disposition: form-data; name="map"

{ "0": ["variables.files.0"], "1": ["variables.files.1"] }

A 'multipart/form-data' GraphQL request puts the query in the name="operations" part and the "file" variables in the name="map" part. The single file upload is a simple example of this, but it can also be done without files and just query & variables or just query when there is nothing to map

{
  query: `
    query($name: String!) {
      echo(name: $name)
    }
  `,
  variables: {
      name: "hello world",
  }
}
--------------------------e6b2b29561e71173
Content-Disposition: form-data; name="operations"

{ "query": "query { echo }", "variables": { "name": "hello world" } }

Do we need any other metadata beyond that specified in the query?

No not really. Filling the blanks for filename="?" should be easy enough. You would just need to construct the map between the files variables and the 'multipart/form-data' GraphQL request body.

That map can be a bit tricky, since it should accommodate for the following:

  1. Single variable
  2. Array variable
  3. Array of Graph type variables (e.g. type Foo { name: String! file: File! } & mutation($input: [Foo!]!))
  4. Duplicate files; same file in multiple variables
// --b7955bd2e1d17b67ac157b9e9ddb6238888caefc6f3541920a1debad284d
// Content-Disposition: form-data; name="map"
//
// `{ "0":["variables.input.file"] }`
// or
// `{ "0":["variables.input.files.0"], "1":["variables.input.files.1"] }`
// or
// `{ "0": ["variables.input.0.file"], "1": ["variables.input.1.file"] }`
// or
// `{ "0": ["variables.req.0.file", "variables.req.1.file"] }`

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Issues that anyone could pick up and implement if useful to them needs design Issues where we don't know exactly what we need yet; discuss on the issue before implementing
Projects
None yet
Development

No branches or pull requests

4 participants