feat: add more rules for parsing

[BorysTheDev]

PR Summary by Qodo

Add composable validation rules to CmdArgParser and refactor command parsing

✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Add rule-based numeric validation to CmdArgParser with optional custom error messages.
• Refactor multiple commands to use in-parser validation and NextOrDefault over manual checks.
• Expand unit tests to cover rule composition and error precedence.

Diagram

graph TD
  A["Server commands"] --> B["CmdArgParser"] --> C["Convert/Next<T>"] --> D["Validated<T, Rules...>"] --> E["Rules (RuleError)"]
  B --> F["ErrorInfo / replies"]
  G["CmdArgParser tests"] --> D
  E --> F
  subgraph Legend
    direction LR
    _cmd["Command handler"] ~~~ _core["Core parser"] ~~~ _rule["Validation rule"]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Return std::expected from reads

• ➕ Makes error propagation explicit and avoids reliance on a latched parser state
• ➕ Enables callers to handle/transform errors locally without TakeError()/Finalize() patterns
• ➖ Large API churn across all call sites (every Next/Apply usage)
• ➖ More verbose call sites when the existing 'parse everything then finalize' style is desired

2. Use a concept/traits-based validator interface (no NTTP function pointers)

• ➕ Potentially simpler template instantiations and error messages
• ➕ Rules could carry state/config without NTTP tricks
• ➖ More boilerplate to define rule types
• ➖ Harder to reuse simple free-function rules across call sites

Recommendation: The chosen RuleError + Validated approach fits the existing CmdArgParser ‘latch first error’ model while eliminating repetitive post-parse checks across commands. The main alternative worth considering is an expected/status-returning API, but that would be a much larger design shift than this PR’s targeted enhancement.

Files changed (14) +253 / -153

Enhancement (2) +75 / -21

cmd_arg_parser.hIntroduce RuleError/Validated framework and reusable numeric rules +74/-21

Introduce RuleError/Validated framework and reusable numeric rules

• Adds RuleError for validation results and updates the validated-number concept to return RuleError instead of bool. Introduces reusable rules (InRange/Bounded/AtLeast/NotNan/Finite/NotEq) and a Validated<T, Rules...> wrapper; rewires Convert/Next<T>() to emit either generic INVALID_* errors or CUSTOM_ERROR with rule-provided messages, and aliases FInt to an InRange-based Validated.

src/facade/cmd_arg_parser.h

error.hAdd dedicated timeout-negative error string +1/-0

Add dedicated timeout-negative error string

• Introduces kTimeoutNegativeErr for consistent error reporting when timeouts are negative.

src/facade/error.h

Refactor (11) +54 / -128

bitops_family.ccEnforce BITPOS bit arg via Validated and simplify optional args +5/-33

Enforce BITPOS bit arg via Validated and simplify optional args

• Replaces manual (value != 0/1) checks with a Bounded<0,1> Validated read using a custom message. Simplifies optional start/end/unit parsing by using NextOrDefault and MapNext guarded by HasNext, relying on Finalize() for error handling.

src/server/bitops_family.cc

bloom_family.ccUse FInt for bloom cursor bounds +3/-10

Use FInt for bloom cursor bounds

• Replaces manual cursor negativity checks with FInt-based parsing for commands that require cursor >= 0 or >= 1, preserving generic INVALID_INT behavior for malformed inputs.

src/server/bloom_family.cc

cuckoo_filter_family.ccMove CF business-rule bounds into Validated rules +13/-22

Move CF business-rule bounds into Validated rules

• Replaces post-parse zero/expansion checks with Validated rule-based parsing for capacity, bucket size, max iterations, and expansion, producing CF-specific error messages when bounds are violated.

src/server/cuckoo_filter_family.cc

generic_family.ccUse FInt for RESTORE ttl and NextOrDefault for PING message +3/-6

Use FInt for RESTORE ttl and NextOrDefault for PING message

• Converts RESTORE TTL parsing to an FInt range to remove the explicit negative check. Simplifies PING optional message parsing via NextOrDefault while retaining arity validation.

src/server/generic_family.cc

hset_family.ccValidate HINCRBYFLOAT input as finite during parse +1/-5

