Improve tensorstore file:// uri parsing

* Relative file uris are now disallowed
  Before file://host/path would parse the authority as a path element,
  allowing relative uris. This is incorrect and  these are no longer
  permitted.  This may break users of uri-syntax expecting relative uris.

* Windows drive path uri handling improved.
  Uris like `file:///c:/foo` which include windows drive letters would
  create illegal windows paths; this has now been corrected.

Fixes: #239
PiperOrigin-RevId: 784309212
Change-Id: Ie341e9d2950b16c679857f2f0c8a7f4480c5a1bd

Author: laramiel

python/tensorstore/kvstore.cc

@@ -1392,9 +1392,9 @@ Example of constructing from the :json:schema:`JSON representation<KvStore>`:
 
 Example of constructing from a :json:schema:`URL<KvStoreUrl>`:
 
-    >>> spec = ts.KvStore.Spec('file://tmp/data/')
+    >>> spec = ts.KvStore.Spec('file:///tmp/data/')
     >>> spec
-    KvStore.Spec({'driver': 'file', 'path': 'tmp/data/'})
+    KvStore.Spec({'driver': 'file', 'path': '/tmp/data/'})
 
 )");
 

python/tensorstore/tests/kvstore_test.py

@@ -14,6 +14,7 @@
 """Tests for tensorstore.KvStore."""
 
 import copy
+import pathlib
 import pickle
 import tempfile
 
@@ -103,17 +104,18 @@ def test_copy_range_to_ocdbt_memory():
 def test_copy_range_to_ocdbt_file():
   context = ts.Context()
   with tempfile.TemporaryDirectory() as dir_path:
+    base_url = pathlib.Path(dir_path).resolve().as_uri()
     child_spec = {
         'driver': 'ocdbt',
-        'base': f'file://{dir_path}/child',
+        'base': f'{base_url}/child',
     }
     child = ts.KvStore.open(child_spec, context=context).result()
     for k in ['a', 'b', 'c']:
       child[k] = f'value_{k}'
 
     parent_spec = {
         'driver': 'ocdbt',
-        'base': f'file://{dir_path}',
+        'base': base_url,
     }
     parent = ts.KvStore.open(parent_spec, context=context).result()
     child.experimental_copy_range_to(parent).result()
@@ -134,12 +136,11 @@ def test_copy_range_to_memory_fails():
 def test_copy_range_to_file_fails():
   context = ts.Context()
   with tempfile.TemporaryDirectory() as dir_path:
-    child = ts.KvStore.open(
-        f'file://{dir_path}/child/', context=context
-    ).result()
+    base_url = pathlib.Path(dir_path).resolve().as_uri()
+    child = ts.KvStore.open(f'{base_url}/child/', context=context).result()
     for k in ['a', 'b', 'c']:
       child[k] = f'value_{k}'
-    parent = ts.KvStore.open(f'file://{dir_path}', context=context).result()
+    parent = ts.KvStore.open(base_url, context=context).result()
     with pytest.raises(NotImplementedError):
       child.experimental_copy_range_to(parent).result()
 

python/tensorstore/tests/tensorstore_test.py

@@ -14,11 +14,10 @@
 """Tests for tensorstore.TensorStore."""
 
 import copy
+import pathlib
 import pickle
 import re
 import tempfile
-import time
-
 import numpy as np
 import pytest
 import tensorstore as ts
@@ -592,8 +591,10 @@ def validate_spec(s):
 
 
 async def test_non_utf8_error():
-  with pytest.raises(ValueError, match='.*local file "\\\\xfa.*'):
-    await ts.open({"driver": "zarr", "kvstore": "file://%fa"})
+  with tempfile.TemporaryDirectory() as dir_path:
+    base_url = pathlib.Path(dir_path).resolve()
+    with pytest.raises(ValueError, match='.*local file ".*\\\\xfa.*'):
+      await ts.open({"driver": "zarr", "kvstore": base_url.as_uri() + "/%fa"})
 
 
 async def test_write_batch():

tensorstore/driver/BUILD

@@ -245,6 +245,7 @@ tensorstore_cc_library(
         "//tensorstore/internal:lock_collection",
         "//tensorstore/internal:nditerable",
         "//tensorstore/internal:nditerable_transformed_array",
+        "//tensorstore/internal:uri_utils",
         "//tensorstore/internal/testing:dynamic",
         "//tensorstore/internal/testing:json_gtest",
         "//tensorstore/internal/testing:queue_testutil",

tensorstore/driver/auto/BUILD

@@ -65,7 +65,7 @@ tensorstore_cc_test(
         "//tensorstore:spec",
         "//tensorstore:transaction",
         "//tensorstore/driver/zarr3",
-        "//tensorstore/internal/os:file_util",
+        "//tensorstore/internal:uri_utils",
         "//tensorstore/internal/testing:json_gtest",
         "//tensorstore/internal/testing:scoped_directory",
         "//tensorstore/kvstore",
@@ -75,6 +75,8 @@ tensorstore_cc_test(
         "//tensorstore/kvstore/zip",
         "//tensorstore/util:status_testutil",
         "@abseil-cpp//absl/status",
+        "@abseil-cpp//absl/strings",
+        "@abseil-cpp//absl/strings:cord",
         "@googletest//:gtest_main",
         "@nlohmann_json//:json",
     ],

tensorstore/driver/auto/driver_test.cc

@@ -14,14 +14,20 @@
 
 #include <stdint.h>
 
+#include <string>
+#include <string_view>
+
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
 #include "absl/status/status.h"
+#include "absl/strings/cord.h"
+#include "absl/strings/str_cat.h"
 #include <nlohmann/json.hpp>
 #include "tensorstore/context.h"
 #include "tensorstore/data_type.h"
 #include "tensorstore/internal/testing/json_gtest.h"
 #include "tensorstore/internal/testing/scoped_directory.h"
+#include "tensorstore/internal/uri_utils.h"
 #include "tensorstore/kvstore/kvstore.h"
 #include "tensorstore/kvstore/operations.h"
 #include "tensorstore/open.h"
@@ -46,6 +52,12 @@ auto MakeInitial(const Context &context) {
                            tensorstore::OpenMode::create);
 }
 
+template <typename... Args>
+std::string AsFileUri(std::string_view path, Args... args) {
+  return absl::StrCat("file://", tensorstore::internal::OsPathToUriPath(path),
+                      std::forward<Args>(args)...);
+}
+
 TEST(AutoTest, ExplicitDriver) {
   auto context = Context::Default();
   TENSORSTORE_ASSERT_OK_AND_ASSIGN(auto store_initial,
@@ -185,12 +197,12 @@ TEST(AutoTest, MemoryOcdbtZarr3) {
 
 TEST(AutoTest, FileZarr3) {
   ScopedTemporaryDirectory tempdir;
-  TENSORSTORE_ASSERT_OK(tensorstore::Open("file://" + tempdir.path() + "|zarr3",
+  TENSORSTORE_ASSERT_OK(tensorstore::Open(AsFileUri(tempdir.path(), "|zarr3"),
                                           tensorstore::dtype_v<int32_t>,
                                           tensorstore::Schema::Shape({5}),
                                           tensorstore::OpenMode::create)
                             .result());
-  TENSORSTORE_ASSERT_OK(tensorstore::Open("file://" + tempdir.path()).result());
+  TENSORSTORE_ASSERT_OK(tensorstore::Open(AsFileUri(tempdir.path())).result());
 }
 
 TEST(AutoTest, MemoryZarr3Transaction) {

tensorstore/driver/auto/index.rst

@@ -57,7 +57,7 @@ Auto-detecting an array
 A :ref:`zarr3<driver/zarr3>` TensorStore can be detected from its path:
 
   >>> # Create new array
-  >>> await ts.open("file://tmp/dataset.zarr|zarr3",
+  >>> await ts.open("file:///tmp/dataset.zarr|zarr3",
   ...               dtype="int32",
   ...               shape=[5],
   ...               create=True)
@@ -72,7 +72,7 @@ A :ref:`zarr3<driver/zarr3>` TensorStore can be detected from its path:
     },
     'driver': 'zarr3',
     'dtype': 'int32',
-    'kvstore': {'driver': 'file', 'path': 'tmp/dataset.zarr/'},
+    'kvstore': {'driver': 'file', 'path': '/tmp/dataset.zarr/'},
     'metadata': {
       'chunk_grid': {'configuration': {'chunk_shape': [5]}, 'name': 'regular'},
       'chunk_key_encoding': {'name': 'default'},
@@ -86,7 +86,7 @@ A :ref:`zarr3<driver/zarr3>` TensorStore can be detected from its path:
     'transform': {'input_exclusive_max': [[5]], 'input_inclusive_min': [0]},
   })
   >>> # Open with auto-detection
-  >>> await ts.open("file://tmp/dataset.zarr")
+  >>> await ts.open("file:///tmp/dataset.zarr")
   TensorStore({
     'context': {
       'cache_pool': {},
@@ -98,7 +98,7 @@ A :ref:`zarr3<driver/zarr3>` TensorStore can be detected from its path:
     },
     'driver': 'zarr3',
     'dtype': 'int32',
-    'kvstore': {'driver': 'file', 'path': 'tmp/dataset.zarr/'},
+    'kvstore': {'driver': 'file', 'path': '/tmp/dataset.zarr/'},
     'metadata': {
       'chunk_grid': {'configuration': {'chunk_shape': [5]}, 'name': 'regular'},
       'chunk_key_encoding': {'name': 'default'},
@@ -115,41 +115,41 @@ A :ref:`zarr3<driver/zarr3>` TensorStore can be detected from its path:
 Explicitly constructing a :py:obj:`~tensorstore.Spec` demonstrates the
 explicit syntax for using the ``auto`` driver:
 
-  >>> ts.Spec("file://tmp/dataset|auto")
-  Spec({'driver': 'auto', 'kvstore': {'driver': 'file', 'path': 'tmp/dataset'}})
-  >>> ts.Spec("file://tmp/dataset")
-  Spec({'driver': 'auto', 'kvstore': {'driver': 'file', 'path': 'tmp/dataset'}})
+  >>> ts.Spec("file:///tmp/dataset|auto")
+  Spec({'driver': 'auto', 'kvstore': {'driver': 'file', 'path': '/tmp/dataset'}})
+  >>> ts.Spec("file:///tmp/dataset")
+  Spec({'driver': 'auto', 'kvstore': {'driver': 'file', 'path': '/tmp/dataset'}})
 
 Chaining TensorStore adapters
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 TensorStore adapters like :ref:`cast<driver/cast>` can also be
 used in conjunction with format auto-detection:
 
-  >>> ts.Spec("file://tmp/dataset.zarr|cast:int64")
+  >>> ts.Spec("file:///tmp/dataset.zarr|cast:int64")
   Spec({
     'base': {
       'driver': 'auto',
-      'kvstore': {'driver': 'file', 'path': 'tmp/dataset.zarr'},
+      'kvstore': {'driver': 'file', 'path': '/tmp/dataset.zarr'},
     },
     'driver': 'cast',
     'dtype': 'int64',
   })
-  >>> ts.Spec("file://tmp/dataset.zarr|auto|cast:int64")
+  >>> ts.Spec("file:///tmp/dataset.zarr|auto|cast:int64")
   Spec({
     'base': {
       'driver': 'auto',
-      'kvstore': {'driver': 'file', 'path': 'tmp/dataset.zarr'},
+      'kvstore': {'driver': 'file', 'path': '/tmp/dataset.zarr'},
     },
     'driver': 'cast',
     'dtype': 'int64',
   })
-  >>> await ts.open("file://tmp/dataset.zarr|cast:int64")
+  >>> await ts.open("file:///tmp/dataset.zarr|cast:int64")
   TensorStore({
     'base': {
       'driver': 'zarr3',
       'dtype': 'int32',
-      'kvstore': {'driver': 'file', 'path': 'tmp/dataset.zarr/'},
+      'kvstore': {'driver': 'file', 'path': '/tmp/dataset.zarr/'},
       'metadata': {
         'chunk_grid': {
           'configuration': {'chunk_shape': [5]},
@@ -186,7 +186,7 @@ Multiple steps of auto-detection are also possible.  Here, a
 from the path to the OCDBT database.
 
   >>> # Create new array within new OCDBT database
-  >>> await ts.open("file://tmp/dataset.ocdbt|ocdbt|zarr3",
+  >>> await ts.open("file:///tmp/dataset.ocdbt|ocdbt|zarr3",
   ...               dtype="int32",
   ...               shape=[5],
   ...               create=True)
@@ -203,7 +203,7 @@ from the path to the OCDBT database.
     'driver': 'zarr3',
     'dtype': 'int32',
     'kvstore': {
-      'base': {'driver': 'file', 'path': 'tmp/dataset.ocdbt/'},
+      'base': {'driver': 'file', 'path': '/tmp/dataset.ocdbt/'},
       'config': {
         'compression': {'id': 'zstd'},
         'max_decoded_node_bytes': 8388608,
@@ -226,7 +226,7 @@ from the path to the OCDBT database.
     'transform': {'input_exclusive_max': [[5]], 'input_inclusive_min': [0]},
   })
   >>> # Open with auto-detection
-  >>> await ts.open("file://tmp/dataset.ocdbt")
+  >>> await ts.open("file:///tmp/dataset.ocdbt")
   TensorStore({
     'context': {
       'cache_pool': {},
@@ -240,7 +240,7 @@ from the path to the OCDBT database.
     'driver': 'zarr3',
     'dtype': 'int32',
     'kvstore': {
-      'base': {'driver': 'file', 'path': 'tmp/dataset.ocdbt/'},
+      'base': {'driver': 'file', 'path': '/tmp/dataset.ocdbt/'},
       'config': {
         'compression': {'id': 'zstd'},
         'max_decoded_node_bytes': 8388608,
@@ -268,7 +268,7 @@ of the OCDBT database:
 
   >>> # Create new array within new OCDBT database
   >>> await ts.open(
-  ...     "file://tmp/dataset2.ocdbt|ocdbt:path/within/database|zarr3",
+  ...     "file:///tmp/dataset2.ocdbt|ocdbt:path/within/database|zarr3",
   ...     dtype="int32",
   ...     shape=[5],
   ...     create=True)
@@ -285,7 +285,7 @@ of the OCDBT database:
     'driver': 'zarr3',
     'dtype': 'int32',
     'kvstore': {
-      'base': {'driver': 'file', 'path': 'tmp/dataset2.ocdbt/'},
+      'base': {'driver': 'file', 'path': '/tmp/dataset2.ocdbt/'},
       'config': {
         'compression': {'id': 'zstd'},
         'max_decoded_node_bytes': 8388608,
@@ -309,10 +309,10 @@ of the OCDBT database:
     'transform': {'input_exclusive_max': [[5]], 'input_inclusive_min': [0]},
   })
   >>> # Open with auto-detection
-  >>> await ts.open("file://tmp/dataset2.ocdbt")
+  >>> await ts.open("file:///tmp/dataset2.ocdbt")
   Traceback (most recent call last):
       ...
-  ValueError: FAILED_PRECONDITION: Error opening "auto" driver: Failed to detect format for "" in OCDBT database at local file "tmp/dataset2.ocdbt/"...
+  ValueError: FAILED_PRECONDITION: Error opening "auto" driver: Failed to detect format for "" in OCDBT database at local file "/tmp/dataset2.ocdbt/"...
 
 .. _multi-step-auto-detection-algorithm:
 

tensorstore/driver/driver_testutil.cc

@@ -72,6 +72,7 @@
 #include "tensorstore/internal/testing/json_gtest.h"
 #include "tensorstore/internal/testing/random_seed.h"
 #include "tensorstore/internal/testing/scoped_directory.h"
+#include "tensorstore/internal/uri_utils.h"
 #include "tensorstore/json_serialization_options_base.h"
 #include "tensorstore/kvstore/generation.h"
 #include "tensorstore/kvstore/memory/memory_key_value_store.h"
@@ -168,8 +169,11 @@ void TestTensorStoreDriverSpecRoundtrip(
     ReplaceStringInJson(options.create_spec, tempdir_key, tempdir->path());
     ReplaceStringInJson(options.minimal_spec, tempdir_key, tempdir->path());
     ReplaceStringInJson(options.full_base_spec, tempdir_key, tempdir->path());
-    options.url =
-        absl::StrReplaceAll(options.url, {{tempdir_key, tempdir->path()}});
+
+    // For the URL, the tempdir path must begin with a leading slash.
+    options.url = absl::StrReplaceAll(
+        options.url,
+        {{tempdir_key, internal::OsPathToUriPath(tempdir->path())}});
   }
   Transaction transaction(mode);
   auto context = Context::Default();

tensorstore/internal/os/error_code.h

@@ -15,6 +15,7 @@
 #ifndef TENSORSTORE_INTERNAL_OS_ERROR_CODE_H_
 #define TENSORSTORE_INTERNAL_OS_ERROR_CODE_H_
 
+#include <cassert>
 #include <cerrno>
 #include <string>
 #include <string_view>
@@ -71,6 +72,7 @@ absl::Status StatusWithOsError(
     absl::StatusCode status_code, OsErrorCode error_code,  //
     A a = {}, B b = {}, C c = {}, D d = {}, E e = {}, F f = {},
     SourceLocation loc = tensorstore::SourceLocation::current()) {
+  assert(status_code != absl::StatusCode::kOk);
   absl::Status status(
       status_code,
       tensorstore::StrCat(a, b, c, d, e, f, " [OS error ", error_code, ": ",

tensorstore/internal/uri_utils.cc

@@ -26,6 +26,8 @@
 #include "absl/strings/ascii.h"
 #include "absl/strings/match.h"
 #include "absl/strings/str_cat.h"
+#include "absl/strings/str_join.h"
+#include "absl/strings/str_split.h"
 #include "tensorstore/internal/ascii_set.h"
 
 namespace tensorstore {
@@ -224,5 +226,33 @@ std::optional<HostPort> SplitHostPort(std::string_view host_port) {
   return HostPort{host_port.substr(0, colon), host_port.substr(colon + 1)};
 }
 
+std::string OsPathToUriPath(std::string_view path) {
+  bool is_root_path =
+      (path.size() > 2 && absl::ascii_isalpha(path[0]) && path[1] == ':') ||
+      (!path.empty() && path[0] == '/');
+#ifdef _WIN32
+  constexpr const char kPathSeparator[] = "/\\";
+#else
+  constexpr const char kPathSeparator[] = "/";
+#endif
+  auto splitter = absl::StrSplit(path, absl::ByAnyChar(kPathSeparator));
+  auto it = splitter.begin();
+  if (it->empty()) it++;
+  std::string joined_path = absl::StrJoin(it, splitter.end(), "/");
+  if (is_root_path) {
+    joined_path.insert(0, 1, '/');
+  }
+  return PercentEncodeUriPath(joined_path);
+}
+
+std::string UriPathToOsPath(std::string_view path) {
+  std::string result = PercentDecode(path);
+  if (result.size() > 3 && result[0] == '/' && absl::ascii_isalpha(result[1]) &&
+      result[2] == ':') {
+    return result.substr(1);
+  }
+  return result;
+}
+
 }  // namespace internal
 }  // namespace tensorstore