Skip to content

Commit

Permalink
bugfix/1247:address comments in #1481 (#1503)
Browse files Browse the repository at this point in the history
#### 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
-->
  • Loading branch information
muhammadhamzasajjad authored Apr 18, 2024
1 parent 3b52859 commit 2799e94
Show file tree
Hide file tree
Showing 3 changed files with 10 additions and 10 deletions.
7 changes: 4 additions & 3 deletions cpp/arcticdb/storage/lmdb/lmdb_storage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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;
}
Expand Down
11 changes: 5 additions & 6 deletions cpp/arcticdb/util/name_validation.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
);
}

}
2 changes: 1 addition & 1 deletion python/tests/integration/arcticdb/test_lmdb.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down

0 comments on commit 2799e94

Please sign in to comment.