Normalise URL paths with posixpath in clean_path

pyhealth/datasets/base_dataset.py

@@ -1,6 +1,7 @@
 import logging
 import os
 import pickle
+import posixpath
 from abc import ABC
 from pathlib import Path
 from typing import Dict, Iterator, Iterable, List, Optional, Any, Callable
@@ -106,10 +107,21 @@ def is_url(path: str) -> bool:
 
 
 def clean_path(path: str) -> str:
-    """Clean a path string."""
+    """Clean a path string.
+
+    URL paths are normalised as POSIX paths on every OS; local paths are
+    expanded and resolved.
+
+    Examples:
+        >>> from pyhealth.datasets.base_dataset import clean_path
+        >>> clean_path("https://example.org/a/b/../c.csv")
+        'https://example.org/a/c.csv'
+    """
     if is_url(path):
         parsed = urlparse(path)
-        cleaned_path = os.path.normpath(parsed.path)
+        # A URL path is POSIX whatever the host OS: os.path.normpath would
+        # rewrite the separators as backslashes on Windows.
+        cleaned_path = posixpath.normpath(parsed.path)
         # Rebuild the full URL
         return urlunparse(parsed._replace(path=cleaned_path))
     else:

tests/core/test_clean_path.py

@@ -0,0 +1,45 @@
+import ntpath
+import unittest
+from unittest.mock import patch
+
+from pyhealth.datasets import base_dataset
+from pyhealth.datasets.base_dataset import clean_path
+
+# (input URL, expected output). A URL path is POSIX whatever the host OS.
+URL_CASES = [
+    (
+        "https://storage.googleapis.com/pyhealth/Synthetic_MIMIC-III/PATIENTS.csv.gz",
+        "https://storage.googleapis.com/pyhealth/Synthetic_MIMIC-III/PATIENTS.csv.gz",
+    ),
+    (
+        "https://storage.googleapis.com/pyhealth/Synthetic_MIMIC-III//PATIENTS.csv",
+        "https://storage.googleapis.com/pyhealth/Synthetic_MIMIC-III/PATIENTS.csv",
+    ),
+    (
+        "https://example.org/a/b/../c.csv",
+        "https://example.org/a/c.csv",
+    ),
+    (
+        "s3://bucket/key/file.csv.gz",
+        "s3://bucket/key/file.csv.gz",
+    ),
+]
+
+
+class TestCleanPathURL(unittest.TestCase):
+    def test_url_path_is_normalised_as_posix(self):
+        for url, expected in URL_CASES:
+            with self.subTest(url=url):
+                self.assertEqual(clean_path(url), expected)
+
+    def test_url_path_is_unaffected_by_windows_os_path(self):
+        # Reproduce the Windows behaviour on any OS: ntpath.normpath rewrites
+        # "/" as "\\", which must not leak into a URL.
+        with patch.object(base_dataset.os, "path", ntpath):
+            for url, expected in URL_CASES:
+                with self.subTest(url=url):
+                    self.assertEqual(clean_path(url), expected)
+
+
+if __name__ == "__main__":
+    unittest.main()