Validate HINCRBYFLOAT input as finite during parse

• Replaces explicit isnan/isinf guard with a Finite<msg> Validated double read, keeping the same error message behavior via parser-reported CUSTOM_ERROR.

src/server/hset_family.cc

list_family.ccCentralize timeout/count parsing via Validated and NextOrDefault +5/-18

Centralize timeout/count parsing via Validated and NextOrDefault

• Replaces repeated negative timeout checks with AtLeast<0,msg> Validated float reads across blocking list operations. Simplifies POP count handling by deriving return_arr from HasNext and using NextOrDefault for the count value.

src/server/list_family.cc

search_family.ccAdapt search numeric validators to RuleError semantics +13/-12

Adapt search numeric validators to RuleError semantics

• Refactors WEIGHT parsing to a VNum-based validated type compatible with RuleError, preserving call-site-specific error text via Next<T>("..."). Updates existing validated numeric helpers to return RuleError instead of bool.

src/server/search/search_family.cc

set_family.ccSimplify SRANDMEMBER count parsing via NextOrDefault +1/-1

Simplify SRANDMEMBER count parsing via NextOrDefault

• Replaces conditional parsing with a defaulted read for count, while still validating extra arguments separately.

src/server/set_family.cc

string_family.ccPrevent INT64_MIN overflow in DECRBY via Validated rule +1/-5

Prevent INT64_MIN overflow in DECRBY via Validated rule

• Replaces the explicit INT64_MIN guard with a NotEq<INT64_MIN,msg> Validated read so the parser emits the overflow error consistently during parse.

src/server/string_family.cc

topk_family.ccValidate TOPK decay/increment ranges during parsing +6/-9

Validate TOPK decay/increment ranges during parsing

• Moves decay finiteness/range checks and increment bounds into Validated Bounded rules with Redis-compatible messages, removing redundant post-parse conditionals.

src/server/topk_family.cc

zset_family.ccValidate NaN score and simplify ZRANDMEMBER count defaulting +3/-7

Validate NaN score and simplify ZRANDMEMBER count defaulting

• Uses NotNan<msg> Validated parsing to reject NaN scores for ZINCRBY with the existing message, and switches count parsing for ZRANDMEMBER to NextOrDefault.

src/server/zset_family.cc

Tests (1) +124 / -4

cmd_arg_parser_test.ccAdd coverage for Validated rules and migrate existing validator +124/-4

Add coverage for Validated rules and migrate existing validator

• Adds new tests verifying custom vs generic error selection, NaN/inf handling, rule composition order, and rule reuse outside Next<T>(). Updates PositiveFinite and other validated-number examples to use RuleError-returning validate().

src/facade/cmd_arg_parser_test.cc

[augmentcode]

🤖 Augment PR Summary

Summary: This PR extends facade::CmdArgParser with composable validation rules so numeric arguments can be parsed and validated in one step, reducing repeated post-parse checks across command handlers.

Changes:

• Add RuleError and a Validated<T, Rules...> wrapper so Next<T>() can apply reusable validation rules during parsing.
• Introduce reusable rules such as InRange, Bounded, NonNegative, NotNan, Finite, and NotEq; redefine FInt in terms of Validated.
• Adjust Next<T>(msg) behavior so caller messages override only generic numeric type errors, while rule-provided messages pass through unchanged.
• Refactor several command handlers (bitops, bloom, cuckoo, generic, hset, lists, search, set, string, topk, zset) to use in-parser validation and NextOrDefault/NextRange instead of manual post-parse checks.
• Add dedicated timeout error strings and centralize blocking-list timeout parsing via a validated Timeout type.
• Add/expand unit tests for rule composition/error precedence and blocking list timeout validation.

Technical Notes: Parser errors still latch on first failure; empty rule messages keep generic INVALID_INT/INVALID_FLOAT unless the call site replaces them via Next<T>(msg).

_{🤖 Was this summary useful? React with 👍 or 👎}

[augmentcode]

Review completed. 1 suggestion posted.

Comment augment review to trigger a new review at any time.

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Invalid decltype member access 🐞 Bug ≡ Correctness

Description

