Mo' Better Uploading #707

designatednerd · 2019-08-12T09:04:48Z

I started trying to get some real documentation together for uploading and noticed a few holes in the implementation. This PR tries to address what I found.

In this PR:

Fixed an issue where we were setting the default Content-Type late enough that it was hammering the form data upload type which is set when there's form data.
Add an UploadingNetworkTransport sub-protocol of NetworkTransport to capture that not all network transports handle uploads.
Make HTTPNetworkTransport and SplitNetworkTransport conform to this protocol.
Add upload support to the main ApolloClient class so you don't need a direct reference to the network transport anymore. Note that this will hit an assertionFailure during development and return an Error in production if you attempt to use it with a NetworkTransport which does not also conform to UploadingNetworkTransport.
Added tests to make sure RequestCreator is properly creating multipart requests.
Actually added the documentation I came to add in the first place 🙃

designatednerd · 2019-08-12T09:06:06Z

@kdvtrifork Would love an extra set of eyes on this from you if you've got some time today

kimdv · 2019-08-12T09:08:36Z

Sure thing! I can try to test with our system :)

kimdv

I will test with our test server when I come tonight 🚀
Otherwise it look really good!

designatednerd · 2019-08-12T09:27:15Z

OK cool - I'll hold off on merging until you're able to test against your test server. Thank you for your help!

Cleversou1983 · 2019-08-12T11:16:50Z

I'do love to test it as well... Thanks a lot for your effort, Ellen!

designatednerd · 2019-08-12T12:23:19Z

@Cleversou1983 I think if you point your Cocoapods/Carthage install at this branch you should be able to. Fair warning that #699, which has already been merged, might cause you an issue or two in setup.

Cleversou1983 · 2019-08-12T12:38:18Z

Testing against this branch, still not working for me, although the content-type has changed, here is my request, if it can help...

▿ http://madari-staging.herokuapp.com/graphql
▿ url : Optional
▿ some : http://madari-staging.herokuapp.com/graphql
- _url : http://madari-staging.herokuapp.com/graphql

cachePolicy : 0
timeoutInterval : 60.0
mainDocumentURL : nil
networkServiceType : __C.NSURLRequestNetworkServiceType
allowsCellularAccess : true
▿ httpMethod : Optional
- some : "POST"
  ▿ allHTTPHeaderFields : Optional<Dictionary<String, String>>
  ▿ some : 2 elements
  ▿ 0 : 2 elements
  - key : "X-APOLLO-OPERATION-NAME"
  - value : "UploadFile"
    ▿ 1 : 2 elements
  - key : "Content-Type"
  - value : "multipart/form-data; boundary=apollo-ios.boundary.9EF8C6B7-3C06-4C98-BFA0-1DAF14475C98"
    ▿ httpBody : Optional
    ▿ some : 2611783 bytes
  - count : 2611783
    ▿ pointer : 0x0000000127e9e000
    - pointerValue : 4964605952
httpBodyStream : nil
httpShouldHandleCookies : true
httpShouldUsePipelining : false

I've still had to make and extension to my mutation input and set an typealias Upload = String

