From 2799e94afe9ff91acb5e053eb95fb7ff2dba934c Mon Sep 17 00:00:00 2001
From: Muhammad Hamza Sajjad
 <45758804+muhammadhamzasajjad@users.noreply.github.com>
Date: Thu, 18 Apr 2024 10:54:15 +0100
Subject: [PATCH] bugfix/1247:address comments in #1481 (#1503)

#### Reference Issues/PRs
Addresses @alexowens90's comments in #1481 after that PR was merged.

#### What does this implement or fix?

#### Any other comments?

#### Checklist

<details>
  <summary>
   Checklist for code changes...
  </summary>

- [ ] Have you updated the relevant docstrings, documentation and
copyright notice?
- [ ] Is this contribution tested against [all ArcticDB's
features](../docs/mkdocs/docs/technical/contributing.md)?
- [ ] Do all exceptions introduced raise appropriate [error
messages](https://docs.arcticdb.io/error_messages/)?
 - [ ] Are API changes highlighted in the PR description?
- [ ] Is the PR labelled as enhancement or bug so it appears in
autogenerated release notes?
</details>

<!--
Thanks for contributing a Pull Request to ArcticDB! Please ensure you
have taken a look at:
- ArcticDB's Code of Conduct:
https://github.com/man-group/ArcticDB/blob/master/CODE_OF_CONDUCT.md
- ArcticDB's Contribution Licensing:
https://github.com/man-group/ArcticDB/blob/master/docs/mkdocs/docs/technical/contributing.md#contribution-licensing
-->
---
 cpp/arcticdb/storage/lmdb/lmdb_storage.cpp     |  7 ++++---
 cpp/arcticdb/util/name_validation.cpp          | 11 +++++------
 python/tests/integration/arcticdb/test_lmdb.py |  2 +-
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/cpp/arcticdb/storage/lmdb/lmdb_storage.cpp b/cpp/arcticdb/storage/lmdb/lmdb_storage.cpp
index db1879cd2a..fa1f3c568d 100644
--- a/cpp/arcticdb/storage/lmdb/lmdb_storage.cpp
+++ b/cpp/arcticdb/storage/lmdb/lmdb_storage.cpp
@@ -259,8 +259,11 @@ void LmdbStorage::do_iterate_type(KeyType key_type, const IterateTypeVisitor& vi
     }
 }
 
-bool LmdbStorage::do_is_path_valid(const std::string_view pathString) const {
+bool LmdbStorage::do_is_path_valid(const std::string_view pathString ARCTICDB_UNUSED) const {
 #ifdef _WIN32
+    // Note that \ and / are valid characters as they will create subdirectories which are expected to work.
+    // The filenames such as COM1, LPT1, AUX, CON etc. are reserved but not strictly disallowed by Windows as directory names.
+    // Therefore, paths with these names are allowed.
     std::string_view invalid_win32_chars = "<>:\"|?*";
     auto found = pathString.find_first_of(invalid_win32_chars);
     if (found != std::string::npos) {
@@ -270,8 +273,6 @@ bool LmdbStorage::do_is_path_valid(const std::string_view pathString) const {
     if (!pathString.empty() && (pathString.back() == '.' || std::isspace(pathString.back()))) {
         return false;
     }
-#else
-    (void) pathString; // suppress -Werror=unused-parameter
 #endif
     return true;
 }
diff --git a/cpp/arcticdb/util/name_validation.cpp b/cpp/arcticdb/util/name_validation.cpp
index b066b97595..d6765c50c3 100644
--- a/cpp/arcticdb/util/name_validation.cpp
+++ b/cpp/arcticdb/util/name_validation.cpp
@@ -121,12 +121,11 @@ void verify_library_path_part(const std::string& library_part, char delim) {
 
 void verify_library_path_on_write(const Store* store, const StringId& library_path) {
     verify_name("library name", library_path, true, UNSUPPORTED_S3_CHARS);
-    if(!store->is_path_valid(library_path)) {
-        user_input::raise<ErrorCode::E_INVALID_CHAR_IN_NAME>(
-                "The library name contains unsupported chars. Library Name: {}",
-                library_path
-        );
-    }
+    user_input::check<ErrorCode::E_INVALID_CHAR_IN_NAME>(
+            store->is_path_valid(library_path),
+            "The library name contains unsupported chars. Library Name: {}",
+            library_path
+    );
 }
 
 }
\ No newline at end of file
diff --git a/python/tests/integration/arcticdb/test_lmdb.py b/python/tests/integration/arcticdb/test_lmdb.py
index aebf42ad97..f8761795f0 100644
--- a/python/tests/integration/arcticdb/test_lmdb.py
+++ b/python/tests/integration/arcticdb/test_lmdb.py
@@ -103,7 +103,7 @@ def test_lmdb_malloc_trim(lmdb_storage):
     lib._nvs.trim()
 
 @pytest.mark.skipif(sys.platform != "win32", reason="Non Windows platforms have different file path name restrictions")
-@pytest.mark.parametrize("invalid_lib_name", ["lib?1", "lib:1", "lib|1", "lib.", "lib "])
+@pytest.mark.parametrize("invalid_lib_name", ["lib?1", "lib:1", "lib|1", "lib\"1", "lib.", "lib "])
 def test_invalid_lmdb_lib_name_windows(lmdb_storage, invalid_lib_name):
     ac = lmdb_storage.create_arctic()
     with pytest.raises(UserInputException) as e: