kvs: C++ key handling changes by arkjedrz · Pull Request #508 · eclipse-score/persistency

arkjedrz · 2026-10-14T11:13:39Z

Add InvalidKey error, replacing KeyTooLong.
Allow only ASCII, UTF-8 compatible keys.
Allow only printable ASCII characters.
Allow only non-empty.
Update tests.

Relates to #504

github-actions · 2026-10-14T11:15:09Z

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: af146d5f-d42e-4537-b73b-f0a83bd4764f
Computing main repo mapping: 
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: 
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 (41 packages loaded, 14 targets configured)

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

Analyzing: target //:license-check (139 packages loaded, 677 targets configured)

Analyzing: target //:license-check (155 packages loaded, 3621 targets configured)

Analyzing: target //:license-check (155 packages loaded, 6420 targets configured)

Analyzing: target //:license-check (156 packages loaded, 8959 targets configured)

Analyzing: target //:license-check (159 packages loaded, 9656 targets configured)

Analyzing: target //:license-check (162 packages loaded, 9666 targets configured)

Analyzing: target //:license-check (162 packages loaded, 9666 targets configured)

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

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

Analyzing: target //:license-check (166 packages loaded, 15647 targets configured)

Analyzing: target //:license-check (167 packages loaded, 15771 targets configured)

Analyzing: target //:license-check (167 packages loaded, 15771 targets configured)

INFO: Analyzed target //:license-check (168 packages loaded, 15897 targets configured).
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
[14 / 16] JavaToolchainCompileBootClasspath external/rules_java+/toolchains/platformclasspath.jar; 1s 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: 32.228s, Critical Path: 2.72s
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 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-10-14T11:20:47Z

Documentation preview for this pull request is available at:
pr-508: https://eclipse-score.github.io/persistency/pr-508/

umaucher · 2026-10-14T12:26:55Z

   :tags: inspected

-   The component shall encode each key as valid UTF-8.
+   The component shall encode each key as valid ASCII, compatible with UTF-8.


IMHO this contradicts to comp_req__kvs__key_naming (see line 30 ff of this file) it states keys that consist solely of alphanumeric characters, underscores, or dashes even not sure if alphanumeric is a exact statement ... still why should we limit us further then the json RFC it would allow keys like café_1 or 用户编号 which IMHO is fine? There is no restriction on the RFC on keys in the requirements of https://github.com/eclipse-score/baselibs/blob/main/score/json/docs/vajson/requirements/index.rst or in the generic json requirements? Also IMHO questionable why we limit us to dashes and underscores .. any technical reason?

Okay, I went the other way - considering std::string an ASCII-encoded type, with an additional limitation being character subset.
Since std::string is encoding-agnostic then I can restore UTF-8 and simply expand unit tests to ensure it's working.
I don't really see the point of limiting the allowed characters either, but it's not always obvious which requirement is discussed and accepted, and which requirement is a candidate for a rewrite.

Ahmed-Elsaka-JC · 2026-10-14T23:35:38Z

   :tags: inspected

-   The component shall limit the maximum length of a key to 32 bytes.
+   The component shall disallow empty keys and limit the maximum length of a key to 32 bytes.


Updating components requirements will need to be revisited after merging component requirements pr

arkjedrz · 2026-10-15T11:52:11Z


 Component Requirements
 ----------------------



Key naming - characters should printable ASCII characters.

arkjedrz · 2026-10-15T11:52:28Z

Encoding shall be ASCII, compliant with UTF-8.

- Add `InvalidKey` error, replacing `KeyTooLong`. - Allow only ASCII, UTF-8 compatible keys. - Allow only printable ASCII characters. - Allow only non-empty. - Update tests.

Copilot

🟢 Approval recommended

Validation, error handling, and tests consistently implement the revised requirements, with no unresolved findings.

0 open findings

What changed in this PR

Updates C++ KVS key validation to match revised requirements for non-empty, printable ASCII keys of at most 32 bytes.

Changes:

Replaces KeyTooLong with InvalidKey for validation failures.
Applies validation when setting, retrieving, and loading keys.
Updates requirements and tests, marking Rust rejection checks as expected failures.

File	Description
score/kvs/tests/test_scenarios/cit/supported_datatypes.cpp	Exercises key validation and logs results.
score/kvs/tests/test_cases/tests/test_cit_supported_datatypes.py	Checks accepted and rejected keys.
score/kvs/rust_kvs/tests/test_scenarios/cit/supported_datatypes.rs	Aligns scenario inputs and logging.
score/kvs/kvs.hpp	Documents the updated error.
score/kvs/kvs.cpp	Implements shared key validation.
score/kvs/kvs_test.cpp	Covers invalid keys and boundaries.
score/kvs/kvs_error_test.cpp	Checks the new error message.
score/kvs/error.hpp	Defines `InvalidKey`.
score/kvs/error.cpp	Maps the new error message.
score/kvs/docs/requirements/index.rst	Revises key naming, encoding, and length requirements.