From server site we get the error:
NoMethodError (undefined method ‘each’ for “[variables.file]”=String

Looks like the 'map' parameters is a string from server side...

////
Swift code:

if let imageData = image.jpegData(compressionQuality: 0.7) {
let fileFieldName = "file"
let file = GraphQLFile(fieldName: fileFieldName, originalName: fileFieldName, mimeType: "image/jpeg", data: imageData)

        let uploadInput     = UploadFileInput(clientId: AppParameters.clientId,
                                          clientSecret: AppParameters.clientSecret,
                                          file: fileFieldName)
        
        
        let uploadMutation = UploadFileMutation(input: uploadInput)
        let transport   = HTTPNetworkTransport(url: AppParameters.APIPublicUrl)
        let upload      = transport.upload(operation: uploadMutation, files: [file]) { result in
            switch result {
            case .success(let data):
                print("Success")
                dump(data)
            case .failure(let error):
                print("Error \(error.localizedDescription)")
            }
        }

}

Maybe I'm doing something wrong? Thanks a lot Ellen

designatednerd · 2019-08-12T12:50:17Z

@Cleversou1983 Nope, good catch - I flipped the order of the parens and the quotes, and it was incorrect. Give it a shot now!

Cleversou1983 · 2019-08-12T12:58:08Z

Ellen, I've tried again with no success, my request:
▿ http://madari-staging.herokuapp.com/graphql
▿ url : Optional
▿ some : http://madari-staging.herokuapp.com/graphql
- _url : http://madari-staging.herokuapp.com/graphql

cachePolicy : 0
timeoutInterval : 60.0
mainDocumentURL : nil
networkServiceType : __C.NSURLRequestNetworkServiceType
allowsCellularAccess : true
▿ httpMethod : Optional
- some : "POST"
  ▿ allHTTPHeaderFields : Optional<Dictionary<String, String>>
  ▿ some : 2 elements
  ▿ 0 : 2 elements
  - key : "Content-Type"
  - value : "multipart/form-data; boundary=apollo-ios.boundary.8B05F7A2-E646-4065-9599-78B5B7F477A8"
    ▿ 1 : 2 elements
  - key : "X-APOLLO-OPERATION-NAME"
  - value : "UploadFile"
    ▿ httpBody : Optional
    ▿ some : 2611783 bytes
  - count : 2611783
    ▿ pointer : 0x0000000132884000
    - pointerValue : 5142757376
httpBodyStream : nil
httpShouldHandleCookies : true
httpShouldUsePipelining : false

To make sure I get te last version, I've deintegrated pod from my project and run pod install again, and clear build folder...

designatednerd · 2019-08-12T13:18:34Z

@Cleversou1983 What's the error you're getting now?

Cleversou1983 · 2019-08-13T02:07:56Z

@designatednerd Hi Ellen, sorry for delay, I'll get the log with my coworker tomorrow, thanks!

designatednerd · 2019-08-13T11:37:37Z

@kimdv @Cleversou1983 Any luck?

Cleversou1983 · 2019-08-13T12:32:39Z

@designatednerd My co-worker it's very busy and couldn't give me feedback about the log error, from the backend perspective, from the iOS perspective, look's like there's no changes, the 'map' doesn't show in the request (I think it should, right?)... Thanks!

kimdv · 2019-08-13T12:33:49Z

I'm currently looking into it.
We get a bad request from the server. Before we can handle it.

I'm looking into it with my backend developer

designatednerd · 2019-08-13T12:40:37Z

@Cleversou1983 Yeah it should - can you share what the outgoing request looks like now?

@kimdv oy. OK, ping me on Spectrum if there's anything I can help with

Cleversou1983 · 2019-08-13T13:07:26Z

@designatednerd Sure, after putting a breakpoint on line 267 of the HTTPNetwokTransport.swift file:

(lldb) po request
▿ http://madari-staging.herokuapp.com/graphql
▿ url : Optional
▿ some : http://madari-staging.herokuapp.com/graphql
- _url : http://madari-staging.herokuapp.com/graphql

cachePolicy : 0
timeoutInterval : 60.0
mainDocumentURL : nil
networkServiceType : __C.NSURLRequestNetworkServiceType
allowsCellularAccess : true
▿ httpMethod : Optional
- some : "POST"
  ▿ allHTTPHeaderFields : Optional<Dictionary<String, String>>
  ▿ some : 2 elements
  ▿ 0 : 2 elements
  - key : "X-APOLLO-OPERATION-NAME"
  - value : "UploadFile"
    ▿ 1 : 2 elements
  - key : "Content-Type"
  - value : "multipart/form-data; boundary=apollo-ios.boundary.EF87ADF0-86D6-42A8-B626-8F0E46F255B1"
    ▿ httpBody : Optional
    ▿ some : 2611783 bytes
  - count : 2611783
    ▿ pointer : 0x0000000133dbe000
    - pointerValue : 5165015040
httpBodyStream : nil
httpShouldHandleCookies : true
httpShouldUsePipelining : false

The app gets stuck waiting for a response, which never happens...

Thanks a lot!

designatednerd · 2019-08-13T13:13:50Z

@Cleversou1983 can you try using something like po String(bytes: request.httpBody!, encoding. utf8)? That should get the full request body printed as a string

Cleversou1983 · 2019-08-13T13:17:10Z

@designatednerd Sure! It's nil...
(lldb) po String(bytes: request.httpBody!, encoding:.utf8)
nil
(lldb)

Cleversou1983 · 2019-08-13T13:20:32Z

Cleversou1983 · 2019-08-13T13:43:34Z

My co-worker send me the server log:

2019-08-13T13:36:34.185478+00:00 app[web.1]: I, [2019-08-13T13:36:34.185374 #4]  INFO -- : [3271fda2-ee67-4bb2-8a0c-1761fbbadf90] Started POST "/graphql" for 170.78.31.234 at 2019-08-13 13:36:34 +0000
2019-08-13T13:36:34.186715+00:00 app[web.1]: I, [2019-08-13T13:36:34.186653 #4]  INFO -- : [3271fda2-ee67-4bb2-8a0c-1761fbbadf90] Processing by Public::GraphqlController#execute as */*
2019-08-13T13:36:34.186894+00:00 app[web.1]: I, [2019-08-13T13:36:34.186831 #4]  INFO -- : [3271fda2-ee67-4bb2-8a0c-1761fbbadf90]   Parameters: {"operations"=>"{\"operationName\":\"UploadFile\",\"query\":\"mutation UploadFile($input: UploadFileInput!) { uploadFile(input: $input) { __typename attachment { __typename id } errors } }\",\"variables\":{\"input\":{\"clientId\”:\”xxxxx\”,\”clientSecret\”:\”xxxxx\”,\”file\":\"file\"}}}", "map"=>"{\"file\":[\"variables.file\"]}", "file"=>#<ActionDispatch::Http::UploadedFile:0x00007ffa144fd858 @tempfile=#<Tempfile:/tmp/RackMultipart20190813-4-1u1jy6q>, @original_filename="file", @content_type="image/jpeg", @headers="Content-Disposition: form-data; name=\"file\"; filename=\"file\"\r\nContent-Type: image/jpeg\r\n">, "operationName"=>"UploadFile", "query"=>"mutation UploadFile($input: UploadFileInput!) { uploadFile(input: $input) { __typename attachment { __typename id } errors } }", "variables"=>{"input"=>{"clientId"=>”xxxxxx”, "clientSecret"=>”xxxxx”, "file"=>"file"}, "file"=>#<ActionDispatch::Http::UploadedFile:0x00007ffa144fd858 @tempfile=#<Tempfile:/tmp/RackMultipart20190813-4-1u1jy6q>, @original_filename="file", @content_type="image/jpeg", @headers="Content-Disposition: form-data; name=\"file\"; filename=\"file\"\r\nContent-Type: image/jpeg\r\n">}}
2019-08-13T13:36:34.194340+00:00 app[web.1]: I, [2019-08-13T13:36:34.194283 #4]  INFO -- : [3271fda2-ee67-4bb2-8a0c-1761fbbadf90] Completed 500 Internal Server Error in 7ms (ActiveRecord: 0.0ms)
2019-08-13T13:36:34.195691+00:00 app[web.1]: F, [2019-08-13T13:36:34.195632 #4] FATAL -- : [3271fda2-ee67-4bb2-8a0c-1761fbbadf90]
2019-08-13T13:36:34.195767+00:00 app[web.1]: F, [2019-08-13T13:36:34.195713 #4] FATAL -- : [3271fda2-ee67-4bb2-8a0c-1761fbbadf90] NoMethodError (undefined method `original_filename' for "file":String):
2019-08-13T13:36:34.195811+00:00 app[web.1]: F, [2019-08-13T13:36:34.195770 #4] FATAL -- : [3271fda2-ee67-4bb2-8a0c-1761fbbadf90]
2019-08-13T13:36:34.195919+00:00 app[web.1]: F, [2019-08-13T13:36:34.195844 #4] FATAL -- : [3271fda2-ee67-4bb2-8a0c-1761fbbadf90] vendor/bundle/ruby/2.5.0/bundler/gems/devmaker-rails-sdk-d77f71eeb0be/app/graphql/devmaker_rails_sdk/types/scalars/upload_type.rb:12:in `coerce_input'

Rudiney · 2019-08-13T13:55:27Z

Hey, i know these logs are hard to understand so i will try to explain my understanding about the issue here:

Looks like the file parameter of the POST request got recognised as a file but something gone wrong with the mapping to the variables. The code assumed the query variable file was the given uploaded file but got a String instead

designatednerd · 2019-08-13T14:01:58Z

@Rudiney Awesome, thanks - I think I might see what's happening here.

I think in the operationDefinition part, I need to send null for the parameter in the variables, and then that map should be readable.

How to do that is a bit more complex though...

designatednerd · 2019-08-13T14:22:05Z

@Cleversou1983 Take a look at what I just pushed up and let me know if that works. Thank you and @Rudiney for your help!

Cleversou1983 · 2019-08-13T14:37:22Z

@designatednerd We're glad to help! Unfortunately still doesn't work... From the iOS side, here is the request (same as before):

▿ http://madari-staging.herokuapp.com/graphql
▿ url : Optional
▿ some : http://madari-staging.herokuapp.com/graphql
- _url : http://madari-staging.herokuapp.com/graphql

cachePolicy : 0
timeoutInterval : 60.0
mainDocumentURL : nil
networkServiceType : __C.NSURLRequestNetworkServiceType
allowsCellularAccess : true
▿ httpMethod : Optional
- some : "POST"
  ▿ allHTTPHeaderFields : Optional<Dictionary<String, String>>
  ▿ some : 2 elements
  ▿ 0 : 2 elements
  - key : "Content-Type"
  - value : "multipart/form-data; boundary=apollo-ios.boundary.17F8C621-EF48-476C-A313-8C5F8B60A3C0"
    ▿ 1 : 2 elements
  - key : "X-APOLLO-OPERATION-NAME"
  - value : "UploadFile"
    ▿ httpBody : Optional
    ▿ some : 2611783 bytes
  - count : 2611783
    ▿ pointer : 0x00000001326d2000
    - pointerValue : 5140979712
httpBodyStream : nil
httpShouldHandleCookies : true
httpShouldUsePipelining : false

@Rudiney is a little busy right now, soon we post the server log here to see if can helps... Thanks again!

Cleversou1983 · 2019-08-13T14:45:23Z

@designatednerd The server log is the same as before... I'm using the 'add/upload-docs' branch. I've executed the 'pod reintegrate' and 'pod install' to make sure it get the fresh files...

designatednerd · 2019-08-14T10:01:16Z

@Cleversou1983 is the callback not getting called because the call fails or is it just never getting called at all?

kimdv · 2019-08-14T10:19:45Z

@designatednerd That sounds good!

I will try to make something, and then we can add an example for Absinthe.

Cleversou1983 · 2019-08-14T10:56:45Z

@designatednerd The upload succeeded from server side, but the completion closure never gets called at all (so I can't test for fail or success) and the app get stuck in my "busy" state...

designatednerd · 2019-08-14T11:29:05Z

When the upload succeeds, are you able to see if the appropriate data is coming back from the server regardless of whether it's being processed properly? I'm trying to figure out if there's some kind of issue with how I've got this set up, or if your server is just not calling you back with anything once the upload succeeds.

You should be able to check this by implementing the HTTPNetworkTransportTaskCompletedDelegate and checking the raw data you're getting back from the server (or if anything is coming in at all)

Cleversou1983 · 2019-08-14T11:33:06Z

@designatednerd I'll implement the delegate to see if I get any result... Regard this: "When the upload succeeds, are you able to see if the appropriate data is coming back from the server regardless of whether it's being processed properly?" - No data at all!

Thanks!

designatednerd · 2019-08-14T11:43:40Z

OK I think you may need to chat with @Rudiney about why that's not getting you any data back then

Cleversou1983 · 2019-08-14T11:46:10Z

@designatednerd I've implemented the HTTPNetworkTransportTaskCompletedDelegate delegate, but still no response. My co-worker @Rudiney is on travel today and will not be able to access the server logs. But he told me that the request was OK and with 200 status... As soon as we can get any additional info, we'll post here... Thanks again!

designatednerd · 2019-08-14T13:31:12Z

OK cool - I've just verified that what i've just pushed is now working with both single and multiple uploads from our local server that adheres to the spec, and that it's returning and parsing the correct info.

Now let's see what our friend travis has to say.

Cleversou1983 · 2019-08-14T13:44:30Z

@designatednerd Well, the info we were able to get was the request I've made:

{"operations"=>"{"operationName":"UploadFile","query":"mutation UploadFile($file: Upload!, $clientId: String!, $clientSecret: String!) { uploadFile(input: {file: $file, clientId: $clientId, clientSecret: $clientSecret}) { __typename attachment { __typename id } errors } }","variables":{"clientId":"xxxxx","clientSecret":"xxxxx","file":null}}", "map"=>"{"file":["variables.file"]}", "file"=>#<ActionDispatch::Http::UploadedFile:0x00005563d5e94b28 @tempfile=#Tempfile:/tmp/RackMultipart20190814-4-dgfkyy, @original_filename="file", @content_type="image/jpeg", @headers="Content-Disposition: form-data; name="file"; filename="file"\r\nContent-Type: image/jpeg\r\n">, "operationName"=>"UploadFile", "query"=>"mutation UploadFile($file: Upload!, $clientId: String!, $clientSecret: String!) { uploadFile(input: {file: $file, clientId: $clientId, clientSecret: $clientSecret}) { __typename attachment { __typename id } errors } }", "variables"=>{"clientId"=>"xxxxx", "clientSecret"=>"xxxxx", "file"=>#<ActionDispatch::Http::UploadedFile:0x00005563d5e94b28 @tempfile=#Tempfile:/tmp/RackMultipart20190814-4-dgfkyy, @original_filename="file", @content_type="image/jpeg", @headers="Content-Disposition: form-data; name="file"; filename="file"\r\nContent-Type: image/jpeg\r\n">}}

And the server returned 200, from the server side, it worked well... I'got the last commit from this branch, but sill there's no response at all when the upload finishes... That's weird... Thanks a lot!

designatednerd · 2019-08-14T15:23:56Z

@Cleversou1983 When you say The server returned a 200, is that 200 response or anything with it actually reaching the phone? If it's not, you'll probably need to work with your server guy to get it working.

…ernate type, it doesn't get hammered.

…the spec

…it network transports conform.

guess we haven't been running these much...

…an just a string

designatednerd · 2019-08-15T12:26:54Z

I'm gonna go ahead and merge this - longer-term I'm gonna talk to my crew about trying to get the Star Wars sample app to also handle uploads so we can do some round-trip tests on that, but for now, I've validated this works against a server that matches the spec.

Cleversou1983 · 2019-08-16T12:07:49Z

@designatednerd Thanks a lot, Ellen... We'll try to make some tests soon...
Cheers,
Cleverson

designatednerd added this to the 0.15.0 milestone Aug 12, 2019

kimdv approved these changes Aug 12, 2019

View reviewed changes

This was referenced Aug 12, 2019

Problem uploading a file with GraphQLFile #695

Closed

File upload v0.13 - Missing documentation #681

Closed

upload Multi Part file #677

Closed

designatednerd force-pushed the add/upload-docs branch from 3332805 to 18f9d2d Compare August 13, 2019 10:24

designatednerd added 15 commits August 14, 2019 23:41

Move where we're setting the default content-type so if we use an alt…

0136f90

…ernate type, it doesn't get hammered.

always sort data if possible

64a194c

fix indentation in test file

3c06031

update request creator to make sure it's creating uploads that match …

cdfafc2

…the spec

update tests and make sure we're also testing the request creator

e42d7a1

Add sub-protocol to NetworkTransport for uploading, make HTTP and spl…

614e60c

…it network transports conform.

Add ability to upload directly to the client

1b26485

oh hai xcode 10.3

197e101

fix result types in performance tests

cd9dcb7

guess we haven't been running these much...

Add documentation on uploading a file

d4f4186

fix request creator to take an array of strings for the map rather th…

e869740

…an just a string

Make sure variables for included files are set to null

3d681fd

use index for name of files

97e7a02

pass in an array of nil for multiple uploads

35390bb

add more documentation about uploading things

fe0d847

designatednerd force-pushed the add/upload-docs branch from 2bbba88 to fe0d847 Compare August 14, 2019 21:44

designatednerd merged commit 426818a into master Aug 15, 2019

designatednerd deleted the add/upload-docs branch August 16, 2019 09:09

Mo' Better Uploading #707

Mo' Better Uploading #707

Conversation

designatednerd commented Aug 12, 2019

designatednerd commented Aug 12, 2019

kimdv commented Aug 12, 2019

kimdv left a comment • edited Loading

Choose a reason for hiding this comment

designatednerd commented Aug 12, 2019

Cleversou1983 commented Aug 12, 2019

designatednerd commented Aug 12, 2019

Cleversou1983 commented Aug 12, 2019 • edited Loading

designatednerd commented Aug 12, 2019

Cleversou1983 commented Aug 12, 2019

designatednerd commented Aug 12, 2019

Cleversou1983 commented Aug 13, 2019

designatednerd commented Aug 13, 2019

Cleversou1983 commented Aug 13, 2019

kimdv commented Aug 13, 2019 • edited Loading

designatednerd commented Aug 13, 2019

Cleversou1983 commented Aug 13, 2019

designatednerd commented Aug 13, 2019

Cleversou1983 commented Aug 13, 2019

Cleversou1983 commented Aug 13, 2019

Cleversou1983 commented Aug 13, 2019 • edited Loading

Rudiney commented Aug 13, 2019

designatednerd commented Aug 13, 2019

designatednerd commented Aug 13, 2019

Cleversou1983 commented Aug 13, 2019

Cleversou1983 commented Aug 13, 2019

designatednerd commented Aug 14, 2019

kimdv commented Aug 14, 2019

Cleversou1983 commented Aug 14, 2019 • edited Loading

designatednerd commented Aug 14, 2019

Cleversou1983 commented Aug 14, 2019 • edited Loading

designatednerd commented Aug 14, 2019

Cleversou1983 commented Aug 14, 2019

designatednerd commented Aug 14, 2019

Cleversou1983 commented Aug 14, 2019 • edited Loading

designatednerd commented Aug 14, 2019

designatednerd commented Aug 15, 2019

Cleversou1983 commented Aug 16, 2019

kimdv left a comment •

edited

Loading

Cleversou1983 commented Aug 12, 2019 •

edited

Loading

kimdv commented Aug 13, 2019 •

edited

Loading

Cleversou1983 commented Aug 13, 2019 •

edited

Loading

Cleversou1983 commented Aug 14, 2019 •

edited

Loading

Cleversou1983 commented Aug 14, 2019 •

edited

Loading

Cleversou1983 commented Aug 14, 2019 •

edited

Loading