Fix downloaded file names keeping the URL query string

climada/test/test_api_client.py

@@ -22,6 +22,7 @@
 import tempfile
 import unittest
 from pathlib import Path
+from unittest import mock
 
 import numpy as np
 
@@ -147,6 +148,33 @@ def fail(x, y):
         download.unlink()
         self.assertFalse(download.is_file())
 
+    def test_download_file_name_from_url(self):
+        """Client._download derives the local file name from the URL path
+        only: a query string or fragment never becomes part of it, while
+        percent-decoding of the file name is kept."""
+        client = Client()
+
+        def download(url, download_dir):
+            with mock.patch("climada.util.api_client.requests.get") as mock_get:
+                stream = mock_get.return_value.__enter__.return_value
+                stream.iter_content.return_value = [b"data"]
+                return client._download(url, Path(download_dir))
+
+        with tempfile.TemporaryDirectory() as tmp_dir:
+            # plain URL (regression)
+            path = download("https://example.org/data/file.zip", tmp_dir)
+            self.assertEqual(path.name, "file.zip")
+            self.assertTrue(path.is_file())
+            # query string
+            path = download("https://example.org/data/file.zip?download=1", tmp_dir)
+            self.assertEqual(path.name, "file.zip")
+            # fragment
+            path = download("https://example.org/data/other.zip#section", tmp_dir)
+            self.assertEqual(path.name, "other.zip")
+            # percent-encoded file name is still decoded
+            path = download("https://example.org/data/my%20file.zip", tmp_dir)
+            self.assertEqual(path.name, "my file.zip")
+
     def test_download_dataset(self):
         """"""
         client = Client()

climada/util/api_client.py

@@ -26,7 +26,7 @@
 from dataclasses import dataclass
 from datetime import datetime
 from os.path import commonprefix
-from pathlib import Path
+from pathlib import Path, PurePosixPath
 from urllib.parse import quote, unquote, urlsplit, urlunsplit
 
 import pandas as pd
@@ -564,7 +564,7 @@ def _download(self, url, path, replace=False):
             and replace is False
         """
         if path.is_dir():
-            path /= unquote(url.split("/")[-1])
+            path /= unquote(PurePosixPath(urlsplit(url).path).name)
         if path.is_file() and not replace:
             raise FileExistsError(path)
         with requests.get(url, stream=True, timeout=Client.DOWNLOAD_TIMEOUT) as stream:

climada/util/files_handler.py

@@ -28,7 +28,8 @@
 import logging
 import math
 import urllib
-from pathlib import Path
+from pathlib import Path, PurePosixPath
+from urllib.parse import urlsplit
 
 import requests
 from tqdm import tqdm
@@ -78,8 +79,9 @@ def download_file(url, download_dir=None, overwrite=True):
     str
         the full path to the eventually downloaded file
     """
-    file_name = url.split("/")[-1]
-    if file_name.strip() == "":
+    url_path = urlsplit(url).path
+    file_name = PurePosixPath(url_path).name
+    if file_name.strip() == "" or url_path.endswith("/"):
         raise ValueError(f"cannot download {url} as a file")
     download_path = (
         CONFIG.local_data.save_dir.dir() if download_dir is None else Path(download_dir)

climada/util/test/test_files.py

@@ -19,8 +19,10 @@
 Test files_handler module.
 """
 
+import tempfile
 import unittest
 from pathlib import Path
+from unittest import mock
 
 from climada.util.constants import (
     DEMO_DIR,
@@ -49,6 +51,56 @@ def test_wrong_url_fail(self):
             pass
 
 
+class TestDownloadFileName(unittest.TestCase):
+    """The local file name is derived from the URL path only, so that a
+    query string or fragment never becomes part of the file name."""
+
+    def _download(self, url, download_dir):
+        response = mock.Mock()
+        response.status_code = 200
+        response.headers = {"content-length": "4"}
+        response.iter_content.return_value = [b"data"]
+        with mock.patch(
+            "climada.util.files_handler.requests.get", return_value=response
+        ):
+            return Path(download_file(url, download_dir=download_dir))
+
+    def test_plain_url_pass(self):
+        """Regression: a plain URL keeps its file name."""
+        with tempfile.TemporaryDirectory() as tmp_dir:
+            path = self._download("https://example.org/data/file.zip", tmp_dir)
+            self.assertEqual(path.name, "file.zip")
+            self.assertTrue(path.is_file())
+
+    def test_query_string_pass(self):
+        """The query string is not part of the file name."""
+        with tempfile.TemporaryDirectory() as tmp_dir:
+            path = self._download(
+                "https://example.org/data/"
+                "GMT_intermediate_coast_distance_01d.zip?download=1",
+                tmp_dir,
+            )
+            self.assertEqual(path.name, "GMT_intermediate_coast_distance_01d.zip")
+            self.assertTrue(path.is_file())
+
+    def test_fragment_pass(self):
+        """The fragment is not part of the file name."""
+        with tempfile.TemporaryDirectory() as tmp_dir:
+            path = self._download("https://example.org/data/file.zip#section", tmp_dir)
+            self.assertEqual(path.name, "file.zip")
+            self.assertTrue(path.is_file())
+
+    def test_no_file_name_fail(self):
+        """A URL whose path holds no file name raises, query string or not."""
+        with tempfile.TemporaryDirectory() as tmp_dir:
+            with self.assertRaises(ValueError):
+                self._download("https://example.org/data/?download=1", tmp_dir)
+            with self.assertRaises(ValueError):
+                self._download("https://example.org/data/", tmp_dir)
+            with self.assertRaises(ValueError):
+                self._download("https://example.org", tmp_dir)
+
+
 class TestToStrList(unittest.TestCase):
     """Test to_list function"""