feat(streams): Add the possibility to have per stream ids - #3793
Conversation
🛡️ Jit Security Scan Results
✅ No security findings were detected in this PR
Security scan by Jit
|
There was a problem hiding this comment.
Pull request overview
Adds per-stream resume IDs support for XREAD to address #3694, enabling callers to specify different IDs for each stream.
Changes:
- Extends
XReadArgswithIDs []string(per-stream IDs) and documents precedence overID. - Updates
XReadargument construction to append per-stream IDs when provided. - Adds tests for per-stream IDs and for
IDsprecedence overID.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| stream_commands.go | Adds XReadArgs.IDs and updates XRead arg building logic to support per-stream IDs. |
| commands_test.go | Adds test coverage for per-stream IDs and precedence behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Streams []string // list of streams, or streams and ids e.g. stream1 stream2 id1 id2 | ||
| Count int64 | ||
| Block time.Duration | ||
| ID string | ||
| // ID is a single ID applied to every stream in Streams. | ||
| // When reading from multiple streams that require different IDs, | ||
| // use IDs instead so each stream can be resumed from its own last ID. | ||
| ID string | ||
| // IDs is the per-stream list of IDs (one per entry in Streams). | ||
| // When non-empty, IDs takes precedence over ID. | ||
| IDs []string |
There was a problem hiding this comment.
The Streams field comment says it can contain both stream names and IDs, but IDs is documented as "one per entry in Streams" (which only makes sense if Streams contains stream names only). This is contradictory and will confuse callers, especially now that there are multiple supported input shapes. Please clarify the docs to explicitly describe the supported encodings (legacy Streams containing keys+ids vs Streams containing only keys when using ID/IDs) and how they interact/are mutually exclusive.
| switch { | ||
| case len(a.IDs) > 0: | ||
| for _, id := range a.IDs { | ||
| args = append(args, id) | ||
| } | ||
| case a.ID != "": |
There was a problem hiding this comment.
When IDs is non-empty, the code appends all IDs without checking that the count matches the number of stream keys being read. If these lengths differ, Redis will return a generic arity error that can be hard to diagnose. Consider validating len(a.IDs) == len(a.Streams) when IDs is used and returning a client-side error (e.g., via cmd.SetErr) with a clear message.
- ID doc claimed the inline form is auto-detected; it must be left empty when Streams carries ids inline, or the command is malformed - add wire-format unit tests for keys+IDs, firstKeyPos, and the length-mismatch error path - set Block: -1 in new integration tests so a regression fails fast instead of blocking forever
# Conflicts: # stream_commands_unit_test.go
fixes #3694
Note
Low Risk
Additive, backward-compatible API on stream command building with validation; no auth or data-model changes.
Overview
Adds
XReadArgs.IDssoXReadcan resume each stream from a different ID whileStreamslists only stream keys (fixes #3694). WhenIDsis set it is appended to the Redis command and overrides the existing singleIDfield; the inlineStreamsshape (keys plus IDs in one slice) is unchanged.XReadnow returns a client error without sending a command ifIDsis non-empty but its length does not matchStreams. Docs onXReadArgsdescribe the two supported argument shapes.Coverage includes integration specs for per-stream reads, precedence over
ID, and the mismatch error, plus unit tests for argument assembly andfirstKeyPos.Reviewed by Cursor Bugbot for commit 7d9af19. Bugbot is set up for automated code reviews on this repo. Configure here.