From 51e83056b2ae1383c2baacb824605798ed024bd6 Mon Sep 17 00:00:00 2001 From: atarekra Date: Mon, 22 Jun 2026 14:58:00 +0300 Subject: [PATCH 1/2] Implementing Key max size feature --- src/cpp/src/internal/error.cpp | 3 ++ src/cpp/src/internal/error.hpp | 3 ++ src/cpp/src/kvs.cpp | 26 ++++++++++++++ src/cpp/src/kvs.hpp | 15 +++++++- src/cpp/tests/test_kvs.cpp | 61 ++++++++++++++++++++++++++++++++ src/cpp/tests/test_kvs_error.cpp | 1 + 6 files changed, 108 insertions(+), 1 deletion(-) diff --git a/src/cpp/src/internal/error.cpp b/src/cpp/src/internal/error.cpp index 00d3694cd..a7715e2d8 100644 --- a/src/cpp/src/internal/error.cpp +++ b/src/cpp/src/internal/error.cpp @@ -84,6 +84,9 @@ std::string_view MyErrorDomain::MessageFor(const score::result::ErrorCode& code) case ErrorCode::InvalidValueType: msg = "Invalid value type"; break; + case ErrorCode::KeyTooLong: + msg = "Key exceeds the maximum allowed length"; + break; default: msg = "Unknown Error!"; break; diff --git a/src/cpp/src/internal/error.hpp b/src/cpp/src/internal/error.hpp index bfd93e9f6..bedb4bf9e 100644 --- a/src/cpp/src/internal/error.hpp +++ b/src/cpp/src/internal/error.hpp @@ -83,6 +83,9 @@ enum class ErrorCode : score::result::ErrorCode /* Invalid value type*/ InvalidValueType, + + /* Key exceeds the maximum allowed length*/ + KeyTooLong, }; class MyErrorDomain final : public score::result::ErrorDomain diff --git a/src/cpp/src/kvs.cpp b/src/cpp/src/kvs.cpp index 1fc39005b..02b3be3ca 100644 --- a/src/cpp/src/kvs.cpp +++ b/src/cpp/src/kvs.cpp @@ -108,6 +108,17 @@ score::Result> Kvs::parse_json_data(co auto sv = element.first.GetAsStringView(); std::string key(sv.data(), sv.size()); + /* FEAT_REQ__KVS__maximum_size: keys read from persistent storage are not + guaranteed to satisfy the limit (e.g. externally edited or corrupted + files). Skip over-length keys so they never enter the store. */ + if (!is_key_length_valid(key)) + { + logger->LogWarn() << "Skipping key with length " << key.length() + << " exceeding maximum allowed " << KVS_MAX_KEY_LENGTH_BYTES + << " bytes while loading"; + continue; + } + auto conv = any_to_kvsvalue(element.second); if (!conv) { @@ -429,9 +440,24 @@ score::Result Kvs::is_value_default(const std::string_view key) const } } +/* FEAT_REQ__KVS__maximum_size: single check for the key-length rule. + std::string_view::length() counts bytes, matching the byte-based requirement. */ +bool Kvs::is_key_length_valid(std::string_view key) +{ + return key.length() <= KVS_MAX_KEY_LENGTH_BYTES; +} + /* Set the value for a key*/ score::ResultBlank Kvs::set_value(const std::string_view key, const KvsValue& value) { + /* FEAT_REQ__KVS__maximum_size: reject keys that exceed the maximum length. */ + if (!is_key_length_valid(key)) + { + logger->LogError() << "Key length " << key.length() << " exceeds maximum allowed " + << KVS_MAX_KEY_LENGTH_BYTES << " bytes"; + return score::MakeUnexpected(ErrorCode::KeyTooLong); + } + score::ResultBlank result = score::MakeUnexpected(ErrorCode::UnmappedError); std::unique_lock lock(kvs_mutex, std::try_to_lock); if (lock.owns_lock()) diff --git a/src/cpp/src/kvs.hpp b/src/cpp/src/kvs.hpp index cdccf1a09..6c545b58e 100644 --- a/src/cpp/src/kvs.hpp +++ b/src/cpp/src/kvs.hpp @@ -33,6 +33,9 @@ namespace score::mw::per::kvs { +/* FEAT_REQ__KVS__maximum_size: Maximum allowed key length in bytes. */ +constexpr std::size_t KVS_MAX_KEY_LENGTH_BYTES = 32U; + struct InstanceId { size_t id; @@ -262,13 +265,18 @@ class Kvs final /** * @brief Stores a key-value pair in the key-value store. * + * Features: + * - FEAT_REQ__KVS__maximum_size: The key length is limited to + * KVS_MAX_KEY_LENGTH_BYTES (32) bytes. + * * @param key The key associated with the value to be stored. * It is represented as a string view to avoid unnecessary copying. * @param value The value to be stored, represented as a KvsValue object. * * @return A score::Result object that indicates the success or failure of the operation. * - On success: Returns a blank score::Result. - * - On failure: Returns an ErrorCode describing the error. + * - On failure: Returns ErrorCode::KeyTooLong if the key exceeds + * KVS_MAX_KEY_LENGTH_BYTES, or another ErrorCode describing the error. */ score::ResultBlank set_value(const std::string_view key, const KvsValue& value); @@ -377,6 +385,11 @@ class Kvs final std::unique_ptr logger; /* Private Methods */ + /* FEAT_REQ__KVS__maximum_size: returns true if the key is within the allowed + length (KVS_MAX_KEY_LENGTH_BYTES). Single source of truth for the key-length + rule, shared by all paths that ingest keys (set_value, parse_json_data). */ + static bool is_key_length_valid(std::string_view key); + score::ResultBlank snapshot_rotate(); score::Result> parse_json_data(const std::string& data); score::Result> open_json(const score::filesystem::Path& prefix, diff --git a/src/cpp/tests/test_kvs.cpp b/src/cpp/tests/test_kvs.cpp index c3d5d0805..5b1818ead 100644 --- a/src/cpp/tests/test_kvs.cpp +++ b/src/cpp/tests/test_kvs.cpp @@ -151,6 +151,47 @@ TEST(kvs_TEST, parse_json_data_failure) cleanup_environment(); } +TEST(kvs_TEST, parse_json_data_skips_over_length_key) +{ + /* FEAT_REQ__KVS__maximum_size: keys longer than the limit found while loading from + storage must be skipped, not loaded into the store. */ + prepare_environment(); + + auto kvs = + Kvs::open(InstanceId(instance_id), OpenNeedDefaults::Optional, OpenNeedKvs::Optional, std::string(data_dir)); + ASSERT_TRUE(kvs); + + const std::string valid_key(KVS_MAX_KEY_LENGTH_BYTES, 'a'); + const std::string too_long_key(KVS_MAX_KEY_LENGTH_BYTES + 1U, 'b'); + + auto mock_parser = std::make_unique(); + score::json::Object obj; + + score::json::Object valid_inner; + valid_inner.emplace("t", score::json::Any(std::string("i32"))); + valid_inner.emplace("v", score::json::Any(42)); + obj.emplace(valid_key, score::json::Any(std::move(valid_inner))); + + score::json::Object long_inner; + long_inner.emplace("t", score::json::Any(std::string("i32"))); + long_inner.emplace("v", score::json::Any(7)); + obj.emplace(too_long_key, score::json::Any(std::move(long_inner))); + + score::json::Any any_obj(std::move(obj)); + EXPECT_CALL(*mock_parser, FromBuffer(::testing::_)) + .WillOnce(::testing::Return(score::Result(std::move(any_obj)))); + + kvs->parser = std::move(mock_parser); + + auto result = kvs->parse_json_data("data_not_used_in_mocking"); + ASSERT_TRUE(result); + /* The valid key is loaded; the over-length key is skipped. */ + EXPECT_EQ(result.value().count(valid_key), 1U); + EXPECT_EQ(result.value().count(too_long_key), 0U); + + cleanup_environment(); +} + TEST(kvs_open_json, open_json_success) { prepare_environment(); @@ -552,6 +593,26 @@ TEST(kvs_set_value, set_value_success) cleanup_environment(); } +TEST(kvs_set_value, set_value_key_too_long) +{ + prepare_environment(); + auto result = Kvs::open(instance_id, OpenNeedDefaults::Required, OpenNeedKvs::Required, std::string(data_dir)); + ASSERT_TRUE(result); + + /* A key of exactly KVS_MAX_KEY_LENGTH_BYTES bytes is accepted (inclusive limit) */ + std::string boundary_key(KVS_MAX_KEY_LENGTH_BYTES, 'k'); + auto boundary_result = result.value().set_value(boundary_key, KvsValue(3.14)); + EXPECT_TRUE(boundary_result); + + /* A key of KVS_MAX_KEY_LENGTH_BYTES + 1 bytes is rejected with KeyTooLong */ + std::string too_long_key(KVS_MAX_KEY_LENGTH_BYTES + 1U, 'k'); + auto set_value_result = result.value().set_value(too_long_key, KvsValue(3.14)); + EXPECT_FALSE(set_value_result); + EXPECT_EQ(static_cast(*set_value_result.error()), ErrorCode::KeyTooLong); + + cleanup_environment(); +} + TEST(kvs_set_value, set_value_failure) { prepare_environment(); diff --git a/src/cpp/tests/test_kvs_error.cpp b/src/cpp/tests/test_kvs_error.cpp index 44c5bfdb6..2e375e3da 100644 --- a/src/cpp/tests/test_kvs_error.cpp +++ b/src/cpp/tests/test_kvs_error.cpp @@ -40,6 +40,7 @@ TEST(kvs_MessageFor, MessageFor) {ErrorCode::ConversionFailed, "Conversion failed"}, {ErrorCode::MutexLockFailed, "Mutex failed"}, {ErrorCode::InvalidValueType, "Invalid value type"}, + {ErrorCode::KeyTooLong, "Key exceeds the maximum allowed length"}, }; for (const auto& test : test_cases) { From 732cdee0762a06eb5838174481c706a938c08941 Mon Sep 17 00:00:00 2001 From: atarekra Date: Tue, 14 Jul 2026 16:45:26 +0300 Subject: [PATCH 2/2] Applying PR review points --- src/cpp/src/kvs.cpp | 37 ++++++++++++++++++++++--------------- src/cpp/src/kvs.hpp | 9 ++------- src/cpp/tests/test_kvs.cpp | 12 +++++------- 3 files changed, 29 insertions(+), 29 deletions(-) diff --git a/src/cpp/src/kvs.cpp b/src/cpp/src/kvs.cpp index 02b3be3ca..318c812f6 100644 --- a/src/cpp/src/kvs.cpp +++ b/src/cpp/src/kvs.cpp @@ -29,6 +29,19 @@ using namespace std; namespace score::mw::per::kvs { +namespace +{ + +/* comp_req__kvs__key_length: single source of truth for the key-length rule, + shared by all paths that ingest keys (set_value, parse_json_data). + std::string_view::length() counts bytes, matching the byte-based requirement. */ +bool is_key_length_valid(std::string_view key) +{ + return key.length() <= KVS_MAX_KEY_LENGTH_BYTES; +} + +} // namespace + /*********************** KVS Implementation *********************/ Kvs::Kvs() : filesystem(std::make_unique( @@ -108,15 +121,16 @@ score::Result> Kvs::parse_json_data(co auto sv = element.first.GetAsStringView(); std::string key(sv.data(), sv.size()); - /* FEAT_REQ__KVS__maximum_size: keys read from persistent storage are not - guaranteed to satisfy the limit (e.g. externally edited or corrupted - files). Skip over-length keys so they never enter the store. */ + /* comp_req__kvs__key_length: a stored file must satisfy the key-length + limit; an over-length key means the file is invalid/corrupted. Halt + loading with an error rather than silently dropping data. */ if (!is_key_length_valid(key)) { - logger->LogWarn() << "Skipping key with length " << key.length() - << " exceeding maximum allowed " << KVS_MAX_KEY_LENGTH_BYTES - << " bytes while loading"; - continue; + logger->LogError() << "Key length " << key.length() << " exceeds maximum allowed " + << KVS_MAX_KEY_LENGTH_BYTES << " bytes while loading"; + result = score::MakeUnexpected(ErrorCode::KeyTooLong); + error = true; + break; } auto conv = any_to_kvsvalue(element.second); @@ -440,17 +454,10 @@ score::Result Kvs::is_value_default(const std::string_view key) const } } -/* FEAT_REQ__KVS__maximum_size: single check for the key-length rule. - std::string_view::length() counts bytes, matching the byte-based requirement. */ -bool Kvs::is_key_length_valid(std::string_view key) -{ - return key.length() <= KVS_MAX_KEY_LENGTH_BYTES; -} - /* Set the value for a key*/ score::ResultBlank Kvs::set_value(const std::string_view key, const KvsValue& value) { - /* FEAT_REQ__KVS__maximum_size: reject keys that exceed the maximum length. */ + /* comp_req__kvs__key_length: reject keys that exceed the maximum length. */ if (!is_key_length_valid(key)) { logger->LogError() << "Key length " << key.length() << " exceeds maximum allowed " diff --git a/src/cpp/src/kvs.hpp b/src/cpp/src/kvs.hpp index 6c545b58e..40259d0cf 100644 --- a/src/cpp/src/kvs.hpp +++ b/src/cpp/src/kvs.hpp @@ -33,7 +33,7 @@ namespace score::mw::per::kvs { -/* FEAT_REQ__KVS__maximum_size: Maximum allowed key length in bytes. */ +/* comp_req__kvs__key_length: Maximum allowed key length in bytes. */ constexpr std::size_t KVS_MAX_KEY_LENGTH_BYTES = 32U; struct InstanceId @@ -266,7 +266,7 @@ class Kvs final * @brief Stores a key-value pair in the key-value store. * * Features: - * - FEAT_REQ__KVS__maximum_size: The key length is limited to + * - comp_req__kvs__key_length: The key length is limited to * KVS_MAX_KEY_LENGTH_BYTES (32) bytes. * * @param key The key associated with the value to be stored. @@ -385,11 +385,6 @@ class Kvs final std::unique_ptr logger; /* Private Methods */ - /* FEAT_REQ__KVS__maximum_size: returns true if the key is within the allowed - length (KVS_MAX_KEY_LENGTH_BYTES). Single source of truth for the key-length - rule, shared by all paths that ingest keys (set_value, parse_json_data). */ - static bool is_key_length_valid(std::string_view key); - score::ResultBlank snapshot_rotate(); score::Result> parse_json_data(const std::string& data); score::Result> open_json(const score::filesystem::Path& prefix, diff --git a/src/cpp/tests/test_kvs.cpp b/src/cpp/tests/test_kvs.cpp index 5b1818ead..bce3b8ae5 100644 --- a/src/cpp/tests/test_kvs.cpp +++ b/src/cpp/tests/test_kvs.cpp @@ -151,10 +151,10 @@ TEST(kvs_TEST, parse_json_data_failure) cleanup_environment(); } -TEST(kvs_TEST, parse_json_data_skips_over_length_key) +TEST(kvs_TEST, parse_json_data_rejects_over_length_key) { - /* FEAT_REQ__KVS__maximum_size: keys longer than the limit found while loading from - storage must be skipped, not loaded into the store. */ + /* comp_req__kvs__key_length: an over-length key found while loading means the file + is invalid/corrupted; loading must halt with an error, not silently drop data. */ prepare_environment(); auto kvs = @@ -184,10 +184,8 @@ TEST(kvs_TEST, parse_json_data_skips_over_length_key) kvs->parser = std::move(mock_parser); auto result = kvs->parse_json_data("data_not_used_in_mocking"); - ASSERT_TRUE(result); - /* The valid key is loaded; the over-length key is skipped. */ - EXPECT_EQ(result.value().count(valid_key), 1U); - EXPECT_EQ(result.value().count(too_long_key), 0U); + ASSERT_FALSE(result); + EXPECT_EQ(static_cast(*result.error()), ErrorCode::KeyTooLong); cleanup_environment(); }