The downloader is no longer confused by params in the URL.

[mbishop-fiksu]

Adding parameters to a specification's source URL confuses the downloader as it assumes the end of the url contains the file extension of the payload.

This change makes sure the file's extension is inferred from only the "path" part of the source url.

[kylef]

Thanks for this pull request @mbishop-fiksu. Would you be able to add an entry for this in our CHANGELOG? Just an entry under a new version called Master.

[mbishop-fiksu]

I'd be happy to.

[AliSoftware]

Thx @mbishop-fiksu

Does anyone know why specs aren't passing? Is it due to the fix or was it failing before?

[mbishop-fiksu]

I added one test: ignores any params in the url. What's weird is that it passes with no issue locally on my machine. The other tests, I'm unsure of as I don't think i touched them.

I understand the error for the ignores any params in the url and it does seem weird to issue a ownload for a file:// url that has params on the end. I'm using Ruby 2. I wonder if it is more forgiving?

[AliSoftware]

That's strange. On my side, your new spec passes, but otherwise the specs that fails on Travis also do fail locally on my machine (and bazaar-related specs fails too, but that's because I don't have bazaar installed so that's to be expected)

HTTP
  ✓ download file and unzip it (18 ms)
  ✓ ignores any params in the url (15 ms)
  ✓ should download file and unzip it when the target folder name contains quotes or spaces (16 ms)
  ✓ should flatten zip archives, when the spec explicitly demands it (17 ms)
  ✓ moves unpacked contents to parent dir when archive contains only a folder (#727) (15 ms)
  ✓ does not move unpacked contents to parent dir when archive contains multiple children (19 ms)
  ✓ raises if it fails to download (151 ms)
  ✓ should verify that the downloaded file matches a sha1 hash (19 ms)
  ✓ should fail if the sha1 hash does not match (11 ms)
  ✓ should verify that the downloaded file matches a sha256 hash (18 ms)
  ✓ should fail if the sha256 hash does not match (13 ms)
  - detects zip files [FAILED]
  - detects tar files [FAILED]
  - detects tgz files [FAILED]
  - detects tbz files [FAILED]
  - detects txz files [FAILED]
  ✓ allows to specify the file type in the sources
  - should download file and extract it with proper type [FAILED]
  ✓ should raise error when an unsupported file type is detected
  ✓ should raise error when an unsupported file type is specified in the options
  ✓ detects the file type if specified with a string
  ✓ returns whether it does not supports the download of the head

I added one test: ignores any params in the url. What's weird is that it passes with no issue locally on my machine. The other tests, I'm unsure of as I don't think i touched them.

Maybe your fix has a side-effect of introducing regressions for other parts. But I mostly suspect that parsing the URL (which is the new thing you do in your code) failed in some cases for those specs. Maybe it is only related to a special case with file:// URLs used in specs context?

I'm using Ruby 2. I wonder if it is more forgiving?

I use system Ruby (2.0.0p481) on Yosemite but tests aren't passing on my side, so that must be something else.

[AliSoftware]

Got it. The specs are using fake URLs that looks like https://file.zip which are not proper/valid URLs. Thus URI::parse(url) sees file.zip as the host name part of an URL having an empty "" path (and in fact it is what this URL really is).

We will have to fix the spec to use proper URLs to test the methods.

[AliSoftware]

@mbishop-fiksu I can't push onto your own repo to apply the fix but here is the diff.

Can you please apply the modifications to http_specs.rb on your branch and push it? It should hopefully fix the specs on Travis as it did fix them on my machine. Thx

       it 'detects zip files' do
-        options = { :http => 'https://file.zip' }
+        options = { :http => 'https://foo/file.zip' }
         downloader = Downloader.for_target(tmp_folder, options)
         downloader.send(:type).should == :zip
       end

       it 'detects tar files' do
-        options = { :http => 'https://file.tar' }
+        options = { :http => 'https://foo/file.tar' }
         downloader = Downloader.for_target(tmp_folder, options)
         downloader.send(:type).should == :tar
       end

       it 'detects tgz files' do
-        options = { :http => 'https://file.tgz' }
+        options = { :http => 'https://foo/file.tgz' }
         downloader = Downloader.for_target(tmp_folder, options)
         downloader.send(:type).should == :tgz
       end

       it 'detects tbz files' do
-        options = { :http => 'https://file.tbz' }
+        options = { :http => 'https://foo/file.tbz' }
         downloader = Downloader.for_target(tmp_folder, options)
         downloader.send(:type).should == :tbz
       end

       it 'detects txz files' do
-        options = { :http => 'https://file.txz' }
+        options = { :http => 'https://foo/file.txz' }
         downloader = Downloader.for_target(tmp_folder, options)
         downloader.send(:type).should == :txz
       end

       it 'allows to specify the file type in the sources' do
-        options = { :http => 'https://file', :type => :zip }
+        options = { :http => 'https://foo/file', :type => :zip }
         downloader = Downloader.for_target(tmp_folder, options)
         downloader.send(:type).should == :zip
       end

       it 'should download file and extract it with proper type' do
-        options = { :http => 'https://file.zip' }
+        options = { :http => 'https://foo/file.zip' }
         downloader = Downloader.for_target(tmp_folder, options)
         downloader.expects(:download_file).with(anything)
         downloader.expects(:extract_with_type).with(anything, :zip).at_least_once
[...]

       it 'should raise error when an unsupported file type is detected' do
-        options = { :http => 'https://file.rar' }
+        options = { :http => 'https://foo/file.rar' }
         downloader = Downloader.for_target(tmp_folder, options)
         lambda { downloader.download }.should.raise Http::UnsupportedFileTypeError
       end

       it 'should raise error when an unsupported file type is specified in the options' do
-        options = { :http => 'https://file', :type => :rar }
+        options = { :http => 'https://foo/file', :type => :rar }
         downloader = Downloader.for_target(tmp_folder, options)
         lambda { downloader.download }.should.raise Http::UnsupportedFileTypeError
       end

       it 'detects the file type if specified with a string' do
-        options = { :http => 'https://file', :type => 'zip' }
+        options = { :http => 'https://foo/file', :type => 'zip' }
         downloader = Downloader.for_target(tmp_folder, options)
         downloader.send(:type).should == :zip
       end

       it 'returns whether it does not supports the download of the head' do
-        options = { :http => 'https://file', :type => 'zip' }
+        options = { :http => 'https://foo/file', :type => 'zip' }
         downloader = Downloader.for_target(tmp_folder('checkout'), options)
         downloader.head_supported?.should.be.false
       end

[AliSoftware]

As for your added it 'ignores any params in the url' do spec, unfortunately @fixture_url is a file:// URL in the case of those specs, and IIRC the file:// URL scheme does not allow query parameters.

But anyway, the change that your patch does is to correctly detect the type of the file based on the extension in the URL, disregarding the params if any only in the type-detection method. So that's what you should only test, without even trying to download anything :wink: Like this for example:

  it 'detects zip files even when URL has params' do
    options = { :http => 'https://foo/file.zip?param=value' }
    downloader = Downloader.for_target(tmp_folder, options)
    downloader.send(:type).should == :zip
  end

Feel free to adapt. And thx again!

[mbishop-fiksu]

Ok, thank you for all the suggestions. I'll apply the fixes...

[AliSoftware]

Note: why move to file:/// instead of keeping http:///https:// and adding a host? Keeping http in the specs would allow testing the most realistic/probable cases and ensure URI::parse works as expected in such case, wouldn't it?

[AliSoftware]

@kylef Do you know why Rubocop limits classes to 119 lines? I mean why 119?

lib/cocoapods-downloader/http.rb:5:5: C: Class definition is too long. [120/119]

[mbishop-fiksu]

When I added the 'foo' host, the specs didn't pass and since file:// is a valid URI prefix, I figured the parsing would still work. I'll try it again just in case I missed something.

[AliSoftware]

If specs didn't pass when you added the foo that's because there is a problem with the code. You shouldn't try to bend the specs so as they pass and hide an issue in the code :wink: You should try and fix the code instead.

[mbishop-fiksu]

The only thing that is now failing is the RuboCop call. All tests use your suggestions verbatim. The http class is 164 lines. I don't think I can fix that up to pass RuboCop's test :disappointed:

[AliSoftware]

Thx!

To pass the Rubocop we might have to split the class into multiple files and extract some methods into an utility class. But that may be sthg for another PR maybe.

@kylef should we merge this (and open an issue to fix rubocop in another PR)?

[alloy]

@AliSoftware I don’t like a proliferation of ‘util’ / ‘helper’ modules and especially not just to satisfy rubocop. This is simply a rule that I don’t feel adds anything for us, as such I have already disabled it on Xcodeproj: https://github.com/CocoaPods/Xcodeproj/commit/6622aa1bc7191738c8b2db7cd0746a6a0a16f494. Please do so here as well.

[AliSoftware]

Gotcha

[AliSoftware]

@alloy I disabled the offending rubocop rule in master. @mbishop-fiksu Could you rebase your branch onto it please?

[AliSoftware]

From: @mbishop-fiksu [mail] I'd be happy to rebase my branch, but I'm not sure how to do that. Can you give me more detailed instructions or point me to some documentation that will help me?

Sure! It's quite simple actually:

In the terminal make sure you are in the directory where you cloned your fork, and that you are on your master branch, then run this command:

git pull --rebase https://github.com/CocoaPods/cocoapods-downloader.git

This will pull the new commit(s) from CocoaPods's original repo, rebasing your own commits on top of them (so GIT will replay (rebase) your commits on top of CP's new commits, instead of doing a merge).

You will then have to force-push the changes on the remote (as rebasing typically rewrites history by removing commits and replaying them anew, with a new sha1):

git push --force

[mbishop-fiksu]

done. Thanks @AliSoftware

[kylef]

Thanks for this pull-request and the updates @mbishop-fiksu.