Uploading files. Change variables map #1081
Conversation
|
@vandyshev: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/ |
Sources/Apollo/RequestCreator.swift
Outdated
| map["\(index)"] = ["variables.\(file.fieldName).\(index)"] | ||
| var map = [String: [String: String]]() | ||
| files.enumerated().forEach { (index, file) in | ||
| var field = map[file.fieldName, default: [:]] |
There was a problem hiding this comment.
One thing that keeps throwing me off when I read this is field.count, partly because field is singular so I don't expect it to have a count. Would recommend renaming it something like fieldsForFieldName for clarity
Sources/Apollo/RequestCreator.swift
Outdated
| } | ||
|
|
||
| let mapData = try serializationFormat.serialize(value: map) | ||
| let mapValue = Dictionary(uniqueKeysWithValues: map.flatMap { $0.1 }.map { ($0.0, [$0.1]) }) |
There was a problem hiding this comment.
This is impressively short, but it's awfully hard to tell what's going on here. Can you please give some of these variables names so it's easier to tell what's going on here?
|
@vandyshev Have you had a chance to look at the questions I asked here? |
|
Change variables mapping for fix bug with two files uploading. Adding tests for FileUpload based on android version and spec |
|
Hey @vandyshev - I appreciate the effort you've put in here, but this changeset has gotten enormous, and it's harder to tell what you've changed to fix the issue you ran into vs. what you've changed to work with the second framework you've added. Is there a way to back this up to where it's just the changes to the existing code rather than this whole changeset? |
|
@designatednerd If you look closely, only Sources/Apollo/RequestCreator.swift has changed. A new target is needed to generate API based on the prepared scheme. Without running a server. |
|
I mostly see that, but it'd be helpful to see what code in the tests is being changed because it's wrong vs what code in the tests is being changed because it's looking at a different lib. Honestly if we're going to do another test lib for the uploading stuff, I'd rather set up an upload server and do it that way rather than just duplicating our existing library and messing around with it. |
Changed tests in Tests/ApolloTests/RequestCreatorTests.swift. What to do next it's up to you. But currently uploading files is sad |
|
I agree the upload situation isn't ideal. IMHO, the upload spec is somewhat bolted on to GraphQL as a concept since GraphQL itself doesn't really have any notion of files. We've started advising people to use other upload methods for anything beyond proof of concept, because we've found that to be a much, much better long term solution. That doesn't mean I don't want this to be working better - it does mean that I want to be really careful about what we do wind up merging into our support. Merging huge pull requests when smaller ones will still fix the issue is against that policy. I understand that you don't feel like this is a huge change, but I respectfully disagree: It's adding yet another framework when we may or may not have to, and we definitely don't have to in order to get the current solution working better. The framework as added may also not be the correct solution: A framework that more specifically targets uploads (and doesn't have the rest of the Star Wars stuff in it) is probably going to be easier to deal with in the long term. If you'd like to continue with this PR, please trim it back down to just the code and test fixes, and remove the new framework. If you'd prefer not to, I understand. |
vandyshev
force-pushed
the
bugfix/uploading-map-variables
branch
from
510088f
to
0a00507
Compare
|
Thank you very much for making that change. It looks like the test changes for |
|
@vandyshev Do you mind if I take over this PR to get it across the finish line? |
|
Hi! I've opened #1279 to address this. One thing to note is that if you're uploading an array of files with the same name, you do still need the |
Fix files uploading with different fieldName's
Changed map in Request payload based on https://github.com/jaydenseric/graphql-multipart-request-spec#multipart-form-field-structure
from:
{"0":["variables.avatar.0"],"1":["variables.background.1"]}to:
{"0":["variables.avatar"],"1":["variables.background"]}