Added Type Command - #46
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds STREAM support and the TYPE command: new StreamId and STREAM RedisValue, implementations for TYPE, XADD, XRANGE, XREAD, service registrations, docs for TYPE/XADD/XRANGE/XREAD, and unit + integration tests for TYPE and stream commands. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/main/java/com/redis/commands/generic/TypeCommand.java`:
- Around line 10-15: The Javadoc on TypeCommand incorrectly lists "stream" as a
possible return type; update the TypeCommand class Javadoc to match the actual
returned types (string, list, set, zset, hash, none) or implement STREAM support
if intended. Locate the TypeCommand class and either remove "stream" from the
comment block above the class/TYPE handler or add handling for STREAM in the
type-resolution logic (the method that maps keys to type strings) so the
implementation and Javadoc stay consistent.
🧹 Nitpick comments (1)
src/test/java/com/redis/commands/generic/TypeCommandTest.java (1)
28-33: Consider cleaning test keys to keep DB state isolated.Since
RedisDatabaseis a singleton, leftover keys can bleed into other tests. Clearing the keys used in this class in@BeforeEach(or@AfterEach) keeps the suite more deterministic.🧹 Suggested cleanup
`@BeforeEach` void setUp() { command = new TypeCommand(); mockCtx = mock(ChannelHandlerContext.class); db = RedisDatabase.getInstance(); + db.remove("nonexistent"); + db.remove("key_str"); + db.remove("key_list"); + db.remove("key_set"); + db.remove("key_hash"); + db.remove("key_zset"); }
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/main/java/com/redis/commands/stream/XReadCommand.java`:
- Around line 94-100: The XReadCommand currently skips non-stream keys when
iterating results from db.getValue(key); instead, detect when value != null &&
value.getType() != RedisValue.Type.STREAM and return the Redis WRONGTYPE error
to the client rather than continuing; update the XReadCommand handling to raise
or send the appropriate WRONGTYPE response (using the command/error handling
mechanism your project uses) as soon as a non-stream key is encountered so
behavior matches Redis semantics.
- Around line 109-114: In XReadCommand, do not silently ignore malformed stream
IDs: when calling StreamId.parse(idArg) (the try/catch around lastId =
StreamId.parse(idArg)), catch IllegalArgumentException and instead return or
send a protocol error to the client indicating an invalid stream ID; update the
error path to use the command's response mechanism (rather than continue) so
callers receive an error for the invalid idArg.
🧹 Nitpick comments (5)
src/main/java/com/redis/commands/stream/XReadCommand.java (1)
155-170: Consider checking if the channel is still active before continuing to poll.If the client disconnects while a blocking read is in progress, the scheduled polling continues until the deadline (potentially indefinitely with
BLOCK 0). This wastes resources.Proposed fix
private void schedulePolling(ChannelHandlerContext ctx, List<String> keys, List<String> ids, int count, long deadline, RedisDatabase db) { ctx.executor().schedule(() -> { + if (!ctx.channel().isActive()) { + return; + } if (System.currentTimeMillis() >= deadline) { writeResponse(ctx, RESP_NIL_ARRAY); return; }README.md (1)
52-57: Add blank line before the table to satisfy Markdown lint rules.The table should be surrounded by blank lines per MD058. Add a blank line between the section heading and the table.
Proposed fix
### 🌊 Stream Operations + | Command | Usage | Documentation | |:---|:---|:---| | `XADD` | `XADD key ID field value [field v ...]` | [XADD.md](./docs/commands/XADD.md) | | `XRANGE` | `XRANGE key start end [COUNT c]` | [XRANGE.md](./docs/commands/XRANGE.md) | | `XREAD` | `XREAD [COUNT c] [BLOCK ms] STREAMS k [k ...] id [id ...]` | [XREAD.md](./docs/commands/XREAD.md) |src/test/java/com/redis/commands/stream/XAddCommandTest.java (1)
25-30: Close AutoCloseable returned byopenMocksto prevent resource leaks.
MockitoAnnotations.openMocks(this)returns anAutoCloseablethat should be closed after tests complete.Proposed fix using `@AfterEach`
+import org.junit.jupiter.api.AfterEach; + class XAddCommandTest { `@Mock` private ChannelHandlerContext ctx; private XAddCommand command; private RedisDatabase db; + private AutoCloseable mocks; `@BeforeEach` void setUp() { - MockitoAnnotations.openMocks(this); + mocks = MockitoAnnotations.openMocks(this); command = new XAddCommand(); db = RedisDatabase.getInstance(); } + + `@AfterEach` + void tearDown() throws Exception { + mocks.close(); + }src/test/java/com/redis/commands/stream/XRangeCommandTest.java (1)
23-29: Close AutoCloseable returned byopenMocksto prevent resource leaks.Same issue as XAddCommandTest - the
AutoCloseablereturned byopenMocksshould be closed.Proposed fix
+import org.junit.jupiter.api.AfterEach; + class XRangeCommandTest { `@Mock` private ChannelHandlerContext ctx; private XRangeCommand command; private XAddCommand addCommand; private RedisDatabase db; + private AutoCloseable mocks; `@BeforeEach` void setUp() { - MockitoAnnotations.openMocks(this); + mocks = MockitoAnnotations.openMocks(this); command = new XRangeCommand(); addCommand = new XAddCommand(); db = RedisDatabase.getInstance(); } + + `@AfterEach` + void tearDown() throws Exception { + mocks.close(); + }src/test/java/com/redis/commands/stream/XReadCommandTest.java (1)
23-29: Close AutoCloseable returned byopenMocksto prevent resource leaks.Same issue as other test files - close the
AutoCloseablereturned byopenMocks.Proposed fix
+import org.junit.jupiter.api.AfterEach; + class XReadCommandTest { `@Mock` private ChannelHandlerContext ctx; private XReadCommand command; private XAddCommand addCommand; private RedisDatabase db; + private AutoCloseable mocks; `@BeforeEach` void setUp() { - MockitoAnnotations.openMocks(this); + mocks = MockitoAnnotations.openMocks(this); command = new XReadCommand(); addCommand = new XAddCommand(); db = RedisDatabase.getInstance(); } + + `@AfterEach` + void tearDown() throws Exception { + mocks.close(); + }
| RedisValue value = db.getValue(key); | ||
| if (value == null) continue; | ||
| if (value.getType() != RedisValue.Type.STREAM) { | ||
| // In Redis, if one key is not a stream, it might error or skip. | ||
| // Usually it errors. | ||
| continue; | ||
| } |
There was a problem hiding this comment.
WRONGTYPE keys should return an error, not be silently skipped.
Redis returns a WRONGTYPE error if any of the specified keys holds a value that is not a stream. The current implementation silently skips non-stream keys, which deviates from Redis behavior and could mask client bugs.
Proposed fix
RedisValue value = db.getValue(key);
if (value == null) continue;
if (value.getType() != RedisValue.Type.STREAM) {
- // In Redis, if one key is not a stream, it might error or skip.
- // Usually it errors.
- continue;
+ return ERR_WRONG_TYPE;
}🤖 Prompt for AI Agents
In `@src/main/java/com/redis/commands/stream/XReadCommand.java` around lines 94 -
100, The XReadCommand currently skips non-stream keys when iterating results
from db.getValue(key); instead, detect when value != null && value.getType() !=
RedisValue.Type.STREAM and return the Redis WRONGTYPE error to the client rather
than continuing; update the XReadCommand handling to raise or send the
appropriate WRONGTYPE response (using the command/error handling mechanism your
project uses) as soon as a non-stream key is encountered so behavior matches
Redis semantics.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
Added Type Command
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.