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

Uploading multiple arrays of files with length 1 fails #1522

Closed
malonehedges opened this issue Nov 17, 2020 · 9 comments
Closed

Uploading multiple arrays of files with length 1 fails #1522

malonehedges opened this issue Nov 17, 2020 · 9 comments

Comments

@malonehedges
Copy link

malonehedges commented Nov 17, 2020

I'm not sure if this PR is responsible for the change, but the v0.36 of Apollo iOS does not upload files with the index when the length of the array is 1. Hitting the same upload mutation with more than one file in each argument works.

It is currently sending something like this:
{"0":["variables.media"],"1":["variables.editedMedia"]}
when the graphql server spec is expecting
{"0":["variables.media.0"],"1":["variables.editedMedia.1"]}
Sending multiple files in the arrays sends it in the second format, but the format changes when the arrays have only 1 file in them.

@designatednerd
Copy link
Contributor

Oof, that's a fun hole in our test coverage 🙃 I'll poke at this.

@designatednerd designatednerd added bug Generally incorrect behavior uploading labels Nov 17, 2020
@designatednerd
Copy link
Contributor

So the bad news is, you're correct that it doesn't add the .0.

The good news is, with the tests I've added, it uploads successfully to a super-basic graphQL server.

Have you had active upload failures with this? If so, what are you using to process the uploads?

@designatednerd
Copy link
Contributor

(the super-basic server I'm using can be found [in this repo in the SimpleUploadServer folder if you want to run it. Requires npm.)

@designatednerd designatednerd removed the bug Generally incorrect behavior label Nov 17, 2020
@malonehedges
Copy link
Author

malonehedges commented Nov 17, 2020

Uploading to a graphene server with graphene-file-upload.

The scenario I am referencing is slightly different than your tests if I'm not mistaken though. It is uploading 2 files in their own arrays.

mutation UploadFiles(
  $fileList: [Upload!]!,
  $secondFileList: [Upload!]!
) {
  uploadFiles(
    fileList: $fileList, 
    secondFileList: $secondFileList
  ) {
    someResponseValue
  }
}

with the upload arrays with 1 item each.

More specifically, and I'm not sure if this is important, one of the arrays has optional values inside of it.

mutation UploadFiles(
  $fileList: [Upload!]!,
  $secondFileList: [Upload]!
) {
  uploadFilesSecondOptional(
    fileList: $fileList, 
    secondFileList: $secondFileList
  ) {
    someResponseValue
  }
}

I'm seeing a failure in the scenario of uploading with the second mutation listed when both arrays have 1 upload. Uploading many files in the arrays works with this setup.

@designatednerd
Copy link
Contributor

In theory optionality should not matter. I'll take a look at what I can set up, but I would also say there's almost certainly differences between the reference implementation that's in Node and the implementation you're using in Python.

@malonehedges
Copy link
Author

That makes sense that graphene-file-upload may be implemented incorrectly. I think I'll have to add a mutation to upload single files in that case.

I was trying to run the tests locally last night but was having a connection error. What target device do you use when running tests?

@designatednerd
Copy link
Contributor

designatednerd commented Nov 17, 2020

you should be able to run any target, though you do have to start the Upload server and the Star Wars server locally since those are both set up to run on localhost (there's a script in the Scripts folder for checking out the star wars server)

@malonehedges
Copy link
Author

We're going to go with forking graphene-file-upload because it appears there are other issues as well with that library. Thanks for looking into this & writing some more tests @designatednerd! Closing now

@designatednerd
Copy link
Contributor

Cool - I'll probably merge the tests I wrote to make sure we've got a safety net on that edge case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants