Remove internal length limit on URIs.

Closes #1285.
TileDB-Inc · May 29, 2019 · 902cd82 · 902cd82
1 parent 9159e6c
commit 902cd82
Show file tree

Hide file tree

Showing 5 changed files with 84 additions and 13 deletions.
diff --git a/test/src/unit-uri.cc b/test/src/unit-uri.cc
@@ -77,11 +77,11 @@ TEST_CASE("URI: Test file URIs", "[uri]") {
   uri = URI("file://path");
   CHECK(uri.is_invalid());
   uri =
-      URI("file:///path/is/too/long/long/long/long/long/long/long/long/long/"
+      URI("file:///path/is/quite/long/long/long/long/long/long/long/long/long/"
           "long/long/long/long/long/long/long/long/long/long/long/long/long/"
           "long/long/long/long/long/long/long/long/long/long/long/long/long/"
           "long/long/long/long/long/long/long/long/long/long/long/long/long");
-  CHECK(uri.is_invalid());
+  CHECK(!uri.is_invalid());
   uri = URI("");
   CHECK(uri.is_invalid());
 

diff --git a/test/src/unit-vfs.cc b/test/src/unit-vfs.cc
@@ -225,3 +225,85 @@ TEST_CASE("VFS: Test read batching", "[vfs]") {
 
   REQUIRE(vfs->terminate().ok());
 }
+
+#ifdef _WIN32
+
+TEST_CASE("VFS: Test long paths (Win32)", "[vfs][windows]") {
+  std::unique_ptr<VFS> vfs(new VFS);
+  std::string tmpdir_base = tiledb::sm::Win::current_dir() + "\\tiledb_test\\";
+  REQUIRE(vfs->create_dir(URI(tmpdir_base)).ok());
+
+  SECTION("- Deep hierarchy") {
+    // Create a nested path with a long total length, eventually will fail.
+    // Win32 path lengths (using traditional API) for CreateDirectory() must
+    // be <= 248 chars:
+    // https://docs.microsoft.com/en-us/windows/desktop/api/fileapi/nf-fileapi-createdirectorya
+    std::string tmpdir = tmpdir_base;
+    while (tmpdir.size() < 512) {
+      tmpdir += "subdir/";
+      if (tmpdir.size() <= 248)
+        REQUIRE(vfs->create_dir(URI(tmpdir)).ok());
+      else
+        REQUIRE(!vfs->create_dir(URI(tmpdir)).ok());
+    }
+  }
+
+  SECTION("- Too long name") {
+    std::string name;
+    for (unsigned i = 0; i < 256; i++)
+      name += "x";
+
+    // Creating the URI is invalid on Win32 (failure to canonicalize path)
+    URI testfile(tmpdir_base + name);
+    REQUIRE(testfile.is_invalid());
+  }
+
+  REQUIRE(vfs->remove_dir(URI(tmpdir_base)).ok());
+}
+
+#else
+
+TEST_CASE("VFS: Test long posix paths", "[vfs]") {
+  std::unique_ptr<VFS> vfs(new VFS);
+  std::string tmpdir_base = Posix::current_dir() + "/tiledb_test/";
+  REQUIRE(vfs->create_dir(URI(tmpdir_base)).ok());
+
+  SECTION("- Deep hierarchy") {
+    // Create a nested path with a long total length
+    std::string tmpdir = tmpdir_base;
+    while (tmpdir.size() < 512) {
+      tmpdir += "subdir/";
+      REQUIRE(vfs->create_dir(URI(tmpdir)).ok());
+    }
+
+    // Check we can create files within the deep hierarchy
+    URI testfile("file://" + tmpdir + "file.txt");
+    REQUIRE(!testfile.is_invalid());
+    bool exists = false;
+    REQUIRE(vfs->is_file(testfile, &exists).ok());
+    if (exists)
+      REQUIRE(vfs->remove_file(testfile).ok());
+    REQUIRE(vfs->touch(testfile).ok());
+    REQUIRE(vfs->remove_file(testfile).ok());
+  }
+
+  SECTION("- Too long name") {
+    // This may not be long enough on some filesystems to pass the fail check.
+    std::string name;
+    for (unsigned i = 0; i < 256; i++)
+      name += "x";
+
+    // Creating the URI and checking its existence is fine
+    URI testfile("file://" + tmpdir_base + name);
+    REQUIRE(!testfile.is_invalid());
+    bool exists = false;
+    REQUIRE(vfs->is_file(testfile, &exists).ok());
+
+    // Creating the file is not
+    REQUIRE(!vfs->touch(testfile).ok());
+  }
+
+  REQUIRE(vfs->remove_dir(URI(tmpdir_base)).ok());
+}
+
+#endif
diff --git a/tiledb/sm/misc/constants.cc b/tiledb/sm/misc/constants.cc
@@ -265,9 +265,6 @@ const uint64_t vfs_file_max_parallel_ops = vfs_num_threads;
 /** Whether or not filelocks are enabled for VFS. */
 const bool vfs_file_enable_filelocks = true;
 
-/** The maximum name length. */
-const uint32_t uri_max_len = 256;
-
 /** The maximum file path length (depending on platform). */
 #ifndef _WIN32
 const uint32_t path_max_len = PATH_MAX;

diff --git a/tiledb/sm/misc/constants.h b/tiledb/sm/misc/constants.h
@@ -253,9 +253,6 @@ extern const uint64_t vfs_file_max_parallel_ops;
 /** Whether or not filelocks are enabled for VFS. */
 extern const bool vfs_file_enable_filelocks;
 
-/** The maximum name length. */
-extern const uint32_t uri_max_len;
-
 /** The maximum file path length (depending on platform). */
 extern const uint32_t path_max_len;
 

diff --git a/tiledb/sm/misc/uri.cc b/tiledb/sm/misc/uri.cc
@@ -69,11 +69,6 @@ URI::URI(const std::string& path) {
     uri_ = path;
   else
     uri_ = "";
-
-  if (uri_.length() > constants::uri_max_len) {
-    LOG_ERROR("URI '" + uri_ + "' exceeds length limit.");
-    uri_ = "";
-  }
 }
 
 URI::~URI() = default;