Repository: man-group/ArcticDB (language: python)

Issue:
Useful error message when empty string provided as symbol name
Currently get logs like:
```
ASSERTION FAILURE: Empty string id in reference key
Loading versions from storage via ref key failed with error: Empty string id in reference key for stream . Retrying
ASSERTION FAILURE: Empty string id in reference key
Loading versions from storage via ref key failed with error: Empty string id in reference key for stream . Retrying
```

but the actual exception thrown is `ASSERTION FAILURE: Couldn't read via ref key even after multiple attempts`, which does not show the underlying cause.

Should just involve checking for empty strings in `verify_name` in `name_validation.cpp`, but need to carefully check that this will not break any other use cases of this function.


Discussion / hints:

Fix patch:
diff --git a/cpp/arcticdb/entity/ref_key.hpp b/cpp/arcticdb/entity/ref_key.hpp
index 57b3fc877c..3ae412e8c2 100644
--- a/cpp/arcticdb/entity/ref_key.hpp
+++ b/cpp/arcticdb/entity/ref_key.hpp
@@ -23,7 +23,10 @@ namespace arcticdb::entity {
                 id_(std::move(id)),
                 key_type_(key_type),
                 old_type_(old_type) {
-            util::check(!std::holds_alternative<StringId>(id_) || !std::get<StringId>(id_).empty(), "Empty symbol in reference key");
+            user_input::check<ErrorCode::E_INVALID_USER_ARGUMENT>(
+                    !std::holds_alternative<StringId>(id_) || !std::get<StringId>(id_).empty(),
+                    "{} names cannot be empty strings",
+                    key_type == KeyType::VERSION_REF ? "Symbol": "Snapshot");
             util::check(old_type || is_ref_key_class(key_type), "Can't create ref key with non-ref key class keytype {}", key_type);
         }
 
diff --git a/cpp/arcticdb/util/name_validation.cpp b/cpp/arcticdb/util/name_validation.cpp
index d6765c50c3..86ad916cb6 100644
--- a/cpp/arcticdb/util/name_validation.cpp
+++ b/cpp/arcticdb/util/name_validation.cpp
@@ -18,8 +18,8 @@ namespace arcticdb {
 // '*', '<' and '>' are problematic for S3
 const auto UNSUPPORTED_S3_CHARS = std::set<char>{'*', '<', '>'};
 
-// We currently require the size to fit in uint8. See entity/serialized_key.hpp
-constexpr auto MAX_SIZE = 255;
+// entity/serialized_key.hpp expects the symbol to be <255 chars
+constexpr size_t MAX_SIZE = std::numeric_limits<uint8_t>::max() - 1;
 
 void verify_name(
         const std::string& name_type_for_error,
@@ -28,6 +28,11 @@ void verify_name(
         const std::set<char>& unsupported_chars = UNSUPPORTED_S3_CHARS,
         std::optional<char> unsupported_prefix = std::nullopt,
         std::optional<char> unsupported_suffix = std::nullopt) {
+    if (name.empty()) {
+        user_input::raise<ErrorCode::E_INVALID_USER_ARGUMENT>(
+                "The {} cannot be an empty string.",
+                name_type_for_error);
+    }
     if (name.size() > MAX_SIZE) {
         user_input::raise<ErrorCode::E_NAME_TOO_LONG>(
                 "The {} length exceeds the max supported length. {} length: {}, Max Supported Length: {}",
@@ -54,7 +59,7 @@ void verify_name(
                     c);
         }
     }
-    if (unsupported_prefix.has_value() && name.size() > 0 && name[0] == unsupported_prefix.value()) {
+    if (unsupported_prefix.has_value() && name[0] == unsupported_prefix.value()) {
         user_input::raise<ErrorCode::E_INVALID_CHAR_IN_NAME>(
                 "The {} starts with an unsupported prefix. {}: {} Unsupported prefix: {} ",
                 name_type_for_error,
@@ -63,7 +68,7 @@ void verify_name(
                 unsupported_prefix.value()
         );
     }
-    if (unsupported_suffix.has_value() && name.size() > 0 && name[name.size() - 1] == unsupported_suffix.value()) {
+    if (unsupported_suffix.has_value() && name[name.size() - 1] == unsupported_suffix.value()) {
         user_input::raise<ErrorCode::E_INVALID_CHAR_IN_NAME>(
                 "The {} ends with an unsupported suffix. {}: {} Unsupported suffix: {} ",
                 name_type_for_error,


Test patch:
diff --git a/python/tests/integration/arcticdb/version_store/test_basic_version_store.py b/python/tests/integration/arcticdb/version_store/test_basic_version_store.py
index 337b0f3c44..cd43f2ed83 100644
--- a/python/tests/integration/arcticdb/version_store/test_basic_version_store.py
+++ b/python/tests/integration/arcticdb/version_store/test_basic_version_store.py
@@ -105,19 +105,6 @@ def test_s3_breaking_chars(object_version_store, breaking_char):
     assert sym not in object_version_store.list_symbols()
 
 
-def test_s3_breaking_chars_exception_compat(object_version_store):
-    """Test that chars that are not supported are raising the appropriate exception and that we fail on write without
-    corrupting the db
-    """
-    sym = "prefix*postfix"
-    df = sample_dataframe()
-    with pytest.raises(UserInputException) as e_info:
-        object_version_store.write(sym, df)
-
-    assert isinstance(e_info.value, UserInputException)
-    assert sym not in object_version_store.list_symbols()
-
-
 @pytest.mark.parametrize("prefix", ["", "prefix"])
 @pytest.mark.parametrize("suffix", ["", "suffix"])
 def test_symbol_names_with_all_chars(object_version_store, prefix, suffix):
@@ -215,6 +202,66 @@ def test_unhandled_chars_already_present_update(object_version_store, unhandled_
     assert len(vitem.data) == len(new_vitem.data)
 
 
+# See AN-765 for why we need no_symbol_list fixture
+def test_large_symbols(basic_store_no_symbol_list):
+    # The following restrictions should be checked in the cpp layer's name_validation
+    MAX_SYMBOL_SIZE = 254
+    # TODO: Make too long name on LMDB raise a friendlier UserInputException (instead of InternalException [E_INVALID_ARGUMENT])
+    with pytest.raises((UserInputException, InternalException)):
+        basic_store_no_symbol_list.write("a" * (MAX_SYMBOL_SIZE + 1), 1)
+
+    valid_sized_sym = "a" * MAX_SYMBOL_SIZE
+    basic_store_no_symbol_list.write(valid_sized_sym, 1)
+    assert basic_store_no_symbol_list.read(valid_sized_sym).data == 1
+
+
+def test_empty_symbol_name_2(object_version_store):
+    lib = object_version_store
+    df = sample_dataframe()
+    exceptions = []
+    with pytest.raises(UserInputException) as e:
+        lib.write("", df)
+    exceptions.append(str(e.value).lower())
+    with pytest.raises(UserInputException) as e:
+        lib.append("", df, write_if_missing=True)
+    exceptions.append(str(e.value).lower())
+    with pytest.raises(UserInputException) as e:
+        lib.update("", df, upsert=True)
+    exceptions.append(str(e.value).lower())
+    with pytest.raises(UserInputException) as e:
+        lib.write_metadata("", {})
+    exceptions.append(str(e.value).lower())
+    with pytest.raises(UserInputException) as e:
+        lib.batch_write([""], [df])
+    exceptions.append(str(e.value).lower())
+    with pytest.raises(UserInputException) as e:
+        lib.batch_append([""], [df])
+    exceptions.append(str(e.value).lower())
+    with pytest.raises(UserInputException) as e:
+        lib.batch_write_metadata([""], [{}])
+    exceptions.append(str(e.value).lower())
+    assert all("symbol" in e for e in exceptions)
+
+
+@pytest.mark.parametrize(
+    "method", ("write", "append", "update", "write_metadata" ,"batch_write", "batch_append", "batch_write_metadata")
+)
+def test_empty_symbol_name(lmdb_version_store_v1, method):
+    first_arg = [""] if method.startswith("batch_") else ""
+    df = sample_dataframe()
+    second_arg = [df] if method.startswith("batch_") else df
+    with pytest.raises(UserInputException) as e:
+        getattr(lmdb_version_store_v1, method)(first_arg, second_arg)
+    assert "symbol" in str(e.value).lower()
+
+
+@pytest.mark.parametrize("method", ("snapshot", "delete_snapshot"))
+def test_empty_snapshot_name(lmdb_version_store_v1, method):
+    with pytest.raises(UserInputException) as e:
+        getattr(lmdb_version_store_v1, method)("")
+    assert "snapshot" in str(e.value).lower()
+
+
 def test_with_prune(object_and_mem_and_lmdb_version_store, symbol):
     version_store = object_and_mem_and_lmdb_version_store
     df = sample_dataframe()
@@ -851,46 +898,6 @@ def test_empty_ndarr(basic_store):
     assert_array_equal(basic_store.read(sym).data, ndarr)
 
 
-# The following restrictions should be checked in the cpp layer's name_validation
-MAX_SYMBOL_SIZE = 255
-UNSUPPORTED_S3_CHARS = {"*", "<", ">"}
-
-
-# See AN-765 for why we need no_symbol_list fixture
-def test_large_symbols(basic_store_no_symbol_list):
-    # TODO: Make too long name on LMDB raise a friendlier UserInputException (instead of InternalException [E_INVALID_ARGUMENT])
-    with pytest.raises((UserInputException, InternalException)):
-        basic_store_no_symbol_list.write("a" * (MAX_SYMBOL_SIZE + 1), 1)
-
-    for _ in range(5):
-        valid_sized_sym = "a" * random.randint(1, MAX_SYMBOL_SIZE - 1)
-        basic_store_no_symbol_list.write(valid_sized_sym, 1)
-        assert basic_store_no_symbol_list.read(valid_sized_sym).data == 1
-
-        valid_punctuations = "".join(list(set(string.punctuation) - set(UNSUPPORTED_S3_CHARS)))
-        valid_char_sym = "".join(
-            [random.choice(string.ascii_letters + string.digits + valid_punctuations) for _ in range(12)]
-        )
-
-        basic_store_no_symbol_list.write(valid_char_sym, 1)
-        assert basic_store_no_symbol_list.read(valid_char_sym).data == 1
-
-
-def test_unsupported_chars_in_symbols(basic_store):
-    for ch in UNSUPPORTED_S3_CHARS:
-        with pytest.raises(UserInputException):
-            basic_store.write(ch, 1)
-
-    for _ in range(5):
-        valid_punctuations = "".join(list(set(string.punctuation) - set(UNSUPPORTED_S3_CHARS)))
-        valid_char_sym = "".join(
-            [random.choice(string.ascii_letters + string.digits + valid_punctuations) for _ in range(12)]
-        )
-
-        basic_store.write(valid_char_sym, 1)
-        assert basic_store.read(valid_char_sym).data == 1
-
-
 def test_partial_read_pickled_df(basic_store):
     will_be_pickled = [1, 2, 3]
     basic_store.write("blah", will_be_pickled)