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]
Extracting Bazel installation...
Starting local Bazel server (8.6.0) and connecting to it...
INFO: Invocation ID: d7b06132-ed2d-40f7-b2b5-917887817d9b
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
Loading: 
Loading: 3 packages loaded
Analyzing: target //:license-check (4 packages loaded, 0 targets configured)
Analyzing: target //:license-check (4 packages loaded, 0 targets configured)

Analyzing: target //:license-check (71 packages loaded, 10 targets configured)

Analyzing: target //:license-check (90 packages loaded, 10 targets configured)

Analyzing: target //:license-check (142 packages loaded, 953 targets configured)

Analyzing: target //:license-check (149 packages loaded, 3125 targets configured)

Analyzing: target //:license-check (149 packages loaded, 3125 targets configured)

Analyzing: target //:license-check (149 packages loaded, 3125 targets configured)

Analyzing: target //:license-check (163 packages loaded, 6228 targets configured)

Analyzing: target //:license-check (163 packages loaded, 6228 targets configured)

Analyzing: target //:license-check (165 packages loaded, 6357 targets configured)

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

Analyzing: target //:license-check (176 packages loaded, 11384 targets configured)

INFO: Analyzed target //:license-check (177 packages loaded, 11510 targets configured).
[6 / 16] Creating runfiles tree bazel-out/k8-fastbuild/bin/license.check.license_check.runfiles; 0s local
[13 / 17] JavaToolchainCompileClasses external/rules_java+/toolchains/platformclasspath_classes; 0s disk-cache, processwrapper-sandbox ... (2 actions running)
INFO: From Generating Dash formatted dependency file ...:
INFO: Successfully converted 66 packages from Cargo.lock to bazel-out/k8-fastbuild/bin/formatted.txt
[15 / 17] JavaToolchainCompileBootClasspath external/rules_java+/toolchains/platformclasspath.jar; 0s disk-cache, processwrapper-sandbox
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: 34.448s, Critical Path: 2.75s
INFO: 17 processes: 12 internal, 4 processwrapper-sandbox, 1 worker.
INFO: Build completed successfully, 17 total actions
INFO: Running command line: bazel-bin/license.check.license_check ./formatted.txt <args omitted>
usage: org.eclipse.dash.licenses.cli.Main [-batch <int>] [-cd <url>]
       [-confidence <int>] [-ef <url>] [-excludeSources <sources>] [-help] [-lic
       <url>] [-project <shortname>] [-repo <url>] [-review] [-summary <file>]
       [-timeout <seconds>] [-token <token>]

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.

Implementing Key max size feature

51e8305

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

Applying PR review points

732cdee

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

Merge branch 'main' into Max-KeyLength-Feature

dca7444

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

Merge branch 'main' into Max-KeyLength-Feature

1a11077

atarekra requested a deployment to workflow-approval August 30, 2026 08:19 — with GitHub Actions Waiting

Solving PR review points

998fab5

atarekra requested a deployment to workflow-approval August 30, 2026 08:48 — with GitHub Actions Waiting

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

Uh oh!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

atarekra commented Jun 22, 2026 •

edited

Loading

github-actions Bot commented Jun 22, 2026 •

edited

Loading

atarekra commented Jul 21, 2026 •

edited

Loading