🧠 Review effort: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Ahmed-Elsaka-JC · 2026-10-16T12:56:14Z

 }

 /* Check if a key exists*/
 score::Result<bool> Kvs::key_exists(const std::string_view key)


should we validate the key in the follwoing functions : key_exists, get_default_value, reset_key, is_defaut_value and remove_key using is_key_valid ?

for ex :
if i called key_exist("") will returns false while get_value("") will return InvalidKey .

Inconsistency inherited from previous change. I removed validation from get_value, so that key is only validated when it's being set. For other functions it can never happen and KeyNotFound is observed.

Key is validated only when set.

Ahmed-Elsaka-JC

from C++ prespective LGFM.

arkjedrz self-assigned this Oct 14, 2026

arkjedrz requested review from PandaeDo, antonkri, umaucher and vinodreddy-g as code owners October 14, 2026 11:13

arkjedrz had a problem deploying to workflow-approval October 14, 2026 11:13 — with GitHub Actions Error

arkjedrz force-pushed the arkjedrz_keys-handling-change branch from 97444b6 to 2191a8c Compare October 14, 2026 11:31

arkjedrz deployed to workflow-approval October 14, 2026 11:31 — with GitHub Actions Active

arkjedrz mentioned this pull request Oct 14, 2026

kvs: Rust key handling changes #509

Merged

umaucher reviewed Oct 14, 2026

View reviewed changes

Ahmed-Elsaka-JC reviewed Oct 14, 2026

View reviewed changes

arkjedrz force-pushed the arkjedrz_keys-handling-change branch from 2191a8c to 4af468d Compare October 15, 2026 09:12

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

arkjedrz requested review from Ahmed-Elsaka-JC and umaucher October 15, 2026 09:16

arkjedrz commented Oct 15, 2026

View reviewed changes

Comment thread score/kvs/docs/requirements/index.rst Outdated

arkjedrz Oct 15, 2026

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.

Encoding shall be ASCII, compliant with UTF-8.

arkjedrz force-pushed the arkjedrz_keys-handling-change branch from 4af468d to 07dcf3a Compare October 15, 2026 13:42

arkjedrz deployed to workflow-approval October 15, 2026 13:42 — with GitHub Actions Active

umaucher linked an issue Oct 15, 2026 that may be closed by this pull request

Only accept keys that consist of printable ASCII characters #504

Closed

umaucher removed a link to an issue Oct 15, 2026

Only accept keys that consist of printable ASCII characters #504

Closed

kvs: C++ key handling changes

a42e46a

- Add `InvalidKey` error, replacing `KeyTooLong`. - Allow only ASCII, UTF-8 compatible keys. - Allow only printable ASCII characters. - Allow only non-empty. - Update tests.

arkjedrz force-pushed the arkjedrz_keys-handling-change branch from 07dcf3a to a42e46a Compare October 16, 2026 06:01

arkjedrz deployed to workflow-approval October 16, 2026 06:01 — with GitHub Actions Active

umaucher requested a balanced review from Copilot October 16, 2026 06:39

Copilot started reviewing on behalf of umaucher October 16, 2026 06:39

View session

Copilot AI reviewed Oct 16, 2026

View reviewed changes

Ahmed-Elsaka-JC suggested changes Oct 16, 2026

View reviewed changes

kvs: remove key validation from get_value

5e14cf6

Key is validated only when set.

arkjedrz deployed to workflow-approval October 16, 2026 13:37 — with GitHub Actions Active

arkjedrz requested a review from Ahmed-Elsaka-JC October 16, 2026 13:39

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

View reviewed changes

umaucher approved these changes Oct 16, 2026

View reviewed changes

umaucher added this pull request to the merge queue Oct 16, 2026

Merged via the queue into eclipse-score:main with commit 272adbc Oct 16, 2026
19 checks passed

Conversation

arkjedrz commented Oct 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Oct 14, 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 Oct 14, 2026

Uh oh!

umaucher Oct 14, 2026

Choose a reason for hiding this comment

Uh oh!

arkjedrz Oct 14, 2026

Choose a reason for hiding this comment

Uh oh!

Ahmed-Elsaka-JC Oct 14, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

arkjedrz Oct 15, 2026

Choose a reason for hiding this comment

Uh oh!

arkjedrz Oct 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Ahmed-Elsaka-JC Oct 16, 2026

Choose a reason for hiding this comment

Uh oh!

arkjedrz Oct 16, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Ahmed-Elsaka-JC left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

4 participants

arkjedrz commented Oct 14, 2026 •

edited

Loading

github-actions Bot commented Oct 14, 2026 •

edited

Loading

Ahmed-Elsaka-JC left a comment •

edited

Loading