CmdArgParser::Convert<T>() uses decltype(T::value) to infer the underlying numeric type for
validated numbers, but value is a non-static data member (from VNum), so naming it as T::value is
ill-formed and breaks template instantiation for Validated/FInt parsing. This can prevent
compilation anywhere Next<Validated<...>>() / Next<FInt<...>>() is used.

Code

src/facade/cmd_arg_parser.h[R524-525]

+      using U = decltype(T::value);
  U val = Num<U>(idx);
-      if (!T::validate(val)) {
-        Report(std::is_floating_point_v<U> ? INVALID_FLOAT : INVALID_INT, idx);

Evidence

VNum clearly declares value as a non-static member, while Convert attempts to reference it as
T::value, which is not valid for non-static members and will fail instantiation for
validated-number types.

src/facade/cmd_arg_parser.h[100-106]
src/facade/cmd_arg_parser.h[513-536]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`CmdArgParser::Convert<T>()` tries to compute the underlying numeric type for `as_vnum` types with `using U = decltype(T::value);`, but `value` is a non-static data member inherited from `VNum<T>`. Referencing it as `T::value` is invalid, so `Convert` will fail to compile when instantiated for `Validated<...>`/`FInt<...>`.
### Issue Context
- `VNum` defines `T value` as a non-static member.
- `Convert` needs the member’s type; it should use an object expression like `std::declval<T>().value` (and strip cv/ref), or reintroduce `using underlying_t = T;` in `VNum`.
### Fix Focus Areas
- src/facade/cmd_arg_parser.h[94-106]
- src/facade/cmd_arg_parser.h[513-536]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Non-finite timeout UB ✓ Resolved 🐞 Bug ☼ Reliability

Description

Blocking list commands now validate timeout with AtLeast<0>, which explicitly accepts NaN and does
not reject +inf, and then cast timeout*1000 to unsigned milliseconds. Converting NaN/inf (or other
out-of-range floats) to unsigned is undefined behavior and can lead to incorrect timeouts or runtime
instability.

Code

src/server/list_family.cc[885]

+  float timeout = parser.Next<Validated<float, AtLeast<0.0f, kTimeoutNegativeErr>>>();

Evidence

The AtLeast rule explicitly allows NaN, and the timeout value is later converted to an unsigned
millisecond count. The parser is already tested to accept textual "inf"/"nan" for floating-point
parsing, making the non-finite path reachable from user input.

src/facade/cmd_arg_parser.h[124-127]
src/facade/cmd_arg_parser.h[546-555]
src/facade/cmd_arg_parser_test.cc[523-534]
src/server/list_family.cc[883-893]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`BRPOPLPUSH`/`BLMOVE`/`BLMPOP` parse `timeout` as `Validated<float, AtLeast<0,...>>`. The `AtLeast` rule is documented to accept NaN, and it also accepts +inf; these values can then flow into `unsigned(timeout * 1000)`, which is undefined behavior for NaN/inf and potentially out-of-range values.
### Issue Context
- `CmdArgParser::Num<float>()` parses floats via `absl::SimpleAtof`.
- The new rule set includes `Finite(double)` but it cannot be applied to `float` timeouts as-is.
### Fix Focus Areas
- src/facade/cmd_arg_parser.h[124-136]
- src/server/list_family.cc[883-893]
### Suggested fix direction
- Add a `Finite` rule overload/template that works for `float` (and ideally any `std::floating_point`), OR use `Bounded<0.0f, max, msg>` with an appropriate maximum.
- Update list blocking commands to require `std::isfinite(timeout)` before converting to an integer timeout.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[dranikpg]

looks cool, but probably using some kind of builder pattern (in the type world) would be more readable

[BorysTheDev]

I want it to be short to read easily. Parsing isn't important logic so I want to make it as simple and declarative as much as possible to allow concentration on the important logic. If you see how to improve my approach, I will be happy to do it.

[BorysTheDev]

augment review

[augmentcode]

Review completed. 1 suggestion posted.

Comment augment review to trigger a new review at any time.

[BorysTheDev]

augment review

[augmentcode]

Review completed. 2 suggestions posted.

Comment augment review to trigger a new review at any time.