Apache Cordova
  1. Apache Cordova
  2. CB-765

Header support for PhoneGap's FileTransfer (Upload)

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.7.0
    • Fix Version/s: 1.9.0
    • Component/s: iOS
    • Labels:
      None

      Description

      It would be great to support headers for FileTransfer's iOS version (as done in Android: CB-78).

      Right now, I had redefined CDVFileTranfer for our needs and added just after userAgent definition:

      if(userAgent)

      { [req setValue: userAgent forHTTPHeaderField:@"User-Agent"]; }

      NSMutableDictionary* headers = [params objectForKey:@"headers"];
      NSEnumerator *enumerator = [headers keyEnumerator];
      id key;
      id val;
      NSString *nkey;

      while (nkey = [enumerator nextObject]) {
      val = [headers objectForKey:nkey];
      if(!val || val == [NSNull null])

      { continue; }

      // if it responds to stringValue selector (eg NSNumber) get the NSString
      if ([val respondsToSelector:@selector(stringValue)])

      { val = [val stringValue]; }

      // finally, check whether it is a NSString (for dataUsingEncoding selector below)
      if (![val isKindOfClass:[NSString class]])

      { continue; }

      //if ([key respondsToSelector:@selector(stringValue)])

      { [req setValue:val forHTTPHeaderField:nkey]; //}


      }

      If you can include this code or similar one into future version of Cordova it will be awesome.

      Thanks,

        Issue Links

          Activity

          Hide
          Shazron Abdullah added a comment -

          Tentatively assigning to 1.9.0 version until I heard back from the Android guys regarding CB-78

          Show
          Shazron Abdullah added a comment - Tentatively assigning to 1.9.0 version until I heard back from the Android guys regarding CB-78
          Hide
          Shazron Abdullah added a comment -
          Show
          Shazron Abdullah added a comment - Adding link to pull request: https://github.com/apache/incubator-cordova-ios/pull/22
          Show
          Herm Wong added a comment - https://git-wip-us.apache.org/repos/asf?p=incubator-cordova-ios.git;a=commit;h=43f789295ff45cfca071dd7c788c602db1124d5c
          Hide
          Ionuț G. Stan added a comment -

          Guys, this is wrong. `headers` shouldn't be a property on `params` but on FileUploadOptions itself. And it shouldn't be a map/hash/plain JS object. It should be a list of pairs. That's because some HTTP headers may appear multiple times in the request. Or it may be a map, but the values can be either strings or list of strings.

          I haven't enough time right now, otherwise I'd provide a patch.

          Show
          Ionuț G. Stan added a comment - Guys, this is wrong. `headers` shouldn't be a property on `params` but on FileUploadOptions itself. And it shouldn't be a map/hash/plain JS object. It should be a list of pairs. That's because some HTTP headers may appear multiple times in the request. Or it may be a map, but the values can be either strings or list of strings. I haven't enough time right now, otherwise I'd provide a patch.
          Hide
          Andrew Grieve added a comment -

          Yeah, I think Ionut has a point. It would make a lot more sense to have the headers be attached to the options object instead of a magic key of params.

          The point about allowing multiple headers with the same name is valid as well, but maybe low enough priority that it can wait for XHR2?

          I can take on adding the headers to FileUploadOptions and the relevant changes to iOS & Android. What do you think about removing the existing support for having headers on the params object though? My vote would be to remove support for this on iOS, but leave it in on Android since it's new on iOS but has existed for a little while on Android.

          Show
          Andrew Grieve added a comment - Yeah, I think Ionut has a point. It would make a lot more sense to have the headers be attached to the options object instead of a magic key of params. The point about allowing multiple headers with the same name is valid as well, but maybe low enough priority that it can wait for XHR2? I can take on adding the headers to FileUploadOptions and the relevant changes to iOS & Android. What do you think about removing the existing support for having headers on the params object though? My vote would be to remove support for this on iOS, but leave it in on Android since it's new on iOS but has existed for a little while on Android.
          Hide
          Ionuț G. Stan added a comment -

          I know you didn't ask me, but I think it's ok to remove it completely instead of deprecating them for a very simple reason. Cordova 1.9 broke FileUploadOptions so badly that no one that used it could upgrade to 1.9 (basically, no HTTP params get sent with the request). It's been fixed in this commit: https://github.com/apache/incubator-cordova-ios/commit/95cfc5121120840764537a069b8f93851e2b7aab

          So, if you intend to perform the new changes until 1.10 gets released, then I think it's ok to remove the special "headers" param. Otherwise, I think you should keep it, but issue some deprecation warnings.

          Show
          Ionuț G. Stan added a comment - I know you didn't ask me, but I think it's ok to remove it completely instead of deprecating them for a very simple reason. Cordova 1.9 broke FileUploadOptions so badly that no one that used it could upgrade to 1.9 (basically, no HTTP params get sent with the request). It's been fixed in this commit: https://github.com/apache/incubator-cordova-ios/commit/95cfc5121120840764537a069b8f93851e2b7aab So, if you intend to perform the new changes until 1.10 gets released, then I think it's ok to remove the special "headers" param. Otherwise, I think you should keep it, but issue some deprecation warnings.
          Hide
          Aurelien MERCIER added a comment -

          Sure, we've been patching CDVFileTranfer since phonegap 1.2 to be able to upload on a server using oauth2 so we can wait 1 or 2 more releases.

          Ps: btw too bad my pull request hasn't been merged properly.

          Show
          Aurelien MERCIER added a comment - Sure, we've been patching CDVFileTranfer since phonegap 1.2 to be able to upload on a server using oauth2 so we can wait 1 or 2 more releases. Ps: btw too bad my pull request hasn't been merged properly.
          Hide
          Shazron Abdullah added a comment -

          @Aurelien - what do you mean exactly that your pull request wasn't merged properly? (this: https://github.com/apache/incubator-cordova-ios/pull/22) Let us know.

          From what I see the dev working on it just copy and pasted some new code in, that may be the cause of the error, instead of cherry-picking your commit.

          Show
          Shazron Abdullah added a comment - @Aurelien - what do you mean exactly that your pull request wasn't merged properly? (this: https://github.com/apache/incubator-cordova-ios/pull/22 ) Let us know. From what I see the dev working on it just copy and pasted some new code in, that may be the cause of the error, instead of cherry-picking your commit.
          Hide
          Aurelien MERCIER added a comment -

          Here the change that has been committed for this issue:
          https://github.com/apache/incubator-cordova-ios/commit/43f789295ff45cfca071dd7c788c602db1124d5c#CordovaLib/Classes/CDVFileTransfer.m

          Here is my pull request:
          https://github.com/apache/incubator-cordova-ios/pull/22/files

          The following line "enumerator = [params keyEnumerator];" has not been copied from my pull request.
          That's why a fix has been committed: https://github.com/apache/incubator-cordova-ios/commit/95cfc5121120840764537a069b8f93851e2b7aab

          Show
          Aurelien MERCIER added a comment - Here the change that has been committed for this issue: https://github.com/apache/incubator-cordova-ios/commit/43f789295ff45cfca071dd7c788c602db1124d5c#CordovaLib/Classes/CDVFileTransfer.m Here is my pull request: https://github.com/apache/incubator-cordova-ios/pull/22/files The following line "enumerator = [params keyEnumerator] ;" has not been copied from my pull request. That's why a fix has been committed: https://github.com/apache/incubator-cordova-ios/commit/95cfc5121120840764537a069b8f93851e2b7aab
          Hide
          Shazron Abdullah added a comment -

          Sorry about this, the change should have been merged/cherry picked in instead of copy and pasted by the dev. This is not the normal procedure for integrating pull requests.

          Show
          Shazron Abdullah added a comment - Sorry about this, the change should have been merged/cherry picked in instead of copy and pasted by the dev. This is not the normal procedure for integrating pull requests.
          Hide
          Aurelien MERCIER added a comment -

          That's OK, shenanigans happen. I was so excited of getting this fix for 1.9.0, imagine my deception when first I learned that my pull request hasn't been merged properly and second I'll have to wait couple releases before being able to use the upload method.

          That's not the end of the world, we're gonna use our own implementation but that's not very funny to maintain this code.
          Anyway, we'll wait and we will definitely hope to see a great implementation of the multiple headers

          Show
          Aurelien MERCIER added a comment - That's OK, shenanigans happen. I was so excited of getting this fix for 1.9.0, imagine my deception when first I learned that my pull request hasn't been merged properly and second I'll have to wait couple releases before being able to use the upload method. That's not the end of the world, we're gonna use our own implementation but that's not very funny to maintain this code. Anyway, we'll wait and we will definitely hope to see a great implementation of the multiple headers
          Show
          Andrew Grieve added a comment - I've now got this implemented for iOS and Android in the following pull requests: JS: https://github.com/apache/incubator-cordova-js/pull/20 Android Impl: https://github.com/apache/incubator-cordova-android/pull/33 iOS Impl: https://github.com/apache/incubator-cordova-ios/pull/35 Mobile-Spec Impl: https://github.com/apache/incubator-cordova-mobile-spec/pull/9 Docs: https://github.com/apache/incubator-cordova-docs/pull/24
          Show
          Andrew Grieve added a comment - These pull requests are now all merged. https://git-wip-us.apache.org/repos/asf?p=incubator-cordova-js.git;a=commit;h=8c46a970a0719d0f16a225b75421ecf6f12dcc02 https://git-wip-us.apache.org/repos/asf?p=incubator-cordova-android.git;a=commit;h=3d53b9244d84cac9bdee445b7c07ffeb41a550bf https://git-wip-us.apache.org/repos/asf?p=incubator-cordova-ios.git;a=commit;h=fa8cbdaba44b9131a5181368efbd451e0761c686 https://git-wip-us.apache.org/repos/asf?p=incubator-cordova-mobile-spec.git;a=commit;h=acbd72fd3d338810ee2b7fab64035c8d9463d1f3 https://git-wip-us.apache.org/repos/asf?p=incubator-cordova-mobile-spec.git;a=commit;h=c101ed2a7d966c530e899d7d1e150e9f668f9fe7 https://git-wip-us.apache.org/repos/asf?p=incubator-cordova-mobile-spec.git;a=commit;h=e19bc127d75324ab6bfadc689d2e78b99eab2551 https://git-wip-us.apache.org/repos/asf?p=incubator-cordova-docs.git;a=commit;h=3a9117a84906ab772140c3b3559026b4d2cb4efa

            People

            • Assignee:
              Herm Wong
              Reporter:
              Aurelien MERCIER
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Time Tracking

                Estimated:
                Original Estimate - 1h
                1h
                Remaining:
                Remaining Estimate - 1h
                1h
                Logged:
                Time Spent - Not Specified
                Not Specified

                  Development