apache / cordova-plugin-file-transfer

Apache Cordova File Transfer Plugin
https://cordova.apache.org/
Apache License 2.0
596 stars 887 forks source link

CB-11067 - Adds content-length to multipart string if file length determined as also iOS plugin does #140

Closed oddcb closed 7 years ago

oddcb commented 8 years ago

ICLA signed and sent per email.

https://issues.apache.org/jira/browse/CB-11067

nikhilkh commented 8 years ago

@daserge to help review. Looks like we might need some tests for this as well.

cordova-qa commented 8 years ago

Cordova CI Build has one or more failures.

Commit - Link Dashboard - Link

Builder Name Console Output Test Report Device Logs
Windows 8.1 Store Link Link Link
Windows 10 Store Link Link Link
Windows 8.1 Phone Link Link Link
iOS Link Link Link
Android Mac Link Link Link
daserge commented 8 years ago

@oddcb, thanks for the contribution! This is not an issue though as the Content-Length header is set by setFixedLengthStreamingMode in https://github.com/apache/cordova-plugin-file-transfer/blob/9347606dd33fe07ea36799b4dd28804019c68835/src/android/FileTransfer.java#L438. I've recently added tests verifying Content-Length, Content-Type and Transfer-Encoding for multipart/non-multipart & chunked/non-chunked modes - and they are passing for Android and iOS.

oddcb commented 8 years ago

Thanks for you reply @daserge

So I'm not very into how file uploads are done properly, but the problem I fixed for our api here was adding the Content-Length into the code block starting at:

https://github.com/apache/cordova-plugin-file-transfer/blob/9347606dd33fe07ea36799b4dd28804019c68835/src/android/FileTransfer.java#L406

Result:


                    beforeData.append(LINE_START).append(BOUNDARY).append(LINE_END);
                    beforeData.append("Content-Disposition: form-data; name=\"").append(fileKey).append("\";");
                    beforeData.append(" filename=\"").append(fileName).append('"').append(LINE_END);

                    if(filesize > -1)
                        beforeData.append("Content-Length: ").append(filesize).append(LINE_END);

                    beforeData.append("Content-Type: ").append(mimeType).append(LINE_END).append(LINE_END);
                    byte[] beforeDataBytes = beforeData.toString().getBytes("UTF-8");
                    byte[] tailParamsBytes = (LINE_END + LINE_START + BOUNDARY + LINE_START + LINE_END).getBytes("UTF-8");
                    int stringLength = beforeDataBytes.length + tailParamsBytes.length;

As far as I can tell it is also what happens for iOS in https://github.com/apache/cordova-plugin-file-transfer/blob/9347606dd33fe07ea36799b4dd28804019c68835/src/ios/CDVFileTransfer.m#L222

[postBodyBeforeFile appendData:[[NSString stringWithFormat:@"Content-Length: %ld\r\n\r\n", (long)[fileData length]] dataUsingEncoding:NSUTF8StringEncoding]];

History for us is just that our ios app was working with the file upload plugin as is, but when we added the android app with the plugin it did not send Content-Length as iOS did and hence our file upload API started to scream. We are using Jersey in our backend.

As far as I can tell your opinion is that this added code should not be required interacting with a standard api?

daserge commented 8 years ago

@oddcb, yes, Content-Length should be added by setFixedLengthStreamingMode. Which Android OS version had the issue? Could you try it again with the master plugin version? Do you use chunked mode for requests (Content-Length should not be present in case of chunked mode)?

vladimir-kotikov commented 8 years ago

@oddcb, any updates? Is this still actual? If not, please close this PR

oddcb commented 7 years ago

We've changed our server side to retrieve content-size as I realize it should be. Closing this.