fix: installing wheel from trailing slash url

[pechersky]

Pull Request Check List

Resolves: n/a

Provide a step-by-step description of the suggested enhancement in as many details as possible.

Currently, the poetry add ... code looks for wheels or sdists at a pypi at an index url. In some cases, the url the server sends back might be of the form https://my.pypi.server/path/to/my_package.whl/. Unfortunately, the code currently does an rsplit and keeps only the terminal result of that split. For such a url, this is the empty string. This causes the filename to where the wheel is to be written to is empty-string. That means poetry add tries to write a file to /tmp/tmpdircreated, which is a directory, which fails.

The expected behavior is proper installation/addition of a package. This is done via a slightly more complicated parsing of the url. If the terminal item after rsplitting is empty, then use the penultimate item instead.

Unfortunately, I cannot provide an explicit example of the url that causes the issue, because it is in a private pypi.

• [x] Added tests for changed code. There are no existing test cases for this particular code path. A single test might not make sense, it only could be an integration test.
• [x] Updated documentation for changed code. The current documentation says nothing about the urls the server sends back.

[pechersky]

For discoverability, this is a possible solution for an error that looks like

 $ poetry update
 Updating dependencies
 Resolving dependencies... (1.4s)
 
   IsADirectoryError
 
   [Errno 21] Is a directory: '/tmp/tmpbs3xycu6'
 
   at ~/.pyenv/versions/3.8.8/envs/my-env/lib/python3.8/site-packages/poetry/utils/helpers.py:101 in download_file
        97│
        98│     with get(url, stream=True) as response:
        99│         response.raise_for_status()
       100│
     → 101│         with open(dest, "wb") as f:
       102│             for chunk in response.iter_content(chunk_size=chunk_size):
       103│                 if chunk:
       104│                     f.write(chunk)
       105│

[coleb]

I'm running into this issue as well even when trying to get poetry to update itself:

$ poetry update self
Updating dependencies
Resolving dependencies... (1.4s)

  IsADirectoryError

  [Errno 21] Is a directory: '/var/folders/xf/dz91t9wj1znc5npw2yq8xzww0000gr/T/tmp5622k9o_'

  at ~/.poetry/lib/poetry/utils/helpers.py:101 in download_file
       97│ 
       98│     with get(url, stream=True) as response:
       99│         response.raise_for_status()
      100│ 
    → 101│         with open(dest, "wb") as f:
      102│             for chunk in response.iter_content(chunk_size=chunk_size):
      103│                 if chunk:
      104│                     f.write(chunk)
      105│ 

When I manually apply the changes from this pull request, it works around the problem.

[pechersky]

Awesome! That looks really good and helps clean this up a lot.

Last step: let's add a regression test that checks that we successfully handle paths with a trailing slash.

Thanks! Sounds good. I'm not very familiar with the poetry tests structure. Are there tests that mock download? Searching for download_file, mock_download, and download_mock only gives the fixture setup code. Would the test go into test_pypi_repository.py?

[neersighted]

Awesome! That looks really good and helps clean this up a lot. Last step: let's add a regression test that checks that we successfully handle paths with a trailing slash.

Thanks! Sounds good. I'm not very familiar with the poetry tests structure. Are there tests that mock download? Searching for download_file, mock_download, and download_mock only gives the fixture setup code. Would the test go into test_pypi_repository.py?

Indeed it would! You should find some mock examples there to get you started.

[pechersky]

Thanks for the guidance. Unfortunately, none of the tests actually deal with the url encoded in the package json info, so I am not sure how to test codepaths on that data. The url is also not kept by PackageInfo.

Making a change in the mocked _download function:

     def _download(self, url, dest):
-        filename = url.split("/")[-1]
+        filename = self._helper_filename(url)
 
         fixture = self.DIST_FIXTURES / filename

still has all the tests pass -- but I am not sure this codepath is actually being tested.

[neersighted]

@pechersky Any chance you can rebase this onto current master? I'd be happy to provide some guidance on tests via Discord if you're still struggling (unit testing the helper and then mocking the install method and passing in a URL with a trailing slash out to be sufficient).

[github-actions[bot]]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.