feat: Add command metadata and update tests by bravo1goingdark · Pull Request #78 · valkey-io/valkey-bloom

bravo1goingdark · 2025-11-23T06:10:53Z

Add `#[valkey_command]` Metadata & KeySpecs for Bloom Filter Commands

This PR adds the #[valkey_command] macro to all Bloom Filter commands in src/lib.rs, enabling proper SetCommandInfo support through the valkeymodule-rs crate.
The macro now provides Valkey with complete metadata, including:

Command name
Flags
Arity
KeySpec definitions (key position, read/write semantics, key ranges)

As a result, COMMAND INFO BF.* now correctly reports the KeySpec metadata for each Bloom Filter command.

Changes Included

Added `#[valkey_command]` to all Bloom Filter commands:

BF.ADD
BF.MADD
BF.EXISTS
BF.MEXISTS
BF.CARD
BF.RESERVE
BF.INFO
BF.INSERT
BF.LOAD

Implemented complete KeySpecs for each command:

begin_search
find_keys
Key flags (rw, insert, update, access)

Updated tests in `tests/test_bloom_command.py`:

Adjusted expected arities
Added KeySpec validation checks

Minor cleanup:

Improved string formatting in:
- src/bloom/data_type.rs
- src/bloom/utils.rs

Notes / Limitations

The current version of valkeymodule-rs does not support argument metadata (args:) in SetCommandInfo.
Because of this limitation, this PR implements all supported metadata (KeySpecs), but cannot yet add full CLI argument autocomplete like the built-in SET command.

Once args support is added upstream in valkeymodule-rs, I can follow up with another PR to provide complete argument specifications for full autocomplete.

Verification

Example output of COMMAND INFO BF.ADD, showing that KeySpec metadata is now present:

KarthikSubbarao · 2025-11-26T06:06:25Z

        "bloom",
    ]
-    commands: [
-        ["BF.ADD", bloom_add_command, "write fast deny-oom", 1, 1, 1, "fast write bloom"],


Does this remove the bloom and other ACL categories from all the commands? Can you run the integration tests to validate if this passes?

You can check the valkey_module! macro to see how the command creation works in there. Specifically, how the ACL category is set through it

This commit adds the `valkey_command` macro to all the bloom filter commands in `src/lib.rs`. This macro provides metadata about the commands, such as their arity, flags, and key specifications. This is important for Valkey to properly handle the commands. The tests in `tests/test_bloom_command.py` have been updated to reflect the new command arities and to add a new test that verifies that the key specifications are present for the commands that have them. Finally, some string formatting in `src/bloom/data_type.rs` and `src/bloom/utils.rs` has been updated to use the more modern and efficient f-string style formatting. Signed-off-by: Ashutosh Kumar <kumarashutosh34169@gmail.com>

Signed-off-by: Ashutosh Kumar <kumarashutosh34169@gmail.com>

…mmand] Signed-off-by: bravo1goingdark <kumarashutosh34169@gmail.com>

Signed-off-by: bravo1goingdark <kumarashutosh34169@gmail.com>

bravo1goingdark · 2026-04-28T20:34:41Z

Added per-command ACL categories in initialize( ) since #[valkey_command] doesn't set them. All 98 integration tests pass

@KarthikSubbarao

bravo1goingdark · 2026-09-21T15:37:31Z

should i close this pr ?

KarthikSubbarao · 2026-09-21T16:05:39Z

We will take a look at the latest PR here.....

@zackcam - Do you have some time this week to help review this?

zackcam · 2026-09-21T16:53:59Z

We will take a look at the latest PR here.....

@zackcam - Do you have some time this week to help review this?

I should be able to take a look this week at it. I'll try and make time to look at it

zackcam · 2026-09-22T22:11:40Z

