Repository navigation
[fix] Raise on missing keys in kv_batch_get and reject duplicate keys before batch writes - #196
Conversation
The controller drops missing keys from the retrieved meta, so a partial miss used to return fewer rows than keys, with nothing telling the caller which rows were dropped. kv_batch_get (and the _by_meta variants, which call it) now raises ValueError naming the missing keys. The check used to apply only when parser_args was given; it now covers every get. Signed-off-by: 0oshowero0 <o0shower0o@outlook.com> Co-authored-by: Cursor <cursoragent@cursor.com>
…iting KVBatchMeta rejects duplicate keys, but only when it is built for the return value, after the data was already written. A kv_batch_put with duplicate keys stored the data and then raised; a kv_batch_update applied the merge and then raised. Both now raise ValueError naming the duplicates before any request is sent. Signed-off-by: 0oshowero0 <o0shower0o@outlook.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47d9cba91c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
|
|
||
| def _check_unique_keys(keys: list[str]) -> None: | ||
| # KVBatchMeta rejects duplicates only after the write has landed, so reject them before any RPC. |
There was a problem hiding this comment.
This explanation is a single # line, while the root repository rule requires explanatory comments to be 2–4 lines. Rewrite the rationale across the required span so this new helper meets the hard comment standard.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
| num_keys = 1 if isinstance(keys, str) else len(keys) | ||
| if parser_args is not None and batch_meta.size != num_keys: | ||
| raise ValueError(f"only {batch_meta.size} of {num_keys} keys were found; parser_args needs every key") | ||
| # The controller drops missing keys from the meta, so the rows would no longer line up with keys. |
There was a problem hiding this comment.
This new rationale is also only one # line, contrary to the root repository rule that explanatory comments occupy 2–4 lines. Split the controller/alignment explanation into the required span before landing it.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
Problem
Two key-validation gaps in the batch KV APIs, both reproducible on
main:kv_batch_getsilently drops missing keys. The controller leaves keys it cannot find out of the retrieved meta, andasync_kv_batch_getonly raised when every key was missing. Asking for["a", "missing", "b"]returned 2 rows without an error, so the rows no longer lined up with the requested keys, and nothing told the caller which key was dropped. [feat] Supportdata_parserinkv_batch_getfor SimpleStorage #191 added a check for this, but only whenparser_argsis given.KVBatchMetarejects duplicate keys, but only when it is built for the return value, after the write:kv_batch_put(keys=["d", "d"], ...)raisedValueError: Got duplicated keys., butdhad already been stored.kv_batch_update(keys=["a", "a"], ...)raised the same error after the merge had been applied:awent from1to21, the result of the second entry's merge.Change
kv_batch_get, and the_by_metavariants that call it, now raiseValueErrorwhenever some keys are missing, and name them:1 of 3 keys were not found in partition 'p': ['missing']. The missing keys are looked up withkv_retrieve_keys, on this error path only. This replaces theparser_args-only check from [feat] Supportdata_parserinkv_batch_getfor SimpleStorage #191.kv_batch_put/kv_batch_updatereject duplicate keys before any request is sent:keys must be unique, got duplicates: ['d']. The sync variants call the async ones, so both are covered. Docstrings are updated.This changes behavior for callers that relied on a partial get returning fewer rows;
transfer_queue/,tutorial/andrecipe/have no such caller. Each fix is a separate commit.Tests
tests/e2e/test_kv_interface_e2e.py, sync and async:test_kv_batch_get_raises_when_some_keys_are_missingtest_kv_batch_put_rejects_duplicate_keys_before_writing, which also checks that the key is absent fromkv_listtest_batch_update_rejects_duplicate_keys_before_merging, which also checks that the stored value is unchangeddata_parserinkv_batch_getfor SimpleStorage #191 parser test now expects the new missing-key message.python -m compileall -q transfer_queue tutorial tests,ruff checkandruff format --checkpass;mypy transfer_queuereports no new errors compared withmain.python -m pytest -q: 834 passed, 12 skipped. The 8 errors are intests/test_yuanrong_storage_client_e2e.pyand are environment-only (datasystemis not installed).tutorial/*.pyscripts run cleanly, and so does every code cell oftutorial/basic.ipynb, executed in order.Found while reviewing #191.