Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/cpp/src/internal/error.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
3 changes: 3 additions & 0 deletions src/cpp/src/internal/error.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
26 changes: 26 additions & 0 deletions src/cpp/src/kvs.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,17 @@ score::Result<std::unordered_map<std::string, KvsValue>> 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is simply invalid. Files are either expected to satisfy the limit or are incorrect.
BTW comp_req__kvs__key_length is the right req.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your points have been resolved.

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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a quiet failure based on invalid assumption. Make it an error and halt the execution.

}

auto conv = any_to_kvsvalue(element.second);
if (!conv)
{
Expand Down Expand Up @@ -429,9 +440,24 @@ score::Result<bool> 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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This helper don't have to rely on anything Kvs class related. Rework it to a function, then place it in anonymous namespace/mark it as static.

{
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. */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Invalid req, like in other places.

if (!is_key_length_valid(key))
{
logger->LogError() << "Key length " << key.length() << " exceeds maximum allowed "

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here - check whitespace behavior.

<< KVS_MAX_KEY_LENGTH_BYTES << " bytes";
return score::MakeUnexpected(ErrorCode::KeyTooLong);
}

score::ResultBlank result = score::MakeUnexpected(ErrorCode::UnmappedError);
std::unique_lock<std::mutex> lock(kvs_mutex, std::try_to_lock);
if (lock.owns_lock())
Expand Down
15 changes: 14 additions & 1 deletion src/cpp/src/kvs.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -377,6 +385,11 @@ class Kvs final
std::unique_ptr<score::mw::log::Logger> 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<std::unordered_map<std::string, KvsValue>> parse_json_data(const std::string& data);
score::Result<std::unordered_map<std::string, KvsValue>> open_json(const score::filesystem::Path& prefix,
Expand Down
61 changes: 61 additions & 0 deletions src/cpp/tests/test_kvs.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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::IJsonParserMock>();
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<score::json::Any>(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();
Expand Down Expand Up @@ -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<ErrorCode>(*set_value_result.error()), ErrorCode::KeyTooLong);

cleanup_environment();
}

TEST(kvs_set_value, set_value_failure)
{
prepare_environment();
Expand Down
1 change: 1 addition & 0 deletions src/cpp/tests/test_kvs_error.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down
Loading