Update URI join semantics

gigl/common/types/uri/uri.py

@@ -29,13 +29,25 @@ def _token_to_string(token: _URI_LIKE) -> str:
             return str(token)
         return ""
 
-    # TODO(kmonte): You should not be able to join a Uri with a Uri of a different type.
-    # *or* join HTTP on HTTP or GCS on GCS.
-    # This is not backwards compatible, so come around to this later.
+    @classmethod
+    def _is_absolute_path(cls, token: _URI_LIKE) -> bool:
+        token_str = cls._token_to_string(token)
+
+        # Note: "://" is used to detect GcsUri and HttpUri prefixes, but will incorrectly
+        # classify relative LocalUri path's containing "://" as absolute
+        return Path(token_str).is_absolute() or "://" in token_str
+
     @classmethod
     def join(cls, token: _URI_LIKE, *tokens: _URI_LIKE) -> Self:
         """
         Join multiple tokens to create a new Uri instance.
+        The first token may be an absolute or relative path. Every additional token must be relative.
+
+        Note:
+            - Rejecting suffix strings containing "://" may break callers that
+              intentionally store URI-looking strings inside paths.
+            - Rejecting absolute LocalUri suffix tokens is stricter than os.path.join,
+              which implicitly discards earlier tokens on the join path.
 
         Args:
             token: The first token to join.
@@ -45,6 +57,12 @@ def join(cls, token: _URI_LIKE, *tokens: _URI_LIKE) -> Self:
             A new Uri instance representing the joined URI.
 
         """
+        for suffix in tokens:
+            if cls._is_absolute_path(suffix):
+                raise TypeError(
+                    f"URI join suffixes must be relative; got absolute path: {suffix}"
+                )
+
         token = cls._token_to_string(token)
         token_strs: list[str] = [cls._token_to_string(token) for token in tokens]
         joined_tmp_path = os.path.join(token, *token_strs)
@@ -88,10 +106,6 @@ def __eq__(self, other: Any) -> bool:
         return False
 
     def __truediv__(self, other: _URI_LIKE) -> Self:
-        if isinstance(other, Uri) and not isinstance(other, type(self)):
-            raise TypeError(
-                f"Cannot use '/' operator to join {type(self).__name__} with {type(other).__name__}"
-            )
         return self.join(self, other)
 
 

tests/unit/common/types/uri_test.py

@@ -23,21 +23,82 @@ def test_can_get_basename(self):
 
     def test_join(self):
         joined: Uri
-        with self.subTest("LocalUri"):
-            joined = LocalUri.join("/foo/bar", "file.txt")
-            self.assertEqual(joined, LocalUri("/foo/bar/file.txt"))
-            self.assertIsInstance(joined, LocalUri)
-        with self.subTest("HttpUri"):
-            joined = HttpUri.join("http://abc.com/xyz", "foo")
-            self.assertEqual(joined, HttpUri("http://abc.com/xyz/foo"))
-            self.assertIsInstance(joined, HttpUri)
-        with self.subTest("GcsUri"):
-            joined = GcsUri.join("gs://bucket/", "file.txt")
-            self.assertEqual(joined, GcsUri("gs://bucket/file.txt"))
-            self.assertIsInstance(joined, GcsUri)
-        with self.subTest("LocalUri with Path"):
-            joined = LocalUri.join("/foo/bar", Path("file.text"))
-            self.assertEqual(joined, LocalUri("/foo/bar/file.text"))
+        with self.subTest("str suffix"):
+            relative_path_str = "file.txt"
+            with self.subTest("LocalUri"):
+                joined = LocalUri.join("/foo/bar", relative_path_str)
+                self.assertEqual(joined, LocalUri("/foo/bar/file.txt"))
+                self.assertIsInstance(joined, LocalUri)
+            with self.subTest("HttpUri"):
+                joined = HttpUri.join("http://abc.com/xyz", relative_path_str)
+                self.assertEqual(joined, HttpUri("http://abc.com/xyz/file.txt"))
+                self.assertIsInstance(joined, HttpUri)
+            with self.subTest("GcsUri"):
+                joined = GcsUri.join("gs://bucket/", relative_path_str)
+                self.assertEqual(joined, GcsUri("gs://bucket/file.txt"))
+                self.assertIsInstance(joined, GcsUri)
+
+        with self.subTest("Path suffix"):
+            relative_path = Path("file.txt")
+            with self.subTest("LocalUri"):
+                joined = LocalUri.join("/foo/bar", relative_path)
+                self.assertEqual(joined, LocalUri("/foo/bar/file.txt"))
+                self.assertIsInstance(joined, LocalUri)
+            with self.subTest("HttpUri"):
+                joined = HttpUri.join("http://abc.com/xyz", relative_path)
+                self.assertEqual(joined, HttpUri("http://abc.com/xyz/file.txt"))
+                self.assertIsInstance(joined, HttpUri)
+            with self.subTest("GcsUri"):
+                joined = GcsUri.join("gs://bucket/", relative_path)
+                self.assertEqual(joined, GcsUri("gs://bucket/file.txt"))
+                self.assertIsInstance(joined, GcsUri)
+
+        with self.subTest("LocalUri suffix"):
+            relative_local_uri = LocalUri("file.txt")
+            with self.subTest("LocalUri"):
+                joined = LocalUri.join("/foo/bar", relative_local_uri)
+                self.assertEqual(joined, LocalUri("/foo/bar/file.txt"))
+                self.assertIsInstance(joined, LocalUri)
+            with self.subTest("HttpUri"):
+                joined = HttpUri.join("http://abc.com/xyz", relative_local_uri)
+                self.assertEqual(joined, HttpUri("http://abc.com/xyz/file.txt"))
+                self.assertIsInstance(joined, HttpUri)
+            with self.subTest("GcsUri"):
+                joined = GcsUri.join("gs://bucket/", relative_local_uri)
+                self.assertEqual(joined, GcsUri("gs://bucket/file.txt"))
+                self.assertIsInstance(joined, GcsUri)
+
+    def test_join_invalid_suffix(self):
+        with self.subTest("absolute LocalUri suffix"):
+            absolute_local_uri = LocalUri("/other/file.txt")
+            with self.assertRaises(TypeError):
+                LocalUri.join("/foo/bar", absolute_local_uri)
+            with self.assertRaises(TypeError):
+                HttpUri.join("http://abc.com/xyz", absolute_local_uri)
+            with self.assertRaises(TypeError):
+                GcsUri.join("gs://bucket/path", absolute_local_uri)
+
+        with self.subTest("HttpUri suffix"):
+            http_uri = HttpUri("http://abc.com/file.txt")
+            with self.assertRaises(TypeError):
+                LocalUri.join("/foo/bar", http_uri)
+            with self.assertRaises(TypeError):
+                HttpUri.join("http://abc.com/xyz", http_uri)
+            with self.assertRaises(TypeError):
+                GcsUri.join("gs://bucket/path", http_uri)
+
+        with self.subTest("GcsUri suffix"):
+            gcs_uri = GcsUri("gs://bucket/file.txt")
+            with self.assertRaises(TypeError):
+                LocalUri.join("/foo/bar", gcs_uri)
+            with self.assertRaises(TypeError):
+                HttpUri.join("http://abc.com/xyz", gcs_uri)
+            with self.assertRaises(TypeError):
+                GcsUri.join("gs://bucket/path", gcs_uri)
+
+    def test_join_rejects_relative_path_with_uri_separator(self):
+        with self.assertRaises(TypeError):
+            LocalUri.join("/foo/bar", "folder://file.txt")
 
     def test_div_join(self):
         joined: Uri