feat(kvs): enforce maximum key length of 32 bytes (C++) by atarekra · Pull Request #308 · eclipse-score/persistency

atarekra · 2026-06-22T12:17:48Z

Summary

Enforces the maximum key length for the C++ KVS, implementing
comp_req__kvs__key_length

Changes

Add KVS_MAX_KEY_LENGTH_BYTES (32) constant and a file-local
is_key_length_valid() helper used as the single source of truth for the rule.
set_value() rejects keys longer than the limit and returns the new
ErrorCode::KeyTooLong.
parse_json_data() treats an over-length key found while loading as an invalid
or corrupted file: it halts loading and returns ErrorCode::KeyTooLong rather
than silently dropping the entry.
Add the KeyTooLong error code and its message.

Tests

set_value accepts a key of exactly 32 bytes and rejects a 33-byte key with
KeyTooLong.
parse_json_data returns KeyTooLong when a loaded object contains an
over-length key.
MessageFor covers the new error code.

Notes

The limit is byte-based; std::string_view::length() counts bytes, matching
the requirement.
Rust side is not covered by this PR.

github-actions · 2026-06-22T12:18:42Z

License Check Results

🚀 The license check job ran with the Bazel command:

bazel run --lockfile_mode=error //:license-check

Status: ⚠️ Needs Review

Click to expand output

[License Check Output]
Starting local Bazel server (8.7.0) and connecting to it...
INFO: Invocation ID: 6a49ac2e-8c14-4e2b-959e-f702fc369619
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
Loading: 
Loading: 2 packages loaded
Loading: 2 packages loaded
    currently loading: 
Loading: 2 packages loaded
    currently loading: 
Loading: 2 packages loaded
    currently loading: 
Loading: 2 packages loaded
    currently loading: 
Loading: 2 packages loaded
    currently loading: 
Analyzing: target //:license-check (3 packages loaded, 0 targets configured)
Analyzing: target //:license-check (3 packages loaded, 0 targets configured)

Analyzing: target //:license-check (77 packages loaded, 14 targets configured)

Analyzing: target //:license-check (92 packages loaded, 14 targets configured)

Analyzing: target //:license-check (147 packages loaded, 814 targets configured)

Analyzing: target //:license-check (161 packages loaded, 4113 targets configured)

Analyzing: target //:license-check (164 packages loaded, 6795 targets configured)

Analyzing: target //:license-check (168 packages loaded, 9844 targets configured)

Analyzing: target //:license-check (168 packages loaded, 9844 targets configured)

Analyzing: target //:license-check (168 packages loaded, 9844 targets configured)

Analyzing: target //:license-check (172 packages loaded, 14061 targets configured)

Analyzing: target //:license-check (173 packages loaded, 15930 targets configured)

INFO: Analyzed target //:license-check (176 packages loaded, 16077 targets configured).
[11 / 16] [Prepa] JavaToolchainCompileClasses external/rules_java+/toolchains/platformclasspath_classes ... (2 actions, 0 running)
INFO: From Generating Dash formatted dependency file ...:
INFO: Successfully converted 50 packages from Cargo.lock to bazel-out/k8-fastbuild/bin/formatted.txt
[14 / 16] [Prepa] JavaToolchainCompileBootClasspath external/rules_java+/toolchains/platformclasspath.jar
[15 / 16] Building license.check.license_check.jar (); 0s disk-cache, multiplex-worker
INFO: Found 1 target...
Target //:license.check.license_check up-to-date:
  bazel-bin/license.check.license_check
  bazel-bin/license.check.license_check.jar
INFO: Elapsed time: 28.693s, Critical Path: 3.14s
INFO: 16 processes: 12 internal, 3 processwrapper-sandbox, 1 worker.
INFO: Build completed successfully, 16 total actions
INFO: Running command line: bazel-bin/license.check.license_check ./formatted.txt <args omitted>
[main] INFO Querying Eclipse Foundation for license data for 42 items.
[main] INFO Found 32 items.
[main] INFO Querying ClearlyDefined for license data for 10 items.
[main] INFO HTTP response 502 (Bad Gateway). Trying again...
[main] INFO Found 10 items.
[main] INFO License information could not be automatically verified for the following content:
[main] INFO 
[main] INFO crate/cratesio/-/lazy_static/1.5.1
[main] INFO 
[main] INFO This content is either not correctly mapped by the system, or requires review.

