Skip to content

Refactor parse_cluster_slots and add slot range validation#307

Merged
bjosv merged 2 commits into
valkey-io:mainfrom
bjosv:fix-parse-cluster-slots
Jun 30, 2026
Merged

Refactor parse_cluster_slots and add slot range validation#307
bjosv merged 2 commits into
valkey-io:mainfrom
bjosv:fix-parse-cluster-slots

Conversation

@bjosv

@bjosv bjosv commented Apr 20, 2026

Copy link
Copy Markdown
Collaborator

Restructure parse_cluster_slots to improve readability and add early validation of slot ranges received from the server.

Changes:

  • Extract node entry parsing into parseClusterSlotsNodeEntry helper, eliminating duplicated validation logic for primary and replica nodes.
  • Flatten the deeply nested inner loop: parse slot range from elements 0 and 1 directly, handle the primary at element 2, then iterate replicas starting at element 3.
  • Validate slot ranges immediately (reject start > end and end >= VALKEYCLUSTER_SLOTS) instead of letting invalid values propagate until caught later in updateNodesAndSlotmap.
  • Simplify error messages.
  • Add unit test for invalid slot ranges.

Reject invalid slot ranges (negative, out of bounds, start > end)
immediately when parsing the CLUSTER SLOTS reply, rather than letting
invalid values propagate through slot creation and node linking until
caught later in updateNodesAndSlotmap.

Refactored the function to make the validation straightforward:
extracted node entry parsing into a helper and flattened the deeply
nested inner loop. Added unit test for invalid slot ranges.

Signed-off-by: Björn Svensson <bjorn.a.svensson@est.tech>
@bjosv bjosv force-pushed the fix-parse-cluster-slots branch from 7622109 to 7407adc Compare June 30, 2026 08:51
Signed-off-by: Björn Svensson <bjorn.a.svensson@est.tech>
@bjosv bjosv merged commit d366730 into valkey-io:main Jun 30, 2026
53 checks passed
@bjosv bjosv deleted the fix-parse-cluster-slots branch June 30, 2026 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant