Feature/transactional commands - #48
Conversation
📝 WalkthroughWalkthroughThis PR introduces Redis transaction support via MULTI/EXEC/DISCARD commands, implements the INCR string command, adds per-channel transaction state management, and updates the command handler to queue commands during transactions. Comprehensive documentation and extensive integration tests validate new functionality across transaction isolation, command execution, and error handling. Changes
Sequence DiagramsequenceDiagram
participant Client
participant CommandHandler
participant TransactionContext
participant Database
Client->>CommandHandler: MULTI
CommandHandler->>TransactionContext: getOrCreate(channel)
CommandHandler->>TransactionContext: startTransaction()
CommandHandler-->>Client: +OK
Client->>CommandHandler: SET key1 value1
CommandHandler->>TransactionContext: isInTransaction()
CommandHandler->>TransactionContext: queueCommand(SetCommand, [key1, value1])
CommandHandler-->>Client: +QUEUED
Client->>CommandHandler: INCR counter
CommandHandler->>TransactionContext: isInTransaction()
CommandHandler->>TransactionContext: queueCommand(IncrCommand, [counter])
CommandHandler-->>Client: +QUEUED
Client->>CommandHandler: EXEC
CommandHandler->>TransactionContext: getQueuedCommands()
loop For each queued command
CommandHandler->>Database: Execute queued command
Database-->>CommandHandler: Command result
end
CommandHandler->>TransactionContext: endTransaction()
CommandHandler-->>Client: *2\r\n+OK\r\n:1\r\n
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/main/java/com/redis/commands/transaction/DiscardCommand.java`:
- Around line 27-43: The execute method in DiscardCommand must validate the
argument count: check args.size() == 0 at the top of execute(List<String> args,
ChannelHandlerContext ctx) and if not, return a wrong-arity error string (e.g.
"-ERR wrong number of arguments for 'discard' command\r\n") instead of
proceeding; keep the existing checks for TransactionContext.get(ctx.channel()),
txCtx.isInTransaction() and use txCtx.discard() and RESP_OK as currently
implemented after the arity check.
In `@src/main/java/com/redis/commands/transaction/ExecCommand.java`:
- Around line 45-53: The EXEC command currently ignores extra arguments; update
ExecCommand.execute to validate that args is empty before proceeding: if
args.size() != 0 return the wrong-arity error (use your project's wrong-argument
constant, e.g., ERR_WRONG_NUMBER_OF_ARGUMENTS) instead of continuing; keep the
existing TransactionContext retrieval and the ERR_NO_MULTI check
(TransactionContext.get and txCtx.isInTransaction) after the arity validation.
In `@src/main/java/com/redis/server/RedisCommandHandler.java`:
- Around line 100-102: The code assumes argsBuffer.getFirst() (commandName) is
non-null before calling toUpperCase(), which will throw if the first bulk string
is null (e.g., `$-1`); update RedisCommandHandler to check if commandName is
null and handle it gracefully: if null, set upperCommandName to a safe fallback
(e.g., empty string or a sentinel) or immediately return a proper Redis protocol
error to the client instead of calling toUpperCase(); use the same symbols
(argsBuffer, commandName, upperCommandName, ICommand,
CommandRegistry.getInstance().get(...)) so the null check is placed before
obtaining upperCommandName and before looking up the command in CommandRegistry.
In `@src/test/java/com/redis/commands/transaction/TransactionCommandTest.java`:
- Around line 32-43: The TransactionCommandTest setUp uses real EmbeddedChannel,
RedisCommandHandler, and RedisDatabase instances which makes it an integration
test; either convert it into a unit test by mocking RedisDatabase and the
handler interactions (replace EmbeddedChannel/RedisCommandHandler/RedisDatabase
usage in TransactionCommandTest with Mockito mocks and inject expected
behaviors) or move the test file out of the unit-test directory into the
integration-test suite so it can exercise real components; update the setUp
method (and any tests referencing EmbeddedChannel, RedisCommandHandler, or
RedisDatabase) accordingly to use mocks when keeping it as a unit test, or
relocate the entire TransactionCommandTest to the integration test source set if
you want to keep the realistic end-to-end behavior.
🧹 Nitpick comments (4)
docs/commands/INCR.md (1)
5-7: Add language specifiers to fenced code blocks.Static analysis flagged missing language identifiers on code blocks. For Redis CLI examples, use
```redisor```textto satisfy linting and enable syntax highlighting in documentation renderers.📝 Example fix for the first code block
-``` +```text INCR keyApply similar changes to code blocks at lines 23, 34, 43, 56, and 69. </details> Also applies to: 23-30, 34-39, 43-48, 56-63, 69-74 </blockquote></details> <details> <summary>src/main/java/com/redis/commands/string/IncrCommand.java (1)</summary><blockquote> `39-74`: **AtomicReference is unnecessary given Netty's single-threaded execution model.** Per learnings, command execution in RedisCommandHandler happens on Netty's event loop, guaranteeing single-threaded execution per channel. The `AtomicReference` can be replaced with a simple `String[]` or a local variable holder, since there's no cross-thread visibility concern here. That said, the current implementation is correct and the overhead is minimal. This is a stylistic preference. <details> <summary>♻️ Simpler alternative using a single-element array</summary> ```diff - // Use AtomicReference to capture result from compute lambda - AtomicReference<String> result = new AtomicReference<>(); + // Use array to capture result from compute lambda + String[] result = new String[1]; db.compute(key, currentValue -> { // ... lambda body unchanged, but use: - result.set(ERR_WRONG_TYPE); + result[0] = ERR_WRONG_TYPE; // ... and similarly for other assignments }); - return result.get(); + return result[0];Based on learnings: "all command execution in RedisCommandHandler happens on Netty's event loop, which guarantees single-threaded execution per channel."
docs/commands/EXEC.md (1)
5-7: Add language specifiers to fenced code blocks.Per static analysis, code blocks at lines 5, 25, 45, 69, 78, and 91 are missing language identifiers. Use
```textor```redisfor Redis CLI examples.Also applies to: 25-41, 45-63, 69-72, 78-87, 91-96
docs/commands/DISCARD.md (1)
5-7: Add language specifiers to fenced code blocks.Per static analysis, code blocks at lines 5, 23, 40, and 61 are missing language identifiers. Use
```textor```redisfor Redis CLI examples.Also applies to: 23-36, 40-55, 61-64
| private static final String RESP_OK = "+OK\r\n"; | ||
| private static final String ERR_NO_MULTI = "-ERR DISCARD without MULTI\r\n"; | ||
|
|
||
| @Override | ||
| public String execute(List<String> args, ChannelHandlerContext ctx) { | ||
| // Get transaction context | ||
| TransactionContext txCtx = TransactionContext.get(ctx.channel()); | ||
|
|
||
| // Verify we're in a transaction | ||
| if (txCtx == null || !txCtx.isInTransaction()) { | ||
| return ERR_NO_MULTI; | ||
| } | ||
|
|
||
| // Discard the transaction | ||
| txCtx.discard(); | ||
| return RESP_OK; | ||
| } |
There was a problem hiding this comment.
Validate argument count for DISCARD.
Extra args should return a wrong-arity error instead of silently discarding the queue.
🛠️ Suggested fix
`@Override`
public String execute(List<String> args, ChannelHandlerContext ctx) {
+ if (!args.isEmpty()) {
+ return "-ERR wrong number of arguments for 'discard' command\r\n";
+ }
// Get transaction context
TransactionContext txCtx = TransactionContext.get(ctx.channel());🤖 Prompt for AI Agents
In `@src/main/java/com/redis/commands/transaction/DiscardCommand.java` around
lines 27 - 43, The execute method in DiscardCommand must validate the argument
count: check args.size() == 0 at the top of execute(List<String> args,
ChannelHandlerContext ctx) and if not, return a wrong-arity error string (e.g.
"-ERR wrong number of arguments for 'discard' command\r\n") instead of
proceeding; keep the existing checks for TransactionContext.get(ctx.channel()),
txCtx.isInTransaction() and use txCtx.discard() and RESP_OK as currently
implemented after the arity check.
| @Override | ||
| public String execute(List<String> args, ChannelHandlerContext ctx) { | ||
| // Get transaction context | ||
| TransactionContext txCtx = TransactionContext.get(ctx.channel()); | ||
|
|
||
| // Verify we're in a transaction | ||
| if (txCtx == null || !txCtx.isInTransaction()) { | ||
| return ERR_NO_MULTI; | ||
| } |
There was a problem hiding this comment.
Validate argument count for EXEC.
Extra args should return a wrong-arity error; the current implementation ignores them.
🛠️ Suggested fix
`@Override`
public String execute(List<String> args, ChannelHandlerContext ctx) {
+ if (!args.isEmpty()) {
+ return "-ERR wrong number of arguments for 'exec' command\r\n";
+ }
// Get transaction context
TransactionContext txCtx = TransactionContext.get(ctx.channel());🤖 Prompt for AI Agents
In `@src/main/java/com/redis/commands/transaction/ExecCommand.java` around lines
45 - 53, The EXEC command currently ignores extra arguments; update
ExecCommand.execute to validate that args is empty before proceeding: if
args.size() != 0 return the wrong-arity error (use your project's wrong-argument
constant, e.g., ERR_WRONG_NUMBER_OF_ARGUMENTS) instead of continuing; keep the
existing TransactionContext retrieval and the ERR_NO_MULTI check
(TransactionContext.get and txCtx.isInTransaction) after the arity validation.
| String commandName = argsBuffer.getFirst(); | ||
| String upperCommandName = commandName.toUpperCase(); | ||
| ICommand cmd = CommandRegistry.getInstance().get(commandName); |
There was a problem hiding this comment.
Guard against null command names before uppercasing.
If the first bulk string is $-1, commandName becomes null and toUpperCase() will throw and close the channel instead of returning an error. Consider a safe fallback.
🛠️ Suggested fix
- String commandName = argsBuffer.getFirst();
- String upperCommandName = commandName.toUpperCase();
+ String commandName = argsBuffer.getFirst();
+ String upperCommandName = commandName == null ? "" : commandName.toUpperCase();🤖 Prompt for AI Agents
In `@src/main/java/com/redis/server/RedisCommandHandler.java` around lines 100 -
102, The code assumes argsBuffer.getFirst() (commandName) is non-null before
calling toUpperCase(), which will throw if the first bulk string is null (e.g.,
`$-1`); update RedisCommandHandler to check if commandName is null and handle it
gracefully: if null, set upperCommandName to a safe fallback (e.g., empty string
or a sentinel) or immediately return a proper Redis protocol error to the client
instead of calling toUpperCase(); use the same symbols (argsBuffer, commandName,
upperCommandName, ICommand, CommandRegistry.getInstance().get(...)) so the null
check is placed before obtaining upperCommandName and before looking up the
command in CommandRegistry.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@README.md`:
- Around line 44-49: Add a blank line immediately before and after the
"Transactions" table and normalize pipe spacing so each column cell and header
has a single space on both sides of the pipe (e.g., "| Command | Usage |
Documentation |") and keep the separator row using the existing alignment tokens
(":---") — update the table under the Transactions section (rows containing the
`MULTI`, `EXEC`, `DISCARD` entries) to follow these spacing rules to satisfy
MD058 and MD060.
| ### 🔄 Transactions | ||
| | Command | Usage | Documentation | | ||
| |:---|:---|:---| | ||
| | `MULTI` | `MULTI` | [MULTI.md](./docs/commands/MULTI.md) | | ||
| | `EXEC` | `EXEC` | [EXEC.md](./docs/commands/EXEC.md) | | ||
| | `DISCARD` | `DISCARD` | [DISCARD.md](./docs/commands/DISCARD.md) | |
There was a problem hiding this comment.
Fix markdownlint table spacing for the Transactions section.
markdownlint flags missing blank lines around the table (MD058) and compact table pipe spacing (MD060). Adjust spacing to satisfy the style rules.
📝 Proposed fix
-### 🔄 Transactions
-| Command | Usage | Documentation |
-|:---|:---|:---|
-| `MULTI` | `MULTI` | [MULTI.md](./docs/commands/MULTI.md) |
-| `EXEC` | `EXEC` | [EXEC.md](./docs/commands/EXEC.md) |
-| `DISCARD` | `DISCARD` | [DISCARD.md](./docs/commands/DISCARD.md) |
+### 🔄 Transactions
+
+| Command | Usage | Documentation |
+| :--- | :--- | :--- |
+| `MULTI` | `MULTI` | [MULTI.md](./docs/commands/MULTI.md) |
+| `EXEC` | `EXEC` | [EXEC.md](./docs/commands/EXEC.md) |
+| `DISCARD`| `DISCARD` | [DISCARD.md](./docs/commands/DISCARD.md) |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### 🔄 Transactions | |
| | Command | Usage | Documentation | | |
| |:---|:---|:---| | |
| | `MULTI` | `MULTI` | [MULTI.md](./docs/commands/MULTI.md) | | |
| | `EXEC` | `EXEC` | [EXEC.md](./docs/commands/EXEC.md) | | |
| | `DISCARD` | `DISCARD` | [DISCARD.md](./docs/commands/DISCARD.md) | | |
| ### 🔄 Transactions | |
| | Command | Usage | Documentation | | |
| | :--- | :--- | :--- | | |
| | `MULTI` | `MULTI` | [MULTI.md](./docs/commands/MULTI.md) | | |
| | `EXEC` | `EXEC` | [EXEC.md](./docs/commands/EXEC.md) | | |
| | `DISCARD`| `DISCARD` | [DISCARD.md](./docs/commands/DISCARD.md) | |
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 45-45: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
[warning] 46-46: Table column style
Table pipe is missing space to the right for style "compact"
(MD060, table-column-style)
[warning] 46-46: Table column style
Table pipe is missing space to the left for style "compact"
(MD060, table-column-style)
[warning] 46-46: Table column style
Table pipe is missing space to the right for style "compact"
(MD060, table-column-style)
[warning] 46-46: Table column style
Table pipe is missing space to the left for style "compact"
(MD060, table-column-style)
[warning] 46-46: Table column style
Table pipe is missing space to the right for style "compact"
(MD060, table-column-style)
[warning] 46-46: Table column style
Table pipe is missing space to the left for style "compact"
(MD060, table-column-style)
🤖 Prompt for AI Agents
In `@README.md` around lines 44 - 49, Add a blank line immediately before and
after the "Transactions" table and normalize pipe spacing so each column cell
and header has a single space on both sides of the pipe (e.g., "| Command |
Usage | Documentation |") and keep the separator row using the existing
alignment tokens (":---") — update the table under the Transactions section
(rows containing the `MULTI`, `EXEC`, `DISCARD` entries) to follow these spacing
rules to satisfy MD058 and MD060.
|
✅ Unit tests committed locally. Commit: |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@src/main/java/com/redis/commands/transaction/MultiCommand.java`:
- Around line 28-42: The MultiCommand.execute currently starts a transaction
without validating arguments; update MultiCommand.execute to enforce zero-arity
by returning the proper RESP wrong-arity error when args is not empty (do this
before creating/getOrCreate TransactionContext), so only when args.size() == 0
proceed to TransactionContext.getOrCreate(ctx.channel()), then check
txCtx.isInTransaction() and call txCtx.startTransaction(); reference symbols:
MultiCommand.execute, TransactionContext.getOrCreate, txCtx.isInTransaction,
txCtx.startTransaction, RESP_OK and ERR_NESTED (use existing or add a
WRONG_ARITY RESP constant consistent with project conventions).
In `@src/main/java/com/redis/transaction/TransactionContext.java`:
- Around line 91-97: The Javadoc for TransactionContext.getQueuedCommands claims
it returns an "Unmodifiable view" but the method currently returns the mutable
internal list queue; change getQueuedCommands to return
Collections.unmodifiableList(queue) (and add the java.util.Collections import if
missing) so callers get a true unmodifiable view, and keep the Javadoc as-is;
ensure the internal field name queue is still used and not exposed directly.
In `@src/test/java/com/redis/commands/transaction/DiscardCommandTest.java`:
- Around line 104-113: The test testDiscardIgnoresArguments is incorrect:
DISCARD should return a wrong-arity error when passed extra arguments. Update
the test (rename to something like testDiscardWrongArity) so it still creates a
TransactionContext and starts a transaction, then call
command.execute(Collections.singletonList("extra"), mockCtx) and assert that the
result is the expected wrong-arity error response for DISCARD (use the same
error string/utility used elsewhere in tests), rather than asserting "+OK";
reference the existing method names TransactionContext.getOrCreate,
ctx.startTransaction(), command.execute and mockCtx to locate and modify the
test.
In `@src/test/java/com/redis/commands/transaction/ExecCommandTest.java`:
- Around line 174-183: The test testExecIgnoresArguments must be updated because
ExecCommand now rejects extra arguments: instead of asserting a nil multi-bulk
("*0\r\n"), call command.execute(Collections.singletonList("extra"), mockCtx)
and assert it returns the wrong-arity error response for EXEC (i.e., the
standard "-ERR wrong number of arguments for 'exec' command" style reply) or use
the project's helper assertion for wrong-arity errors; update the assertion in
the test method to expect that error string (or helper) so the test aligns with
ExecCommand's new behavior.
In `@src/test/java/com/redis/commands/transaction/TransactionCommandTest.java`:
- Around line 45-55: The sendCommand helper creates and reads Netty ByteBufs
(buf and response) but never releases them; update sendCommand to release the
ByteBufs after use: after converting response.toString(StandardCharsets.UTF_8)
store the result in a local String, call response.release() (and also release
buf if it is not consumed by channel) before returning the string; reference the
sendCommand method, the ByteBuf variables buf and response, and the
channel.writeInbound/readOutbound calls when making the change.
- Around line 1-15: The test uses RedisValue.list() in TransactionCommandTest
but the RedisValue class is not imported, causing the build error; add the
missing import for the RedisValue type (the class that defines
RedisValue.list()) at the top of the file so the symbol resolves, then re-run
the tests to confirm the compilation passes.
- Around line 264-284: The testLargeTransaction creates 100 keys
(large_tx_key_0..large_tx_key_99) and never cleans them up; modify the test
fixture to remove these keys after each test (or before each test) to avoid test
pollution: add cleanup logic in your `@AfterEach` (or setUp/tearDown) method that
iterates the same key pattern and deletes them from the test DB (use the
existing db.delete/db.remove or sendCommand("DEL", ...) util used elsewhere),
ensuring the keys created by testLargeTransaction are removed; reference
testLargeTransaction, sendCommand, and db.get to locate where the keys are
created and validated.
🧹 Nitpick comments (4)
docs/commands/DISCARD.md (1)
5-7: Consider adding language hints to fenced code blocks.The documentation content is comprehensive and accurate. Static analysis flags missing language specifiers on code blocks (MD040). For redis-cli session examples, you could use
redisortextas the language hint for consistency.📝 Example fix for one block
-``` +```redis DISCARD</details> Also applies to: 23-36, 40-55, 61-64 </blockquote></details> <details> <summary>src/test/java/com/redis/commands/string/IncrCommandTest.java (2)</summary><blockquote> `36-41`: **Add test key cleanup to prevent test pollution.** The `setUp` method doesn't clean up test keys from previous runs. Tests modify keys like `incr_test_existing`, `incr_test_concurrent`, etc., which could persist across test runs and cause flaky behavior. <details> <summary>🧹 Proposed fix to add cleanup</summary> ```diff `@BeforeEach` void setUp() { command = new IncrCommand(); mockCtx = mock(ChannelHandlerContext.class); db = RedisDatabase.getInstance(); + // Clean up test keys to prevent pollution + db.remove("incr_test_existing"); + db.remove("incr_test_nonexistent"); + db.remove("incr_test_zero"); + db.remove("incr_test_negative"); + db.remove("incr_test_multi"); + db.remove("incr_test_string"); + db.remove("incr_test_float"); + db.remove("incr_test_list"); + db.remove("incr_test_overflow"); + db.remove("incr_test_large"); + db.remove("incr_test_large_neg"); + db.remove("incr_test_near_max"); + db.remove("incr_test_leading_zeros"); + db.remove("incr_test_whitespace"); + db.remove("incr_test_empty"); + db.remove("incr_test_scientific"); + db.remove("incr_test_underflow"); + db.remove("incr_test_concurrent"); + db.remove("incr_test_plus"); }
302-316: Test name is misleading for sequential execution.
testIncrConcurrentLikesuggests concurrent behavior, but this is sequential single-threaded execution. Given the learning that "all command execution in RedisCommandHandler happens on Netty's event loop, which guarantees single-threaded execution per channel," this test validates rapid sequential increments rather than actual concurrency.📝 Suggested rename for clarity
`@Test` - `@DisplayName`("INCR: Concurrent-like behavior with multiple increments") - void testIncrConcurrentLike() { + `@DisplayName`("INCR: Rapid sequential increments") + void testIncrRapidSequentialIncrements() {src/test/java/com/redis/commands/transaction/MultiCommandTest.java (1)
160-176: Consider using imported mock instead of fully qualified name.Line 167 uses
mock(com.redis.commands.ICommand.class)with a fully qualified class name. Since Mockito is already imported, you could add an import forICommandfor consistency.✨ Proposed cleanup
Add import:
import com.redis.transaction.TransactionContext; +import com.redis.commands.ICommand; import io.netty.channel.ChannelHandlerContext;Then simplify:
- ctx.queueCommand(mock(com.redis.commands.ICommand.class), Collections.emptyList()); + ctx.queueCommand(mock(ICommand.class), Collections.emptyList());
| private static final String RESP_OK = "+OK\r\n"; | ||
| private static final String ERR_NESTED = "-ERR MULTI calls can not be nested\r\n"; | ||
|
|
||
| @Override | ||
| public String execute(List<String> args, ChannelHandlerContext ctx) { | ||
| // Get or create transaction context for this connection | ||
| TransactionContext txCtx = TransactionContext.getOrCreate(ctx.channel()); | ||
|
|
||
| // Check for nested MULTI (not allowed) | ||
| if (txCtx.isInTransaction()) { | ||
| return ERR_NESTED; | ||
| } | ||
|
|
||
| // Start the transaction | ||
| txCtx.startTransaction(); |
There was a problem hiding this comment.
Enforce MULTI arity (no arguments).
Extra args should return a wrong-arity RESP error instead of starting a transaction.
🛠️ Suggested fix
public class MultiCommand implements ICommand {
private static final String RESP_OK = "+OK\r\n";
private static final String ERR_NESTED = "-ERR MULTI calls can not be nested\r\n";
+ private static final String ERR_WRONG_ARITY =
+ "-ERR wrong number of arguments for 'multi' command\r\n";
`@Override`
public String execute(List<String> args, ChannelHandlerContext ctx) {
+ if (!args.isEmpty()) {
+ return ERR_WRONG_ARITY;
+ }
// Get or create transaction context for this connection
TransactionContext txCtx = TransactionContext.getOrCreate(ctx.channel());🤖 Prompt for AI Agents
In `@src/main/java/com/redis/commands/transaction/MultiCommand.java` around lines
28 - 42, The MultiCommand.execute currently starts a transaction without
validating arguments; update MultiCommand.execute to enforce zero-arity by
returning the proper RESP wrong-arity error when args is not empty (do this
before creating/getOrCreate TransactionContext), so only when args.size() == 0
proceed to TransactionContext.getOrCreate(ctx.channel()), then check
txCtx.isInTransaction() and call txCtx.startTransaction(); reference symbols:
MultiCommand.execute, TransactionContext.getOrCreate, txCtx.isInTransaction,
txCtx.startTransaction, RESP_OK and ERR_NESTED (use existing or add a
WRONG_ARITY RESP constant consistent with project conventions).
| /** | ||
| * Gets the queued commands for execution. | ||
| * @return Unmodifiable view of queued commands | ||
| */ | ||
| public List<QueuedCommand> getQueuedCommands() { | ||
| return queue; // Direct access for performance; caller should not modify | ||
| } |
There was a problem hiding this comment.
Javadoc contradicts implementation.
The Javadoc states "Unmodifiable view of queued commands" but the method returns the mutable internal list directly. Either wrap the return with Collections.unmodifiableList() or update the Javadoc to reflect the actual behavior (which the inline comment does correctly).
📝 Option 1: Fix the Javadoc to match implementation
/**
* Gets the queued commands for execution.
- * `@return` Unmodifiable view of queued commands
+ * `@return` The internal list of queued commands (callers should not modify)
*/
public List<QueuedCommand> getQueuedCommands() {
return queue; // Direct access for performance; caller should not modify
}🛡️ Option 2: Return unmodifiable view (safer but slight overhead)
/**
* Gets the queued commands for execution.
* `@return` Unmodifiable view of queued commands
*/
public List<QueuedCommand> getQueuedCommands() {
- return queue; // Direct access for performance; caller should not modify
+ return Collections.unmodifiableList(queue);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Gets the queued commands for execution. | |
| * @return Unmodifiable view of queued commands | |
| */ | |
| public List<QueuedCommand> getQueuedCommands() { | |
| return queue; // Direct access for performance; caller should not modify | |
| } | |
| /** | |
| * Gets the queued commands for execution. | |
| * `@return` The internal list of queued commands (callers should not modify) | |
| */ | |
| public List<QueuedCommand> getQueuedCommands() { | |
| return queue; // Direct access for performance; caller should not modify | |
| } |
| /** | |
| * Gets the queued commands for execution. | |
| * @return Unmodifiable view of queued commands | |
| */ | |
| public List<QueuedCommand> getQueuedCommands() { | |
| return queue; // Direct access for performance; caller should not modify | |
| } | |
| /** | |
| * Gets the queued commands for execution. | |
| * `@return` Unmodifiable view of queued commands | |
| */ | |
| public List<QueuedCommand> getQueuedCommands() { | |
| return Collections.unmodifiableList(queue); | |
| } |
🤖 Prompt for AI Agents
In `@src/main/java/com/redis/transaction/TransactionContext.java` around lines 91
- 97, The Javadoc for TransactionContext.getQueuedCommands claims it returns an
"Unmodifiable view" but the method currently returns the mutable internal list
queue; change getQueuedCommands to return Collections.unmodifiableList(queue)
(and add the java.util.Collections import if missing) so callers get a true
unmodifiable view, and keep the Javadoc as-is; ensure the internal field name
queue is still used and not exposed directly.
| @Test | ||
| @DisplayName("DISCARD: Ignores arguments") | ||
| void testDiscardIgnoresArguments() { | ||
| TransactionContext ctx = TransactionContext.getOrCreate(channel); | ||
| ctx.startTransaction(); | ||
|
|
||
| String result = command.execute(Collections.singletonList("extra"), mockCtx); | ||
|
|
||
| assertEquals("+OK\r\n", result); | ||
| } |
There was a problem hiding this comment.
DISCARD with extra args should expect a wrong-arity error.
This test should align with the command’s arity validation.
🧪 Proposed fix
String result = command.execute(Collections.singletonList("extra"), mockCtx);
- assertEquals("+OK\r\n", result);
+ assertTrue(result.contains("ERR"));
+ assertTrue(result.contains("wrong number of arguments"));🤖 Prompt for AI Agents
In `@src/test/java/com/redis/commands/transaction/DiscardCommandTest.java` around
lines 104 - 113, The test testDiscardIgnoresArguments is incorrect: DISCARD
should return a wrong-arity error when passed extra arguments. Update the test
(rename to something like testDiscardWrongArity) so it still creates a
TransactionContext and starts a transaction, then call
command.execute(Collections.singletonList("extra"), mockCtx) and assert that the
result is the expected wrong-arity error response for DISCARD (use the same
error string/utility used elsewhere in tests), rather than asserting "+OK";
reference the existing method names TransactionContext.getOrCreate,
ctx.startTransaction(), command.execute and mockCtx to locate and modify the
test.
| package com.redis.commands.transaction; | ||
|
|
||
| import com.redis.server.RedisCommandHandler; | ||
| import com.redis.storage.RedisDatabase; | ||
| import io.netty.buffer.ByteBuf; | ||
| import io.netty.buffer.Unpooled; | ||
| import io.netty.channel.embedded.EmbeddedChannel; | ||
| import org.junit.jupiter.api.BeforeEach; | ||
| import org.junit.jupiter.api.DisplayName; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import java.nio.charset.StandardCharsets; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.*; | ||
|
|
There was a problem hiding this comment.
Missing import for RedisValue causes build failure.
The file uses RedisValue.list() on line 307 but lacks the corresponding import, causing the pipeline error:
cannot find symbol: variable RedisValue
🐛 Proposed fix to add the missing import
import com.redis.server.RedisCommandHandler;
import com.redis.storage.RedisDatabase;
+import com.redis.storage.RedisValue;
import io.netty.buffer.ByteBuf;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| package com.redis.commands.transaction; | |
| import com.redis.server.RedisCommandHandler; | |
| import com.redis.storage.RedisDatabase; | |
| import io.netty.buffer.ByteBuf; | |
| import io.netty.buffer.Unpooled; | |
| import io.netty.channel.embedded.EmbeddedChannel; | |
| import org.junit.jupiter.api.BeforeEach; | |
| import org.junit.jupiter.api.DisplayName; | |
| import org.junit.jupiter.api.Test; | |
| import java.nio.charset.StandardCharsets; | |
| import static org.junit.jupiter.api.Assertions.*; | |
| package com.redis.commands.transaction; | |
| import com.redis.server.RedisCommandHandler; | |
| import com.redis.storage.RedisDatabase; | |
| import com.redis.storage.RedisValue; | |
| import io.netty.buffer.ByteBuf; | |
| import io.netty.buffer.Unpooled; | |
| import io.netty.channel.embedded.EmbeddedChannel; | |
| import org.junit.jupiter.api.BeforeEach; | |
| import org.junit.jupiter.api.DisplayName; | |
| import org.junit.jupiter.api.Test; | |
| import java.nio.charset.StandardCharsets; | |
| import static org.junit.jupiter.api.Assertions.*; |
🤖 Prompt for AI Agents
In `@src/test/java/com/redis/commands/transaction/TransactionCommandTest.java`
around lines 1 - 15, The test uses RedisValue.list() in TransactionCommandTest
but the RedisValue class is not imported, causing the build error; add the
missing import for the RedisValue type (the class that defines
RedisValue.list()) at the top of the file so the symbol resolves, then re-run
the tests to confirm the compilation passes.
| private String sendCommand(String... args) { | ||
| StringBuilder cmd = new StringBuilder(); | ||
| cmd.append("*").append(args.length).append("\r\n"); | ||
| for (String arg : args) { | ||
| cmd.append("$").append(arg.length()).append("\r\n").append(arg).append("\r\n"); | ||
| } | ||
| ByteBuf buf = Unpooled.copiedBuffer(cmd.toString(), StandardCharsets.UTF_8); | ||
| channel.writeInbound(buf); | ||
| ByteBuf response = channel.readOutbound(); | ||
| return response != null ? response.toString(StandardCharsets.UTF_8) : null; | ||
| } |
There was a problem hiding this comment.
Potential memory leak: ByteBuf response not released.
The sendCommand helper reads a ByteBuf response but never releases it. In Netty, ByteBuf instances are reference-counted and should be released after use to avoid memory leaks in long test runs.
🔧 Proposed fix to release the ByteBuf
private String sendCommand(String... args) {
StringBuilder cmd = new StringBuilder();
cmd.append("*").append(args.length).append("\r\n");
for (String arg : args) {
cmd.append("$").append(arg.length()).append("\r\n").append(arg).append("\r\n");
}
ByteBuf buf = Unpooled.copiedBuffer(cmd.toString(), StandardCharsets.UTF_8);
channel.writeInbound(buf);
ByteBuf response = channel.readOutbound();
- return response != null ? response.toString(StandardCharsets.UTF_8) : null;
+ if (response == null) {
+ return null;
+ }
+ try {
+ return response.toString(StandardCharsets.UTF_8);
+ } finally {
+ response.release();
+ }
}🤖 Prompt for AI Agents
In `@src/test/java/com/redis/commands/transaction/TransactionCommandTest.java`
around lines 45 - 55, The sendCommand helper creates and reads Netty ByteBufs
(buf and response) but never releases them; update sendCommand to release the
ByteBufs after use: after converting response.toString(StandardCharsets.UTF_8)
store the result in a local String, call response.release() (and also release
buf if it is not consumed by channel) before returning the string; reference the
sendCommand method, the ByteBuf variables buf and response, and the
channel.writeInbound/readOutbound calls when making the change.
| @Test | ||
| @DisplayName("Large transaction with many commands") | ||
| void testLargeTransaction() { | ||
| sendCommand("MULTI"); | ||
|
|
||
| // Queue 100 commands | ||
| for (int i = 0; i < 100; i++) { | ||
| String queueResult = sendCommand("SET", "large_tx_key_" + i, "value_" + i); | ||
| assertEquals("+QUEUED\r\n", queueResult); | ||
| } | ||
|
|
||
| String result = sendCommand("EXEC"); | ||
|
|
||
| // Should return array of 100 results | ||
| assertTrue(result.startsWith("*100\r\n")); | ||
|
|
||
| // Verify all values were set | ||
| for (int i = 0; i < 100; i++) { | ||
| assertEquals("value_" + i, db.get("large_tx_key_" + i)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Large transaction test creates keys without cleanup.
The testLargeTransaction test creates 100 keys (large_tx_key_0 through large_tx_key_99) but doesn't clean them up in setUp() or via @AfterEach. This could cause test pollution if run repeatedly or in parallel.
🧹 Proposed fix to clean up large transaction keys
Add cleanup in setUp():
`@BeforeEach`
void setUp() {
channel = new EmbeddedChannel(new RedisCommandHandler());
db = RedisDatabase.getInstance();
// Clean up test keys
db.remove("tx_test_key");
db.remove("tx_test_key2");
db.remove("tx_counter");
+ // Clean up large transaction test keys
+ for (int i = 0; i < 100; i++) {
+ db.remove("large_tx_key_" + i);
+ }
}🤖 Prompt for AI Agents
In `@src/test/java/com/redis/commands/transaction/TransactionCommandTest.java`
around lines 264 - 284, The testLargeTransaction creates 100 keys
(large_tx_key_0..large_tx_key_99) and never cleans them up; modify the test
fixture to remove these keys after each test (or before each test) to avoid test
pollution: add cleanup logic in your `@AfterEach` (or setUp/tearDown) method that
iterates the same key pattern and deletes them from the test DB (use the
existing db.delete/db.remove or sendCommand("DEL", ...) util used elsewhere),
ensuring the keys created by testLargeTransaction are removed; reference
testLargeTransaction, sendCommand, and db.get to locate where the keys are
created and validated.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/test/java/com/redis/integration/ListCommandsIT.java`:
- Around line 323-328: The JavaDoc for BLPOP currently claims "Timeout of 0
means block indefinitely" but the implementation (see testBLPopTimeout and the
special-case handling around assertNullArray and the BLPOP timeout parsing
logic) treats timeout=0 as immediate non-blocking check; update the JavaDoc to
state "Timeout of 0 means check once and return immediately (non-standard
behavior)" and add a brief comment near the BLPOP handling code explaining this
deviation from Redis semantics and why it was chosen, referencing the
testBLPopTimeout behavior to keep docs and tests consistent.
🧹 Nitpick comments (8)
.github/workflows/ci.yml (1)
33-37: Build cache is written but never restored by dependent jobs.The
targetdirectory is cached withactions/cache@v4, butunit-testsandintegration-testsjobs don't restore this cache. They re-checkout and re-run Maven (which downloads dependencies via Maven cache but doesn't reuse compiled classes). Either:
- Remove this cache step (Maven's dependency cache at line 28 handles dependencies)
- Add a restore step in dependent jobs to actually use the compiled artifacts
Currently this cache write is unused, adding ~5-10 seconds per run with no benefit.
♻️ Option 1: Remove unused cache step
- name: Build project (skip tests) run: mvn clean compile test-compile -DskipTests -q - - - name: Cache build artifacts - uses: actions/cache@v4 - with: - path: target - key: ${{ runner.os }}-build-${{ github.sha }}src/test/java/com/redis/integration/BaseIntegrationTest.java (1)
164-191: Consider handling nested arrays inparseArrayResponse.The parser handles flat arrays with bulk strings, integers, simple strings, and errors. If EXEC returns nested arrays (e.g., for commands that return arrays), this parser would not recursively parse them.
This is a minor limitation for current tests but worth noting if future tests need nested array parsing.
src/test/java/com/redis/integration/StringCommandsIT.java (1)
62-69: Test nametestSetUnicodeis misleading.The test name suggests it tests Unicode handling, but the comment and implementation explicitly use ASCII characters to avoid encoding issues. Consider renaming to better reflect what's being tested.
💡 Suggested rename
`@Test` - `@DisplayName`("SET with special characters in value") - void testSetUnicode() { - // Use ASCII characters to avoid encoding issues in tests + `@DisplayName`("SET with alphanumeric and hyphen characters") + void testSetAlphanumericValue() { + // Use ASCII characters for reliable cross-platform testing String value = "hello-world-123";src/test/java/com/redis/commands/string/IncrCommandTest.java (1)
36-41: Consider adding@AfterEachcleanup for test keys.The tests use unique key names prefixed with
incr_test_*but don't clean up after each test. While this works due to unique naming, adding cleanup would prevent potential test pollution as the suite grows.💡 Suggested addition
`@BeforeEach` void setUp() { command = new IncrCommand(); mockCtx = mock(ChannelHandlerContext.class); db = RedisDatabase.getInstance(); } + + `@AfterEach` + void tearDown() { + // Clean up test keys to prevent pollution + db.remove("incr_test_existing"); + db.remove("incr_test_nonexistent"); + db.remove("incr_test_zero"); + db.remove("incr_test_negative"); + db.remove("incr_test_multi"); + db.remove("incr_test_string"); + db.remove("incr_test_float"); + db.remove("incr_test_list"); + db.remove("incr_test_large"); + db.remove("incr_test_overflow"); + db.remove("incr_test_large_neg"); + db.remove("incr_test_near_max"); + db.remove("incr_test_leading_zeros"); + db.remove("incr_test_whitespace"); + db.remove("incr_test_empty"); + db.remove("incr_test_scientific"); + db.remove("incr_test_underflow"); + db.remove("incr_test_concurrent"); + db.remove("incr_test_plus"); + }src/test/java/com/redis/integration/GenericCommandsIT.java (2)
212-219: Test may be flaky due to timing sensitivity.The assertion
expiry <= System.currentTimeMillis() + 1500relies on test execution completing within ~500ms of setting the expiry. On slow CI systems or under load, this could fail intermittently.Consider using a more generous bound or restructuring to avoid timing dependencies.
💡 Suggested improvement
`@Test` `@DisplayName`("EXPIRE with 1 second") void testExpireOneSecond() { db.put("expire_key", "value"); + long beforeExpire = System.currentTimeMillis(); assertInteger(1, "EXPIRE", "expire_key", "1"); long expiry = db.getExpiryTime("expire_key"); - assertTrue(expiry > System.currentTimeMillis()); - assertTrue(expiry <= System.currentTimeMillis() + 1500); // Within ~1.5 seconds + // Expiry should be within 1 second of command execution + assertTrue(expiry > beforeExpire); + assertTrue(expiry <= beforeExpire + 2000); // Allow 2s buffer for slow systems }
278-287: Consider tightening the TTL wrong-args assertion.The test accepts either an error OR
-2forTTLwith no arguments. This permissiveness might mask actual behavioral changes. If the expected behavior is an error, the assertion should be strict.src/test/java/com/redis/integration/ListCommandsIT.java (1)
168-184: Tighten LPOP count assertions to verify ordering.
Current checks only assert presence; order is part of LPOP semantics.Proposed change
String result = assertArraySize(3, "LPOP", "lpop_key", "3"); - assertTrue(result.contains("$1\r\na\r\n")); - assertTrue(result.contains("$1\r\nb\r\n")); - assertTrue(result.contains("$1\r\nc\r\n")); + List<String> elements = parseArrayResponse(result); + assertEquals(List.of("a", "b", "c"), elements); @@ String result = assertArraySize(2, "LPOP", "lpop_key", "5"); - assertTrue(result.contains("$1\r\na\r\n")); - assertTrue(result.contains("$1\r\nb\r\n")); + List<String> elements = parseArrayResponse(result); + assertEquals(List.of("a", "b"), elements);src/test/java/com/redis/integration/StreamCommandsIT.java (1)
210-224: Strengthen XREAD assertions to avoid false positives.
Current checks allow any array response, which can pass even if the IDs are wrong or data returns for missing streams.Proposed change
String result = sendCommand("XREAD", "STREAMS", "xread_key", "1-0"); assertNotNull(result); // Should return entry 2-0 (after 1-0) - assertTrue(result.contains("2-0") || result.startsWith("*")); + assertTrue(result.contains("2-0"), "Expected entry after 1-0"); @@ String result = sendCommand("XREAD", "STREAMS", "nonexistent", "0"); // Should return nil or empty - assertTrue(result.equals("*-1\r\n") || result.equals("*0\r\n") || result.startsWith("*")); + assertTrue(result.equals("*-1\r\n") || result.equals("*0\r\n"), + "Expected nil or empty array for non-existent stream");
| @Test | ||
| @DisplayName("BLPOP timeout on empty key") | ||
| void testBLPopTimeout() { | ||
| // With timeout 0 and no data, should return nil immediately for check-once behavior | ||
| assertNullArray("BLPOP", "blpop_key", "0"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Redis BLPOP command timeout 0 semantics documentation
💡 Result:
In Redis, BLPOP key [key ...] timeout interprets timeout as the maximum number of seconds to block (a double value). If timeout is 0, Redis will block indefinitely (i.e., it will wait forever until another client pushes an element to one of the specified lists). If timeout is non-zero and expires before any push happens, the command returns a nil reply. [1]
🏁 Script executed:
# Search for BLPOP implementation in the codebase
fd -t f "\.java$" | xargs grep -l "BLPOP" | head -20Repository: unikdahal/redis-java
Length of output: 235
🏁 Script executed:
# Look for timeout handling in list commands
rg -i "timeout.*0|block.*indefinite" -A 3 -B 3Repository: unikdahal/redis-java
Length of output: 5315
🏁 Script executed:
cat -n src/main/java/com/redis/commands/list/BLPopCommand.javaRepository: unikdahal/redis-java
Length of output: 10872
🏁 Script executed:
# Also check if there are any comments or documentation about this design choice
rg -i "timeout.*0|block.*indefinite|check.*once" -B 5 -A 5 src/main/java/com/redis/commands/list/BLPopCommand.javaRepository: unikdahal/redis-java
Length of output: 2321
🏁 Script executed:
# Check the test file to see if there are other timeout tests
grep -n "testBLPop\|void test" src/test/java/com/redis/integration/ListCommandsIT.java | head -20Repository: unikdahal/redis-java
Length of output: 849
Fix JavaDoc: timeout=0 behavior contradicts implementation.
The JavaDoc states "Timeout of 0 means block indefinitely" (line 25), but the implementation explicitly treats timeout=0 as a special case that returns immediately (lines 87-91). The test is correct for this implementation, so it won't hang. However, the documentation is misleading and should be updated to reflect the actual behavior: "Timeout of 0 means check once and return immediately (non-standard behavior)." Consider adding a comment explaining why this deviates from Redis semantics.
🤖 Prompt for AI Agents
In `@src/test/java/com/redis/integration/ListCommandsIT.java` around lines 323 -
328, The JavaDoc for BLPOP currently claims "Timeout of 0 means block
indefinitely" but the implementation (see testBLPopTimeout and the special-case
handling around assertNullArray and the BLPOP timeout parsing logic) treats
timeout=0 as immediate non-blocking check; update the JavaDoc to state "Timeout
of 0 means check once and return immediately (non-standard behavior)" and add a
brief comment near the BLPOP handling code explaining this deviation from Redis
semantics and why it was chosen, referencing the testBLPopTimeout behavior to
keep docs and tests consistent.
1 participant
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.