github-actions · 2026-06-22T12:23:30Z

The created documentation from the pull request is available at: docu-html

arkjedrz

Replace PR description with a human-made one. Current is both inconcise and wrong.

arkjedrz · 2026-07-06T11:59:33Z

                auto sv = element.first.GetAsStringView();
                std::string key(sv.data(), sv.size());

+                /* FEAT_REQ__KVS__maximum_size: keys read from persistent storage are not


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.

Your points have been resolved.

arkjedrz · 2026-07-06T12:03:41Z

+                    logger->LogWarn() << "Skipping key with length " << key.length()
+                                      << " exceeding maximum allowed " << KVS_MAX_KEY_LENGTH_BYTES
+                                      << " bytes while loading";
+                    continue;


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

arkjedrz · 2026-07-06T12:05:18Z


+/* 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)


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.

arkjedrz · 2026-07-06T12:05:39Z

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


Invalid req, like in other places.

anmittag · 2026-07-21T11:48:28Z

@atarekra there are some comments, please adapt

atarekra · 2026-07-21T20:46:26Z

@atarekra there are some comments, please adapt

if you mean the points mentioned by @arkjedrz , it has been resolved and updated the PR 1 week ago

arkjedrz

Logging to be confirmed, otherwise LGTM.

arkjedrz · 2026-07-22T11:49:18Z

+                   loading with an error rather than silently dropping data. */
+                if (!is_key_length_valid(key))
+                {
+                    logger->LogError() << "Key length " << key.length() << " exceeds maximum allowed "


Please check for whitespaces - IIRC spaces are already inserted between streamed fields (e.g., "Key length " << key.length() will show Key length 1234 instead of expected Key length 1234).

Ok working on it

arkjedrz · 2026-07-22T11:50:55Z

+    /* 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 "


Same here - check whitespace behavior.

github-actions · 2026-08-27T01:23:28Z

Thanks for the pull request. This thread has been quiet for 30 days, so we are marking it as stale for now. Please take a quick look and let us know whether it is still up to date, still relevant, needs review, or is ready to merge. Any new activity will remove the stale label automatically. If nothing changes in the next 10 days, we will close it to keep the backlog current.

Ahmed-Elsaka-JC · 2026-09-18T13:15:04Z

@@ -432,6 +457,14 @@ score::Result<bool> Kvs::is_value_default(const std::string_view key) const
 /* Set the value for a key*/
 score::ResultBlank Kvs::set_value(const std::string_view key, const KvsValue& value)
 {


why did not you validate also in get_value function ?

set_value is where the limit is actually enforced , an over-length key can never get into the map, so lookups fail anyway. But the error is misleading: you get KeyNotFound when the real problem is an invalid key.

Ahmed-Elsaka-JC · 2026-09-18T13:19:52Z

+                {
+                    logger->LogError() << "Key length" << key.length() << "exceeds maximum allowed"
+                                       << KVS_MAX_KEY_LENGTH_BYTES << "bytes while loading";
+                    result = score::MakeUnexpected(ErrorCode::KeyTooLong);


i would suggest her to return ErrorCode::KvsFileReadError with logging key string and key length

Agreed on logging the key that's missing and should be there.

On the code: KvsFileReadError means the file couldn't be read, but here it read and parsed fine and the checksum was valid , it's the content that breaks a rule. ValidationFailed is what we already return for a bad checksum, so it may fit better, and it keeps KeyTooLong (new in this PR) meaning purely "the key you passed is invalid". What do you think?

Ahmed-Elsaka-JC

LGFM

Implement comp_req__kvs__key_length. Keys longer than KVS_MAX_KEY_LENGTH_BYTES (32) are rejected with the new ErrorCode::KeyTooLong: - set_value() refuses to store them. - get_value() reports KeyTooLong rather than KeyNotFound, since such a key can never have been stored. - Loading a file that contains one halts with KeyTooLong rather than silently dropping the entry. All three log the offending key and its length. The rule lives in one helper, is_key_length_valid(), shared by every path that reads a key. Signed-off-by: atarekra <ahmed.tarek-ramadan@valeo.com>

atarekra requested review from PandaeDo, antonkri, umaucher and vinodreddy-g as code owners June 22, 2026 12:17

arkjedrz requested changes Jul 6, 2026

View reviewed changes

atarekra temporarily deployed to workflow-approval July 14, 2026 13:45 — with GitHub Actions Inactive

atarekra deployed to workflow-approval July 14, 2026 16:00 — with GitHub Actions Active

anmittag assigned atarekra Jul 21, 2026

anmittag added this to PER - Persistency FT Jul 21, 2026

github-project-automation Bot moved this to Backlog in PER - Persistency FT Jul 21, 2026

arkjedrz requested changes Jul 22, 2026

View reviewed changes

github-actions Bot added the stale label Aug 27, 2026

atarekra had a problem deploying to workflow-approval August 30, 2026 08:19 — with GitHub Actions Failure

atarekra deployed to workflow-approval August 30, 2026 08:48 — with GitHub Actions Active

github-actions Bot removed the stale label Aug 31, 2026

atarekra mentioned this pull request Aug 31, 2026

Log messages in kvs.cpp have doubled spaces #402

Closed

2 tasks

arkjedrz previously approved these changes Sep 1, 2026

View reviewed changes

Ahmed-Elsaka-JC reviewed Sep 18, 2026

View reviewed changes

atarekra dismissed arkjedrz’s stale review via 818b207 October 4, 2026 14:43

atarekra deployed to workflow-approval October 4, 2026 14:43 — with GitHub Actions Active

arkjedrz previously approved these changes Oct 5, 2026

View reviewed changes

Ahmed-Elsaka-JC approved these changes Oct 7, 2026

View reviewed changes

Ahmed-Elsaka-JC self-requested a review October 7, 2026 08:19

Ahmed-Elsaka-JC approved these changes Oct 7, 2026

View reviewed changes

atarekra dismissed arkjedrz’s stale review via e4316fe October 12, 2026 09:32

atarekra force-pushed the Max-KeyLength-Feature branch from 818b207 to e4316fe Compare October 12, 2026 09:32

atarekra deployed to workflow-approval October 12, 2026 09:32 — with GitHub Actions Active

arkjedrz previously approved these changes Oct 12, 2026

View reviewed changes

PandaeDo previously approved these changes Oct 12, 2026

View reviewed changes

PandaeDo added this pull request to the merge queue Oct 12, 2026

github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Oct 12, 2026

atarekra dismissed stale reviews from PandaeDo and arkjedrz via 0c329e8 October 12, 2026 10:58

atarekra force-pushed the Max-KeyLength-Feature branch from e4316fe to 0c329e8 Compare October 12, 2026 10:58

atarekra deployed to workflow-approval October 12, 2026 10:58 — with GitHub Actions Active

PandaeDo approved these changes Oct 12, 2026

View reviewed changes

PandaeDo added this pull request to the merge queue Oct 12, 2026

Merged via the queue into eclipse-score:main with commit f272d48 Oct 12, 2026
23 checks passed

github-project-automation Bot moved this from Backlog to Done in PER - Persistency FT Oct 12, 2026

Conversation

atarekra commented Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Tests

Notes

Uh oh!

github-actions Bot commented Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

License Check Results

Uh oh!

github-actions Bot commented Jun 22, 2026

Uh oh!

arkjedrz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anmittag commented Jul 21, 2026

Uh oh!

atarekra commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arkjedrz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Aug 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ahmed-Elsaka-JC left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This branch was successfully deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

atarekra commented Jun 22, 2026 •

edited

Loading

github-actions Bot commented Jun 22, 2026 •

edited

Loading

atarekra commented Jul 21, 2026 •

edited

Loading