+    arity: 3,
+    key_spec: [{
+        begin_search: Index({ index: 1 }),
+        find_keys: Range({ last_key: 1, steps: 1, limit: 0 }),


Maybe I'm reading wrong: https://valkey.io/topics/key-specs/ but should these have last_key as 0? so like 0, 1, 0 would be the same for all I think?

I was looking at:

The SET command has a range of 0, 1 and 0.

Which I think would be the same sort as we do?

You were reading it right: lastkey is relative to begin_search, so 0 means only the first key. Updated all 9 BF.* commands to Range({ last_key: 0, steps: 1, limit: 0 }).

This also fixes the legacy values derived from it — COMMAND INFO now reports first/last/step as 1, 1, 1 instead of 1, 2, 1 (previously the value argument was reported as a key). The test now asserts both the key spec range and those legacy positions.

zackcam · 2026-09-22T22:14:18Z

+        self.verify_command_arity('BF.INFO', -2)
+        self.verify_command_arity('BF.INSERT', -2)
+
+    def test_bloom_command_keyspecs_present(self):


For this do we want to check that keyspecs are correct not just present?

Good call — replaced test_bloom_command_keyspecs_present with test_bloom_command_keyspecs, which now checks the actual values for all 9 commands instead of presence for 4:

key spec flags (RW/insert for writes, RO/access for reads)

begin_search type index with index 1

find_keys type range with 0, 1, 0

legacy first_key_pos/last_key_pos/step_count = 1/1/1

Verified the assertions actually bite: with last_key back at 1 the test fails on last_key_pos.

zackcam · 2026-09-22T22:21:20Z

 /// Command handler for BF.ADD <key> <item>
+#[valkey_command({
+    name: "BF.ADD",
+    summary: "Add a single item to a bloom filter; creates the filter if it does not exist",


I think we want these to be the same as commands json files. If those aren't punctually correct I'm fine with us changing those to match these. (This one is same idea just different wording on it)

Aligned the three that differed: BF.ADD, BF.MADD and BF.LOAD summaries now use the exact wording from src/commands/*.json (the other six already matched).

While diffing the two, the JSON arity for BF.MADD/BF.MEXISTS said 3 but both are variadic, so I changed those to -3 to match the macro and what the server reports. Happy to split that into a separate change if you'd rather keep the JSON edits out of this PR.

zackcam

Just two actual comments really, testing one is a nit. Once these are done I'll do another review and if it looks good I'll approve!

Thanks for working on this!

The find_keys range for all BF.* commands now uses last_key 0 instead of 1, so the key spec describes only the single <key> argument instead of also treating the value argument as a key. This matches the range of 0, 1, 0 that SET uses as documented in the key specs documentation and fixes the legacy first/last key/step values reported by COMMAND INFO (1, 1, 1 instead of 1, 2, 1). Summaries of BF.ADD, BF.MADD and BF.LOAD now match the wording in the command JSON files. The arity of BF.MADD and BF.MEXISTS in those JSON files is corrected from 3 to -3 to match their variadic syntax. test_bloom_command_keyspecs_present is replaced by test_bloom_command_keyspecs, which validates the actual key spec values (flags, begin_search, find_keys range) and the legacy key positions for all 9 commands instead of only checking presence for 4 of them, and an arity assertion for BF.LOAD is added. Signed-off-by: bravo1goingdark <kumarashutosh34169@gmail.com>

bravo1goingdark · 2026-09-24T18:10:08Z

Addressed all three review comments in 42fcf5f:

Key spec range — all 9 BF.* commands now use last_key: 0 (0, 1, 0), matching SET. Legacy COMMAND INFO positions are now 1/1/1 instead of 1/2/1.
Key spec test — now validates flags, begin_search, find_keys range and legacy key positions for all 9 commands (was: presence check for 4). Confirmed it fails if last_key is reverted to 1. Also added the missing BF.LOAD arity assertion.
Summaries — BF.ADD, BF.MADD, BF.LOAD now use the wording from the src/commands/*.json files; the other six already matched. Also corrected arity for BF.MADD/BF.MEXISTS in those JSONs from 3 to -3 since both are variadic.

coderabbitai · 2026-09-24T18:17:28Z

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 8f7180ca-8125-454a-bb97-9860af4eb7a9

📥 Commits

Reviewing files that changed from the base of the PR and between 42fcf5f and 06f3e5e.

📒 Files selected for processing (2)

tests/test_bloom_command.py
tests/test_bloom_replication.py

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Walkthrough

Walkthrough

Bloom command handlers now declare metadata through attributes, with ACL categories assigned after the server-version check. Command arity and key-spec tests reflect the declarations. The change also updates Rust format strings and replication-test accounting assertions.

Changes

Bloom command metadata

Layer / File(s)	Summary
Command declarations and ACL categories `src/lib.rs`, `src/commands/bf.madd.json`, `src/commands/bf.mexists.json`	Nine command handlers declare metadata through `#[valkey_command]` attributes. ACL categories are assigned after the server-version check, and the legacy command list is empty. `BF.MADD` and `BF.MEXISTS` have variadic arities.
Command arity and key-spec tests `tests/test_bloom_command.py`	Tests check command arities and use `COMMAND GETKEYS` sample calls to verify key positions and key-spec flags.

Format string updates

Layer / File(s)	Summary
Inline format argument captures `src/bloom/data_type.rs`, `src/bloom/utils.rs`	The RDB warning and utility test-helper format strings use inline captures. Their output text and assertions remain unchanged.

Replication test accounting

Layer / File(s)	Summary
Invalid write command accounting `tests/test_bloom_replication.py`	The test expects server-rejected calls for `BF.ADD` and `BF.MADD`, and module-failed calls for `BF.RESERVE` and `BF.INSERT`.

Suggested reviewers: zackcam

Change: Feature

Merge Risk: ⚪ Minimal · up to 06f3e

No actionable issue remains that would prevent merging after normal checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 54.17% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 5 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main changes: adding command metadata and updating tests for Bloom filter commands.
Description check	✅ Passed	The description directly explains the added #[valkey_command] metadata, KeySpecs, arity updates, tests, and known limitation.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Fix all pre-merge checks with AI

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

zackcam

Just one comment on the test, I think utilizing the https://valkey.io/commands/command-getkeys/ might be a nicer way of checking

zackcam · 2026-09-24T18:36:42Z

+
+    def test_bloom_command_keyspecs(self):
+        for command, expected_flags in self.EXPECTED_BLOOM_KEYSPECS.items():
+            self.verify_command_keyspec(command, expected_flags)


keys = self.client.execute_command('COMMAND', 'GETKEYS', command, 'k', 'item') assert keys == [b'k'], f"{command} GETKEYS returned {keys}, expected [b'k']" info = self.client.execute_command('COMMAND', 'INFO', command)[command] assert (info['first_key_pos'], info['last_key_pos'], info['step_count']) \ == (1, 1, 1), f"{command} legacy key positions wrong: {info}" specs = info['key_specifications'] assert len(specs) == 1, f"{command} expected 1 key spec, got {len(specs)}" flags = {f.decode() if isinstance(f, bytes) else f for f in specs[0]['flags']} assert flags == expected_flags, \ f"{command} flags {sorted(flags)}, expected {sorted(expected_flags)}"

Maybe something like this would be cleaner for testing. Right now seems quite long of a test.

Done in a9344bf — GETKEYS is the main check now and I dropped the field-by-field key spec assertions, so the test went from ~75 lines to ~30. It still catches the old bug: with last_key at 1, GETKEYS BF.ADD k item returns [k, item].

Two heads-ups from running it: sending GETKEYS as split args (execute_command('COMMAND', 'GETKEYS', ...)) errors because valkey-py runs its COMMAND INFO parser over the reply — one string works, same as the ACL test. And GETKEYS validates arity, so each command needs its own sample args.

Perfect thank you, yeah I didn't actually make the changes locally to test my snippet sorry just wanted to give the general idea but looks good now. Will run the workflows and see about getting it merged

COMMAND GETKEYS is now the primary assertion: it proves the key spec extracts the filter key and nothing else, which covers the begin_search index and the find_keys range without inspecting them field by field. The legacy first/last key/step tuple and the key spec flags are still checked directly. Sample arguments after the key differ per command because GETKEYS validates arity. Verified the test still catches the old bug: with last_key 1, GETKEYS BF.ADD k item returns [k, item] instead of [k]. Signed-off-by: bravo1goingdark <kumarashutosh34169@gmail.com>

KarthikSubbarao · 2026-09-24T19:48:01Z

+            was_rejected = primary_cmd_stats.get("rejected_calls", 0) == 1
+            was_failed = primary_cmd_stats.get("calls", 0) == 1 and primary_cmd_stats.get("failed_calls", 0) == 1
+            assert was_rejected or was_failed


Why did this have to change from the original?

Because this PR gives the commands real arity metadata, so the server now catches the two arity mistakes before the module ever runs.

Before, all four reached the module: calls=1, failed_calls=1. Now BF.ADD key item1 item2 and BF.MADD key are rejected at the server: rejected_calls=1, calls=0. BF.RESERVE (bad error rate) and BF.INSERT (bad capacity) still run and fail inside the module, so they keep the original numbers.

I've replaced the rejected or failed with the exact expectation per command in 06f3e5e, so the reason is visible in the test. Checked on valkey 8.0, 8.1 and unstable — the replication test passes on all three.

The two arity mistakes in this list are now rejected by the server before the module runs, since the commands carry real arity metadata, so they show up as rejected_calls with no calls at all. BF.RESERVE and BF.INSERT still run and fail inside the module, keeping calls 1 and failed_calls 1. Replace "rejected or failed" with the exact expectation per command so the reason for the change is visible in the test itself. Verified against valkey 8.0, 8.1 and unstable. Signed-off-by: bravo1goingdark <kumarashutosh34169@gmail.com>

bravo1goingdark force-pushed the unstable branch from 4b981f0 to 01cb0cd Compare November 23, 2025 06:14

bravo1goingdark marked this pull request as ready for review November 23, 2025 07:03

bravo1goingdark marked this pull request as draft November 23, 2025 07:29

bravo1goingdark marked this pull request as ready for review November 23, 2025 08:07

bravo1goingdark mentioned this pull request Nov 23, 2025

Valkey-bloom doesn't provide command info #71

Open

KarthikSubbarao reviewed Nov 26, 2025

View reviewed changes

bravo1goingdark force-pushed the unstable branch from f6b7b22 to a90bf9e Compare April 28, 2026 20:03

bravo1goingdark added 3 commits April 29, 2026 01:36

Fix key specs for BF commands

4a82afd

Signed-off-by: Ashutosh Kumar <kumarashutosh34169@gmail.com>

Restore per-command ACL categories lost when migrating to #[valkey_co…

e6323d7

…mmand] Signed-off-by: bravo1goingdark <kumarashutosh34169@gmail.com>

bravo1goingdark force-pushed the unstable branch from a90bf9e to e6323d7 Compare April 28, 2026 20:06

bravo1goingdark added 2 commits April 29, 2026 01:49

Fix key_specs -> key_specifications field name in test

8a283ab

Signed-off-by: bravo1goingdark <kumarashutosh34169@gmail.com>

Fix replication test: handle server-level arity rejection stats

2b4fd26

Signed-off-by: bravo1goingdark <kumarashutosh34169@gmail.com>

bravo1goingdark requested a review from KarthikSubbarao April 28, 2026 20:35

zackcam reviewed Sep 22, 2026

View reviewed changes

zackcam reviewed Sep 24, 2026

View reviewed changes

zackcam approved these changes Sep 24, 2026

View reviewed changes

KarthikSubbarao reviewed Sep 24, 2026

View reviewed changes

Conversation

bravo1goingdark commented Nov 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Add #[valkey_command] Metadata & KeySpecs for Bloom Filter Commands

Changes Included

Added #[valkey_command] to all Bloom Filter commands:

Implemented complete KeySpecs for each command:

Updated tests in tests/test_bloom_command.py:

Minor cleanup:

Notes / Limitations

Verification

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bravo1goingdark commented Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bravo1goingdark commented Sep 21, 2026

Uh oh!

KarthikSubbarao commented Sep 21, 2026

Uh oh!

zackcam commented Sep 21, 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!

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!

zackcam 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!

bravo1goingdark commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

zackcam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bravo1goingdark Sep 24, 2026 • 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bravo1goingdark commented Nov 23, 2025 •

edited

Loading

Add `#[valkey_command]` Metadata & KeySpecs for Bloom Filter Commands

Added `#[valkey_command]` to all Bloom Filter commands:

Updated tests in `tests/test_bloom_command.py`:

bravo1goingdark commented Apr 28, 2026 •

edited

Loading

zackcam left a comment •

edited

Loading

bravo1goingdark commented Sep 24, 2026 •

edited

Loading

coderabbitai Bot commented Sep 24, 2026 •

edited

Loading

bravo1goingdark Sep 24, 2026 •

edited

Loading