Apache Cordova
  1. Apache Cordova
  2. CB-861

Header support for PhoneGap's FileTransfer (download)

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.7.0
    • Fix Version/s: None
    • Component/s: Android, CordovaJS
    • Labels:
      None

      Activity

      Hide
      Shazron Abdullah added a comment -

      Isn't this a dupe of CB-78? Which has been resolved?

      Show
      Shazron Abdullah added a comment - Isn't this a dupe of CB-78 ? Which has been resolved?
      Hide
      Aurelien MERCIER added a comment -

      To be able to download a file from the community, I had to patch the download method since we are using oauth2.
      I was wondering if you could add this modification for future Cordova releases?
      Here are my fix:

      cordova-1.7.0.js: (add params as a parameter of the download method)

      FileTransfer.prototype.download = function(source, target, successCallback, errorCallback, params)

      { [...] exec(win, errorCallback, 'FileTransfer', 'download', [source, target, params]); }

      ;

      FileTransfer.java:
      Change the download method signature (add params) l 444 :
      private PluginResult download(String source, String target, JSONObject params) {

      add the following code after line 459:
      connection.setRequestMethod("GET");

      // Handle headers
      try {
      JSONObject headers = params.getJSONObject("headers");
      for (Iterator iter = headers.keys(); iter.hasNext()

      { String headerKey = iter.next().toString(); connection.setRequestProperty(headerKey, headers.getString(headerKey)); }

      } catch (JSONException e1)

      { // No headers to be manipulated! }

      By the way, this issue has not been solved by CB-78 since it was only for the upload method. So, if you could add that to the download method it would be great.

      Show
      Aurelien MERCIER added a comment - To be able to download a file from the community, I had to patch the download method since we are using oauth2. I was wondering if you could add this modification for future Cordova releases? Here are my fix: cordova-1.7.0.js: (add params as a parameter of the download method) FileTransfer.prototype.download = function(source, target, successCallback, errorCallback, params) { [...] exec(win, errorCallback, 'FileTransfer', 'download', [source, target, params]); } ; FileTransfer.java: Change the download method signature (add params) l 444 : private PluginResult download(String source, String target, JSONObject params) { add the following code after line 459: connection.setRequestMethod("GET"); // Handle headers try { JSONObject headers = params.getJSONObject("headers"); for (Iterator iter = headers.keys(); iter.hasNext() { String headerKey = iter.next().toString(); connection.setRequestProperty(headerKey, headers.getString(headerKey)); } } catch (JSONException e1) { // No headers to be manipulated! } By the way, this issue has not been solved by CB-78 since it was only for the upload method. So, if you could add that to the download method it would be great.
      Hide
      Shazron Abdullah added a comment -

      Ah ok now I see - this is for download.

      Show
      Shazron Abdullah added a comment - Ah ok now I see - this is for download.
      Hide
      Joe Bowser added a comment -

      @Aurelein Can you put this on a git repository so we can properly attribute this to you? Also, it's not fully required, but preferred if you sign the CLA and send it in. It can be found at http://www.apache.org/licenses/

      Show
      Joe Bowser added a comment - @Aurelein Can you put this on a git repository so we can properly attribute this to you? Also, it's not fully required, but preferred if you sign the CLA and send it in. It can be found at http://www.apache.org/licenses/
      Hide
      Joe Bowser added a comment -

      @Aurelein: I don't see the Java changes that you wrote here. This also requires changing the API across the board, which is probably why it wasn't done in the first place. I'm going to assign this to Fil since it's Cordova-JS related.

      Show
      Joe Bowser added a comment - @Aurelein: I don't see the Java changes that you wrote here. This also requires changing the API across the board, which is probably why it wasn't done in the first place. I'm going to assign this to Fil since it's Cordova-JS related.
      Show
      Aurelien MERCIER added a comment - Hi Joe, I've committed the changes this morning => Java: https://github.com/quanta42/incubator-cordova-android/commit/07197e2a569473ec373f95dbb0b26915b53fb1e6 Js: https://github.com/quanta42/incubator-cordova-android/commit/6bf70c43392dab99fa57439e72906b0ef7ece93e Thanks,
      Hide
      Filip Maj added a comment -

      I've started a larger discussion about this feature on the mailing list: http://apache.markmail.org/thread/jllrdsifpsv7qrvi

      Show
      Filip Maj added a comment - I've started a larger discussion about this feature on the mailing list: http://apache.markmail.org/thread/jllrdsifpsv7qrvi
      Hide
      Filip Maj added a comment -

      Hey Andrew,

      I am doing a sweet of JIRA issues assigned to me. I am a bit out of the loop after my vacation

      I remember you dealing with FileTransfer in general. I am assigning this issue to you - if you feel this is better assigned to someone else please feel free to do that.

      Show
      Filip Maj added a comment - Hey Andrew, I am doing a sweet of JIRA issues assigned to me. I am a bit out of the loop after my vacation I remember you dealing with FileTransfer in general. I am assigning this issue to you - if you feel this is better assigned to someone else please feel free to do that.
      Hide
      Joe Bowser added a comment -

      What's the deal with this issue? It hasn't been touched in six months. Can we close this, or is this still outstanding?

      Show
      Joe Bowser added a comment - What's the deal with this issue? It hasn't been touched in six months. Can we close this, or is this still outstanding?
      Hide
      Aurelien MERCIER added a comment -

      tested and the new FT works with http headers passed as params.

      Show
      Aurelien MERCIER added a comment - tested and the new FT works with http headers passed as params.
      Hide
      Andrew Grieve added a comment -

      How did you test this? There's no code in FileTransfer's download path for adding headers:
      https://git-wip-us.apache.org/repos/asf?p=cordova-android.git;a=blob;f=framework/src/org/apache/cordova/FileTransfer.java;h=fdc9c75dc8169ce9c9996f8c7c97d1b416a8ff06;hb=HEAD

      I think the status of it is that it's still not implemented.

      If I'm confused and it does actually work, we should add a spec test for it (there's already one that tests upload headers)

      Show
      Andrew Grieve added a comment - How did you test this? There's no code in FileTransfer's download path for adding headers: https://git-wip-us.apache.org/repos/asf?p=cordova-android.git;a=blob;f=framework/src/org/apache/cordova/FileTransfer.java;h=fdc9c75dc8169ce9c9996f8c7c97d1b416a8ff06;hb=HEAD I think the status of it is that it's still not implemented. If I'm confused and it does actually work, we should add a spec test for it (there's already one that tests upload headers)
      Hide
      Aurelien MERCIER added a comment -

      Andrew, you are totally right, I misread the issue. I opened couple issues around download and upload and I was talking about iPhone, not android. Anyway, now we use xhr2, which solved our problem.
      I think you might wanna check some discussions around xhr2 that were engaged couple month ago by people @ Cordova.

      Show
      Aurelien MERCIER added a comment - Andrew, you are totally right, I misread the issue. I opened couple issues around download and upload and I was talking about iPhone, not android. Anyway, now we use xhr2, which solved our problem. I think you might wanna check some discussions around xhr2 that were engaged couple month ago by people @ Cordova.
      Hide
      Samik R added a comment -

      Need this - would be useful. See this thread: http://stackoverflow.com/q/14396380/194742

      Show
      Samik R added a comment - Need this - would be useful. See this thread: http://stackoverflow.com/q/14396380/194742
      Hide
      Tommy-Carlos Williams added a comment -

      OK. So after the discussion on the mailing list, I had a go at implementing FileTransfer.download headers for Android and iOS.

      Tested downloads using basic authentication.

      https://github.com/apache/cordova-js/pull/13
      https://github.com/apache/cordova-ios/pull/20
      https://github.com/apache/cordova-android/pull/21

      One possible problem is the order of the arguments in the JS. I am worried that the changes might need at least to be ignored on the other platforms...

      It seemed strange to have the options argument be after the objectId arg in the actual exec call, but if it creates less impact on the other platforms, I am happy to make whatever changes deemed needed. Just let me know.

      Show
      Tommy-Carlos Williams added a comment - OK. So after the discussion on the mailing list, I had a go at implementing FileTransfer.download headers for Android and iOS. Tested downloads using basic authentication. https://github.com/apache/cordova-js/pull/13 https://github.com/apache/cordova-ios/pull/20 https://github.com/apache/cordova-android/pull/21 One possible problem is the order of the arguments in the JS. I am worried that the changes might need at least to be ignored on the other platforms... It seemed strange to have the options argument be after the objectId arg in the actual exec call, but if it creates less impact on the other platforms, I am happy to make whatever changes deemed needed. Just let me know.
      Hide
      Andrew Grieve added a comment -

      Tommy - left you some comments on github, but overall your code looks great! Thanks for taking this on!

      Once these are in, we'll still need to:
      1. Update cordova-docs
      2. File bugs for other platforms to support this
      3. Add a test to mobile-spec to test that the headers are being added correctly (copy & paste the upload header test).

      I can take on these remaining tasks if you'd like

      Show
      Andrew Grieve added a comment - Tommy - left you some comments on github, but overall your code looks great! Thanks for taking this on! Once these are in, we'll still need to: 1. Update cordova-docs 2. File bugs for other platforms to support this 3. Add a test to mobile-spec to test that the headers are being added correctly (copy & paste the upload header test). I can take on these remaining tasks if you'd like
      Hide
      Tommy-Carlos Williams added a comment -

      Andrew,

      Thanks for looking at this. I'll get onto the suggested changes today.

      Show
      Tommy-Carlos Williams added a comment - Andrew, Thanks for looking at this. I'll get onto the suggested changes today.
      Hide
      Tommy-Carlos Williams added a comment -

      Andrew Grieve - I have made the changes you suggested and pushed them to the pull request.

      I have also ensured that mobile-spec still passed on Android where the upload method was affected by the change (refactoring the headers into a helper method to DRY it up a bit).

      Show
      Tommy-Carlos Williams added a comment - Andrew Grieve - I have made the changes you suggested and pushed them to the pull request. I have also ensured that mobile-spec still passed on Android where the upload method was affected by the change (refactoring the headers into a helper method to DRY it up a bit).
      Show
      Andrew Grieve added a comment - Pulled in your requests: JS: https://git-wip-us.apache.org/repos/asf?p=cordova-js.git;a=commit;h=6049d66747221874bd1e6821b4f3690faa60ab99 iOS: https://git-wip-us.apache.org/repos/asf?p=cordova-ios.git;a=commit;h=227452b1cafad6d4173e3782553e86f8c295d0d5 Android: https://git-wip-us.apache.org/repos/asf?p=cordova-android.git;a=commit;h=aa4820c3b79543120639f2fe37c1f200fc820d98
      Hide
      Tommy-Carlos Williams added a comment -

      Thanks, Andrew.

      So I can close the PR's on GitHub now, correct?

      Do you want me to do any of the three tasks remaining or are you fine with doing them?

      Show
      Tommy-Carlos Williams added a comment - Thanks, Andrew. So I can close the PR's on GitHub now, correct? Do you want me to do any of the three tasks remaining or are you fine with doing them?
      Hide
      Andrew Grieve added a comment -

      Yes! Please close them

      I'd love it if you'd like to do them.

      Show
      Andrew Grieve added a comment - Yes! Please close them I'd love it if you'd like to do them.
      Hide
      Tommy-Carlos Williams added a comment -

      Andrew Grieve - I am happy to do the docs and the mobile-spec tests if you can raise the issues against the other platforms?

      Show
      Tommy-Carlos Williams added a comment - Andrew Grieve - I am happy to do the docs and the mobile-spec tests if you can raise the issues against the other platforms?
      Hide
      Andrew Grieve added a comment -

      Deal

      Show
      Andrew Grieve added a comment - Deal
      Hide
      Andrew Grieve added a comment -

      subtasks created.

      Show
      Andrew Grieve added a comment - subtasks created.
      Show
      Michael Brooks added a comment - - edited Pull in your requests: Docs: https://git-wip-us.apache.org/repos/asf?p=cordova-docs.git;a=commit;h=5716e84a73901c146b3a232223588388519580b3 Docs: https://git-wip-us.apache.org/repos/asf?p=cordova-docs.git;a=commit;h=e9d7093394305c7a9cb9e30d40aaaa80432b6ece Thanks a bunch Tommy-Carlos Williams !
      Hide
      Michael Brooks added a comment -

      Andrew Grieve, should we keep this parent issue open until the sub-tasks are complete?

      Show
      Michael Brooks added a comment - Andrew Grieve , should we keep this parent issue open until the sub-tasks are complete?
      Hide
      Andrew Grieve added a comment -

      Sounds good. created one more sub-task for mobile-spec tests.

      Show
      Andrew Grieve added a comment - Sounds good. created one more sub-task for mobile-spec tests.

        People

        • Assignee:
          Andrew Grieve
          Reporter:
          Aurelien MERCIER
        • Votes:
          3 Vote for this issue
          Watchers:
          11 Start watching this issue

          Dates

          • Created:
            Updated:
            Resolved:

            Development